Re: [PATCH v2 2/3] config.c: don't leak memory in handle_path_include()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 22, 2021, 22:30 UTC
Message-ID: <211023.86wnm4isfc.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqh7d8eox7.fsf@gitster.g>

On Fri, Oct 22 2021, Junio C Hamano wrote:

Show 19 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> Not a problem introduced by this function, but if you look at this
>>> change with "git show -W", we'd notice that the function name on the
>>> hunk header looks strange.  I think we should add a blank line
>>> before the beginning of the function.
>>
>> I think this is a bug in -W, after all if without it we we show the
>> function context line, but with it we advance further, then that means
>> that -W didn't find the correct function boundary.
>
> That's a chicken-and-egg argument, and I do not think it is a bug in
> "-W" nor the funcname regular expression pattern we use.  We expect
> a blank line there and the pattern reflects that expectation, so not
> having an expected blank line is what causes this problem.
>
> In any case, we should add a blank linke before the beginning of the
> function, and of course that is obviously outside the scope of these
> patches.

Sort of, if you were running with the patch I posted at [1] you wouldn't see the bad value at @@, but we still extend upwards with -W, which I consider a bug.

I.e. both the current context we display and the over-extension there is ultimately a symptom of the same issue, which is that what we're doing with -W gets conflated with behavior that makes sense without -W, notice how if you do "git log -W" on anything that the @@ context we display is the prototype of the function /above/ the one you're likely looking at the code change in.

So the blank line is the cause of the over-extension, but we'd still show the (IMO) incorrect context in either case.

Anyway, as you say a discussion for some other thread. I've been meaning to get back to those patches at some point, the first problem is that our test coverage for what function context we should find when is really lacking, so any changes in that part of the xdiff code are likely to break things. I had those tests, but they got lost in some bikeshedding...

1. https://lore.kernel.org/git/20210215155020.2804-2-avarab@gmail.com/