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

From: Jeff King <peff@peff.net>
Date: Jun 24, 2021, 15:41 UTC
Message-ID: <YNSnlhbE30xDfVMY@coredump.intra.peff.net>
In-Reply-To: <60D45FE4020000A100041FCE@gwsmtp.uni-regensburg.de>

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

Show 6 quoted lines

> 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?