[PATCH] update-index: Don't copy memory around
- From
Stefan Beller <sbeller@google.com>
- Date
- Mar 23, 2015, 16:53 UTC
- Message-ID
- <1427129620-13380-1-git-send-email-sbeller@google.com>
- In-Reply-To
- <xmqqwq2baui7.fsf@gitster.dls.corp.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:
Show 27 quoted lines
> 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