Signed-off-by: Stefan Beller <[email protected]>
---
On Fri, Mar 20, 2015 at 8:40 PM, Junio C Hamano <[email protected]> wrote:
> Stefan Beller <[email protected]> writes:
>
>> `old` is not used outside the loop and would get lost
>> once we reach the goto.
>>
>> Signed-off-by: Stefan Beller <[email protected]>
>> ---
>> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
