{"thread":{"id":"45821","subject":"[PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.","startedAt":"2017-04-27T20:59:43Z","lastAt":"2017-05-09T08:04:45Z","messageCount":35,"participants":["Marc Branchaud","Jeff King","Stefan Beller","Michael Haggerty","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"318091","messageId":"20170427205037.1787-3-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170427205037.1787-1-marcnarc@xiplink.com","subject":"[PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-27T20:50:37Z","receivedAt":"2017-04-27T20:59:43Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"This makes the commands respect diff configuration options, such as\nindentHeuristic.\n\nSigned-off-by: Marc Branchaud \n---\n builtin/diff-files.c | 2 +-\n builtin/diff-index.c | 2 +-\n builtin/diff-tree.c | 2 +-\n 3 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 15c61fd8d..a572da9d5 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tint result;\n \tunsigned options = 0;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 1af373d00..f084826a2 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint result;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 326f88b65..36a3a1976 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct setup_revision_opt s_r_opt;\n \tint read_stdin = 0;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\n-- \n2.13.0.rc1.15.g7dbea34e1.dirty\n\n"},{"id":"318092","messageId":"20170427205037.1787-2-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170427205037.1787-1-marcnarc@xiplink.com","subject":"[PATCH 1/2] Make the indent heuristic part of diff's basic configuration.","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-27T20:50:36Z","receivedAt":"2017-04-27T20:59:53Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"Signed-off-by: Marc Branchaud \n---\n diff.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 11eef1c85..da96577ea 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (git_diff_heuristic_config(var, value, cb) < 0)\n-\t\treturn -1;\n-\n \tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n \t\tint val = parse_ws_error_highlight(value);\n \t\tif (val < 0)\n@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \tif (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n+\tif (git_diff_heuristic_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n \treturn git_default_config(var, value, cb);\n }\n \n-- \n2.13.0.rc1.15.g7dbea34e1.dirty\n\n"},{"id":"318093","messageId":"20170427205037.1787-1-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"19607a03-71e0-440b-7213-64d25f6fa8da@xiplink.com","subject":"[PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-27T20:50:35Z","receivedAt":"2017-04-27T20:59:59Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"So here's my attempt at fixing this.\n\nThe thing I was missing is that init_revisions() calls diff_setup(), which\nsets the xdl options. It's therefore necessary to have the\ndiff_indent_heuristic flag set before calling init_revisions().\n\nA naive way to get the indentHeuristic config option respected in the\ndiff-* plumbing commands is to make them use the git_diff_heuristic_config()\ncallback right at the start of their main cmd functions.\n\nBut I did not like that for two reasons:\n\n* It would make these commands invoke git_config() twice.\n\n* It doesn't avoid the problem if/when someone creates a new diff-something\n plumbing command, and forgets to set the diff_indent_heuristic flag before\n calling init_revisions().\n\nSo instead I chose to make the indentHeuristic option part of diff's basic\nconfiguration, and in each of the diff plumbing commands I moved the call to\ngit_config() before the call to init_revisions().\n\nThis still doesn't really future-proof things for possible new diff plumbing\ncommands, because someone could still invoke init_revisions() before setting\nup diff's basic configuration. But I don't see an obvious way of ensuring\nthat the diff_indent_heuristic flag is respected regardless of when\ndiff_setup() is invoked.\n\n\t\tM.\n\nMarc Branchaud (2):\n Make the indent heuristic part of diff's basic configuration.\n Have the diff-* builtins configure diff before initializing revisions.\n\n builtin/diff-files.c | 2 +-\n builtin/diff-index.c | 2 +-\n builtin/diff-tree.c | 2 +-\n diff.c | 6 +++---\n 4 files changed, 6 insertions(+), 6 deletions(-)\n"},{"id":"318121","messageId":"20170428075635.x5rqcvgv6tohj74n@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170427205037.1787-1-marcnarc@xiplink.com","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-28T07:56:36Z","receivedAt":"2017-04-28T07:56:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 27, 2017 at 04:50:35PM -0400, Marc Branchaud wrote:\n\n> So here's my attempt at fixing this.\n> \n> The thing I was missing is that init_revisions() calls diff_setup(), which\n> sets the xdl options. It's therefore necessary to have the\n> diff_indent_heuristic flag set before calling init_revisions().\n> \n> A naive way to get the indentHeuristic config option respected in the\n> diff-* plumbing commands is to make them use the git_diff_heuristic_config()\n> callback right at the start of their main cmd functions.\n> \n> But I did not like that for two reasons:\n> \n> * It would make these commands invoke git_config() twice.\n> \n> * It doesn't avoid the problem if/when someone creates a new diff-something\n> plumbing command, and forgets to set the diff_indent_heuristic flag before\n> calling init_revisions().\n\nYeah, I think that would be the wrong way to go. Either this option is\ndiff_basic_config() or it is not. And if it is not, the plumbing commands\nhave no business checking it.\n\nThinking on it more, I think it probably should be basic config. We\ndiscussed all along making these options the default, which shows that\nplumbing interfaces do not need to be protected from them.\n\n> So instead I chose to make the indentHeuristic option part of diff's basic\n> configuration, and in each of the diff plumbing commands I moved the call to\n> git_config() before the call to init_revisions().\n\nYes, I think that's the right thing to do.\n\n> This still doesn't really future-proof things for possible new diff plumbing\n> commands, because someone could still invoke init_revisions() before setting\n> up diff's basic configuration. But I don't see an obvious way of ensuring\n> that the diff_indent_heuristic flag is respected regardless of when\n> diff_setup() is invoked.\n\nI think the config-based stuff would have to be bumped down to\nsetup_revisions(), leaving init_revisions() as a initialization\nfunction. But that would break new cases, as some callers would\nwant to do:\n\n init_revisions(&revs);\n revs.foo = 1; /* override any config */\n setup_revisions(argc, argv, &revs); /* let command-line override us */\n\nSo you'd really need three phases:\n\n init_revisions(&revs); /* baked-in defaults */\n setup_revision_config(&revs);\n setup_revision_args(argc, argv, &revs);\n\nand you could override each set of defaults in between the calls. But\nthat would require tweaking each caller. In practice, I think the rule\n\"set up your config before calling init_revisions\" is probably OK, now\nthat we know about it.\n\nI think there is existing breakage in any case where a diff_basic_config\noption is handled in diff_setup(). The only one I see is the dirstat\nstuff. So I think:\n\n git config diff.dirstat changes,5\n git rev-list HEAD | git diff-tree --stdin --dirstat\n\nwas supposed to respect that \"5\" but doesn't. I didn't test, though.\n\n-Peff\n"},{"id":"318122","messageId":"20170428075932.vnngf3pbl4oh2fta@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170427205037.1787-2-marcnarc@xiplink.com","subject":"Re: [PATCH 1/2] Make the indent heuristic part of diff's basic configuration.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-28T07:59:32Z","receivedAt":"2017-04-28T07:59:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 27, 2017 at 04:50:36PM -0400, Marc Branchaud wrote:\n\n> Subject: [PATCH 1/2] Make the indent heuristic part of diff's basic configuration.\n>\n> Signed-off-by: Marc Branchaud \n\nThis needs to be explained better. Why were the options originally in\nUI config, and why is it OK to move them to the basic config?\n\nI think the argument is along the lines of \"scripts shouldn't care\nbecause the result is syntactically identical, and we reserve the right\nto find any valid diff\".\n\nAlso, our subject lines usually do not start with a capital, nor end\nwith a period. We usually try to specify the subsystem with a colon.\nLike:\n\n diff: move indent heuristic to \"basic\" config\n\nor similar.\n\n-Peff\n"},{"id":"318123","messageId":"20170428080620.cbnreh5invwpasxr@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170427205037.1787-3-marcnarc@xiplink.com","subject":"Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-28T08:06:20Z","receivedAt":"2017-04-28T08:06:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:\n\n> Subject: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.\n>\n> This makes the commands respect diff configuration options, such as\n> indentHeuristic.\n> \n> Signed-off-by: Marc Branchaud \n\nI think it would be helpful to future readers to explain what is going\non here. I.e., the bit about calling diff_setup_done(), which copies the\nglobals into the diff struct.\n\nThe same comments about the subject line from the last patch apply here,\ntoo.\n\n> builtin/diff-files.c | 2 +-\n> builtin/diff-index.c | 2 +-\n> builtin/diff-tree.c | 2 +-\n> 3 files changed, 3 insertions(+), 3 deletions(-)\n\nIt would be nice to have a test. Testing that dirstat's permille option\nhas an effect might be the easiest way to do so.\n\n> diff --git a/builtin/diff-files.c b/builtin/diff-files.c\n> index 15c61fd8d..a572da9d5 100644\n> --- a/builtin/diff-files.c\n> +++ b/builtin/diff-files.c\n> @@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n> \tint result;\n> \tunsigned options = 0;\n> \n> +\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> \tinit_revisions(&rev, prefix);\n> \tgitmodules_config();\n> -\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> \trev.abbrev = 0;\n> \tprecompose_argv(argc, argv);\n\nIt's somewhat odd to me that diff_files uses a rev_info struct at all.\nIt doesn't traverse at all, and doesn't respect most of the options.\nThere's a half-hearted attempt to reject some obviously bogus cases, but\nmost of the options are just silently ignored.\n\nI think it's mostly a historical wart (especially around the fact that\nsome diff options like combine_merges are in rev_info, which they\nprobably should not be). Anyway, none of that is your problem, and is\nway outside the scope of this patch.\n\n-Peff\n"},{"id":"318191","messageId":"CAGZ79kbUqVfz+6Y0XkTL7FCZfaD+2YRMZ_v0vP8-DOFhWc+ELw@mail.gmail.com","threadId":"45821","inReplyTo":"20170427205037.1787-1-marcnarc@xiplink.com","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-28T17:34:15Z","receivedAt":"2017-04-28T17:34:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 27, 2017 at 1:50 PM, Marc Branchaud wrote:\n> So here's my attempt at fixing this.\n>\n> The thing I was missing is that init_revisions() calls diff_setup(), which\n> sets the xdl options. It's therefore necessary to have the\n> diff_indent_heuristic flag set before calling init_revisions().\n>\n> A naive way to get the indentHeuristic config option respected in the\n> diff-* plumbing commands is to make them use the git_diff_heuristic_config()\n> callback right at the start of their main cmd functions.\n>\n> But I did not like that for two reasons:\n>\n> * It would make these commands invoke git_config() twice.\n>\n> * It doesn't avoid the problem if/when someone creates a new diff-something\n> plumbing command, and forgets to set the diff_indent_heuristic flag before\n> calling init_revisions().\n>\n> So instead I chose to make the indentHeuristic option part of diff's basic\n> configuration, and in each of the diff plumbing commands I moved the call to\n> git_config() before the call to init_revisions().\n>\n> This still doesn't really future-proof things for possible new diff plumbing\n> commands, because someone could still invoke init_revisions() before setting\n> up diff's basic configuration. But I don't see an obvious way of ensuring\n> that the diff_indent_heuristic flag is respected regardless of when\n> diff_setup() is invoked.\n>\n\nThe feature was included in v2.11 (released 2016-11-29) and we got no\nnegative feedback. Quite the opposite, all feedback we got, was positive.\nThis could be explained by having the feature as an experimental feature\nand users who would be broken by it, did not test it yet or did not speak up.\n\nSo I'd propose to turn it on by default and anyone negatively impacted by that\ncould then use the config to turn it off for themselves (including plumbing).\n\nSomething like this, maybe?\n\n---8<---\nFrom 58d9a1ef135eff9f85b165986e4bc13479914f8e Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller \nDate: Thu, 27 Apr 2017 14:26:59 -0700\nSubject: [PATCH] diff.c: enable indent heuristic by default\n\nThe feature was included in v2.11 (released 2016-11-29) and we got no\nnegative feedback. Quite the opposite, all feedback we got was positive.\n\nTurn it on by default. users who dislike the feature can turn it off\nusing by setting diff.compactionHeuristic (which includes plumbing\ncommands, see prior patches)\n\nSigned-off-by: Stefan Beller \n---\ndiff --git a/diff.c b/diff.c\nindex 11eef1c85d..7f301c1b62 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -27,7 +27,7 @@\n #endif\n\n static int diff_detect_rename_default;\n-static int diff_indent_heuristic; /* experimental */\n+static int diff_indent_heuristic = 1;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n"},{"id":"318212","messageId":"20170428220450.olqitnuwhrxzg3pv@sigill.intra.peff.net","threadId":"45821","inReplyTo":"CAGZ79kbUqVfz+6Y0XkTL7FCZfaD+2YRMZ_v0vP8-DOFhWc+ELw@mail.gmail.com","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-28T22:04:50Z","receivedAt":"2017-04-28T22:07:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:\n\n> > So instead I chose to make the indentHeuristic option part of diff's basic\n> > configuration, and in each of the diff plumbing commands I moved the call to\n> > git_config() before the call to init_revisions().\n> [...]\n> \n> The feature was included in v2.11 (released 2016-11-29) and we got no\n> negative feedback. Quite the opposite, all feedback we got, was positive.\n> This could be explained by having the feature as an experimental feature\n> and users who would be broken by it, did not test it yet or did not speak up.\n\nYeah, if the point you're trying to make is \"nobody will be mad if this\nis turned on by default\", I don't think shipping it as a config option\nis very conclusive.\n\nI think a more interesting argument is: we turned on renames by default\na few versions ago, which changes the diff in a much bigger way, and\nnobody complained.\n\n As a side note, I do happen to know of one program that depends\n heavily on diffs remaining stable. Imagine you have a Git hosting site\n which lets people comment on lines of diffs. You need some way to\n address the lines of the diff so that the annotations appear in the\n correct position when you regenerate the diff later.\n\n One way to do it is to just position the comment at the n'th line of\n the diff. Which obviously breaks if the diff changes. IMHO that is a\n bug in that program, which should be fixed to use the line numbers\n from the original blob (which is still not foolproof, because a\n different diff algorithm may move a change such that the line isn't\n even part of the diff anymore).\n\n I'm not worried about this particular program, as I happen to know it\n has already been fixed. But it's possible others have made a similar\n mistake.\n\n> So I'd propose to turn it on by default and anyone negatively impacted by that\n> could then use the config to turn it off for themselves (including plumbing).\n> \n> Something like this, maybe?\n\nYeah, as long as this is on top of Marc's patches, I think it is the\nnatural conclusion that we had planned.\n\nI don't know if we would want to be extra paranoid about patch-ids.\nThere is no helping:\n\n git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable\n\nbecause diff-tree doesn't know that it's trying for \"--stable\" output.\nBut the diffs we compute internally for patch-id could disable the\nheuristics. I'm not sure if those matter, though. AFAIK those are used\nonly for internal comparisons within a single program. I.e., we never\ncompare them against input from the user, nor do we output them to the\nuser. So they'll change, but I don't think anybody would care.\n\n-Peff\n"},{"id":"318213","messageId":"CAGZ79kZsddmxmMFbD7NQXw=j56WKgaFaz90_zC4Lm++9Lj887w@mail.gmail.com","threadId":"45821","inReplyTo":"20170428220450.olqitnuwhrxzg3pv@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-28T22:13:36Z","receivedAt":"2017-04-28T22:15:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 28, 2017 at 3:04 PM, Jeff King wrote:\n> On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:\n>\n>> > So instead I chose to make the indentHeuristic option part of diff's basic\n>> > configuration, and in each of the diff plumbing commands I moved the call to\n>> > git_config() before the call to init_revisions().\n>> [...]\n>>\n>> The feature was included in v2.11 (released 2016-11-29) and we got no\n>> negative feedback. Quite the opposite, all feedback we got, was positive.\n>> This could be explained by having the feature as an experimental feature\n>> and users who would be broken by it, did not test it yet or did not speak up.\n>\n> Yeah, if the point you're trying to make is \"nobody will be mad if this\n> is turned on by default\", I don't think shipping it as a config option\n> is very conclusive.\n>\n> I think a more interesting argument is: we turned on renames by default\n> a few versions ago, which changes the diff in a much bigger way, and\n> nobody complained.\n>\n> As a side note, I do happen to know of one program that depends\n> heavily on diffs remaining stable. Imagine you have a Git hosting site\n> which lets people comment on lines of diffs. You need some way to\n> address the lines of the diff so that the annotations appear in the\n> correct position when you regenerate the diff later.\n>\n> One way to do it is to just position the comment at the n'th line of\n> the diff. Which obviously breaks if the diff changes. IMHO that is a\n> bug in that program, which should be fixed to use the line numbers\n> from the original blob (which is still not foolproof, because a\n> different diff algorithm may move a change such that the line isn't\n> even part of the diff anymore).\n>\n> I'm not worried about this particular program, as I happen to know it\n> has already been fixed. But it's possible others have made a similar\n> mistake.\n\nThanks for this enlightenment. :)\n\n>> So I'd propose to turn it on by default and anyone negatively impacted by that\n>> could then use the config to turn it off for themselves (including plumbing).\n>>\n>> Something like this, maybe?\n>\n> Yeah, as long as this is on top of Marc's patches, I think it is the\n> natural conclusion that we had planned.\n\nThat is what I was trying to hint at with the commit message given.\n\n> I don't know if we would want to be extra paranoid about patch-ids.\n> There is no helping:\n>\n> git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable\n>\n> because diff-tree doesn't know that it's trying for \"--stable\" output.\n> But the diffs we compute internally for patch-id could disable the\n> heuristics. I'm not sure if those matter, though. AFAIK those are used\n> only for internal comparisons within a single program. I.e., we never\n> compare them against input from the user, nor do we output them to the\n> user. So they'll change, but I don't think anybody would care.\n>\n> -Peff\n\nWell, I think as long as we have a reasonable easy way to undo\nthe default (hence keeping the option), we can proceed with caution\nhere, too.\n\nThanks,\nStefan\n"},{"id":"318214","messageId":"20170428223315.17140-1-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170427205037.1787-1-marcnarc@xiplink.com","subject":"[PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-28T22:33:12Z","receivedAt":"2017-04-28T22:33:22Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"v2: Fixed up the commit messages and added tests.\n\nMarc Branchaud (2):\n diff: make the indent heuristic part of diff's basic configuration\n diff: have the diff-* builtins configure diff before initializing\n revisions\n\nStefan Beller (1):\n diff: enable indent heuristic by default\n\n builtin/diff-files.c | 2 +-\n builtin/diff-index.c | 2 +-\n builtin/diff-tree.c | 2 +-\n diff.c | 8 +++---\n t/t4061-diff-indent.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 5 files changed, 73 insertions(+), 7 deletions(-)\n\n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318215","messageId":"20170428223315.17140-2-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170428223315.17140-1-marcnarc@xiplink.com","subject":"[PATCHv2 1/3] diff: make the indent heuristic part of diff's basic configuration","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-28T22:33:13Z","receivedAt":"2017-04-28T22:33:28Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"This heuristic was originally introduced as an experimental feature,\nand therefore part of the UI configuration.\n\nBut the user often sees diffs generated by plumbing commands like\ndiff-tree. Moving the indent heuristic into diff's basic configuration\nprepares the way for diff plumbing commands to respect the setting.\n\nThe heuristic itself merely makes the diffs more aesthetically\npleasing, without changing their correctness. Scripts that rely on\nthe diff plumbing commands should not care whether or not the heuristic\nis employed.\n\nSigned-off-by: Marc Branchaud \n---\n diff.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 11eef1c85..da96577ea 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (git_diff_heuristic_config(var, value, cb) < 0)\n-\t\treturn -1;\n-\n \tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n \t\tint val = parse_ws_error_highlight(value);\n \t\tif (val < 0)\n@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \tif (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n+\tif (git_diff_heuristic_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n \treturn git_default_config(var, value, cb);\n }\n \n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318216","messageId":"20170428223315.17140-3-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170428223315.17140-1-marcnarc@xiplink.com","subject":"[PATCHv2 2/3] diff: have the diff-* builtins configure diff before initializing revisions","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-28T22:33:14Z","receivedAt":"2017-04-28T22:33:29Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"This makes the commands respect diff configuration options, such as\nindentHeuristic, because init_revisions() calls diff_setup() which fills\nin the diff_options struct.\n\nSigned-off-by: Marc Branchaud \n---\n builtin/diff-files.c | 2 +-\n builtin/diff-index.c | 2 +-\n builtin/diff-tree.c | 2 +-\n t/t4061-diff-indent.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 69 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 15c61fd8d..a572da9d5 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tint result;\n \tunsigned options = 0;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 1af373d00..f084826a2 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint result;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 326f88b65..36a3a1976 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct setup_revision_opt s_r_opt;\n \tint read_stdin = 0;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\ndiff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\nindex 556450609..13d3dc96a 100755\n--- a/t/t4061-diff-indent.sh\n+++ b/t/t4061-diff-indent.sh\n@@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' '\n \tcompare_blame spaces-expect out-blame2\n '\n \n+test_expect_success 'diff-tree: nice spaces with --indent-heuristic' '\n+\tgit diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted &&\n+\tcompare_diff spaces-compacted-expect out-diff-tree-compacted\n+'\n+\n+test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' '\n+\tgit -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 &&\n+\tcompare_diff spaces-compacted-expect out-diff-tree-compacted2\n+'\n+\n+test_expect_success 'diff-tree: --no-indent-heuristic overrides config' '\n+\tgit -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree &&\n+\tcompare_diff spaces-expect out-diff-tree\n+'\n+\n+test_expect_success 'diff-index: nice spaces with --indent-heuristic' '\n+\tgit checkout -B diff-index &&\n+\tgit reset --soft HEAD~ &&\n+\tgit diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted &&\n+\tcompare_diff spaces-compacted-expect out-diff-index-compacted &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' '\n+\tgit checkout -B diff-index &&\n+\tgit reset --soft HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 &&\n+\tcompare_diff spaces-compacted-expect out-diff-index-compacted2 &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-index: --no-indent-heuristic overrides config' '\n+\tgit checkout -B diff-index &&\n+\tgit reset --soft HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index &&\n+\tcompare_diff spaces-expect out-diff-index &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '\n+\tgit checkout -B diff-files &&\n+\tgit reset HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw &&\n+\tgrep -v index out-diff-files-raw >out-diff-files-compacted &&\n+\tcompare_diff spaces-compacted-expect out-diff-files-compacted &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '\n+\tgit checkout -B diff-files &&\n+\tgit reset HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw2 &&\n+\tgrep -v index out-diff-files-raw2 >out-diff-files-compacted2 &&\n+\tcompare_diff spaces-compacted-expect out-diff-files-compacted2 &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-files: --no-indent-heuristic overrides config' '\n+\tgit checkout -B diff-files &&\n+\tgit reset HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-files --no-indent-heuristic -p spaces.txt >out-diff-files-raw3 &&\n+\tgrep -v index out-diff-files-raw3 >out-diff-files &&\n+\tcompare_diff spaces-expect out-diff-files &&\n+\tgit checkout -f master\n+'\n+\n test_done\n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318217","messageId":"20170428223315.17140-4-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170428223315.17140-1-marcnarc@xiplink.com","subject":"[PATCH 3/3] diff: enable indent heuristic by default","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-04-28T22:33:15Z","receivedAt":"2017-04-28T22:33:30Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"From: Stefan Beller \n\nThe feature was included in v2.11 (released 2016-11-29) and we got no\nnegative feedback. Quite the opposite, all feedback we got was positive.\n\nTurn it on by default. Users who dislike the feature can turn it off\nby setting diff.compactionHeuristic (which includes plumbing commands,\nsee prior patches).\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Marc Branchaud \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex da96577ea..2c47ccb4a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -27,7 +27,7 @@\n #endif\n \n static int diff_detect_rename_default;\n-static int diff_indent_heuristic; /* experimental */\n+static int diff_indent_heuristic = 1;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318236","messageId":"20170429124052.yhgwofbbd5pkd24p@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170428223315.17140-1-marcnarc@xiplink.com","subject":"Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-29T12:40:52Z","receivedAt":"2017-04-29T12:41:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:\n\n> v2: Fixed up the commit messages and added tests.\n> \n> Marc Branchaud (2):\n> diff: make the indent heuristic part of diff's basic configuration\n> diff: have the diff-* builtins configure diff before initializing\n> revisions\n> \n> Stefan Beller (1):\n> diff: enable indent heuristic by default\n\nThanks, these look fine to me. I'd like to get an ACK from Michael, in\ncase he had some other reason for omitting them from git_diff_ui_config\n(from my recollection, it's probably just a mix of conservatism and\nfollowing what the compaction heuristic had done).\n\n-Peff\n"},{"id":"318237","messageId":"20170429131439.ohgren3i7xr4tjex@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170429124052.yhgwofbbd5pkd24p@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-29T13:14:40Z","receivedAt":"2017-04-29T13:14:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:\n\n> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:\n> \n> > v2: Fixed up the commit messages and added tests.\n> > \n> > Marc Branchaud (2):\n> > diff: make the indent heuristic part of diff's basic configuration\n> > diff: have the diff-* builtins configure diff before initializing\n> > revisions\n> > \n> > Stefan Beller (1):\n> > diff: enable indent heuristic by default\n> \n> Thanks, these look fine to me. I'd like to get an ACK from Michael, in\n> case he had some other reason for omitting them from git_diff_ui_config\n> (from my recollection, it's probably just a mix of conservatism and\n> following what the compaction heuristic had done).\n\nSorry, I spoke too soon. The third one needs a few test adjustments\nsquashed in to pass the tests.\n\nThe fixes below for t4061 are pretty obvious, but the one in t4051 is\nanything but. What happens is that we have a function:\n\n int dummy();\n {\n some body\n }\n\nand modify it to look like:\n\n int dummy();\n int dummy();\n {\n some body\n }\n\nand we expect that this counts as making a change to the function for\nthe purposes of -W. But with the indent heuristic, instead of saying \"we\nadded an extra line after the first dummy()\", we say \"we added an extra\ndummy() before the function\". Which is perfectly reasonable.\n\nI don't think duplicating the line is important to the test, as opposed\nto making any random change. But we can't just change a line in the\nbody itself, because the point of the test is making sure -W shows the\nwhole function. And the function is short enough that a change in the\nbody would show the whole thing via context anyway. So I opted to make\nthe change at the same spot, but make the diff less ambiguous.\n\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 6154acb45..5f46c0341 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -72,7 +72,7 @@ test_expect_success 'setup' '\n \n \t# overlap function context of 1st change and -u context of 2nd change\n \tgrep -v \"delete me from hello\" <\"$dir/hello.c\" >file.c &&\n-\tsed 2p <\"$dir/dummy.c\" >>file.c &&\n+\tsed \"2aextra line\" <\"$dir/dummy.c\" >>file.c &&\n \tcommit_and_tag changed_hello_dummy file.c &&\n \n \tgit checkout initial &&\ndiff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\nindex 13d3dc96a..56d7d7760 100755\n--- a/t/t4061-diff-indent.sh\n+++ b/t/t4061-diff-indent.sh\n@@ -153,7 +153,7 @@ test_expect_success 'prepare' '\n '\n \n test_expect_success 'diff: ugly spaces' '\n-\tgit diff old new -- spaces.txt >out &&\n+\tgit diff --no-indent-heuristic old new -- spaces.txt >out &&\n \tcompare_diff spaces-expect out\n '\n \n@@ -183,7 +183,7 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '\n '\n \n test_expect_success 'diff: ugly functions' '\n-\tgit diff old new -- functions.c >out &&\n+\tgit diff --no-indent-heuristic old new -- functions.c >out &&\n \tcompare_diff functions-expect out\n '\n \n@@ -193,7 +193,7 @@ test_expect_success 'diff: nice functions with --indent-heuristic' '\n '\n \n test_expect_success 'blame: ugly spaces' '\n-\tgit blame old..new -- spaces.txt >out-blame &&\n+\tgit blame --no-indent-heuristic old..new -- spaces.txt >out-blame &&\n \tcompare_blame spaces-expect out-blame\n '\n \n"},{"id":"318238","messageId":"20170429131515.6xthmwdfjsw53nnd@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170429124052.yhgwofbbd5pkd24p@sigill.intra.peff.net","subject":"[PATCH 4/3] add--interactive: drop diff.indentHeuristic handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-29T13:15:16Z","receivedAt":"2017-04-29T13:15:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:\n\n> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:\n> \n> > v2: Fixed up the commit messages and added tests.\n> > \n> > Marc Branchaud (2):\n> > diff: make the indent heuristic part of diff's basic configuration\n> > diff: have the diff-* builtins configure diff before initializing\n> > revisions\n> > \n> > Stefan Beller (1):\n> > diff: enable indent heuristic by default\n> \n> Thanks, these look fine to me. I'd like to get an ACK from Michael, in\n> case he had some other reason for omitting them from git_diff_ui_config\n> (from my recollection, it's probably just a mix of conservatism and\n> following what the compaction heuristic had done).\n\nWe can also do this simplification on top (once the other test problems\nare fixed, of course).\n\n-- >8 --\nSubject: add--interactive: drop diff.indentHeuristic handling\n\nNow that diff.indentHeuristic is handled automatically by the plumbing\ncommands, there's no need to propagate it manually.\n\nSigned-off-by: Jeff King \n---\nI actually wonder if diff.algorithm should go into git_diff_basic_config\nby the same rationale as the rest of this series.\n\n git-add--interactive.perl | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 709a5f6ce..79d675b5b 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -46,7 +46,6 @@ my ($diff_new_color) =\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $diff_algorithm = $repo->config('diff.algorithm');\n-my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');\n my $diff_filter = $repo->config('interactive.difffilter');\n \n my $use_readkey = 0;\n@@ -730,9 +729,6 @@ sub parse_diff {\n \tif (defined $diff_algorithm) {\n \t\tsplice @diff_cmd, 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n \t}\n-\tif ($diff_indent_heuristic) {\n-\t\tsplice @diff_cmd, 1, 0, \"--indent-heuristic\";\n-\t}\n \tif (defined $patch_mode_revision) {\n \t\tpush @diff_cmd, get_diff_reference($patch_mode_revision);\n \t}\n-- \n2.13.0.rc1.407.g644207685\n\n"},{"id":"318252","messageId":"a0d14b53-01c2-c653-15c9-ff566d24a782@alum.mit.edu","threadId":"45821","inReplyTo":"20170429124052.yhgwofbbd5pkd24p@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-30T03:26:41Z","receivedAt":"2017-04-30T03:29:46Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/29/2017 02:40 PM, Jeff King wrote:\n> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:\n> \n>> v2: Fixed up the commit messages and added tests.\n>>\n>> Marc Branchaud (2):\n>> diff: make the indent heuristic part of diff's basic configuration\n>> diff: have the diff-* builtins configure diff before initializing\n>> revisions\n>>\n>> Stefan Beller (1):\n>> diff: enable indent heuristic by default\n> \n> Thanks, these look fine to me. I'd like to get an ACK from Michael, in\n> case he had some other reason for omitting them from git_diff_ui_config\n> (from my recollection, it's probably just a mix of conservatism and\n> following what the compaction heuristic had done).\n\nThat's exactly right. The only discussion I remember about broadening\nthe scope of diff options was with regards to `blame` [1]. I don't\nreally have enough overview of these configuration topics to have much\nan opinion.\n\nMichael\n\n[1]\nhttp://public-inbox.org/git/xmqqtwebwhbg.fsf@gitster.mtv.corp.google.com/\n\n"},{"id":"318276","messageId":"xmqqpoftjspa.fsf@gitster.mtv.corp.google.com","threadId":"45821","inReplyTo":"20170428080620.cbnreh5invwpasxr@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T01:01:37Z","receivedAt":"2017-05-01T01:01:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 27, 2017 at 04:50:37PM -0400, Marc Branchaud wrote:\n>\n>> Subject: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.\n>>\n>> This makes the commands respect diff configuration options, such as\n>> indentHeuristic.\n>> \n>> Signed-off-by: Marc Branchaud \n>\n> I think it would be helpful to future readers to explain what is going\n> on here. I.e., the bit about calling diff_setup_done(), which copies the\n> globals into the diff struct.\n>\n>...\n>\n>> +\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n>> \tinit_revisions(&rev, prefix);\n>> \tgitmodules_config();\n>> -\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n>> \trev.abbrev = 0;\n>> \tprecompose_argv(argc, argv);\n\nThis somehow made me worried at the first glance, but the matches\nwhat is done by the Porcelain \"git diff\". In other words, it was a\nbug that the original called init_revisions() before doing the diff\nconfiguration, and it does not have much to do with \"now let's have\nthem honor indentHeuristic\". Even if we wanted to keep the indent\nheuristic in the ui-config (not basic) section of the diff tweak\nknobs, we should be applying this patch as a bugfix.\n\n> It's somewhat odd to me that diff_files uses a rev_info struct at all.\n> It doesn't traverse at all, and doesn't respect most of the options.\n> There's a half-hearted attempt to reject some obviously bogus cases, but\n> most of the options are just silently ignored.\n>\n> I think it's mostly a historical wart (especially around the fact that\n> some diff options like combine_merges are in rev_info, which they\n> probably should not be). Anyway, none of that is your problem, and is\n> way outside the scope of this patch.\n\nYeah. The underlying diff machinery was built to be easily usable\nby revision traversal and that is probably the reason why we have\nthis entanglement that we probably could (and maybe we would want\nto) detangle. Surely not a theme of this topic.\n\nThanks.\n\n"},{"id":"318277","messageId":"xmqqlgqhjs8b.fsf@gitster.mtv.corp.google.com","threadId":"45821","inReplyTo":"20170429131439.ohgren3i7xr4tjex@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T01:11:48Z","receivedAt":"2017-05-01T01:16:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\n> index 6154acb45..5f46c0341 100755\n> --- a/t/t4051-diff-function-context.sh\n> +++ b/t/t4051-diff-function-context.sh\n> @@ -72,7 +72,7 @@ test_expect_success 'setup' '\n> \n> \t# overlap function context of 1st change and -u context of 2nd change\n> \tgrep -v \"delete me from hello\" <\"$dir/hello.c\" >file.c &&\n> -\tsed 2p <\"$dir/dummy.c\" >>file.c &&\n> +\tsed \"2aextra line\" <\"$dir/dummy.c\" >>file.c &&\n\nI've never used 'a' (or 'i') command of sed without having it\nimmediately followed by a backslash-newline in my scripts. How\nportable is this addition, I have to wonder. Can BSD folks give it\na quick test?\n\nThanks.\n"},{"id":"318349","messageId":"20170501051528.tpw3zlcpercdwdln@sigill.intra.peff.net","threadId":"45821","inReplyTo":"xmqqlgqhjs8b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T05:15:28Z","receivedAt":"2017-05-01T05:15:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 30, 2017 at 06:11:48PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\n> > index 6154acb45..5f46c0341 100755\n> > --- a/t/t4051-diff-function-context.sh\n> > +++ b/t/t4051-diff-function-context.sh\n> > @@ -72,7 +72,7 @@ test_expect_success 'setup' '\n> > \n> > \t# overlap function context of 1st change and -u context of 2nd change\n> > \tgrep -v \"delete me from hello\" <\"$dir/hello.c\" >file.c &&\n> > -\tsed 2p <\"$dir/dummy.c\" >>file.c &&\n> > +\tsed \"2aextra line\" <\"$dir/dummy.c\" >>file.c &&\n> \n> I've never used 'a' (or 'i') command of sed without having it\n> immediately followed by a backslash-newline in my scripts. How\n> portable is this addition, I have to wonder. Can BSD folks give it\n> a quick test?\n\nI think you're right that it needs the backslash. It's so rarely used\nthat I always forget which one is the portable way.\n\n-Peff\n\nPS Outside of our test scripts, I'd probably just have written:\n\n perl -lpe 'print \"extra line\" if $. == 2'\n\n I think we have traditionally preferred sed/awk to perl, but given\n the heavy use of vanilla perl elsewhere in the test suite, I think\n that is maybe just superstition at this point.\n"},{"id":"318350","messageId":"20170501051758.bei3odg3gehtbxbv@sigill.intra.peff.net","threadId":"45821","inReplyTo":"xmqqpoftjspa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T05:17:58Z","receivedAt":"2017-05-01T05:18:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 30, 2017 at 06:01:37PM -0700, Junio C Hamano wrote:\n\n> >> +\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> >> \tinit_revisions(&rev, prefix);\n> >> \tgitmodules_config();\n> >> -\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n> >> \trev.abbrev = 0;\n> >> \tprecompose_argv(argc, argv);\n> \n> This somehow made me worried at the first glance, but the matches\n> what is done by the Porcelain \"git diff\". In other words, it was a\n> bug that the original called init_revisions() before doing the diff\n> configuration, and it does not have much to do with \"now let's have\n> them honor indentHeuristic\". Even if we wanted to keep the indent\n> heuristic in the ui-config (not basic) section of the diff tweak\n> knobs, we should be applying this patch as a bugfix.\n\nYeah, I agree it is an existing bug. The only other case I found is\ndirstat. Doing:\n\n mkdir a b\n for i in 1 2; do echo content >a/$i; done\n for i in 1 2 3; do echo content >b/$i; done\n git -c diff.dirstat=50 diff-tree --dirstat --root HEAD\n\nshould respect the config and show only \"b\", but it doesn't currently\n(and does with Marc's patch).\n\n-Peff\n"},{"id":"318351","messageId":"20170501052903.nkcg3ndcgbfj7t2p@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170501051758.bei3odg3gehtbxbv@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Have the diff-* builtins configure diff before initializing revisions.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T05:29:03Z","receivedAt":"2017-05-01T05:43:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2017 at 01:17:58AM -0400, Jeff King wrote:\n\n> Yeah, I agree it is an existing bug. The only other case I found is\n> dirstat. Doing:\n> \n> mkdir a b\n> for i in 1 2; do echo content >a/$i; done\n> for i in 1 2 3; do echo content >b/$i; done\n> git -c diff.dirstat=50 diff-tree --dirstat --root HEAD\n> \n> should respect the config and show only \"b\", but it doesn't currently\n> (and does with Marc's patch).\n\nEr, there's a \"git add a b; git commit -m foo\" missing before running\nthe diff, of course.\n\n-Peff\n"},{"id":"318357","messageId":"CACBZZX5f81HKCjRjTDyXzNMVuef9Z_ECS+0SVk2xpbwXudgxCw@mail.gmail.com","threadId":"45821","inReplyTo":"20170428220450.olqitnuwhrxzg3pv@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-01T10:34:38Z","receivedAt":"2017-05-01T10:35:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Apr 29, 2017 at 12:04 AM, Jeff King wrote:\n> On Fri, Apr 28, 2017 at 10:34:15AM -0700, Stefan Beller wrote:\n>\n>> > So instead I chose to make the indentHeuristic option part of diff's basic\n>> > configuration, and in each of the diff plumbing commands I moved the call to\n>> > git_config() before the call to init_revisions().\n>> [...]\n>>\n>> The feature was included in v2.11 (released 2016-11-29) and we got no\n>> negative feedback. Quite the opposite, all feedback we got, was positive.\n>> This could be explained by having the feature as an experimental feature\n>> and users who would be broken by it, did not test it yet or did not speak up.\n>\n> Yeah, if the point you're trying to make is \"nobody will be mad if this\n> is turned on by default\", I don't think shipping it as a config option\n> is very conclusive.\n>\n> I think a more interesting argument is: we turned on renames by default\n> a few versions ago, which changes the diff in a much bigger way, and\n> nobody complained.\n>\n> As a side note, I do happen to know of one program that depends\n> heavily on diffs remaining stable. Imagine you have a Git hosting site\n> which lets people comment on lines of diffs. You need some way to\n> address the lines of the diff so that the annotations appear in the\n> correct position when you regenerate the diff later.\n>\n> One way to do it is to just position the comment at the n'th line of\n> the diff. Which obviously breaks if the diff changes. IMHO that is a\n> bug in that program, which should be fixed to use the line numbers\n> from the original blob (which is still not foolproof, because a\n> different diff algorithm may move a change such that the line isn't\n> even part of the diff anymore).\n>\n> I'm not worried about this particular program, as I happen to know it\n> has already been fixed. But it's possible others have made a similar\n> mistake.\n>\n>> So I'd propose to turn it on by default and anyone negatively impacted by that\n>> could then use the config to turn it off for themselves (including plumbing).\n>>\n>> Something like this, maybe?\n>\n> Yeah, as long as this is on top of Marc's patches, I think it is the\n> natural conclusion that we had planned.\n>\n> I don't know if we would want to be extra paranoid about patch-ids.\n> There is no helping:\n>\n> git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable\n>\n> because diff-tree doesn't know that it's trying for \"--stable\" output.\n> But the diffs we compute internally for patch-id could disable the\n> heuristics. I'm not sure if those matter, though. AFAIK those are used\n> only for internal comparisons within a single program. I.e., we never\n> compare them against input from the user, nor do we output them to the\n> user. So they'll change, but I don't think anybody would care.\n\nI have a few-million row table with commit_id as one column & patch_id\nas another. I.e. a commit -> patch_id mapping.\n\nThis is used for an hourly reporting system of sorts, i.e. you can see\nthat you were working on commit X on Friday. It uses the patch-id to\nde-duplicate that commit against some commit Y you may have pushed to\na topic branch, and already used for filling in hours.\n\nI.e. it's a thing do DWYM in a rebase-heavy workflow where the commit\nids change, and where you're too lazy to have deleted the topic branch\nafterwards (because they get pruned anyway).\n\nIt's fine for me if the patch-id changes, for most diffs it'll be the\nsame, and if not it'll auto-adjust eventually since the records I'm\ninserting will all use the new algorithm.\n\nBut as the diff algorithm changes this system will start presenting\nboth X and Y in the UI as not-duplicate, because it'll have computed\nthe patch id of X with an older version of git, and the id of Y with\nthe new one.\n\nI'm surely not the only one who's written something like this. Just\nwanted to point out that systems like this do exist, I don't know of\nany easier way with git to get a \"is this rebased commit the same\"\nthan patch-id, and for performance reasons you may be comparing patch\nids across git versions.\n"},{"id":"318411","messageId":"20170501221345.4025-3-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170501221345.4025-1-marcnarc@xiplink.com","subject":"[PATCHv3 2/4] diff: have the diff-* builtins configure diff before initializing revisions","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-05-01T22:13:43Z","receivedAt":"2017-05-01T22:13:54Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"This matches how the diff Porcelain works. It makes the plumbing commands\nrespect diff's configuration options, such as indentHeuristic, because\ninit_revisions() calls diff_setup() which fills in the diff_options struct.\n\nSigned-off-by: Marc Branchaud \n---\n builtin/diff-files.c | 2 +-\n builtin/diff-index.c | 2 +-\n builtin/diff-tree.c | 2 +-\n t/t4061-diff-indent.sh | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 69 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 15c61fd8d..a572da9d5 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -20,9 +20,9 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)\n \tint result;\n \tunsigned options = 0;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 1af373d00..f084826a2 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -17,9 +17,9 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint result;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(&rev, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \trev.abbrev = 0;\n \tprecompose_argv(argc, argv);\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 326f88b65..36a3a1976 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -105,9 +105,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct setup_revision_opt s_r_opt;\n \tint read_stdin = 0;\n \n+\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \tinit_revisions(opt, prefix);\n \tgitmodules_config();\n-\tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \topt->abbrev = 0;\n \topt->diff = 1;\n \topt->disable_stdin = 1;\ndiff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\nindex 556450609..13d3dc96a 100755\n--- a/t/t4061-diff-indent.sh\n+++ b/t/t4061-diff-indent.sh\n@@ -213,4 +213,70 @@ test_expect_success 'blame: --no-indent-heuristic overrides config' '\n \tcompare_blame spaces-expect out-blame2\n '\n \n+test_expect_success 'diff-tree: nice spaces with --indent-heuristic' '\n+\tgit diff-tree --indent-heuristic -p old new -- spaces.txt >out-diff-tree-compacted &&\n+\tcompare_diff spaces-compacted-expect out-diff-tree-compacted\n+'\n+\n+test_expect_success 'diff-tree: nice spaces with diff.indentHeuristic' '\n+\tgit -c diff.indentHeuristic=true diff-tree -p old new -- spaces.txt >out-diff-tree-compacted2 &&\n+\tcompare_diff spaces-compacted-expect out-diff-tree-compacted2\n+'\n+\n+test_expect_success 'diff-tree: --no-indent-heuristic overrides config' '\n+\tgit -c diff.indentHeuristic=true diff-tree --no-indent-heuristic -p old new -- spaces.txt >out-diff-tree &&\n+\tcompare_diff spaces-expect out-diff-tree\n+'\n+\n+test_expect_success 'diff-index: nice spaces with --indent-heuristic' '\n+\tgit checkout -B diff-index &&\n+\tgit reset --soft HEAD~ &&\n+\tgit diff-index --indent-heuristic -p old -- spaces.txt >out-diff-index-compacted &&\n+\tcompare_diff spaces-compacted-expect out-diff-index-compacted &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-index: nice spaces with diff.indentHeuristic' '\n+\tgit checkout -B diff-index &&\n+\tgit reset --soft HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-index -p old -- spaces.txt >out-diff-index-compacted2 &&\n+\tcompare_diff spaces-compacted-expect out-diff-index-compacted2 &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-index: --no-indent-heuristic overrides config' '\n+\tgit checkout -B diff-index &&\n+\tgit reset --soft HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-index --no-indent-heuristic -p old -- spaces.txt >out-diff-index &&\n+\tcompare_diff spaces-expect out-diff-index &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '\n+\tgit checkout -B diff-files &&\n+\tgit reset HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw &&\n+\tgrep -v index out-diff-files-raw >out-diff-files-compacted &&\n+\tcompare_diff spaces-compacted-expect out-diff-files-compacted &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-files: nice spaces with diff.indentHeuristic' '\n+\tgit checkout -B diff-files &&\n+\tgit reset HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-files -p spaces.txt >out-diff-files-raw2 &&\n+\tgrep -v index out-diff-files-raw2 >out-diff-files-compacted2 &&\n+\tcompare_diff spaces-compacted-expect out-diff-files-compacted2 &&\n+\tgit checkout -f master\n+'\n+\n+test_expect_success 'diff-files: --no-indent-heuristic overrides config' '\n+\tgit checkout -B diff-files &&\n+\tgit reset HEAD~ &&\n+\tgit -c diff.indentHeuristic=true diff-files --no-indent-heuristic -p spaces.txt >out-diff-files-raw3 &&\n+\tgrep -v index out-diff-files-raw3 >out-diff-files &&\n+\tcompare_diff spaces-expect out-diff-files &&\n+\tgit checkout -f master\n+'\n+\n test_done\n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318412","messageId":"20170501221345.4025-2-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170501221345.4025-1-marcnarc@xiplink.com","subject":"[PATCHv3 1/4] diff: make the indent heuristic part of diff's basic configuration","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-05-01T22:13:42Z","receivedAt":"2017-05-01T22:13:55Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"This heuristic was originally introduced as an experimental feature,\nand therefore part of the UI configuration.\n\nBut the user often sees diffs generated by plumbing commands like\ndiff-tree. Moving the indent heuristic into diff's basic configuration\nprepares the way for diff plumbing commands to respect the setting.\n\nThe heuristic itself merely makes the diffs more aesthetically\npleasing, without changing their correctness. Scripts that rely on\nthe diff plumbing commands should not care whether or not the heuristic\nis employed.\n\nSigned-off-by: Marc Branchaud \n---\n diff.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 11eef1c85..da96577ea 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -290,9 +290,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (git_diff_heuristic_config(var, value, cb) < 0)\n-\t\treturn -1;\n-\n \tif (!strcmp(var, \"diff.wserrorhighlight\")) {\n \t\tint val = parse_ws_error_highlight(value);\n \t\tif (val < 0)\n@@ -351,6 +348,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \tif (starts_with(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n+\tif (git_diff_heuristic_config(var, value, cb) < 0)\n+\t\treturn -1;\n+\n \treturn git_default_config(var, value, cb);\n }\n \n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318413","messageId":"20170501221345.4025-1-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170429131439.ohgren3i7xr4tjex@sigill.intra.peff.net","subject":"[PATCHv3 0/4] Make diff plumbing commands respect the indentHeuristic.","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-05-01T22:13:41Z","receivedAt":"2017-05-01T22:14:01Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 2017-04-29 09:14 AM, Jeff King wrote:\n> On Sat, Apr 29, 2017 at 08:40:52AM -0400, Jeff King wrote:\n> \n>> On Fri, Apr 28, 2017 at 06:33:12PM -0400, Marc Branchaud wrote:\n>>\n>>> v2: Fixed up the commit messages and added tests.\n>>>\n>>> Marc Branchaud (2):\n>>> diff: make the indent heuristic part of diff's basic configuration\n>>> diff: have the diff-* builtins configure diff before initializing\n>>> revisions\n>>>\n>>> Stefan Beller (1):\n>>> diff: enable indent heuristic by default\n>>\n>> Thanks, these look fine to me. I'd like to get an ACK from Michael, in\n>> case he had some other reason for omitting them from git_diff_ui_config\n>> (from my recollection, it's probably just a mix of conservatism and\n>> following what the compaction heuristic had done).\n> \n> Sorry, I spoke too soon. The third one needs a few test adjustments\n> squashed in to pass the tests.\n\nDoh! That'll teach me to try to do this stuff at the end of a Friday...\n\nOne more try, then:\n\nChanges since v2:\n\n Patch 1/4 : Unchanged.\n\n Patch 2/4 : Mentioned how the new behaviour matches the diff Porcelain.\n\n Patch 3/4 : Updated the tests.\n\n Patch 4/4 : (New) Jeff's add--interactive patch.\n\nThanks for all the help, Jeff!\n\n\t\tM.\n\n\nJeff King (1):\n add--interactive: drop diff.indentHeuristic handling\n\nMarc Branchaud (2):\n diff: make the indent heuristic part of diff's basic configuration\n diff: have the diff-* builtins configure diff before initializing\n revisions\n\nStefan Beller (1):\n diff: enable indent heuristic by default\n\n builtin/diff-files.c | 2 +-\n builtin/diff-index.c | 2 +-\n builtin/diff-tree.c | 2 +-\n diff.c | 8 ++---\n git-add--interactive.perl | 4 ---\n t/t4051-diff-function-context.sh | 3 +-\n t/t4061-diff-indent.sh | 72 ++++++++++++++++++++++++++++++++++++++--\n 7 files changed, 78 insertions(+), 15 deletions(-)\n\n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318414","messageId":"20170501221345.4025-5-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170501221345.4025-1-marcnarc@xiplink.com","subject":"[PATCHv3 4/4] add--interactive: drop diff.indentHeuristic handling","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-05-01T22:13:45Z","receivedAt":"2017-05-01T22:14:02Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"From: Jeff King \n\nNow that diff.indentHeuristic is handled automatically by the plumbing\ncommands, there's no need to propagate it manually.\n\nSigned-off-by: Jeff King \nSigned-off-by: Marc Branchaud \n---\n git-add--interactive.perl | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 709a5f6ce..79d675b5b 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -46,7 +46,6 @@ my ($diff_new_color) =\n my $normal_color = $repo->get_color(\"\", \"reset\");\n \n my $diff_algorithm = $repo->config('diff.algorithm');\n-my $diff_indent_heuristic = $repo->config_bool('diff.indentheuristic');\n my $diff_filter = $repo->config('interactive.difffilter');\n \n my $use_readkey = 0;\n@@ -730,9 +729,6 @@ sub parse_diff {\n \tif (defined $diff_algorithm) {\n \t\tsplice @diff_cmd, 1, 0, \"--diff-algorithm=${diff_algorithm}\";\n \t}\n-\tif ($diff_indent_heuristic) {\n-\t\tsplice @diff_cmd, 1, 0, \"--indent-heuristic\";\n-\t}\n \tif (defined $patch_mode_revision) {\n \t\tpush @diff_cmd, get_diff_reference($patch_mode_revision);\n \t}\n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318415","messageId":"20170501221345.4025-4-marcnarc@xiplink.com","threadId":"45821","inReplyTo":"20170501221345.4025-1-marcnarc@xiplink.com","subject":"[PATCHv3 3/4] diff: enable indent heuristic by default","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2017-05-01T22:13:44Z","receivedAt":"2017-05-01T22:14:03Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"From: Stefan Beller \n\nThe feature was included in v2.11 (released 2016-11-29) and we got no\nnegative feedback. Quite the opposite, all feedback we got was positive.\n\nTurn it on by default. Users who dislike the feature can turn it off\nby setting diff.indentHeuristic (which also configures plumbing commands,\nsee prior patches).\n\nThe change to t/t4051-diff-function-context.sh is needed because the\nheuristic shifts the changed hunk in the patch. To get the same result\nregardless of the heuristic configuration, we modify the test file\ndifferently: We insert a completely new line after line 2, instead of\nsimply duplicating it.\n\nHelped-by: Jeff King \nSigned-off-by: Stefan Beller \nSigned-off-by: Marc Branchaud \n\n---\n\nTested the sed \"2a\" command's escaping in both Ubuntu 17.04 and FreeBSD 10.3.\nThrew in a little indenting so that it isn't too ugly.\n\n diff.c | 2 +-\n t/t4051-diff-function-context.sh | 3 ++-\n t/t4061-diff-indent.sh | 6 +++---\n 3 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex da96577ea..2c47ccb4a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -27,7 +27,7 @@\n #endif\n \n static int diff_detect_rename_default;\n-static int diff_indent_heuristic; /* experimental */\n+static int diff_indent_heuristic = 1;\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\ndiff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh\nindex 6154acb45..3e6b485ec 100755\n--- a/t/t4051-diff-function-context.sh\n+++ b/t/t4051-diff-function-context.sh\n@@ -72,7 +72,8 @@ test_expect_success 'setup' '\n \n \t# overlap function context of 1st change and -u context of 2nd change\n \tgrep -v \"delete me from hello\" <\"$dir/hello.c\" >file.c &&\n-\tsed 2p <\"$dir/dummy.c\" >>file.c &&\n+\tsed \"2a\\\\\n+\t extra line\" <\"$dir/dummy.c\" >>file.c &&\n \tcommit_and_tag changed_hello_dummy file.c &&\n \n \tgit checkout initial &&\ndiff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\nindex 13d3dc96a..56d7d7760 100755\n--- a/t/t4061-diff-indent.sh\n+++ b/t/t4061-diff-indent.sh\n@@ -153,7 +153,7 @@ test_expect_success 'prepare' '\n '\n \n test_expect_success 'diff: ugly spaces' '\n-\tgit diff old new -- spaces.txt >out &&\n+\tgit diff --no-indent-heuristic old new -- spaces.txt >out &&\n \tcompare_diff spaces-expect out\n '\n \n@@ -183,7 +183,7 @@ test_expect_success 'diff: --indent-heuristic with --histogram' '\n '\n \n test_expect_success 'diff: ugly functions' '\n-\tgit diff old new -- functions.c >out &&\n+\tgit diff --no-indent-heuristic old new -- functions.c >out &&\n \tcompare_diff functions-expect out\n '\n \n@@ -193,7 +193,7 @@ test_expect_success 'diff: nice functions with --indent-heuristic' '\n '\n \n test_expect_success 'blame: ugly spaces' '\n-\tgit blame old..new -- spaces.txt >out-blame &&\n+\tgit blame --no-indent-heuristic old..new -- spaces.txt >out-blame &&\n \tcompare_blame spaces-expect out-blame\n '\n \n-- \n2.13.0.rc1.15.gf67d331ad\n\n"},{"id":"318416","messageId":"CAGZ79ka6Bnq=U4s2bZhLKwUD3SJEBmD-AvP63-RrpJ8MYGbKTw@mail.gmail.com","threadId":"45821","inReplyTo":"20170501221345.4025-1-marcnarc@xiplink.com","subject":"Re: [PATCHv3 0/4] Make diff plumbing commands respect the indentHeuristic.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T22:18:36Z","receivedAt":"2017-05-01T22:18:41Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Changes since v2:\n>\n> Patch 1/4 : Unchanged.\n>\n> Patch 2/4 : Mentioned how the new behaviour matches the diff Porcelain.\n>\n> Patch 3/4 : Updated the tests.\n\nThanks for picking up that patch and carrying it further!\nThe whole series looks good to me.\n\nThanks,\nStefan\n"},{"id":"318417","messageId":"20170501222051.svylxazjwnot3wwg@sigill.intra.peff.net","threadId":"45821","inReplyTo":"20170501221345.4025-4-marcnarc@xiplink.com","subject":"Re: [PATCHv3 3/4] diff: enable indent heuristic by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-01T22:20:51Z","receivedAt":"2017-05-01T22:21:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2017 at 06:13:44PM -0400, Marc Branchaud wrote:\n\n> From: Stefan Beller \n> \n> The feature was included in v2.11 (released 2016-11-29) and we got no\n> negative feedback. Quite the opposite, all feedback we got was positive.\n> \n> Turn it on by default. Users who dislike the feature can turn it off\n> by setting diff.indentHeuristic (which also configures plumbing commands,\n> see prior patches).\n> \n> The change to t/t4051-diff-function-context.sh is needed because the\n> heuristic shifts the changed hunk in the patch. To get the same result\n> regardless of the heuristic configuration, we modify the test file\n> differently: We insert a completely new line after line 2, instead of\n> simply duplicating it.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Marc Branchaud \n> \n> ---\n> \n> Tested the sed \"2a\" command's escaping in both Ubuntu 17.04 and FreeBSD 10.3.\n> Threw in a little indenting so that it isn't too ugly.\n\nI think that should be fine. The indenting will go into the output, but\nwe really don't care _what_ that line is, just as long as it's new\ncontent.\n\n> diff --git a/t/t4061-diff-indent.sh b/t/t4061-diff-indent.sh\n> index 13d3dc96a..56d7d7760 100755\n> --- a/t/t4061-diff-indent.sh\n> +++ b/t/t4061-diff-indent.sh\n> @@ -153,7 +153,7 @@ test_expect_success 'prepare' '\n> '\n> \n> test_expect_success 'diff: ugly spaces' '\n> -\tgit diff old new -- spaces.txt >out &&\n> +\tgit diff --no-indent-heuristic old new -- spaces.txt >out &&\n> \tcompare_diff spaces-expect out\n> '\n\nI guess one could argue that most of t4061 should be rewritten. This\nfixes the failures that the \"ugly\" version is no longer the default. But\nthe tests checking that diff.indentHeuristic=true works are basically\ndoing nothing (the interesting thing after this patch is that setting it\nto false goes back to the ugly output).\n\nI dunno. If we drop the config and options as experiments, then the\nwhole script basically goes away in favor of checking that the test case\nhas the non-ugly output. If that's our endgame, it may not be worth\nspending too much time refactoring it. But if we're going to keep the\nconfig indefinitely, maybe we should make sure it works.\n\n-Peff\n"},{"id":"318423","messageId":"xmqq60hkgnwq.fsf@gitster.mtv.corp.google.com","threadId":"45821","inReplyTo":"20170501051528.tpw3zlcpercdwdln@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/3] Make diff plumbing commands respect the indentHeuristic.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-01T23:25:41Z","receivedAt":"2017-05-01T23:25:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> PS Outside of our test scripts, I'd probably just have written:\n>\n> perl -lpe 'print \"extra line\" if $. == 2'\n>\n> I think we have traditionally preferred sed/awk to perl, but given\n> the heavy use of vanilla perl elsewhere in the test suite, I think\n> that is maybe just superstition at this point.\n\nI would have avoided sed with 'a', 'i' and 'c' in one-liners myself,\nand a Perl script like the above would probably have been my choice.\n\n\n"},{"id":"319152","messageId":"20170509031653.7hc36pil3vzrobsz@sigill.intra.peff.net","threadId":"45821","inReplyTo":"CACBZZX5f81HKCjRjTDyXzNMVuef9Z_ECS+0SVk2xpbwXudgxCw@mail.gmail.com","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T03:16:54Z","receivedAt":"2017-05-09T03:17:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > I don't know if we would want to be extra paranoid about patch-ids.\n> > There is no helping:\n> >\n> > git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable\n> >\n> > because diff-tree doesn't know that it's trying for \"--stable\" output.\n> > But the diffs we compute internally for patch-id could disable the\n> > heuristics. I'm not sure if those matter, though. AFAIK those are used\n> > only for internal comparisons within a single program. I.e., we never\n> > compare them against input from the user, nor do we output them to the\n> > user. So they'll change, but I don't think anybody would care.\n> \n> I have a few-million row table with commit_id as one column & patch_id\n> as another. I.e. a commit -> patch_id mapping.\n\nThanks for this data point. It's always interesting to hear about\nunforeseen uses of the tools.\n\nOut of curiosity, how do you generate the patch-ids? Is it with\nsomething like diff-tree piped to patch-id?\n\nI do feel a bit sad about breaking this case (or at the very least\nforcing you to set an option to retain cross-version compatibility). But\nmy gut says that we don't want to lock ourselves into never changing the\ndiff algorithm (and I'm sure we've done it inadvertently a few times\nover the years; even the recent switch to turning on renames would have\nhad that impact).\n\n-Peff\n"},{"id":"319161","messageId":"xmqqbmr24qt2.fsf@gitster.mtv.corp.google.com","threadId":"45821","inReplyTo":"20170509031653.7hc36pil3vzrobsz@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T04:06:33Z","receivedAt":"2017-05-09T04:06:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do feel a bit sad about breaking this case (or at the very least\n> forcing you to set an option to retain cross-version compatibility). But\n> my gut says that we don't want to lock ourselves into never changing the\n> diff algorithm (and I'm sure we've done it inadvertently a few times\n> over the years; even the recent switch to turning on renames would have\n> had that impact).\n\nIIRC, we never promised that different versions of Git produce the\nsame patch ID for the same change. I do share that sadness, as not\nmaking that promise would affect not just such an external database\nkeyed with patch-ids but also affects the rerere database, and those\nwho heavily depend on the rerere database would need to refresh them\nevery time diff/merge algorithm is updated, using something like\ncontrib/rerere-train.sh script.\n\nBut at the same time, I am very much against promising that the\ntextual diff output will never improve for human readability.\n\n"},{"id":"319170","messageId":"CACBZZX5yHRDU6R3F8=qnqEGu6UCYNOj3rqMDbP1G7cJnvG7R=g@mail.gmail.com","threadId":"45821","inReplyTo":"20170509031653.7hc36pil3vzrobsz@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-09T07:58:28Z","receivedAt":"2017-05-09T07:59:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, May 9, 2017 at 5:16 AM, Jeff King wrote:\n> On Mon, May 01, 2017 at 12:34:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > I don't know if we would want to be extra paranoid about patch-ids.\n>> > There is no helping:\n>> >\n>> > git rev-list HEAD | git diff-tree --stdin -p | git patch-id --stable\n>> >\n>> > because diff-tree doesn't know that it's trying for \"--stable\" output.\n>> > But the diffs we compute internally for patch-id could disable the\n>> > heuristics. I'm not sure if those matter, though. AFAIK those are used\n>> > only for internal comparisons within a single program. I.e., we never\n>> > compare them against input from the user, nor do we output them to the\n>> > user. So they'll change, but I don't think anybody would care.\n>>\n>> I have a few-million row table with commit_id as one column & patch_id\n>> as another. I.e. a commit -> patch_id mapping.\n>\n> Thanks for this data point. It's always interesting to hear about\n> unforeseen uses of the tools.\n>\n> Out of curiosity, how do you generate the patch-ids? Is it with\n> something like diff-tree piped to patch-id?\n\nThis:\n\n my $cmd = qq[git --git-dir=\"$repository_path\" log --since=\"$since\"\n--until=\"$until\" --all --pretty=format:%H --binary | git patch-id];\n open my $patch_id_fh, \" $cmd |\";\n\nWhich is part of a loop that generates since/until for continuous\npull/insertion. Also, a few lines later there's a workaround for the\ngit.git bug of patch-id being ^0+$ (fixed in 2485eab55c\n(\"git-patch-id: do not trip over \"no newline\" markers\", 2011-02-17)),\nwhich gives you a sense of how long it's been since anyone's touched\nthis.\n\n> I do feel a bit sad about breaking this case (or at the very least\n> forcing you to set an option to retain cross-version compatibility). But\n> my gut says that we don't want to lock ourselves into never changing the\n> diff algorithm (and I'm sure we've done it inadvertently a few times\n> over the years; even the recent switch to turning on renames would have\n> had that impact).\n\nAs noted I think it's completely fine to change the patch-ids by\nchanging the diff algorithm.\n\nI'm about to give some more detail on this in the other thread, but I\nfind that on our repos the indent heuristic changes the patch-id for\naround 2% of patches, which seems fairly typical for non-changelog-y\ncode. You *then* need to be using topic branches you didn't delete as\nwell as having authored such a patch for this change to kick in, so\nthe impact is really minimal.\n\nEven if it somehow changed 100% of the ids that would be fine too. It\nwould auto-heal as the same git version started reading & inserting\nthe ids, which are only relevant in a moving window.\n"},{"id":"319171","messageId":"20170509080434.dkqa7pvsq34sygn3@sigill.intra.peff.net","threadId":"45821","inReplyTo":"CACBZZX5yHRDU6R3F8=qnqEGu6UCYNOj3rqMDbP1G7cJnvG7R=g@mail.gmail.com","subject":"Re: [PATCH 0/2] Make diff plumbing commands respect the indentHeuristic.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T08:04:34Z","receivedAt":"2017-05-09T08:04:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 09:58:28AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Out of curiosity, how do you generate the patch-ids? Is it with\n> > something like diff-tree piped to patch-id?\n> \n> This:\n> \n> my $cmd = qq[git --git-dir=\"$repository_path\" log --since=\"$since\"\n> --until=\"$until\" --all --pretty=format:%H --binary | git patch-id];\n> open my $patch_id_fh, \" $cmd |\";\n\nAh, OK. I was specifically curious whether the decision to respect the\nconfig switch in plumbing would have any impact for your script. But it\nwouldn't, as it was already using log (though I suspect the real\nprotection for your script is that it is used from a vanilla\nenvironment, not by random users).\n\n-Peff\n"}]}