Re: [PATCH v2 5/7] ls-files: fix a trivial dir_clear() leak

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 14, 2021, 00:15 UTC
Message-ID: <87lf2wh2fu.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqczo8hhq2.fsf@gitster.g>

On Wed, Oct 13 2021, Junio C Hamano wrote:

Show 25 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> -       if (ps_matched) {
>> -               int bad;
>> -               bad = report_path_error(ps_matched, &pathspec);
>> -               if (bad)
>> -                       fprintf(stderr, "Did you forget to 'git add'?\n");
>> -
>> -               return bad ? 1 : 0;
>> +       if (ps_matched && report_path_error(ps_matched, &pathspec)) {
>> +               fprintf(stderr, "Did you forget to 'git add'?\n");
>> +               ret = 1;
>>         }
>>  
>>         dir_clear(&dir);
>>         free(max_prefix);
>> -       return 0;
>> +       return ret;
>>  }
>>
>> Doesn't make much sense, but I can re-roll with it if you feel strongly
>> about it. I think the current version is ready to be picked up.
>
> I do not see where that "doesn't make much sense" comes from.  If it
> does not make sense, I wouldn't have mentioned it.

I meant "I don't think that makes much sense".

Show 6 quoted lines

>> Yeah we should avoid refactoring-while-at-it, but in cases where a patch
>> removes the only reason a nested if/if statement exists, unrolling it
>
> And I do not quite see what "the only reason" is in this case, or
> what it has to do with the restructuring, either.  Care to either
> clarify, or fix the patch, or perhaps both?

I mean that we generally don't write code like:

	if (x) {
		if (y) {
			fprintf(...);
			ret = 1;
		}
	}

And instead write:

	if (x && y) {
		fprintf(...);
		ret = 1;
	}

So aside from the specific change here I thought your objection would also apply to e.g. removing braces from a standalone "if" as it's reduced to a one statement body, which we we generally do.

But reading your upthread:

    [...]the "ah, report_path_error() always returns true" does not
    belong here.

I think the objection might be in folding that in particular into the "if" statement?

The point of this change is that if report_path_error() was non-zero we'd skip the freeing before, but shouldn't.

So any change here would have wanted to fall through the "if", but we need to do the fprintf() if report_path_error returns true.

So I don't really get that "always returns true" comment means in this context. It returns true on error, and dealing with that code branch in relation to the freeing is what this change needs to concern itself with.