Re: Enabling the diff "indent" heuristic by default

From: Jeff King <peff@peff.net>
Date: May 9, 2017, 03:20 UTC
Message-ID: <20170509032034.zbdeu2xsawcm32xb@sigill.intra.peff.net>
In-Reply-To: <f867af6f-b601-251a-86a4-ede0bb942efb@xiplink.com>

On Mon, May 08, 2017 at 10:54:10AM -0400, Marc Branchaud wrote:

Show 15 quoted lines

> On 2017-05-08 03:48 AM, Junio C Hamano wrote:
> > 
> > * mb/diff-default-to-indent-heuristics (2017-05-02) 4 commits
> >   (merged to 'next' on 2017-05-08 at 158f401a92)
> 
> I think there's a general open question about this, which is whether or not
> we should just drop the diff.indentHeuristic configuration setting
> altogether.
> 
> Peff made the point [0] that if we keep the setting then t4061 should be
> rewritten.
> 
> My instinct is to keep the setting, at least until the changed default has a
> bit of time to settle in.  So I'll re-send the topic with the renovated
> t4061.

My instinct matches that, too. It gives people like Ævar, with his patch-id database, a way to keep compatibility if he chooses. If we were designing from the ground up, I'd say the option is probably just clutter, but the backwards compatibility issue means we should probably keep it around more or less forever.

And since Junio wasn't on the other thread, I'll repeat what I wrote there:

  I do feel a bit sad about breaking this case (or at the very least
  forcing you to set an option to retain cross-version compatibility).
  But my gut says that we don't want to lock ourselves into never
  changing the diff algorithm (and I'm sure we've done it inadvertently
  a few times over the years; even the recent switch to turning on
  renames would have had that impact).

Show 9 quoted lines

> Both Peff [1] and Ævar [2] mentioned situations where enabling the heuristic
> has a small impact on them.  If/when this graduates, it's perhaps worth
> adding a backward-compatibility note that the default patch IDs are
> changing.  Maybe something like:
> 
> The diff "indent" heuristic is now enabled by default.  This changes the
> patch IDs calculated by git-patch-id and used by git-cherry, which could
> affect patch-based workflows that rely on previously-computed patch IDs.
> The heuristic can be disabled by setting diff.indentHeuristic to false.

I think a note like this is a good idea.

-Peff