Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> --- On Fri, Mar 20, 2015 at 8:40 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >> `old` is not used outside the loop and would get lost >> once we reach the goto. >> >> Signed-off-by: Stefan Beller <sbeller@xxxxxxxxxx> >> --- >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html