[PATCH] update-index: Don't copy memory around

[Stefan Beller <sbeller@google.com>]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

On Fri, Mar 20, 2015 at 8:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> `old` is not used outside the loop and would get lost
>> once we reach the goto.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/update-index.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index 5878986..6271b54 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -584,6 +584,7 @@ static int do_reupdate(int ac, const char **av,
>>               path = xstrdup(ce->name);
>>               update_one(path);
>>               free(path);
>> +             free(old);
>
> The change looks trivially correct.
>
> A tangent; I wonder if we want to make path a strbuf that is
> declared in the function scope, reset (not released) at each
> iteration of the loop and released after the loop; keep allocating
> and freeing a small piece of string all the time feels somewhat
> wasteful.

> Also, we might want to add to the "Be careful" comment to describe
> why this cannot just be a call to "update_one(ce->name)" that does
> not use "path".

Indeed I am rather wondering why we need to pass in a copy to update_one
instead of ce->name. I was reading update_one and looking for why, but
eventually noticed update_one accepts a 'const char *', so it's not changed
in there. Digging down the into update_one also doesn't make it obvious to me.

I found (5699d17ee094, 2013-11-14, read-cache.c: fix memory leaks caused by
removed cache entries), which adds this duplication, though I do not understand
why. This passes the test suite, so I wonder if this patch would be a subtle bug
now.

 builtin/update-index.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 6271b54..5857405 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -564,7 +564,6 @@ static int do_reupdate(int ac, const char **av,
 		const struct cache_entry *ce = active_cache[pos];
 		struct cache_entry *old = NULL;
 		int save_nr;
-		char *path;
 
 		if (ce_stage(ce) || !ce_path_match(ce, &pathspec, NULL))
 			continue;
@@ -581,9 +580,7 @@ static int do_reupdate(int ac, const char **av,
 		 * or worse yet 'allow_replace', active_nr may decrease.
 		 */
 		save_nr = active_nr;
-		path = xstrdup(ce->name);
-		update_one(path);
-		free(path);
+		update_one(ce->name);
 		free(old);
 		if (save_nr != active_nr)
 			goto redo;
-- 
2.3.0.81.gc37f363