Re: [PATCH] git-add--interactive: Preserve diff heading when splitting hunks

From: Jeff King <peff@peff.net>
Date: May 12, 2014, 20:09 UTC
Message-ID: <20140512200952.GA2329@sigill.intra.peff.net>
In-Reply-To: <CACBZZX4W=1jPGcuBYstQkBDaT_wu38Fhxu642qwaFtCCqBdnyQ@mail.gmail.com>

On Mon, May 12, 2014 at 09:42:56PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 22 quoted lines

> Good suggestion, but tricky as you point out. Another thing I've
> wanted many times is to make it smart enough that when you edit code
> like:
> 
>   A()
>   B();
> 
> And change it to:
> 
>   X();
> 
>   Y();
> 
> The change from A->X and B->Y may be completely unrelated and just
> made in code where the author didn't add whitespace between unrelated
> statements.
> 
> But because you change all the lines the tool can't split them up, it
> could try harder and split hunks like that if you add a whitespace
> boundary, or just go all the way down to adding/removing individual
> lines, so you wouldn't have to fall down to "edit" mode and do so
> manually.

Yeah, I frequently run into this, too, and have just gotten good at editing the patch. :)

I think splitting line-by-line, as you suggest, would be more general. However, working within a single cluster of lines is difficult (both line-by-line and in your whitespace example), because you have to correlate removed and added lines. E.g., the diff for your change above might look like:

  @@ ... @@
   context
   -A();
   -B();
   +X();
   +
   +Y();
   more context

We would want to end up with three hunks:

  -A();
  +X();

  -B();
  +Y();

but how do we know which preimage lines correspond with which postimage lines? You can probably come up with heuristics for this case (e.g., introduced whitespace gets its own hunk, and then count postimage lines from each end), but there are many harder cases.

> > I didn't prepare a commit message because I think it should probably
> > just be squashed in.
> 
> Well spotted, indeed, that should be squashed in.

If you do (or Junio does), I think the commit message needs a slight update.

Show 5 quoted lines

> On a related note I thought by doing color.ui=auto I was turning on
> all the colors, it would be nice if there was a built-in colorscheme
> that added more coloring to items like these across our tools, it's
> useful to have the hunk headers colored differently so they stand out
> more.

I don't set color.diff.func myself, but having just looked at it when playing with your patch, I think it looks nice (I did "yellow").

I also set color.diff.meta to "magenta", which I think makes the hunk header stand out a bit more.

So yeah, I'd be fine with proposing changes to the default color scheme, though I do not envy you the inevitable bikeshed war. :)

-Peff