Re: Enhancing --show-function and --function-context in default configurations

From: heapcrash heapcrash <heapcrash@gmail.com>
Date: Aug 3, 2021, 02:34 UTC
Message-ID: <CAM1Tk16Duh9ySS1xwW3yK4ogz=HbMp9k5B7bjCGSgqOEx=pdtA@mail.gmail.com>
In-Reply-To: <xmqqmtpzwzbm.fsf@gitster.g>

Thanks all for the feedback!  I'll try to address it below:

> Two other places may be diff hunk headers and --diff-words output, I think.

I didn't think of those. Does this mean that diffs generated with a given e.g. diff=cpp configuration might not apply cleanly if run on some other user's system without that setting?

> That depends.  If you are going to introduce a mechanism to
> introduce hardcoded configurations, depending on how it is done, it
> will become a huge security headache.

I don't intend to add hard-coded configurations, just hard-coded defaults. User configurations (~/.gitattributes and gitconfig diff.funcname/diff.xcfuncname) will still take precedence.

> It makes emitting the diffs take more CPU, but the same is true of other
> options like colorMoved etc, so that in itself is not a dealbreaker.

I didn't think of this scenario, that it would add CPU time even without -W/--function-context/--show-function. I'd definitely be fine with preserving the current behavior in these cases (more below).

Show 9 quoted lines

> As long as the default built-in ones are
>
> (1) at least 90% of the time improvement over, or at least is not
>     broken compared to, the unconfigured case, and
>
> (2) at the lowest priority that users can easily countermand for
>     the rest 10% cases
>
> I do not think it is too bad to resurrect the old patches from these

The currently-hard-coded regexps for e.g. cpp, python, objc, etc. (in userdiff.c) are all VAST improvements over the default, which is effectively '^[A-Za-z0-9_:]+' (i.e., any word which starts on the zeroth column, IIRC). This has lots of issues for e.g. Python where the function name is often indented (e.g. inside a class definition) and cpp is compatible with, and an improvement over, the default "C" setting (which is particularly broken as it marks any "goto" label as a function start / end).

All of those points covered, I think we can make this work in a way that preserves backward compatibility (no defaults at all, user must manually configure ~/.gitattributes) by only setting "smart" defaults (e.g. "*.cpp diff=cpp", "*.cc diff=cpp" etc) when the command is EXPLICITLY invoked with -W/--function-context/--show-function.

Another case I didn't think of in the original post is with the "git log -L:funcname:path/to/file.cpp" option, which tracks changes within a function over time. Having "better" default function boundary detection here would also be useful.

Ultimately, I agree we should not have any extra overhead for any commands (log, diff, grep, etc) unless the user EXPLICITLY uses flags that indicate function boundary detection are key to functionality of those flags.

On Mon, Aug 2, 2021 at 12:06 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 42 quoted lines

>
> Jeff King <peff@peff.net> writes:
>
> > On Mon, Aug 02, 2021 at 10:45:25AM +0200, Ævar Arnfjörð Bjarmason wrote:
> >
> >> I would like to see us have a setting to turn these on by default, but
> >> think it would be better to make that a diff.* config setting to put
> >> into ~/.gitconfig, i.e. we'd extend git itself to know about a list of
> >> extensions for the given userdiff drivers, and use them when rendering
> >> diffs.
> >
> > A long time ago we discussed doing this. The relevant thread is:
> >
> >   https://lore.kernel.org/git/20111216110000.GA15676@sigill.intra.peff.net/
> >
> > which references a few others:
> >
> >   https://lore.kernel.org/git/4E569F10.8060808@panasas.com/
> >
> >   https://lore.kernel.org/git/4E6E928A.6080003@sunshineco.com/
> > ...
>
> Thanks for pointers.
>
> One good suggestion given there was to use diff=c and diff=perl in
> our own .gitattributes to use the patterns ourselves, which we seem
> to have been doing just fine ;-)
>
> As long as the default built-in ones are
>
>  (1) at least 90% of the time improvement over, or at least is not
>      broken compared to, the unconfigured case, and
>
>  (2) at the lowest priority that users can easily countermand for
>      the rest 10% cases
>
> I do not think it is too bad to resurrect the old patches from these
> threads.
>
> Thanks.
>
>