git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stefan Beller <sbeller@google.com>
Date
Apr 28, 2017, 22:13 UTC
Message-ID
<CAGZ79kZsddmxmMFbD7NQXw=j56WKgaFaz90_zC4Lm++9Lj887w@mail.gmail.com>
In-Reply-To
<20170428220450.olqitnuwhrxzg3pv@sigill.intra.peff.net>
On Fri, Apr 28, 2017 at 3:04 PM, Jeff King <peff@peff.net> wrote:
Show 36 quoted lines
> 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. :)
Show 7 quoted lines
>> 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.
Show 13 quoted lines
> 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

Previous: Jeff KingNext: Ævar Arnfjörð Bjarmason
Message 12 of 35 in “Make diff plumbing commands respect the indentHeuristic.”
  1. 0/2 Make diff plumbing commands respect the indentHeuristic.Marc Branchaud, Apr 27, 2017
  2. 2/2 Have the diff-* builtins configure diff before initializing revisions.Marc Branchaud, Apr 27, 2017
  3. Jeff KingApr 28, 2017
  4. Junio C HamanoMay 1, 2017
  5. Jeff KingMay 1, 2017
  6. Jeff KingMay 1, 2017
  7. 1/2 Make the indent heuristic part of diff's basic configuration.Marc Branchaud, Apr 27, 2017
  8. Jeff KingApr 28, 2017
  9. Jeff KingApr 28, 2017
  10. Stefan BellerApr 28, 2017
  11. Jeff KingApr 28, 2017
  12. Stefan BellerApr 28, 2017
  13. Ævar Arnfjörð BjarmasonMay 1, 2017
  14. Jeff KingMay 9, 2017
  15. Junio C HamanoMay 9, 2017
  16. Ævar Arnfjörð BjarmasonMay 9, 2017
  17. Jeff KingMay 9, 2017
  18. 0/3 Make diff plumbing commands respect the indentHeuristic.Marc Branchaud, Apr 28, 2017
  19. 1/3 diff: make the indent heuristic part of diff's basic configurationMarc Branchaud, Apr 28, 2017
  20. 2/3 diff: have the diff-* builtins configure diff before initializing revisionsMarc Branchaud, Apr 28, 2017
  21. 3/3 diff: enable indent heuristic by defaultMarc Branchaud, Apr 28, 2017
  22. Jeff KingApr 29, 2017
  23. Jeff KingApr 29, 2017
  24. Junio C HamanoMay 1, 2017
  25. Jeff KingMay 1, 2017
  26. Junio C HamanoMay 1, 2017
  27. 0/4 Make diff plumbing commands respect the indentHeuristic.Marc Branchaud, May 1, 2017
  28. 2/4 diff: have the diff-* builtins configure diff before initializing revisionsMarc Branchaud, May 1, 2017
  29. 1/4 diff: make the indent heuristic part of diff's basic configurationMarc Branchaud, May 1, 2017
  30. 4/4 add--interactive: drop diff.indentHeuristic handlingMarc Branchaud, May 1, 2017
  31. 3/4 diff: enable indent heuristic by defaultMarc Branchaud, May 1, 2017
  32. Jeff KingMay 1, 2017
  33. Stefan BellerMay 1, 2017
  34. 4/3 add--interactive: drop diff.indentHeuristic handlingJeff King, Apr 29, 2017
  35. Michael HaggertyApr 30, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.