On Fri, Apr 28, 2017 at 3:04 PM, Jeff King <peff@xxxxxxxx> wrote: > On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote: > >> > 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. Thanks for this enlightenment. :) >> 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. That is what I was trying to hint at with the commit message given. > 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 Well, I think as long as we have a reasonable easy way to undo the default (hence keeping the option), we can proceed with caution here, too. Thanks, Stefan