{"thread":{"id":"48392","subject":"[PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","startedAt":"2018-05-01T09:50:31Z","lastAt":"2018-05-04T15:13:33Z","messageCount":14,"participants":["Eckhard S. Maaß","Ævar Arnfjörð Bjarmason","Matthieu Moy","Eckhard Maaß","Elijah Newren","Junio C Hamano","Ben Peart"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"346226","messageId":"20180501094940.17772-1-eckhard.s.maass@gmail.com","threadId":"48392","inReplyTo":"c466854f-6087-e7f1-264a-1d2df9fd9b5a@gmail.com","subject":"[PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Eckhard S. Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-01T09:49:40Z","receivedAt":"2018-05-01T09:50:31Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"Since the very beginning, git status behaved differently for rename\ndetection than other rename aware commands like git log or git show as\nit has the use of rename hard coded into it.  After 5404c116aa (\"diff:\nactivate diff.renames by default\", 2016-02-25) the default behaves the\nsame by coincidence, but a work flow like\n\n    - git add .\n    - git status\n    - git commit\n    - git show\n\nshould give you the same information on renames (and/or copies if\nactivated) accordingly to the diff.renames and diff.renameLimit setting.\n\nWith this commit the hard coded settings are dropped from the status\ncommand.\n\nSigned-off-by: Eckhard S. Maaß <eckhard.s.maass@gmail.com>\nReviewed-by: Elijah Newren <newren@gmail.com>\n---\n builtin/commit.c       |  2 +-\n t/t4001-diff-rename.sh | 12 ++++++++++++\n wt-status.c            |  4 ----\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5571d4a3e2..5240f11225 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -161,9 +161,9 @@ static void determine_whence(struct wt_status *s)\n static void status_init_config(struct wt_status *s, config_fn_t fn)\n {\n \twt_status_prepare(s);\n+\tinit_diff_ui_defaults();\n \tgit_config(fn, s);\n \tdetermine_whence(s);\n-\tinit_diff_ui_defaults();\n \ts->hints = advice_status_hints; /* must come after git_config() */\n }\n \ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex a07816d560..bf4030371a 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -138,6 +138,18 @@ test_expect_success 'favour same basenames over different ones' '\n \ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n '\n \n+test_expect_success 'test diff.renames=true for git status' '\n+\tgit -c diff.renames=true status >out &&\n+\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n+'\n+\n+test_expect_success 'test diff.renames=false for git status' '\n+\tgit -c diff.renames=false status >out &&\n+\ttest_i18ngrep ! \"renamed: .*path1 -> subdir/path1\" out &&\n+\ttest_i18ngrep \"new file: .*subdir/path1\" out &&\n+\ttest_i18ngrep \"deleted: .*[^/]path1\" out\n+'\n+\n test_expect_success 'favour same basenames even with minor differences' '\n \tgit show HEAD:path1 | sed \"s/15/16/\" > subdir/path1 &&\n \tgit status >out &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 50815e5faf..32f3bcaebd 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -625,9 +625,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = wt_status_collect_updated_cb;\n \trev.diffopt.format_callback_data = s;\n-\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n-\trev.diffopt.rename_limit = 200;\n-\trev.diffopt.break_opt = 0;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n }\n@@ -985,7 +982,6 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \tsetup_revisions(0, NULL, &rev, &opt);\n \n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n-\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\n-- \n2.17.0.252.gfe0a9eaf31\n\n"},{"id":"346232","messageId":"87bmdzzlll.fsf@evledraar.gmail.com","threadId":"48392","inReplyTo":"20180501094940.17772-1-eckhard.s.maass@gmail.com","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-01T11:00:54Z","receivedAt":"2018-05-01T11:01:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 01 2018, Eckhard S. Maaß wrote:\n\n> Since the very beginning, git status behaved differently for rename\n> detection than other rename aware commands like git log or git show as\n> it has the use of rename hard coded into it.\n\n\nCan you elaborate on this? It seems initial rename detection was added\nin 5c97558c9a (\"[PATCH] Detect renames in diff family.\", 2005-05-19) and\nthe first version of the status script added by Linus in a3e870f2e2\n(\"Add \"commit\" helper script\", 2005-05-30), and that one piggy-backs on\n\"diff\" for rename detection.\n\nSo didn't we use diff heuristics to begin with, and then regressed? I've\nonly given this a skimming, but it's useful to have that sort of\nhistorical context mentioned explicitly with commit ids.\n\n> After 5404c116aa (\"diff:\n> activate diff.renames by default\", 2016-02-25) the default behaves the\n> same by coincidence, but a work flow like\n>\n>     - git add .\n>     - git status\n>     - git commit\n>     - git show\n>\n> should give you the same information on renames (and/or copies if\n> activated) accordingly to the diff.renames and diff.renameLimit setting.\n>\n> With this commit the hard coded settings are dropped from the status\n> command.\n\nIt's unclear to me what this means, so the only difference between\n\"status\" and \"diff\" is that the former had a hardcoded limit of 200? In\nthat case it was added at 100 (later adusted) in 0024a54923 (\"Fix the\nrename detection limit checking\", 2007-09-14), so not since \"the very\nbeginning...\".\n\n> Signed-off-by: Eckhard S. Maaß <eckhard.s.maass@gmail.com>\n> Reviewed-by: Elijah Newren <newren@gmail.com>\n> ---\n>  builtin/commit.c       |  2 +-\n>  t/t4001-diff-rename.sh | 12 ++++++++++++\n>  wt-status.c            |  4 ----\n>  3 files changed, 13 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 5571d4a3e2..5240f11225 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -161,9 +161,9 @@ static void determine_whence(struct wt_status *s)\n>  static void status_init_config(struct wt_status *s, config_fn_t fn)\n>  {\n>  \twt_status_prepare(s);\n> +\tinit_diff_ui_defaults();\n>  \tgit_config(fn, s);\n>  \tdetermine_whence(s);\n> -\tinit_diff_ui_defaults();\n>  \ts->hints = advice_status_hints; /* must come after git_config() */\n>  }\n>\n> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\n> index a07816d560..bf4030371a 100755\n> --- a/t/t4001-diff-rename.sh\n> +++ b/t/t4001-diff-rename.sh\n> @@ -138,6 +138,18 @@ test_expect_success 'favour same basenames over different ones' '\n>  \ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n>  '\n>\n> +test_expect_success 'test diff.renames=true for git status' '\n> +\tgit -c diff.renames=true status >out &&\n> +\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n> +'\n> +\n> +test_expect_success 'test diff.renames=false for git status' '\n> +\tgit -c diff.renames=false status >out &&\n> +\ttest_i18ngrep ! \"renamed: .*path1 -> subdir/path1\" out &&\n> +\ttest_i18ngrep \"new file: .*subdir/path1\" out &&\n> +\ttest_i18ngrep \"deleted: .*[^/]path1\" out\n> +'\n> +\n>  test_expect_success 'favour same basenames even with minor differences' '\n>  \tgit show HEAD:path1 | sed \"s/15/16/\" > subdir/path1 &&\n>  \tgit status >out &&\n> diff --git a/wt-status.c b/wt-status.c\n> index 50815e5faf..32f3bcaebd 100644\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -625,9 +625,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n>  \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n>  \trev.diffopt.format_callback = wt_status_collect_updated_cb;\n>  \trev.diffopt.format_callback_data = s;\n> -\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n> -\trev.diffopt.rename_limit = 200;\n> -\trev.diffopt.break_opt = 0;\n>  \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n>  \trun_diff_index(&rev, 1);\n>  }\n> @@ -985,7 +982,6 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n>  \tsetup_revisions(0, NULL, &rev, &opt);\n>\n>  \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n> -\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n>  \trev.diffopt.file = s->fp;\n>  \trev.diffopt.close_file = 0;\n>  \t/*\n"},{"id":"346234","messageId":"907020160.11403426.1525172946040.JavaMail.zimbra@inria.fr","threadId":"48392","inReplyTo":"50c60ddfeb9a44a99f556be2c2ca9a34@BPMBX2013-01.univ-lyon1.fr","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-05-01T11:09:06Z","receivedAt":"2018-05-01T11:09:10Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\"Eckhard S. Maaß\" <eckhard.s.maass@googlemail.com> wrote:\n\n> Since the very beginning, git status behaved differently for rename\n> detection than other rename aware commands like git log or git show as\n> it has the use of rename hard coded into it.\n\nMy understanding is that the succession of events went stg like:\n\n1) invent the rename detection, but consider it experimental\n   hence don't activate it by default;\n\n2) add commands using the rename detection, and since it works\n   well, use it by default;\n\n3) activate rename detection by default for diff.\n\nThe next logical step is what you patch does indeed.\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -161,9 +161,9 @@ static void determine_whence(struct wt_status *s)\n> static void status_init_config(struct wt_status *s, config_fn_t fn)\n> {\n> \twt_status_prepare(s);\n> +\tinit_diff_ui_defaults();\n> \tgit_config(fn, s);\n> \tdetermine_whence(s);\n> -\tinit_diff_ui_defaults();\n> \ts->hints = advice_status_hints; /* must come after git_config() */\n> }\n\nThat init_diff_ui_defaults() should indeed have been before\ngit_config() from the beginning. My bad, I'm the one who\nmisplaced it apparently :-(.\n\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -625,9 +625,6 @@ static void wt_status_collect_changes_index(struct wt_status\n> *s)\n> \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n> \trev.diffopt.format_callback = wt_status_collect_updated_cb;\n> \trev.diffopt.format_callback_data = s;\n> -\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n> -\trev.diffopt.rename_limit = 200;\n> -\trev.diffopt.break_opt = 0;\n\nThis \"break_opt = 0\" deserves a mention in the commit message\nIMHO. I'm not 100% sure it's a good change actually.\n\nbreak_opt is normally controlled by \"-B/--break-rewrites\".\nI'm not sure why it was set to 0.\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"346242","messageId":"20180501113706.GA13919@esm","threadId":"48392","inReplyTo":"87bmdzzlll.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-01T11:37:06Z","receivedAt":"2018-05-01T11:37:13Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Tue, May 01, 2018 at 01:00:54PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> So didn't we use diff heuristics to begin with, and then regressed? I've\n> only given this a skimming, but it's useful to have that sort of\n> historical context mentioned explicitly with commit ids.\n\nSorry for not making this too explicit: I traced wt-status.c to its\nbeginning in c91f0d92ef (\"git-commit.sh: convert run_status to a C\nbuiltin\", 2006-09-08). Here I lost track of other changes - but the\ncommit you gave is earlier also has rename detection hard coded in the\nstatus command. Should I add this as a starting point instead of the\ncommit mentioned so far? And this also seems like the very beginning of\ngit status.\n\nThe point I wanted to make, is that git show showed you renaming of\nfiles out of the box whereas other commands did not. At least I remember\nthis form 5+ years ago.\n\nThis changed with 5404c116aa, so that the out of the box behaviour is\nthe same, but this is more a coincidence that the hard coded flag is the\nsame as the default configuration value.\n\nMy comment was targeted at the hard coded rename detection flag - the\nother two just have seem to pile up on that and I wanted to clean them\nup, too. Maybe a phrasing like \"While at it, also remove the two other\nhard coded values concerning rename detection in git status.\" is better?\nIs my intent clearer now?\n\nGreetings,\nEckhard\n"},{"id":"346246","messageId":"20180501114316.GB13919@esm","threadId":"48392","inReplyTo":"907020160.11403426.1525172946040.JavaMail.zimbra@inria.fr","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-01T11:43:16Z","receivedAt":"2018-05-01T11:43:23Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Tue, May 01, 2018 at 01:09:06PM +0200, Matthieu Moy wrote:\n> That init_diff_ui_defaults() should indeed have been before\n> git_config() from the beginning. My bad, I'm the one who\n> misplaced it apparently :-(.\n\nShould I have done this \"bug fix\" in a separate commit or mention it in\nthe commit message?\n\n> This \"break_opt = 0\" deserves a mention in the commit message IMHO.\n> I'm not 100% sure it's a good change actually.\n\nHm, what problems do you see here? The purpose of my patch is have the\nsame kind of output from git status and, after commit, git show. I\ncannot find a good reason for git status to behave there differently,\nbut I am interested to see where I am wrong.\n\nOn the other hand, one could a configuration option to the diff.* family\nfor controlling that toggle also.\n\nGreetings,\nEckhard\n"},{"id":"346284","messageId":"1652522802.213664.1525177431907.JavaMail.zimbra@matthieu-moy.fr","threadId":"48392","inReplyTo":"20180501114316.GB13919@esm","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Matthieu Moy","fromEmail":"git@matthieu-moy.fr","sentAt":"2018-05-01T12:23:51Z","receivedAt":"2018-05-01T12:56:38Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\"Eckhard Maaß\" <eckhard.s.maass@googlemail.com>:\n\n> On Tue, May 01, 2018 at 01:09:06PM +0200, Matthieu Moy wrote:\n> > That init_diff_ui_defaults() should indeed have been before\n> > git_config() from the beginning. My bad, I'm the one who\n> > misplaced it apparently :-(.\n\n> Should I have done this \"bug fix\" in a separate commit or mention it in\n> the commit message?\n\nI'm fine with it as-is. Before your \"fix\", the config was ignored\nbecause overwritten by init_diff_ui_defaults() after reading the\nconfig, so effect of your change is indeed what the commit message\ndescribes.\n\nI'm often thinking aloud while reviewing, don't take my comments as\nobjections.\n\n> > This \"break_opt = 0\" deserves a mention in the commit message IMHO.\n> > I'm not 100% sure it's a good change actually.\n\n> Hm, what problems do you see here?\n\nI don't see any \"problem\", I *think* your change is good, but I can't\nfully convince myself that it is without further explanation.\n\nUnlike the other two, this option has no corresponding configuration\nvariable, so the \"let the config\" argument doesn't apply. For \"git\nstatus\", there's actually not even a command-line option. So, this\nassignment removed, there's no way in the user-interface to re-enable\nthe previous behavior. *If* there was a good reason to get \"break_opt\n= 0\", then your patch is breaking it.\n\nUnfortunately, the commit introducing it doesn't help much: f714fb8\n(Enable rewrite as well as rename detection in git-status,\n2007-12-02) is just a one-liner message with a one-liner patch.\n\nBut actually, I never used -B/--break-rewrites, and writting this\nmessage I tried to get a case where -B would make a difference and I'm\nnot even able to find one. So, as someone who never understood the\nreal point of -B, I'm not sure I'm qualified to juge on what's the\nbest default ;-).\n\n-- \nMatthieu Moy\nhttps://matthieu-moy.fr/\n"},{"id":"346295","messageId":"CABPp-BFbVP3iwAbaa2cEPw9Sr+ANJoHHYHOCQ4oAZoVdyX164A@mail.gmail.com","threadId":"48392","inReplyTo":"1652522802.213664.1525177431907.JavaMail.zimbra@matthieu-moy.fr","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-01T15:52:30Z","receivedAt":"2018-05-01T15:52:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 1, 2018 at 5:23 AM, Matthieu Moy <git@matthieu-moy.fr> wrote:\n> \"Eckhard Maaß\" <eckhard.s.maass@googlemail.com>:\n>\n>> On Tue, May 01, 2018 at 01:09:06PM +0200, Matthieu Moy wrote:\n>> > That init_diff_ui_defaults() should indeed have been before\n>> > git_config() from the beginning. My bad, I'm the one who\n>> > misplaced it apparently :-(.\n>\n>> Should I have done this \"bug fix\" in a separate commit or mention it in\n>> the commit message?\n>\n> I'm fine with it as-is. Before your \"fix\", the config was ignored\n> because overwritten by init_diff_ui_defaults() after reading the\n> config, so effect of your change is indeed what the commit message\n> describes.\n>\n> I'm often thinking aloud while reviewing, don't take my comments as\n> objections.\n>\n>> > This \"break_opt = 0\" deserves a mention in the commit message IMHO.\n>> > I'm not 100% sure it's a good change actually.\n>\n>> Hm, what problems do you see here?\n>\n> I don't see any \"problem\", I *think* your change is good, but I can't\n> fully convince myself that it is without further explanation.\n>\n> Unlike the other two, this option has no corresponding configuration\n> variable, so the \"let the config\" argument doesn't apply. For \"git\n> status\", there's actually not even a command-line option. So, this\n> assignment removed, there's no way in the user-interface to re-enable\n> the previous behavior. *If* there was a good reason to get \"break_opt\n> = 0\", then your patch is breaking it.\n>\n> Unfortunately, the commit introducing it doesn't help much: f714fb8\n> (Enable rewrite as well as rename detection in git-status,\n> 2007-12-02) is just a one-liner message with a one-liner patch.\n>\n> But actually, I never used -B/--break-rewrites, and writting this\n> message I tried to get a case where -B would make a difference and I'm\n> not even able to find one. So, as someone who never understood the\n> real point of -B, I'm not sure I'm qualified to juge on what's the\n> best default ;-).\n\nIn git.git, just make non-sensical changes like the following (a\nnormal rename, and a break-rename, for comparison):\n\n    git mv oidset.c another-file.c\n    echo \"// Modifying normally renamed file for fun\" >>another-file.c\n    git rm merge.c\n    git mv object.c merge.c\n    echo \"// Random change to break-rename file\" >>merge.c\n    git add merge.c another-file.c\n\nNow compare, git before Eckhard's change:\n\n$ /usr/bin/git status\nHEAD detached at v2.17.0\nChanges to be committed:\n  (use \"git reset HEAD <file>...\" to unstage)\n\n    renamed:    oidset.c -> another-file.c\n    renamed:    object.c -> merge.c\n\nand git after Eckhard's change:\n\n$ git status\nHEAD detached at v2.17.0\nChanges to be committed:\n  (use \"git reset HEAD <file>...\" to unstage)\n\n    renamed:    oidset.c -> another-file.c\n    modified:   merge.c\n    deleted:    object.c\n\nWhich is better?  Well, gut reaction only looking at the above output\nfolks would probably say the former is. However, compare the output to\nthis:\n\n$ git diff --name-status HEAD\nR094    oidset.c        another-file.c\nM       merge.c\nD       object.c\n\ngit status and git diff are inconsistent for no good reason.  We can\ninstruct diff to behave the same as old status, of course:\n\n$ git diff --name-status -B HEAD\nR094    oidset.c        another-file.c\nR099    object.c        merge.c\n\n...but why does the user have to instruct diff to get the same default\nbehavior they get from status?  I'll note here that log and show have\nthe same default as diff.\n\n\nI'm not certain what the default should be, but I do believe that it\nshould be consistent between these commands.  I lean towards\nconsidering break detection being on by default a good thing, but\nthere are some interesting issues to address:\n  - there is no knob to adjust break detection for status, only for\ndiff/log/show/etc.\n  - folks have a knob to turn break detection on (for diff/log/show),\nbut not one to turn it off\n  - for status, break detection makes no sense if rename detection is off.\n  - for diff/log/show, break detection provides almost no value if\nrename detection is off (only a dissimilarity index), suggesting that\nif user turns rename detection off and doesn't explicitly ask for\nbreak detection, then it's a waste of time to have it be on\n  - merge-recursive would break horribly right now if someone turned\nbreak detection on for it.  Turning it on might be a good long term\ngoal, but it's not an easy change.\n\n\nSo, where does that leave us?  My opinion is\n  - these commands should be consistent.  Eckhard's patch makes them so.\n  - we might want to move towards break detection being on as the\ndefault.  That's a couple patch series (one for everything but\nmerge-recursive, and a separate much bigger series for\nmerge-recursive).\n\nBut I can see others saying we should leave things inconsistent until\nwe can fix the other commands to use break detection as the default.\nSo...thoughts?\n\nElijah\n"},{"id":"346296","messageId":"20180501160911.GA14477@esm","threadId":"48392","inReplyTo":"1652522802.213664.1525177431907.JavaMail.zimbra@matthieu-moy.fr","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-01T16:09:11Z","receivedAt":"2018-05-01T16:09:18Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Tue, May 01, 2018 at 02:23:51PM +0200, Matthieu Moy wrote:\n> I'm fine with it as-is. Before your \"fix\", the config was ignored\n> because overwritten by init_diff_ui_defaults() after reading the\n> config, so effect of your change is indeed what the commit message\n> describes.\n> \n> I'm often thinking aloud while reviewing, don't take my comments as\n> objections.\n\nNo worries, I was wondering while writing the patch to extract it - the\ninit should be changed to the appropriate location even if there is\nconsensus to leave all the other knobs as they are, shouldn't it?\n\nGreetings,\nEckhard\n"},{"id":"346350","messageId":"xmqqlgd3x972.fsf@gitster-ct.c.googlers.com","threadId":"48392","inReplyTo":"CABPp-BFbVP3iwAbaa2cEPw9Sr+ANJoHHYHOCQ4oAZoVdyX164A@mail.gmail.com","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-01T23:11:45Z","receivedAt":"2018-05-01T23:11:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> I'm not certain what the default should be, but I do believe that it\n> should be consistent between these commands.  I lean towards\n> considering break detection being on by default a good thing, but\n> there are some interesting issues to address:\n>   - there is no knob to adjust break detection for status, only for\n> diff/log/show/etc.\n>   - folks have a knob to turn break detection on (for diff/log/show),\n> but not one to turn it off\n>   - for status, break detection makes no sense if rename detection is off.\n>   - for diff/log/show, break detection provides almost no value if\n> rename detection is off (only a dissimilarity index), suggesting that\n> if user turns rename detection off and doesn't explicitly ask for\n> break detection, then it's a waste of time to have it be on\n>   - merge-recursive would break horribly right now if someone turned\n> break detection on for it.  Turning it on might be a good long term\n> goal, but it's not an easy change.\n\nMany of the issues in the above list are surmountable.  A new option\ncould be added to \"status\" to enable break or \"diff\" family to\ndisable it if we really wanted to.  A new \"rewritten\" state can be\nadded alongside with \"modified\" to \"status\" output.\n\nA more serious issue around \"-B\" is this one:\n\n    https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/\n\nEven though the message is back from 2015 and asks users not to use\n-B/-M together for anything critical \"for now\", the issue has not\nbeen resolved and the same bug remains with us in the current code.\n\nIn the longer term, I suspect that it might make sense to have an\noption to let users choose among \"I do not want to have anything to\ndo with -B\", \"I always want -B when I ask for -M\" and \"I always want\n-B whether I ask for -M\".  But unfortunately the latter two with the\ncurrent codebase is an unacceptably risky/broken choice.\n\n>\n> So, where does that leave us?  My opinion is\n>   - these commands should be consistent.  Eckhard's patch makes them so.\n>   - we might want to move towards break detection being on as the\n> default.  That's a couple patch series (one for everything but\n> merge-recursive, and a separate much bigger series for\n> merge-recursive).\n>\n> But I can see others saying we should leave things inconsistent until\n> we can fix the other commands to use break detection as the default.\n> So...thoughts?\n>\n> Elijah\n"},{"id":"346355","messageId":"CABPp-BELX8u_CG8BswenAKCo8yvfxxOAOHjAbvh8jAm9gN7Qgw@mail.gmail.com","threadId":"48392","inReplyTo":"xmqqlgd3x972.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-02T00:08:27Z","receivedAt":"2018-05-02T00:08:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 1, 2018 at 4:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> I'm not certain what the default should be, but I do believe that it\n>> should be consistent between these commands.  I lean towards\n>> considering break detection being on by default a good thing, but\n>> there are some interesting issues to address:\n>>   - there is no knob to adjust break detection for status, only for\n>> diff/log/show/etc.\n>>   - folks have a knob to turn break detection on (for diff/log/show),\n>> but not one to turn it off\n>>   - for status, break detection makes no sense if rename detection is off.\n>>   - for diff/log/show, break detection provides almost no value if\n>> rename detection is off (only a dissimilarity index), suggesting that\n>> if user turns rename detection off and doesn't explicitly ask for\n>> break detection, then it's a waste of time to have it be on\n>>   - merge-recursive would break horribly right now if someone turned\n>> break detection on for it.  Turning it on might be a good long term\n>> goal, but it's not an easy change.\n>\n> Many of the issues in the above list are surmountable.  A new option\n> could be added to \"status\" to enable break or \"diff\" family to\n> disable it if we really wanted to.  A new \"rewritten\" state can be\n> added alongside with \"modified\" to \"status\" output.\n>\n> A more serious issue around \"-B\" is this one:\n>\n>     https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/\n>\n> Even though the message is back from 2015 and asks users not to use\n> -B/-M together for anything critical \"for now\", the issue has not\n> been resolved and the same bug remains with us in the current code.\n>\n> In the longer term, I suspect that it might make sense to have an\n> option to let users choose among \"I do not want to have anything to\n> do with -B\", \"I always want -B when I ask for -M\" and \"I always want\n> -B whether I ask for -M\".  But unfortunately the latter two with the\n> current codebase is an unacceptably risky/broken choice.\n\nVery interesting; I didn't know that break detection and rename\ndetection were unsafe to use together.\n\nI also just realized that in addition to status being inconsistent\nwith diff/log/show, it was also inconsistent with itself -- it handles\nstaged and unstaged changes differently.\n(wt_status_collect_changes_worktree() had different settings for break\ndetection than wt_status_collect_changes_index().)\n\nEckhard, can you add some comments to your commit message mentioning\nthe email pointed to by Junio about break detection and rename\ndetection being unsafe to use together, as well as the inconsistencies\nin break detection between different commands?  That may help future\nreaders understand why break detection was turned off for\nwt_status_collect_changes_index().\n"},{"id":"346452","messageId":"443ada66-708e-2034-437f-350797ef9439@gmail.com","threadId":"48392","inReplyTo":"CABPp-BELX8u_CG8BswenAKCo8yvfxxOAOHjAbvh8jAm9gN7Qgw@mail.gmail.com","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-02T14:20:16Z","receivedAt":"2018-05-02T14:20:22Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/1/2018 8:08 PM, Elijah Newren wrote:\n> On Tue, May 1, 2018 at 4:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Elijah Newren <newren@gmail.com> writes:\n>>\n\n> \n> I also just realized that in addition to status being inconsistent\n> with diff/log/show, it was also inconsistent with itself -- it handles\n> staged and unstaged changes differently.\n> (wt_status_collect_changes_worktree() had different settings for break\n> detection than wt_status_collect_changes_index().)\n> \n> Eckhard, can you add some comments to your commit message mentioning\n> the email pointed to by Junio about break detection and rename\n> detection being unsafe to use together, as well as the inconsistencies\n> in break detection between different commands?  That may help future\n> readers understand why break detection was turned off for\n> wt_status_collect_changes_index().\n> \n\nWow, lots of inconsistent behaviors here with diff/merge/status and the \nvarious options being used.  Let me add another one:\n\nI've been sitting on another patch that we've been using internally for \nsome time that enables us to turn off rename and break detection for \nstatus via config settings and new command-line options.\n\nThe issue that triggered the creation of the patch was that if someone \nran status while in a merge conflict state, the status would take a very \nlong time.  Turning off rename and break detection \"fixed\" the problem.\n\nI was waiting for some of these inflight changes to settle down and get \naccepted before I started another patch series but I thought I should at \nleast let everyone know about this additional issue that will need to be \naddressed.\n"},{"id":"346501","messageId":"20180503052257.GA7576@esm","threadId":"48392","inReplyTo":"CABPp-BELX8u_CG8BswenAKCo8yvfxxOAOHjAbvh8jAm9gN7Qgw@mail.gmail.com","subject":"Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-03T05:22:57Z","receivedAt":"2018-05-03T05:23:04Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Tue, May 01, 2018 at 05:08:27PM -0700, Elijah Newren wrote:\n> Eckhard, can you add some comments to your commit message mentioning\n> the email pointed to by Junio about break detection and rename\n> detection being unsafe to use together, as well as the inconsistencies\n> in break detection between different commands?\n\nI will work on that.\n\nGreetings,\nEckhard\n"},{"id":"346644","messageId":"20180504111215.5975-1-eckhard.s.maass@gmail.com","threadId":"48392","inReplyTo":"CABPp-BELX8u_CG8BswenAKCo8yvfxxOAOHjAbvh8jAm9gN7Qgw@mail.gmail.com","subject":"[PATCH v3] wt-status: use settings from git_diff_ui_config","fromName":"Eckhard S. Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-04T11:12:15Z","receivedAt":"2018-05-04T11:12:29Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"If you do something like\n\n    - git add .\n    - git status\n    - git commit\n    - git show (or git diff HEAD)\n\none would expect to have analogous output from git status and git show\n(or similar diff-related programs). This is generally not the case, as\ngit status has hard coded values for diff related options.\n\nWith this commit the hard coded settings are dropped from the status\ncommand in favour for values provided by git_diff_ui_config.\n\nWhat follows are some remarks on the concrete options which were hard\ncoded in git status:\n\ndiffopt.detect_rename\n\nSince the very beginning of git status in a3e870f2e2 (\"Add \"commit\"\nhelper script\", 2005-05-30), git status always used rename detection,\nwhereas with commands like show and log one had to activate it with a\ncommand line option. After 5404c116aa (\"diff: activate diff.renames by\ndefault\", 2016-02-25) the default behaves the same by coincidence, but\nchanging diff.renames to other values can break the consistency between\ngit status and other commands again. With this commit one control the\nsame default behaviour with diff.renames.\n\ndiffopt.rename_limit\n\nSimilarly one has the option diff.renamelimit to adjust this limit for\nall commands but git status. With this commit git status will also honor\nthose.\n\ndiffopt.break_opt\n\nUnlike the other two options this cannot be configured by a\nconfiguration option yet. This commit will also change the default\nbehaviour to not use break rewrites. But as rename detection is most\nlikely on, this is dangerous to be activated anyway as one can see\nhere:\n\n    https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/\n\nSigned-off-by: Eckhard S. Maaß <eckhard.s.maass@gmail.com>\nReviewed-by: Elijah Newren <newren@gmail.com>\n---\n\nHello,\n\nHopefully I have addressed all issues that have come up so far.\n\nGreetings,\nEckhard\n\n builtin/commit.c       |  2 +-\n t/t4001-diff-rename.sh | 12 ++++++++++++\n wt-status.c            |  4 ----\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5571d4a3e2..5240f11225 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -161,9 +161,9 @@ static void determine_whence(struct wt_status *s)\n static void status_init_config(struct wt_status *s, config_fn_t fn)\n {\n \twt_status_prepare(s);\n+\tinit_diff_ui_defaults();\n \tgit_config(fn, s);\n \tdetermine_whence(s);\n-\tinit_diff_ui_defaults();\n \ts->hints = advice_status_hints; /* must come after git_config() */\n }\n \ndiff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh\nindex a07816d560..bf4030371a 100755\n--- a/t/t4001-diff-rename.sh\n+++ b/t/t4001-diff-rename.sh\n@@ -138,6 +138,18 @@ test_expect_success 'favour same basenames over different ones' '\n \ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n '\n \n+test_expect_success 'test diff.renames=true for git status' '\n+\tgit -c diff.renames=true status >out &&\n+\ttest_i18ngrep \"renamed: .*path1 -> subdir/path1\" out\n+'\n+\n+test_expect_success 'test diff.renames=false for git status' '\n+\tgit -c diff.renames=false status >out &&\n+\ttest_i18ngrep ! \"renamed: .*path1 -> subdir/path1\" out &&\n+\ttest_i18ngrep \"new file: .*subdir/path1\" out &&\n+\ttest_i18ngrep \"deleted: .*[^/]path1\" out\n+'\n+\n test_expect_success 'favour same basenames even with minor differences' '\n \tgit show HEAD:path1 | sed \"s/15/16/\" > subdir/path1 &&\n \tgit status >out &&\ndiff --git a/wt-status.c b/wt-status.c\nindex 50815e5faf..32f3bcaebd 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -625,9 +625,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)\n \trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = wt_status_collect_updated_cb;\n \trev.diffopt.format_callback_data = s;\n-\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n-\trev.diffopt.rename_limit = 200;\n-\trev.diffopt.break_opt = 0;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n }\n@@ -985,7 +982,6 @@ static void wt_longstatus_print_verbose(struct wt_status *s)\n \tsetup_revisions(0, NULL, &rev, &opt);\n \n \trev.diffopt.output_format |= DIFF_FORMAT_PATCH;\n-\trev.diffopt.detect_rename = DIFF_DETECT_RENAME;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\n-- \n2.17.0.252.gfe0a9eaf31\n\n"},{"id":"346653","messageId":"CABPp-BEujnVhp11L38+LJN+Tv-SoWX2YT80LKX+HZCFjta5hdg@mail.gmail.com","threadId":"48392","inReplyTo":"20180504111215.5975-1-eckhard.s.maass@gmail.com","subject":"Re: [PATCH v3] wt-status: use settings from git_diff_ui_config","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-04T15:13:28Z","receivedAt":"2018-05-04T15:13:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, May 4, 2018 at 4:12 AM, Eckhard S. Maaß\n<eckhard.s.maass@googlemail.com> wrote:\n> If you do something like\n>\n>     - git add .\n>     - git status\n>     - git commit\n>     - git show (or git diff HEAD)\n>\n> one would expect to have analogous output from git status and git show\n> (or similar diff-related programs). This is generally not the case, as\n> git status has hard coded values for diff related options.\n>\n<snip>\n> ---\n>\n> Hello,\n>\n> Hopefully I have addressed all issues that have come up so far.\n\nLooks good to me, thanks.\n\nElijah\n"}]}