Am 04.05.2017 um 15:56 schrieb Johannes Schindelin:
This change addresses part of the NEEDSWORK comment above the code,
therefore the comment needs to be adjusted, too.
Discovered via Coverity.
Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
builtin/checkout.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfa5419f335..5faea3a05fa 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -235,14 +235,14 @@ static int checkout_merged(int pos, const struct checkout
*state)
/*
* NEEDSWORK:
* There is absolutely no reason to write this as a blob object
- * and create a phony cache entry just to leak. This hack is
- * primarily to get to the write_entry() machinery that massages
- * the contents to work-tree format and writes out which only
- * allows it for a cache entry. The code in write_entry() needs
- * to be refactored to allow us to feed a <buffer, size, mode>
- * instead of a cache entry. Such a refactoring would help
- * merge_recursive as well (it also writes the merge result to the
- * object database even when it may contain conflicts).
+ * and create a phony cache entry. This hack is primarily to get
+ * to the write_entry() machinery that massages the contents to
+ * work-tree format and writes out which only allows it for a
+ * cache entry. The code in write_entry() needs to be refactored
+ * to allow us to feed a <buffer, size, mode> instead of a cache
+ * entry. Such a refactoring would help merge_recursive as well
+ * (it also writes the merge result to the object database even
+ * when it may contain conflicts).
*/
if (write_sha1_file(result_buf.ptr, result_buf.size,
blob_type, oid.hash))
Random observation: Using pretend_sha1_file here would at least avoid
writing the blob.
@@ -251,6 +251,7 @@ static int checkout_merged(int pos, const struct checkout
*state)
if (!ce)
die(_("make_cache_entry failed for path '%s'"), path);
status = checkout_entry(ce, state, NULL);
+ free(ce);
return status;
}
I wonder if that's safe. Why document a leak when it could have been
plugged this easily instead? A leak is better than a use after free, so
let's be extra careful here. Would it leave the index inconsistent? Or
perhaps freeing it has become safe in the meantime?
@Junio: Do you remember the reason for the leaks in 0cf8581e330
(checkout -m: recreate merge when checking out of unmerged index).
And result_buf is still leaked here, right?
FWIW, after fixing a different issue (patch just sent separately)
t*checkout*.sh complete successfully for me with --valgrind, with and
without freeing both ce and result_buf.ptr. So it *seems* safe..
René
