On Mon, Nov 22 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> diff --git a/add-patch.c b/add-patch.c
>> index 8c41cdfe39b..573eef0cc4a 100644
>> --- a/add-patch.c
>> +++ b/add-patch.c
>> @@ -413,7 +413,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>> strvec_push(&args, ps->items[i].original);
>>
>> setup_child_process(s, &cp, NULL);
>> - cp.argv = args.v;
>> + strvec_pushv(&cp.args, args.v);
>> res = capture_command(&cp, plain, 0);
>> if (res) {
>> strvec_clear(&args);
>> @@ -431,7 +431,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>>
>> setup_child_process(s, &colored_cp, NULL);
>> xsnprintf((char *)args.v[color_arg_index], 8, "--color");
>> - colored_cp.argv = args.v;
>> + strvec_pushv(&colored_cp.args, args.v);
>> colored = &s->colored;
>> res = capture_command(&colored_cp, colored, 0);
>> strvec_clear(&args);
>
> We used to use the caller-supplied args, run-command API borrowed
> that strvec by pointing at cp.argv, and because it is borrowed,
> capture_command() did not use cp.args/cp.env_array and there was no
> leak inside run-command API side, whether capture_command succeeded
> or failed. The code was using its own args, so it already correctly
> releases it (we can see one such strvec_clear() here).
>
> OK.
>
> I gave the remainder only a cursory look so I cannot call it quite
> "reviewed", but presumably all the other changes in this patch are
> the same way?
Yes, I tried to group all of these cases together.
