Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.

From: Jeff King <peff@peff.net>
Date: Apr 28, 2017, 22:04 UTC
Message-ID: <20170428220450.olqitnuwhrxzg3pv@sigill.intra.peff.net>
In-Reply-To: <CAGZ79kbUqVfz+6Y0XkTL7FCZfaD+2YRMZ_v0vP8-DOFhWc+ELw@mail.gmail.com>

On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:

Show 9 quoted lines

> > So instead I chose to make the indentHeuristic option part of diff's basic
> > configuration, and in each of the diff plumbing commands I moved the call to
> > git_config() before the call to init_revisions().
> [...]
> 
> The feature was included in v2.11 (released 2016-11-29) and we got no
> negative feedback. Quite the opposite, all feedback we got, was positive.
> This could be explained by having the feature as an experimental feature
> and users who would be broken by it, did not test it yet or did not speak up.

Yeah, if the point you're trying to make is "nobody will be mad if this is turned on by default", I don't think shipping it as a config option is very conclusive.

I think a more interesting argument is: we turned on renames by default a few versions ago, which changes the diff in a much bigger way, and nobody complained.

  As a side note, I do happen to know of one program that depends
  heavily on diffs remaining stable. Imagine you have a Git hosting site
  which lets people comment on lines of diffs. You need some way to
  address the lines of the diff so that the annotations appear in the
  correct position when you regenerate the diff later.

  One way to do it is to just position the comment at the n'th line of
  the diff. Which obviously breaks if the diff changes. IMHO that is a
  bug in that program, which should be fixed to use the line numbers
  from the original blob (which is still not foolproof, because a
  different diff algorithm may move a change such that the line isn't
  even part of the diff anymore).

  I'm not worried about this particular program, as I happen to know it
  has already been fixed. But it's possible others have made a similar
  mistake.

> So I'd propose to turn it on by default and anyone negatively impacted by that
> could then use the config to turn it off for themselves (including plumbing).
> 
> Something like this, maybe?

Yeah, as long as this is on top of Marc's patches, I think it is the natural conclusion that we had planned.

I don't know if we would want to be extra paranoid about patch-ids. There is no helping:

  git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable

because diff-tree doesn't know that it's trying for "--stable" output. But the diffs we compute internally for patch-id could disable the heuristics. I'm not sure if those matter, though. AFAIK those are used only for internal comparisons within a single program. I.e., we never compare them against input from the user, nor do we output them to the user. So they'll change, but I don't think anybody would care.

-Peff