Re: git add --interactive patch improvement for split hunks

[Jeff King <peff@peff.net>]

On Thu, Jun 24, 2021 at 12:35:16PM +0200, Ulrich Windl wrote:

> I noticed that git add -interactive's patch displays the function
> context for the diffs, but that function context is lost when the
> hunks are split.
>
> It would help the user (especially for hunks covering multiple
> functioins) if function context were still provided for split hunks.

This was discussed a while ago (and there is even a patch) in this
thread:

  https://lore.kernel.org/git/20201117020522.GD19433@coredump.intra.peff.net/

The short of it is that the upcoming builtin-in-C version of the code
will preserve the function header when splitting. The patch in that
message adds it to the existing perl version, but I didn't really bother
moving it forward, since that code is all supposed to eventually go
away[0].

One thing you may not like, though: both the builtin version and that
patch only put the funcname context in the _first_ hunk of the split.
Doing it for subsequent hunks is much trickier, since there can be a
funcname in the split context itself. E.g.:

  @@ ... @@ void foo()
           int x;
  -        int y = 1;
  +        int y = 2;
   
  -        x = 3;
  +        x = 4;
   }

could split into two hunks, both annotated with "void foo()". But:

  @@ ... @@ void foo()
           int x;
  -        x = 3;
  +        x = 4;
   }
   void bar()
   {
  -        int y = 1;
  +        int y = 2;
   }

would be wrong to say "void foo()" for the second hunk. We'd have to
re-scan the interior context lines for a funcname to find it. That's
all-but-impossible in the perl version, but might be do-able in the C
version (since it has easy access to the funcname-matching patterns and
machinery).

-Peff

[0] I'm not sure what the timetable is for switching to the C version of
    add--interactive. If it's going to be a while, I don't mind moving
    forward the other patch I showed. But maybe the time is here to
    think about switching the default of add.interactive.useBuiltin, and
    ironing out any final bugs?