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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 13, 2021, 19:01 UTC
Message-ID: <xmqqczo8hhq2.fsf@gitster.g>
In-Reply-To: <8735p5hwg6.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 20 quoted lines

> -       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.

> 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?

Thanks.