Hello all!
Thanks to all of the Git community for making such amazing tools.
Some of my favorite features of Git are the --show-function and
--function-context features of git {grep,log,diff}.
However, the default configuration leaves a bit to be desired --
setting some simple flags in ~/.gitattributes for e.g.
*.cpp diff=cpp
*.py diff=python
Makes these features MUCH more accurate and usable. However, one has
to know about gitattributes, diff filters, xfuncname, etc. in order to
turn these settings on.
I'd like to contribute changes to Git that makes the "obvious"
correlations be the default setting. Before I start, I wanted to
gauge whether these changes would be accepted or not. As far as I can
tell, these would not change the default behavior of plain git
{grep,log,diff} unless the --show-function or --function-context flags
are specified -- and if they are, the results would be improved.
Should I work on a patch that does this?
Thanks in advance,
Heapcrash
On Sun, Aug 01 2021, heapcrash heapcrash wrote:
Hello all!
Thanks to all of the Git community for making such amazing tools.
Some of my favorite features of Git are the --show-function and
--function-context features of git {grep,log,diff}.
However, the default configuration leaves a bit to be desired --
setting some simple flags in ~/.gitattributes for e.g.
*.cpp diff=cpp
*.py diff=python
Makes these features MUCH more accurate and usable. However, one has
to know about gitattributes, diff filters, xfuncname, etc. in order to
turn these settings on.
I'd like to contribute changes to Git that makes the "obvious"
correlations be the default setting. Before I start, I wanted to
gauge whether these changes would be accepted or not. As far as I can
tell, these would not change the default behavior of plain git
{grep,log,diff} unless the --show-function or --function-context flags
are specified -- and if they are, the results would be improved.
Should I work on a patch that does this?
As Junio hinted at in his reply this doesn't just change diff behavior
with the -W or --function-context flag, but changes the work we do (even
if the output is the same) on all unified diff output.
I.e. to generate the "@@" context line we by default use a C function in
xdiff/, this is replaced with a regex-in-a-loop invoking the
userdiff.[ch] code.
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.
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.
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/
From my re-read of the threads, it seemed like people were mostly on
board with the idea, but:
- there was some question about a few of the more obscure extensions
(e.g., '.m' as objective-c versus matlab). Obviously we could drop
any contentious ones, though I do wonder if this is simply opening
up a can of worms where people will fight about what ought to go
into the "official" list.
- there was some question over whether some of our builtin funcname
regexps were actually worse than the default behavior (especially
the "cpp" one). In response we started using .gitattributes more
heavily within git.git itself, and we've seen quite a few
improvements in the intervening decade.
So hopefully there is no reason anybody would _not_ want the
content-specific driver to kick in, assuming that the extension
mapping is accurate. There should be no security risk or anything
like that, since we'd already respect such a mapping from an
untrusted repository via its .gitattributes file.
It looks like Junio picked up the patch at one point, but it got bumped
to the next release cycle. It was marked as "expecting a re-roll" until
finally getting discarded in Oct 2011; you can see the progression
by searching for "jk/default-attr":
https://lore.kernel.org/git/?q=jk%2Fdefault-attr
Then in Dec 2011 I posted that re-roll (the top link I gave above), and
it looks like it never got picked up. I don't see any conclusions
either way in the thread, so I think it just kind of died out due to
lack of anybody pushing it forward (and I don't remember making a
decision either way on that; it may have been the "can of worms" thing
above scaring me off).
-Peff