{"thread":{"id":"56195","subject":"Enhancing --show-function and --function-context in default configurations","startedAt":"2021-08-01T21:41:12Z","lastAt":"2021-08-03T14:55:22Z","messageCount":7,"participants":["heapcrash heapcrash","Junio C Hamano","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"431656","messageId":"CAM1Tk15N6uK3aU5bY6mWQVX5UdEOraysrRmO+v4TA+P-Wa9_hQ@mail.gmail.com","threadId":"56195","inReplyTo":null,"subject":"Enhancing --show-function and --function-context in default configurations","fromName":"heapcrash heapcrash","fromEmail":"heapcrash@gmail.com","sentAt":"2021-08-01T21:40:58Z","receivedAt":"2021-08-01T21:41:12Z","isPatch":false,"sender":{"key":"heapcrash@gmail.com","avatar":null},"body":"Hello all!\n\nThanks to all of the Git community for making such amazing tools.\n\nSome of my favorite features of Git are the --show-function and\n--function-context features of git {grep,log,diff}.\n\nHowever, the default configuration leaves a bit to be desired --\nsetting some simple flags in ~/.gitattributes for e.g.\n\n *.cpp diff=cpp\n *.py diff=python\n\nMakes these features MUCH more accurate and usable. However, one has\nto know about gitattributes, diff filters, xfuncname, etc. in order to\nturn these settings on.\n\nI'd like to contribute changes to Git that makes the \"obvious\"\ncorrelations be the default setting. Before I start, I wanted to\ngauge whether these changes would be accepted or not. As far as I can\ntell, these would not change the default behavior of plain git\n{grep,log,diff} unless the --show-function or --function-context flags\nare specified -- and if they are, the results would be improved.\n\nShould I work on a patch that does this?\n\nThanks in advance,\nHeapcrash\n"},{"id":"431659","messageId":"xmqqzgu0y4an.fsf@gitster.g","threadId":"56195","inReplyTo":"CAM1Tk15N6uK3aU5bY6mWQVX5UdEOraysrRmO+v4TA+P-Wa9_hQ@mail.gmail.com","subject":"Re: Enhancing --show-function and --function-context in default configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-02T02:21:52Z","receivedAt":"2021-08-02T02:21:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"heapcrash heapcrash writes:\n\n> gauge whether these changes would be accepted or not. As far as I can\n> tell, these would not change the default behavior of plain git\n> {grep,log,diff} unless the --show-function or --function-context flags\n> are specified\n\nTwo other places may be diff hunk headers and --diff-words output, I think.\n\n> Should I work on a patch that does this?\n\nThat depends. If you are going to introduce a mechanism to\nintroduce hardcoded configurations, depending on how it is done, it\nwill become a huge security headache.\n"},{"id":"431664","messageId":"875ywoqldr.fsf@evledraar.gmail.com","threadId":"56195","inReplyTo":"CAM1Tk15N6uK3aU5bY6mWQVX5UdEOraysrRmO+v4TA+P-Wa9_hQ@mail.gmail.com","subject":"Re: Enhancing --show-function and --function-context in default configurations","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-02T08:45:25Z","receivedAt":"2021-08-02T08:52:27Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Aug 01 2021, heapcrash heapcrash wrote:\n\n> Hello all!\n>\n> Thanks to all of the Git community for making such amazing tools.\n>\n> Some of my favorite features of Git are the --show-function and\n> --function-context features of git {grep,log,diff}.\n>\n> However, the default configuration leaves a bit to be desired --\n> setting some simple flags in ~/.gitattributes for e.g.\n>\n> *.cpp diff=cpp\n> *.py diff=python\n>\n> Makes these features MUCH more accurate and usable. However, one has\n> to know about gitattributes, diff filters, xfuncname, etc. in order to\n> turn these settings on.\n>\n> I'd like to contribute changes to Git that makes the \"obvious\"\n> correlations be the default setting. Before I start, I wanted to\n> gauge whether these changes would be accepted or not. As far as I can\n> tell, these would not change the default behavior of plain git\n> {grep,log,diff} unless the --show-function or --function-context flags\n> are specified -- and if they are, the results would be improved.\n>\n> Should I work on a patch that does this?\n\nAs Junio hinted at in his reply this doesn't just change diff behavior\nwith the -W or --function-context flag, but changes the work we do (even\nif the output is the same) on all unified diff output.\n\nI.e. to generate the \"@@\" context line we by default use a C function in\nxdiff/, this is replaced with a regex-in-a-loop invoking the\nuserdiff.[ch] code.\n\nI would like to see us have a setting to turn these on by default, but\nthink it would be better to make that a diff.* config setting to put\ninto ~/.gitconfig, i.e. we'd extend git itself to know about a list of\nextensions for the given userdiff drivers, and use them when rendering\ndiffs.\n\nIt makes emitting the diffs take more CPU, but the same is true of other\noptions like colorMoved etc, so that in itself is not a dealbreaker.\n"},{"id":"431704","messageId":"YQgahVPT8g0+U1B3@coredump.intra.peff.net","threadId":"56195","inReplyTo":"875ywoqldr.fsf@evledraar.gmail.com","subject":"Re: Enhancing --show-function and --function-context in default configurations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-02T16:17:09Z","receivedAt":"2021-08-02T16:17:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 02, 2021 at 10:45:25AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I would like to see us have a setting to turn these on by default, but\n> think it would be better to make that a diff.* config setting to put\n> into ~/.gitconfig, i.e. we'd extend git itself to know about a list of\n> extensions for the given userdiff drivers, and use them when rendering\n> diffs.\n\nA long time ago we discussed doing this. The relevant thread is:\n\n https://lore.kernel.org/git/20111216110000.GA15676@sigill.intra.peff.net/\n\nwhich references a few others:\n\n https://lore.kernel.org/git/4E569F10.8060808@panasas.com/\n\n https://lore.kernel.org/git/4E6E928A.6080003@sunshineco.com/\n\nFrom my re-read of the threads, it seemed like people were mostly on\nboard with the idea, but:\n\n - there was some question about a few of the more obscure extensions\n (e.g., '.m' as objective-c versus matlab). Obviously we could drop\n any contentious ones, though I do wonder if this is simply opening\n up a can of worms where people will fight about what ought to go\n into the \"official\" list.\n\n - there was some question over whether some of our builtin funcname\n regexps were actually worse than the default behavior (especially\n the \"cpp\" one). In response we started using .gitattributes more\n heavily within git.git itself, and we've seen quite a few\n improvements in the intervening decade.\n\n So hopefully there is no reason anybody would _not_ want the\n content-specific driver to kick in, assuming that the extension\n mapping is accurate. There should be no security risk or anything\n like that, since we'd already respect such a mapping from an\n untrusted repository via its .gitattributes file.\n\nIt looks like Junio picked up the patch at one point, but it got bumped\nto the next release cycle. It was marked as \"expecting a re-roll\" until\nfinally getting discarded in Oct 2011; you can see the progression\nby searching for \"jk/default-attr\":\n\n https://lore.kernel.org/git/?q=jk%2Fdefault-attr\n\nThen in Dec 2011 I posted that re-roll (the top link I gave above), and\nit looks like it never got picked up. I don't see any conclusions\neither way in the thread, so I think it just kind of died out due to\nlack of anybody pushing it forward (and I don't remember making a\ndecision either way on that; it may have been the \"can of worms\" thing\nabove scaring me off).\n\n-Peff\n"},{"id":"431727","messageId":"xmqqmtpzwzbm.fsf@gitster.g","threadId":"56195","inReplyTo":"YQgahVPT8g0+U1B3@coredump.intra.peff.net","subject":"Re: Enhancing --show-function and --function-context in default configurations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-02T17:06:53Z","receivedAt":"2021-08-02T17:06:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 02, 2021 at 10:45:25AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> I would like to see us have a setting to turn these on by default, but\n>> think it would be better to make that a diff.* config setting to put\n>> into ~/.gitconfig, i.e. we'd extend git itself to know about a list of\n>> extensions for the given userdiff drivers, and use them when rendering\n>> diffs.\n>\n> A long time ago we discussed doing this. The relevant thread is:\n>\n> https://lore.kernel.org/git/20111216110000.GA15676@sigill.intra.peff.net/\n>\n> which references a few others:\n>\n> https://lore.kernel.org/git/4E569F10.8060808@panasas.com/\n>\n> https://lore.kernel.org/git/4E6E928A.6080003@sunshineco.com/\n> ...\n\nThanks for pointers.\n\nOne good suggestion given there was to use diff=c and diff=perl in\nour own .gitattributes to use the patterns ourselves, which we seem\nto have been doing just fine ;-)\n\nAs long as the default built-in ones are\n\n (1) at least 90% of the time improvement over, or at least is not\n broken compared to, the unconfigured case, and\n\n (2) at the lowest priority that users can easily countermand for\n the rest 10% cases\n\nI do not think it is too bad to resurrect the old patches from these\nthreads.\n\nThanks.\n\n\n"},{"id":"431793","messageId":"CAM1Tk16Duh9ySS1xwW3yK4ogz=HbMp9k5B7bjCGSgqOEx=pdtA@mail.gmail.com","threadId":"56195","inReplyTo":"xmqqmtpzwzbm.fsf@gitster.g","subject":"Re: Enhancing --show-function and --function-context in default configurations","fromName":"heapcrash heapcrash","fromEmail":"heapcrash@gmail.com","sentAt":"2021-08-03T02:34:32Z","receivedAt":"2021-08-03T02:34:47Z","isPatch":false,"sender":{"key":"heapcrash@gmail.com","avatar":null},"body":"Thanks all for the feedback! I'll try to address it below:\n\n> Two other places may be diff hunk headers and --diff-words output, I think.\n\nI didn't think of those. Does this mean that diffs generated with a\ngiven e.g. diff=cpp configuration might not apply cleanly if run on\nsome other user's system without that setting?\n\n> That depends. If you are going to introduce a mechanism to\n> introduce hardcoded configurations, depending on how it is done, it\n> will become a huge security headache.\n\nI don't intend to add hard-coded configurations, just hard-coded\ndefaults. User configurations (~/.gitattributes and gitconfig\ndiff.funcname/diff.xcfuncname) will still take precedence.\n\n> It makes emitting the diffs take more CPU, but the same is true of other\n> options like colorMoved etc, so that in itself is not a dealbreaker.\n\nI didn't think of this scenario, that it would add CPU time even\nwithout -W/--function-context/--show-function. I'd definitely be fine\nwith preserving the current behavior in these cases (more below).\n\n> As long as the default built-in ones are\n>\n> (1) at least 90% of the time improvement over, or at least is not\n> broken compared to, the unconfigured case, and\n>\n> (2) at the lowest priority that users can easily countermand for\n> the rest 10% cases\n>\n> I do not think it is too bad to resurrect the old patches from these\n\nThe currently-hard-coded regexps for e.g. cpp, python, objc, etc. (in\nuserdiff.c) are all VAST improvements over the default, which is\neffectively '^[A-Za-z0-9_:]+' (i.e., any word which starts on the\nzeroth column, IIRC). This has lots of issues for e.g. Python where\nthe function name is often indented (e.g. inside a class definition)\nand cpp is compatible with, and an improvement over, the default \"C\"\nsetting (which is particularly broken as it marks any \"goto\" label as\na function start / end).\n\nAll of those points covered, I think we can make this work in a way\nthat preserves backward compatibility (no defaults at all, user must\nmanually configure ~/.gitattributes) by only setting \"smart\" defaults\n(e.g. \"*.cpp diff=cpp\", \"*.cc diff=cpp\" etc) when the command is\nEXPLICITLY invoked with -W/--function-context/--show-function.\n\nAnother case I didn't think of in the original post is with the \"git\nlog -L:funcname:path/to/file.cpp\" option, which tracks changes within\na function over time. Having \"better\" default function boundary\ndetection here would also be useful.\n\nUltimately, I agree we should not have any extra overhead for any\ncommands (log, diff, grep, etc) unless the user EXPLICITLY uses flags\nthat indicate function boundary detection are key to functionality of\nthose flags.\n\nOn Mon, Aug 2, 2021 at 12:06 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> > On Mon, Aug 02, 2021 at 10:45:25AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> >\n> >> I would like to see us have a setting to turn these on by default, but\n> >> think it would be better to make that a diff.* config setting to put\n> >> into ~/.gitconfig, i.e. we'd extend git itself to know about a list of\n> >> extensions for the given userdiff drivers, and use them when rendering\n> >> diffs.\n> >\n> > A long time ago we discussed doing this. The relevant thread is:\n> >\n> > https://lore.kernel.org/git/20111216110000.GA15676@sigill.intra.peff.net/\n> >\n> > which references a few others:\n> >\n> > https://lore.kernel.org/git/4E569F10.8060808@panasas.com/\n> >\n> > https://lore.kernel.org/git/4E6E928A.6080003@sunshineco.com/\n> > ...\n>\n> Thanks for pointers.\n>\n> One good suggestion given there was to use diff=c and diff=perl in\n> our own .gitattributes to use the patterns ourselves, which we seem\n> to have been doing just fine ;-)\n>\n> As long as the default built-in ones are\n>\n> (1) at least 90% of the time improvement over, or at least is not\n> broken compared to, the unconfigured case, and\n>\n> (2) at the lowest priority that users can easily countermand for\n> the rest 10% cases\n>\n> I do not think it is too bad to resurrect the old patches from these\n> threads.\n>\n> Thanks.\n>\n>\n"},{"id":"431823","messageId":"YQlY18DK198JF22E@coredump.intra.peff.net","threadId":"56195","inReplyTo":"CAM1Tk16Duh9ySS1xwW3yK4ogz=HbMp9k5B7bjCGSgqOEx=pdtA@mail.gmail.com","subject":"Re: Enhancing --show-function and --function-context in default configurations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-08-03T14:55:19Z","receivedAt":"2021-08-03T14:55:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 02, 2021 at 09:34:32PM -0500, heapcrash heapcrash wrote:\n\n> Thanks all for the feedback! I'll try to address it below:\n> \n> > Two other places may be diff hunk headers and --diff-words output, I think.\n> \n> I didn't think of those. Does this mean that diffs generated with a\n> given e.g. diff=cpp configuration might not apply cleanly if run on\n> some other user's system without that setting?\n\nNo, because the text in the hunk headers is purely cosmetic. They are\njust for humans, and applying the patch ignores them.\n\nThe word-diffs are likewise just meant for human consumption, and can't\nbe applied at all.\n\n> > It makes emitting the diffs take more CPU, but the same is true of other\n> > options like colorMoved etc, so that in itself is not a dealbreaker.\n> \n> I didn't think of this scenario, that it would add CPU time even\n> without -W/--function-context/--show-function. I'd definitely be fine\n> with preserving the current behavior in these cases (more below).\n\nI think the CPU increase is negligible. Either way, the funcname header\ninvolves walking backwards through the file applying a pattern to see if\na line is a good candidate. The type-specific patterns are a little more\ncomplicated, but I doubt the difference is even measurable in practice.\n\nIf we are going to have defaults for extension-to-diff mapping, I would\nprefer apply them consistently (so for -W, but also for funcnames,\nword-diffs, etc). That is both easier to implement and easier to explain\nto users (and assuming our mapping is accurate, more likely to be what\nthey want).\n\n> Another case I didn't think of in the original post is with the \"git\n> log -L:funcname:path/to/file.cpp\" option, which tracks changes within\n> a function over time. Having \"better\" default function boundary\n> detection here would also be useful.\n\nYes, I think this uses the same diff funcname patterns, so once we have\ndefault mappings, it would start to use them.\n\n-Peff\n"}]}