Delta: 🚀 Improve on git's function/class detection

[zachriggle]

The logic in git diff --function-context will always print the preceding function on the top line, since that's "technically" the preceding code. This happens even if -U0 is provided, unfortunately.

Note that here, other_routine is the top line, even though no changes are within other_routine.

int other_routine() {
}

int main() {
    puts("Hello, world!");
    return 0;
}

$ git diff -U0 --function-context
diff --git i/example.c w/example.c
index 346e2a7..0b40a82 100644
--- i/example.c
+++ w/example.c
@@ -6,3 +6,4 @@ int other_routine() {
 int main() {
     puts("Hello, world!");
+    return 0;
 }
\ No newline at end of file

This breaks scripts that rely on the top line to determine which function the changes are in, including delta.

Here we can see that delta boxes the name other_routine, based on this information.

It would be nice if there were a way to pass e.g. --function-context to delta such that it's able to recognize this situation.

[Amorymeltzer]

delta in general doesn't handle -U0 well at all

[dandavison]

Hi @zachriggle and @Amorymeltzer, thanks for opening this issue. I think that I'm failing so far to understand what delta should be doing differently (perhaps because I'm not familiar with --function-context or -U0). So would you be able to help me understand? I've made the images below, and annotated it on the left with my understanding. Hopefully this will help us get on the same page! Basically it's seeming to me that in all cases delta does what's expected -- it boxes the hunk header and displays and highlights correctly all the code that git sends in its standard output. I do not know why git sometimes selects other_routine as the hunk header. I know it's possible to change git's behavior by configuring the xfuncname regex (see e.g. here), but I don't know whether that leads to a fix for git's selection of other_routine here. Are you suggesting that delta should override git's choice of hunk header?

delta in general doesn't handle -U0 well at all

@Amorymeltzer can you expand? :) As you can see I'm a bit lost!

[Amorymeltzer]

@dandavison As far as -U0 goes, have you tried it on something other than a test file? When I run it on something meaningful, all of the hunk headers are displayed one right after another with no code, then all of the code is displayed. Try this for the delta repo: git diff -U0 b2257cf~..b2257cf

[dandavison]

@Amorymeltzer wow thanks, well that's certainly a bug! I opened https://github.com/dandavison/delta/issues/263 Hopefully something easy to fix in the main diff parsing/line-emitting procedure.

OK, so what about --function-context?

[Amorymeltzer]

I'd sooner have @zachriggle comment as I'm not overly familiar with it (and my intent was not to hijack this), but from quickly playing around, it seems that delta is using the first line of context rather than the function itself. To use the shas I gave above, git diff b2257cf~ b2257cf src/config.rs shows hunk headers for pub struct Config { and impl From<cli::Opt> for Config { whereas git diff b2257cf~ b2257cf src/config.rs uses use crate::style::Style; and impl Config {

[zachriggle]

The only reason I used -U0 was to show that the function name is still wrong, even if zero lines of the preceding function is printed in the diff. This still happens at all -U levels.

[dandavison]

it seems that delta is using the first line of context rather than the function itself.

Delta uses the part of the git/unified diff output that is marked with @@. Git documentation refers to this as the "hunk header" and "frag" (see here and here) and delta also uses that terminology in its CLI (hunk-header-style, hunk-header-decoration-style). I believe it's technically incorrect to refer to that as the first line of context: it's not part of the context -- it's typically determined by indentation level and may in fact occur in the code many lines prior to the start of the context.

the function name is still wrong, even if zero lines of the preceding function is printed in the diff.

OK, but it was git that made this choice, not delta; delta takes git's output and correctly puts the hunk header in a box. E.g. taking the third row of the table above, the hunk header identified by git is int other_routine() {:

Does that sound right so far? If so, then is what we're contemplating here that what delta puts in a box is no longer necessarily the hunk header selected by git but possibly something that delta has identified as preferable?

[Amorymeltzer]

Thanks — that seems an accurate summary. I (clearly!) don't know --function-context well (and this isn't my issue) but I would imagine that would be rather undesirable long-term.

[dandavison]

@zachriggle do you feel that there's something Delta could be doing better, or do you think that this is an aspect of Git's behavior that Delta should just accept and display? To recap what I was saying above:

Note that here, other_routine is the top line, even though no changes are within other_routine.

I believe that what you're referring to as the "top line" is what I'm calling the "hunk header" or "frag" (and I'm saying that it's not part of the context). Accordingly, this

This breaks scripts that rely on the top line to determine which function the changes are in, including delta.

isn't the way I think about it: I would say that the hunk header (top line) is the _definition_ of which function/class/code context the changes are in.

Does this make sense? Sorry if not; it's definitely possible that I'm missing something! In particular, I do not understand at all why Git is choosing other_routine { for the hunk header instead of int main() { (and I haven't yet gone and looked at the regexes and code that Git is using to do that, which is probably what I should do).

[zachriggle]

@dandavison Right now, this is a limitation of Git's implementation. I also think that this is something that Delta could parse out the first function it finds, if given a special flag. I don't expect the default behavior of delta to change, but rather something extra it could do if given e.g. --dont-trust-git-function-detection or something.

A good stop-gap measure would be to have a flag to simply disable the boxed function name, via a flag.

[dandavison]

@zachriggle thanks for bearing with me here.

I don't expect the default behavior of delta to change, but rather something extra it could do if given e.g. --dont-trust-git-function-detection or something.

Great, that makes sense and is interesting. I've had the same thought. I'll keep this open to record that feature idea.

A good stop-gap measure would be to have a flag to simply disable the boxed function name, via a flag.

Unless I misunderstood, this is already available:

[delta]
    hunk-header-style = omit

--hunk-header-style <hunk-header-style>
    Style (foreground, background, attributes) for the hunk-header. See STYLES section. The style 'omit' can be
    used to remove the hunk header section from the output [default: syntax]