{"thread":{"id":"48442","subject":"[PATCH v1] add status config and command line options for rename detection","startedAt":"2018-05-09T14:42:39Z","lastAt":"2018-05-14T12:57:53Z","messageCount":17,"participants":["Ben Peart","Duy Nguyen","Elijah Newren","Junio C Hamano","Eckhard Maaß"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347083","messageId":"20180509144213.18032-1-benpeart@microsoft.com","threadId":"48442","inReplyTo":null,"subject":"[PATCH v1] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-09T14:42:34Z","receivedAt":"2018-05-09T14:42:39Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Add a new config status.renames setting to enable turning off rename detection\nduring status. This setting will default to the value of diff.renames.\n\nAdd a new config status.renamelimit setting to to enable bounding the time spent\nfinding out inexact renames during status. This setting will default to the\nvalue of diff.renamelimit.\n\nAdd status --no-renames command line option that enables overriding the config\nsetting from the command line. Add --find-renames[=] to enable detecting\nrenames and optionaly setting the similarity index from the command line.\n\nOrigional-Patch-by: Alejandro Pauly \nSigned-off-by: Ben Peart \n---\n\nNotes:\n Base Ref:\n Web-Diff: https://github.com/benpeart/git/commit/aa977d2964\n Checkout: git fetch https://github.com/benpeart/git status-renames-v1 && git checkout aa977d2964\n\n Documentation/config.txt | 9 ++++\n builtin/commit.c | 57 +++++++++++++++++++++++++\n diff.c | 2 +-\n diff.h | 1 +\n t/t7525-status-rename.sh | 90 ++++++++++++++++++++++++++++++++++++++++\n wt-status.c | 12 ++++++\n wt-status.h | 4 +-\n 7 files changed, 173 insertions(+), 2 deletions(-)\n create mode 100644 t/t7525-status-rename.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2659153cb3..b79b83c587 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3119,6 +3119,15 @@ status.displayCommentPrefix::\n \tbehavior of linkgit:git-status[1] in Git 1.8.4 and previous.\n \tDefaults to false.\n \n+status.renameLimit::\n+\tThe number of files to consider when performing rename detection;\n+\tif not specified, defaults to the value of diff.renameLimit.\n+\n+status.renames::\n+\tWhether and how Git detects renames. If set to \"false\",\n+\trename detection is disabled. If set to \"true\", basic rename\n+\tdetection is enabled. Defaults to the value of diff.renames.\n+\n status.showStash::\n \tIf set to true, linkgit:git-status[1] will display the number of\n \tentries currently stashed away.\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5240f11225..a545096ddd 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -109,6 +109,10 @@ static int have_option_m;\n static struct strbuf message = STRBUF_INIT;\n \n static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;\n+static int diff_detect_rename = -1;\n+static int status_detect_rename = -1;\n+static int diff_rename_limit = -1;\n+static int status_rename_limit = -1;\n \n static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)\n {\n@@ -143,6 +147,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)\n+{\n+\tconst char **value = opt->value;\n+\tif (arg != NULL && *arg == '=')\n+\t\targ = arg + 1;\n+\n+\t*value = arg;\n+\treturn 0;\n+}\n+\n static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head()))\n@@ -1259,11 +1273,29 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\t\treturn error(_(\"Invalid untracked files mode '%s'\"), v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"diff.renamelimit\")) {\n+\t\tdiff_rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renamelimit\")) {\n+\t\tstatus_rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"diff.renames\")) {\n+\t\tdiff_detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renames\")) {\n+\t\tstatus_detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_ui_config(k, v, NULL);\n }\n \n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n+\tstatic int no_renames = -1;\n+\tstatic const char *rename_score_arg = (const char *)-1;\n \tstatic struct wt_status s;\n \tint fd;\n \tstruct object_id oid;\n@@ -1297,6 +1329,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t N_(\"ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)\"),\n \t\t PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n \t\tOPT_COLUMN(0, \"column\", &s.colopts, N_(\"list untracked files in columns\")),\n+\t\tOPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n+\t\t{ OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n+\t\t N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n+\t\t PARSE_OPT_OPTARG, opt_parse_rename_score },\n \t\tOPT_END(),\n \t};\n \n@@ -1336,6 +1372,27 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \ts.ignore_submodule_arg = ignore_submodule_arg;\n \ts.status_format = status_format;\n \ts.verbose = verbose;\n+\ts.detect_rename = no_renames >= 0 ? !no_renames :\n+\t\t\t\t\t status_detect_rename >= 0 ? status_detect_rename :\n+\t\t\t\t\t diff_detect_rename >= 0 ? diff_detect_rename :\n+\t\t\t\t\t s.detect_rename;\n+\tif ((intptr_t)rename_score_arg != -1) {\n+\t\ts.detect_rename = DIFF_DETECT_RENAME;\n+\t\tif (rename_score_arg)\n+\t\t\ts.rename_score = parse_rename_score(&rename_score_arg);\n+\t}\n+\ts.rename_limit = status_rename_limit >= 0 ? status_rename_limit :\n+\t\t\t\t\t diff_rename_limit >= 0 ? diff_rename_limit :\n+\t\t\t\t\t s.rename_limit;\n+\n+\t/*\n+\t * We do not have logic to handle the detection of copies. In\n+\t * fact, it may not even make sense to add such logic: would we\n+\t * really want a change to a base file to be propagated through\n+\t * multiple other files by a merge?\n+\t */\n+\tif (s.detect_rename > DIFF_DETECT_RENAME)\n+\t\ts.detect_rename = DIFF_DETECT_RENAME;\n \n \twt_status_collect(&s);\n \ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..5dfc24aa6d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n-static int git_config_rename(const char *var, const char *value)\n+int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn DIFF_DETECT_RENAME;\ndiff --git a/diff.h b/diff.h\nindex d29560f822..dedac472ca 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -324,6 +324,7 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n extern void diff_setup_done(struct diff_options *);\n+extern int git_config_rename(const char *var, const char *value);\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\nnew file mode 100644\nindex 0000000000..311df8038a\n--- /dev/null\n+++ b/t/t7525-status-rename.sh\n@@ -0,0 +1,90 @@\n+#!/bin/sh\n+\n+test_description='git status rename detection options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo 1 >original &&\n+\tgit add . &&\n+\tgit commit -m\"Adding original file.\" &&\n+\tmv original renamed &&\n+\techo 2 >> renamed &&\n+\tgit add .\n+'\n+\n+cat >.gitignore <<\\EOF\n+.gitignore\n+expect*\n+actual*\n+EOF\n+\n+test_expect_success 'status no-options' '\n+\tgit status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status --no-renames' '\n+\tgit status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames false' '\n+\tgit -c diff.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames true' '\n+\tgit -c diff.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides diff.renames false' '\n+\tgit -c diff.renames=true -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides from diff.renames true' '\n+\tgit -c diff.renames=false -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status status.renames=false' '\n+\tgit -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status status.renames=true' '\n+\tgit -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status config overriden' '\n+\tgit -c status.renames=true status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=100%' '\n+\tgit status -M=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual &&\n+\n+\tgit status --find-rename=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=01%' '\n+\tgit status -M=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual &&\n+\n+\tgit status --find-rename=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 32f3bcaebd..172f07cbb0 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -138,6 +138,9 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_stash = 0;\n \ts->ahead_behind_flags = AHEAD_BEHIND_UNSPECIFIED;\n \ts->display_comment_prefix = 0;\n+\ts->detect_rename = -1;\n+\ts->rename_score = -1;\n+\ts->rename_limit = -1;\n }\n \n static void wt_longstatus_print_unmerged_header(struct wt_status *s)\n@@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \t}\n \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n \trev.diffopt.format_callback_data = s;\n+\trev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n }\n@@ -625,6 +631,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n }\n@@ -982,6 +991,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\ndiff --git a/wt-status.h b/wt-status.h\nindex 430770b854..1673d146fa 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -89,7 +89,9 @@ struct wt_status {\n \tint show_stash;\n \tint hints;\n \tenum ahead_behind_flags ahead_behind_flags;\n-\n+\tint detect_rename;\n+\tint rename_score;\n+\tint rename_limit;\n \tenum wt_status_format status_format;\n \tunsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */\n \n\nbase-commit: a92ae92585d8db14b7871f760f157256cd96742c\n-- \n2.17.0.windows.1\n\n"},{"id":"347090","messageId":"CACsJy8CdvKO3aityyP3Ax0ZqaS6JzwH_i2Gn_8NmCUDKHMMQrw@mail.gmail.com","threadId":"48442","inReplyTo":"20180509144213.18032-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] add status config and command line options for rename detection","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-05-09T15:59:19Z","receivedAt":"2018-05-09T15:59:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, May 9, 2018 at 4:42 PM, Ben Peart wrote:\n> Add a new config status.renames setting to enable turning off rename detection\n> during status. This setting will default to the value of diff.renames.\n\nPlease add the reason you need this config key in the commit message.\nMy guess (probably correct) is on super large repo (how large?),\nrename detection is just too slow (how long?) that it practically\nmakes git-status unusable.\n\nThis information could be helpful when we optimize rename detection to\nbe more efficient.\n\n>\n> Add a new config status.renamelimit setting to to enable bounding the time spent\n> finding out inexact renames during status. This setting will default to the\n> value of diff.renamelimit.\n>\n> Add status --no-renames command line option that enables overriding the config\n> setting from the command line. Add --find-renames[=] to enable detecting\n> renames and optionaly setting the similarity index from the command line.\n>\n> Origional-Patch-by: Alejandro Pauly \n> Signed-off-by: Ben Peart \n-- \nDuy\n"},{"id":"347100","messageId":"CABPp-BEkQN55diiovv+33P=Ouk+FcK8N4p85EZZqVmw-mxuL1A@mail.gmail.com","threadId":"48442","inReplyTo":"20180509144213.18032-1-benpeart@microsoft.com","subject":"Re: [PATCH v1] add status config and command line options for rename detection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-09T16:56:14Z","receivedAt":"2018-05-09T16:56:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOverall I think this is good, but I have lots of nit-picky things to\nbring up. :-)\n\n\nOn Wed, May 9, 2018 at 7:42 AM, Ben Peart wrote:\n> Add status --no-renames command line option that enables overriding the config\n> setting from the command line. Add --find-renames[=] to enable detecting\n> renames and optionaly setting the similarity index from the command line.\n\ns/optionaly/optionally/\n\n> Notes:\n> Base Ref:\n\nNo base ref? ;-)\n\n> +status.renameLimit::\n> + The number of files to consider when performing rename detection;\n> + if not specified, defaults to the value of diff.renameLimit.\n> +\n> +status.renames::\n> + Whether and how Git detects renames. If set to \"false\",\n> + rename detection is disabled. If set to \"true\", basic rename\n> + detection is enabled. Defaults to the value of diff.renames.\n\nI suspect that status.renames should mention \"copy\", just as\ndiff.renames does. (We didn't mention it in merge.renames, because\nmerge isn't an operation for which copy detection can be useful -- at\nleast not until the diffstat at the end of the merge is controlled by\nmerge.renames as well...)\n\nAlso, do these two config settings only affect 'git status', or does\nit also affect the status shown when composing a commit message\n(assuming the user hasn't turned commit.status off)? Does it affect\n`git commit --dry-run` too?\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -109,6 +109,10 @@ static int have_option_m;\n> static struct strbuf message = STRBUF_INIT;\n>\n> static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;\n> +static int diff_detect_rename = -1;\n> +static int status_detect_rename = -1;\n> +static int diff_rename_limit = -1;\n> +static int status_rename_limit = -1;\n\nCould we replace these four variables with just two: detect_rename and\nrename_limit? Keeping these separate invites people to write code\nusing only one of the settings rather than the appropriate inherited\nmixture of them, resulting in a weird bug. More on this below...\n\n> @@ -1259,11 +1273,29 @@ static int git_status_config(const char *k, const char *v, void *cb)\n> return error(_(\"Invalid untracked files mode '%s'\"), v);\n> return 0;\n> }\n> + if (!strcmp(k, \"diff.renamelimit\")) {\n> + diff_rename_limit = git_config_int(k, v);\n> + return 0;\n> + }\n> + if (!strcmp(k, \"status.renamelimit\")) {\n> + status_rename_limit = git_config_int(k, v);\n> + return 0;\n> + }\n\nHere, since you're already checking diff.renamelimit first, you can\njust set rename_limit in both blocks and not need both\ndiff_rename_limit and status_rename_limit. (Similar can be said for\ndiff.renames/status.renames.)\n\n\n\n> @@ -1297,6 +1329,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> N_(\"ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)\"),\n> PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n> OPT_COLUMN(0, \"column\", &s.colopts, N_(\"list untracked files in columns\")),\n> + OPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n> + { OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n> + N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n> + PARSE_OPT_OPTARG, opt_parse_rename_score },\n\nShould probably also document these options in\nDocumentation/git-status.txt (and maybe Documentation/git-commit.txt\nas well).\n\nNot sure if we want to add a flag for copy detection (similar to\ngit-diff's -C/--find-copies and --find-copies-harder), or just leave\nthat for when someone finds a need. If left out, might want to just\nmention that it was considered and intentionally omitted for now in\nthe commit message.\n\n> @@ -1336,6 +1372,27 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n> s.ignore_submodule_arg = ignore_submodule_arg;\n> s.status_format = status_format;\n> s.verbose = verbose;\n> + s.detect_rename = no_renames >= 0 ? !no_renames :\n> + status_detect_rename >= 0 ? status_detect_rename :\n> + diff_detect_rename >= 0 ? diff_detect_rename :\n\nCombining the four vars into two as mentioned above would allow\ncombining the last two lines above into one.\n\n> + if ((intptr_t)rename_score_arg != -1) {\n\nI don't understand why rename_score_arg is a (char*) and then you need\nto cast to intptr_t, but that may just be because I haven't done much\nof anything with option parsing. A quick look at the docs isn't\nmaking it clear to me, though; could you enlighten me?\n\n> + s.detect_rename = DIFF_DETECT_RENAME;\n\nWhat if status.renames is 'copy' but someone wants to override the\nscore for detecting renames and pass --find-renames=40? Does the\n--find-renames override and degrade the 'copy'? I think it'd make\nmore sense to increase s.detect_rename to at least DIFF_DETECT_RENAME,\nrather than just outright setting it here.\n\n> + if (rename_score_arg)\n> + s.rename_score = parse_rename_score(&rename_score_arg);\n> + }\n> + s.rename_limit = status_rename_limit >= 0 ? status_rename_limit :\n> + diff_rename_limit >= 0 ? diff_rename_limit :\n\nAgain, combination of variables could allow these last two lines to be combined.\n\n> + s.rename_limit;\n> +\n> + /*\n> + * We do not have logic to handle the detection of copies. In\n> + * fact, it may not even make sense to add such logic: would we\n> + * really want a change to a base file to be propagated through\n> + * multiple other files by a merge?\n> + */\n> + if (s.detect_rename > DIFF_DETECT_RENAME)\n> + s.detect_rename = DIFF_DETECT_RENAME;\n\nThis comment and code made sense in merge-recursive.c (which doesn't\nshow detected diffs/renames/copies but just uses them for internal\nprocessing logic). It does not make sense here; git status could show\ndetected copies much like `git diff -C --name-status` shows it. In\nfact, a quick grep for DIFF_STATUS_COPIED shows multiple hits in\nwt-status.c, so I suspect it already has the necessary logic for\ndisplaying copies.\n\n\nI looked over the rest of the patch. Nice testcases. Adding a couple\nmore testcases around copy detection could be useful.\n"},{"id":"347102","messageId":"80ddf6cf-0a38-9cd0-18b1-83114c2d1f5d@gmail.com","threadId":"48442","inReplyTo":"CACsJy8CdvKO3aityyP3Ax0ZqaS6JzwH_i2Gn_8NmCUDKHMMQrw@mail.gmail.com","subject":"Re: [PATCH v1] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-09T17:04:25Z","receivedAt":"2018-05-09T17:04:36Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/9/2018 11:59 AM, Duy Nguyen wrote:\n> On Wed, May 9, 2018 at 4:42 PM, Ben Peart wrote:\n>> Add a new config status.renames setting to enable turning off rename detection\n>> during status. This setting will default to the value of diff.renames.\n> \n> Please add the reason you need this config key in the commit message.\n> My guess (probably correct) is on super large repo (how large?),\n> rename detection is just too slow (how long?) that it practically\n> makes git-status unusable.\n> \n\nYes, the reasons for this change are the same as for the patch that \nadded these same flags for merge and have to do with the poor \nperformance of rename detection with large repos. I'll update the \ncommit message to be more descriptive (see below) and correct some \nspelling errors.\n\n\nadd status config and command line options for rename detection\n\nAfter performing a merge that has conflicts, git status will by default \nattempt to detect renames which causes many objects to be examined. In \na virtualized repo, those objects do not exist locally so the rename \nlogic triggers them to be fetched from the server. This results in the \nstatus call taking hours to complete on very large repos. Even in a \nsmall repo (the GVFS repo) turning off break and rename detection has a \nsignificant impact:\n\ngit status --no-renames:\n31 secs., 105 loose object downloads\n\ngit status --no-breaks\n7 secs., 17 loose object downloads\n\ngit status --no-breaks --no-renames\n1 sec., 1 loose object download\n\nAdd a new config status.renames setting to enable turning off rename \ndetection during status. This setting will default to the value of \ndiff.renames.\n\nAdd a new config status.renamelimit setting to to enable bounding the \ntime spent finding out inexact renames during status. This setting will \ndefault to the value of diff.renamelimit.\n\nAdd status --no-renames command line option that enables overriding the \nconfig setting from the command line. Add --find-renames[=] to enable \ndetecting renames and optionally setting the similarity index from the \ncommand line.\n\nNote: I removed the --no-breaks command line option from the original \npatch as it will no longer be needed once the default has been changed \n[1] to turn it off.\n\n[1] \nhttps://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@gmail.com/\n\nOriginal-Patch-by: Alejandro Pauly \nSigned-off-by: Ben Peart \n\n\n> This information could be helpful when we optimize rename detection to\n> be more efficient.\n> \n>>\n>> Add a new config status.renamelimit setting to to enable bounding the time spent\n>> finding out inexact renames during status. This setting will default to the\n>> value of diff.renamelimit.\n>>\n>> Add status --no-renames command line option that enables overriding the config\n>> setting from the command line. Add --find-renames[=] to enable detecting\n>> renames and optionaly setting the similarity index from the command line.\n>>\n>> Origional-Patch-by: Alejandro Pauly \n>> Signed-off-by: Ben Peart \n"},{"id":"347128","messageId":"c386ec5c-4e82-2b52-10ef-885a335a14dc@gmail.com","threadId":"48442","inReplyTo":"CABPp-BEkQN55diiovv+33P=Ouk+FcK8N4p85EZZqVmw-mxuL1A@mail.gmail.com","subject":"Re: [PATCH v1] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-09T19:54:47Z","receivedAt":"2018-05-09T19:54:57Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/9/2018 12:56 PM, Elijah Newren wrote:\n> Hi Ben,\n> \n> Overall I think this is good, but I have lots of nit-picky things to\n> bring up. :-)\n> \n> \n\nThank you for the review. I appreciate the extra set of eyes on these \nchanges. Especially when dealing with the merge logic and settings \nwhich I am unfamiliar with.\n\n> \n> I suspect that status.renames should mention \"copy\", just as\n> diff.renames does. (We didn't mention it in merge.renames, because\n> merge isn't an operation for which copy detection can be useful -- at\n> least not until the diffstat at the end of the merge is controlled by\n> merge.renames as well...)\n> \n\nI wasn't supporting copy (as you noticed later in the patch) but will \nupdate the patch to do so and update the documentation appropriately.\n\n> Also, do these two config settings only affect 'git status', or does\n> it also affect the status shown when composing a commit message\n> (assuming the user hasn't turned commit.status off)? Does it affect\n> `git commit --dry-run` too?\n> \n\nThe config settings only affect 'git status'\n\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -109,6 +109,10 @@ static int have_option_m;\n>> static struct strbuf message = STRBUF_INIT;\n>>\n>> static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;\n>> +static int diff_detect_rename = -1;\n>> +static int status_detect_rename = -1;\n>> +static int diff_rename_limit = -1;\n>> +static int status_rename_limit = -1;\n> \n> Could we replace these four variables with just two: detect_rename and\n> rename_limit? Keeping these separate invites people to write code\n> using only one of the settings rather than the appropriate inherited\n> mixture of them, resulting in a weird bug. More on this below...\n> \n\nThis model was inherited from the diff code and replicated to the merge \ncode. However, it would be nice to get rid of these 4 static variables. \n See below for a proposal on how to do that...\n\n>> @@ -1259,11 +1273,29 @@ static int git_status_config(const char *k, const char *v, void *cb)\n>> return error(_(\"Invalid untracked files mode '%s'\"), v);\n>> return 0;\n>> }\n>> + if (!strcmp(k, \"diff.renamelimit\")) {\n>> + diff_rename_limit = git_config_int(k, v);\n>> + return 0;\n>> + }\n>> + if (!strcmp(k, \"status.renamelimit\")) {\n>> + status_rename_limit = git_config_int(k, v);\n>> + return 0;\n>> + }\n> \n> Here, since you're already checking diff.renamelimit first, you can\n> just set rename_limit in both blocks and not need both\n> diff_rename_limit and status_rename_limit. (Similar can be said for\n> diff.renames/status.renames.)\n\nIt really doesn't work that way - the call back is called for every \nconfig setting and there is no specific order they are called with. \nTypically, you just test for and save off any that you care about like \nI\"m doing here.\n\nI can update the logic here so that as I save off the settings that it \nwill also enforce the priority model (ie the diff setting can't override \nthe status setting) and then compute the final value once I have the \ncommand line arguments as they override either config setting (if present).\n\nOn the plus side, this change passes the red/green test but it now \nsplits the priority logic into two places and doesn't match with how \ndiff and merge handle this same issue.\n\n> \n> \n> \n>> @@ -1297,6 +1329,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n>> N_(\"ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)\"),\n>> PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n>> OPT_COLUMN(0, \"column\", &s.colopts, N_(\"list untracked files in columns\")),\n>> + OPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n>> + { OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n>> + N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n>> + PARSE_OPT_OPTARG, opt_parse_rename_score },\n> \n> Should probably also document these options in\n> Documentation/git-status.txt (and maybe Documentation/git-commit.txt\n> as well).\n\nGood point, will do.\n\n> \n> Not sure if we want to add a flag for copy detection (similar to\n> git-diff's -C/--find-copies and --find-copies-harder), or just leave\n> that for when someone finds a need. If left out, might want to just\n> mention that it was considered and intentionally omitted for now in\n> the commit message.\n> \n\nI tend to only implement the features I know are actually needed so \nintentionally omitted this (along with many other potential diff options).\n\n>> + if ((intptr_t)rename_score_arg != -1) {\n> \n> I don't understand why rename_score_arg is a (char*) and then you need\n> to cast to intptr_t, but that may just be because I haven't done much\n> of anything with option parsing. A quick look at the docs isn't\n> making it clear to me, though; could you enlighten me?\n> \n\nYes, it is related to making parse_options() do what we need. -1 means \nthe command line option wasn't passed so use the default. NULL means \nthe command line argument was passed but without the optional score. A \nnon NULL, non -1 value means the optional score was passed and needs to \nbe parsed. The (intptr_t_) cast is to enable comparing a pointer to an \ninteger (-1) without generating a compiler warning.\n\n>> + s.detect_rename = DIFF_DETECT_RENAME;\n> \n> What if status.renames is 'copy' but someone wants to override the\n> score for detecting renames and pass --find-renames=40? Does the\n> --find-renames override and degrade the 'copy'? I think it'd make\n> more sense to increase s.detect_rename to at least DIFF_DETECT_RENAME,\n> rather than just outright setting it here.\n> \n\nI understand your argument and agree that it makes some sense. I am \nmatching the same logic in merge-recursive.c which just sets \ndetect_rename to 1 in this case. I believe more strongly that they \nshould be consistent than in one option over the other.\n\nIf I'm reading the merge logic for this case incorrectly or if you're \nwilling to patch the merge logic to match :), I'm happy to change this to:\n\n\t\tif (s.detect_rename < DIFF_DETECT_RENAME)\n\t\t\ts.detect_rename = DIFF_DETECT_RENAME;\n\n>> + s.rename_limit;\n>> +\n>> + /*\n>> + * We do not have logic to handle the detection of copies. In\n>> + * fact, it may not even make sense to add such logic: would we\n>> + * really want a change to a base file to be propagated through\n>> + * multiple other files by a merge?\n>> + */\n>> + if (s.detect_rename > DIFF_DETECT_RENAME)\n>> + s.detect_rename = DIFF_DETECT_RENAME;\n> \n> This comment and code made sense in merge-recursive.c (which doesn't\n> show detected diffs/renames/copies but just uses them for internal\n> processing logic). It does not make sense here; git status could show\n> detected copies much like `git diff -C --name-status` shows it. In\n> fact, a quick grep for DIFF_STATUS_COPIED shows multiple hits in\n> wt-status.c, so I suspect it already has the necessary logic for\n> displaying copies.\n> \n\nClearly I was following my similar patch series for merge too closely \nwithout thinking through how they should be different. Thanks for \ncatching this. Gone. :)\n\n"},{"id":"347230","messageId":"20180510141621.9668-1-benpeart@microsoft.com","threadId":"48442","inReplyTo":"20180509144213.18032-1-benpeart@microsoft.com","subject":"[PATCH v2] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-10T14:16:37Z","receivedAt":"2018-05-10T14:16:45Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"After performing a merge that has conflicts, git status will by default attempt\nto detect renames which causes many objects to be examined. In a virtualized\nrepo, those objects do not exist locally so the rename logic triggers them to be\nfetched from the server. This results in the status call taking hours to\ncomplete on very large repos. Even in a small repo (the GVFS repo) turning off\nbreak and rename detection has a significant impact:\n\ngit status --no-renames:\n31 secs., 105 loose object downloads\n\ngit status --no-breaks\n7 secs., 17 loose object downloads\n\ngit status --no-breaks --no-renames\n1 sec., 1 loose object download\n\nAdd a new config status.renames setting to enable turning off rename detection\nduring status. This setting will default to the value of diff.renames.\n\nAdd a new config status.renamelimit setting to to enable bounding the time spent\nfinding out inexact renames during status. This setting will default to the\nvalue of diff.renamelimit.\n\nAdd status --no-renames command line option that enables overriding the config\nsetting from the command line. Add --find-renames[=] to enable detecting\nrenames and optionally setting the similarity index from the command line.\n\nNote: I removed the --no-breaks command line option from the original patch as\nit will no longer be needed once the default has been changed [1] to turn it off.\n\n[1] https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@gmail.com/\n\nOriginal-Patch-by: Alejandro Pauly \nSigned-off-by: Ben Peart \n---\n\nNotes:\n Base Ref: master\n Web-Diff: https://github.com/benpeart/git/commit/823212725b\n Checkout: git fetch https://github.com/benpeart/git status-renames-v2 && git checkout 823212725b\n \n ### Interdiff (v1..v2):\n \n diff --git a/Documentation/config.txt b/Documentation/config.txt\n index b79b83c587..9c8eca05b1 100644\n --- a/Documentation/config.txt\n +++ b/Documentation/config.txt\n @@ -3126,7 +3126,8 @@ status.renameLimit::\n status.renames::\n \tWhether and how Git detects renames. If set to \"false\",\n \trename detection is disabled. If set to \"true\", basic rename\n -\tdetection is enabled. Defaults to the value of diff.renames.\n +\tdetection is enabled. If set to \"copies\" or \"copy\", Git will\n +\tdetect copies, as well. Defaults to the value of diff.renames.\n \n status.showStash::\n \tIf set to true, linkgit:git-status[1] will display the number of\n diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\n index c16e27e63d..c4467ffb98 100644\n --- a/Documentation/git-status.txt\n +++ b/Documentation/git-status.txt\n @@ -135,6 +135,16 @@ ignored, then the directory is not shown, but all contents are shown.\n \tDisplay or do not display detailed ahead/behind counts for the\n \tbranch relative to its upstream branch. Defaults to true.\n \n +--renames::\n +--no-renames::\n +\tTurn on/off rename detection regardless of user configuration.\n +\tSee also linkgit:git-diff[1] `--no-renames`.\n +\n +--find-renames[=]::\n +\tTurn on rename detection, optionally setting the similarity\n +\tthreshold.\n +\tSee also linkgit:git-diff[1] `--find-renames`.\n +\n ...::\n \tSee the 'pathspec' entry in linkgit:gitglossary[7].\n \n diff --git a/builtin/commit.c b/builtin/commit.c\n index a545096ddd..db886277f4 100644\n --- a/builtin/commit.c\n +++ b/builtin/commit.c\n @@ -109,10 +109,6 @@ static int have_option_m;\n static struct strbuf message = STRBUF_INIT;\n \n static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;\n -static int diff_detect_rename = -1;\n -static int status_detect_rename = -1;\n -static int diff_rename_limit = -1;\n -static int status_rename_limit = -1;\n \n static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset)\n {\n @@ -1274,19 +1270,21 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"diff.renamelimit\")) {\n -\t\tdiff_rename_limit = git_config_int(k, v);\n +\t\tif (s->rename_limit == -1)\n +\t\t\ts->rename_limit = git_config_int(k, v);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"status.renamelimit\")) {\n -\t\tstatus_rename_limit = git_config_int(k, v);\n +\t\ts->rename_limit = git_config_int(k, v);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"diff.renames\")) {\n -\t\tdiff_detect_rename = git_config_rename(k, v);\n +\t\tif (s->detect_rename == -1)\n +\t\t\ts->detect_rename = git_config_rename(k, v);\n \t\treturn 0;\n \t}\n \tif (!strcmp(k, \"status.renames\")) {\n -\t\tstatus_detect_rename = git_config_rename(k, v);\n +\t\ts->detect_rename = git_config_rename(k, v);\n \t\treturn 0;\n \t}\n \treturn git_diff_ui_config(k, v, NULL);\n @@ -1372,27 +1370,13 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \ts.ignore_submodule_arg = ignore_submodule_arg;\n \ts.status_format = status_format;\n \ts.verbose = verbose;\n -\ts.detect_rename = no_renames >= 0 ? !no_renames :\n -\t\t\t\t\t status_detect_rename >= 0 ? status_detect_rename :\n -\t\t\t\t\t diff_detect_rename >= 0 ? diff_detect_rename :\n -\t\t\t\t\t s.detect_rename;\n +\tif (no_renames != -1)\n +\t\ts.detect_rename = !no_renames;\n \tif ((intptr_t)rename_score_arg != -1) {\n \t\ts.detect_rename = DIFF_DETECT_RENAME;\n \t\tif (rename_score_arg)\n \t\t\ts.rename_score = parse_rename_score(&rename_score_arg);\n \t}\n -\ts.rename_limit = status_rename_limit >= 0 ? status_rename_limit :\n -\t\t\t\t\t diff_rename_limit >= 0 ? diff_rename_limit :\n -\t\t\t\t\t s.rename_limit;\n -\n -\t/*\n -\t * We do not have logic to handle the detection of copies. In\n -\t * fact, it may not even make sense to add such logic: would we\n -\t * really want a change to a base file to be propagated through\n -\t * multiple other files by a merge?\n -\t */\n -\tif (s.detect_rename > DIFF_DETECT_RENAME)\n -\t\ts.detect_rename = DIFF_DETECT_RENAME;\n \n \twt_status_collect(&s);\n \n ### Patches\n\n Documentation/config.txt | 10 ++++\n Documentation/git-status.txt | 10 ++++\n builtin/commit.c | 41 ++++++++++++++++\n diff.c | 2 +-\n diff.h | 1 +\n t/t7525-status-rename.sh | 90 ++++++++++++++++++++++++++++++++++++\n wt-status.c | 12 +++++\n wt-status.h | 4 +-\n 8 files changed, 168 insertions(+), 2 deletions(-)\n create mode 100644 t/t7525-status-rename.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2659153cb3..9c8eca05b1 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3119,6 +3119,16 @@ status.displayCommentPrefix::\n \tbehavior of linkgit:git-status[1] in Git 1.8.4 and previous.\n \tDefaults to false.\n \n+status.renameLimit::\n+\tThe number of files to consider when performing rename detection;\n+\tif not specified, defaults to the value of diff.renameLimit.\n+\n+status.renames::\n+\tWhether and how Git detects renames. If set to \"false\",\n+\trename detection is disabled. If set to \"true\", basic rename\n+\tdetection is enabled. If set to \"copies\" or \"copy\", Git will\n+\tdetect copies, as well. Defaults to the value of diff.renames.\n+\n status.showStash::\n \tIf set to true, linkgit:git-status[1] will display the number of\n \tentries currently stashed away.\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex c16e27e63d..c4467ffb98 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -135,6 +135,16 @@ ignored, then the directory is not shown, but all contents are shown.\n \tDisplay or do not display detailed ahead/behind counts for the\n \tbranch relative to its upstream branch. Defaults to true.\n \n+--renames::\n+--no-renames::\n+\tTurn on/off rename detection regardless of user configuration.\n+\tSee also linkgit:git-diff[1] `--no-renames`.\n+\n+--find-renames[=]::\n+\tTurn on rename detection, optionally setting the similarity\n+\tthreshold.\n+\tSee also linkgit:git-diff[1] `--find-renames`.\n+\n ...::\n \tSee the 'pathspec' entry in linkgit:gitglossary[7].\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5240f11225..db886277f4 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -143,6 +143,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)\n+{\n+\tconst char **value = opt->value;\n+\tif (arg != NULL && *arg == '=')\n+\t\targ = arg + 1;\n+\n+\t*value = arg;\n+\treturn 0;\n+}\n+\n static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head()))\n@@ -1259,11 +1269,31 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\t\treturn error(_(\"Invalid untracked files mode '%s'\"), v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"diff.renamelimit\")) {\n+\t\tif (s->rename_limit == -1)\n+\t\t\ts->rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renamelimit\")) {\n+\t\ts->rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"diff.renames\")) {\n+\t\tif (s->detect_rename == -1)\n+\t\t\ts->detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renames\")) {\n+\t\ts->detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_ui_config(k, v, NULL);\n }\n \n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n+\tstatic int no_renames = -1;\n+\tstatic const char *rename_score_arg = (const char *)-1;\n \tstatic struct wt_status s;\n \tint fd;\n \tstruct object_id oid;\n@@ -1297,6 +1327,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t N_(\"ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)\"),\n \t\t PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n \t\tOPT_COLUMN(0, \"column\", &s.colopts, N_(\"list untracked files in columns\")),\n+\t\tOPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n+\t\t{ OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n+\t\t N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n+\t\t PARSE_OPT_OPTARG, opt_parse_rename_score },\n \t\tOPT_END(),\n \t};\n \n@@ -1336,6 +1370,13 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \ts.ignore_submodule_arg = ignore_submodule_arg;\n \ts.status_format = status_format;\n \ts.verbose = verbose;\n+\tif (no_renames != -1)\n+\t\ts.detect_rename = !no_renames;\n+\tif ((intptr_t)rename_score_arg != -1) {\n+\t\ts.detect_rename = DIFF_DETECT_RENAME;\n+\t\tif (rename_score_arg)\n+\t\t\ts.rename_score = parse_rename_score(&rename_score_arg);\n+\t}\n \n \twt_status_collect(&s);\n \ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..5dfc24aa6d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n-static int git_config_rename(const char *var, const char *value)\n+int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn DIFF_DETECT_RENAME;\ndiff --git a/diff.h b/diff.h\nindex d29560f822..dedac472ca 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -324,6 +324,7 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n extern void diff_setup_done(struct diff_options *);\n+extern int git_config_rename(const char *var, const char *value);\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\nnew file mode 100644\nindex 0000000000..311df8038a\n--- /dev/null\n+++ b/t/t7525-status-rename.sh\n@@ -0,0 +1,90 @@\n+#!/bin/sh\n+\n+test_description='git status rename detection options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo 1 >original &&\n+\tgit add . &&\n+\tgit commit -m\"Adding original file.\" &&\n+\tmv original renamed &&\n+\techo 2 >> renamed &&\n+\tgit add .\n+'\n+\n+cat >.gitignore <<\\EOF\n+.gitignore\n+expect*\n+actual*\n+EOF\n+\n+test_expect_success 'status no-options' '\n+\tgit status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status --no-renames' '\n+\tgit status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames false' '\n+\tgit -c diff.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames true' '\n+\tgit -c diff.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides diff.renames false' '\n+\tgit -c diff.renames=true -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides from diff.renames true' '\n+\tgit -c diff.renames=false -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status status.renames=false' '\n+\tgit -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status status.renames=true' '\n+\tgit -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status config overriden' '\n+\tgit -c status.renames=true status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=100%' '\n+\tgit status -M=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual &&\n+\n+\tgit status --find-rename=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=01%' '\n+\tgit status -M=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual &&\n+\n+\tgit status --find-rename=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 32f3bcaebd..172f07cbb0 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -138,6 +138,9 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_stash = 0;\n \ts->ahead_behind_flags = AHEAD_BEHIND_UNSPECIFIED;\n \ts->display_comment_prefix = 0;\n+\ts->detect_rename = -1;\n+\ts->rename_score = -1;\n+\ts->rename_limit = -1;\n }\n \n static void wt_longstatus_print_unmerged_header(struct wt_status *s)\n@@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \t}\n \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n \trev.diffopt.format_callback_data = s;\n+\trev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n }\n@@ -625,6 +631,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n }\n@@ -982,6 +991,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\ndiff --git a/wt-status.h b/wt-status.h\nindex 430770b854..1673d146fa 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -89,7 +89,9 @@ struct wt_status {\n \tint show_stash;\n \tint hints;\n \tenum ahead_behind_flags ahead_behind_flags;\n-\n+\tint detect_rename;\n+\tint rename_score;\n+\tint rename_limit;\n \tenum wt_status_format status_format;\n \tunsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */\n \n\nbase-commit: a92ae92585d8db14b7871f760f157256cd96742c\n-- \n2.17.0.windows.1\n\n"},{"id":"347256","messageId":"CABPp-BGE6RXv3ka8wGXruFjk3W=kDEDJ6zpH3t5=_CGSTONCHQ@mail.gmail.com","threadId":"48442","inReplyTo":"20180510141621.9668-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] add status config and command line options for rename detection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-10T16:19:00Z","receivedAt":"2018-05-10T16:19:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOn Thu, May 10, 2018 at 7:16 AM, Ben Peart wrote:\n> After performing a merge that has conflicts, git status will by default attempt\n> to detect renames which causes many objects to be examined. In a virtualized\n> repo, those objects do not exist locally so the rename logic triggers them to be\n> fetched from the server. This results in the status call taking hours to\n> complete on very large repos. Even in a small repo (the GVFS repo) turning off\n> break and rename detection has a significant impact:\n\nIt'd be nice if you could show that impact by comparing 'git status'\nto 'git status --no-renames', for some repo. Showing only the latter\ngives us no way to assess the impact.\n\n> git status --no-renames:\n> 31 secs., 105 loose object downloads\n>\n> git status --no-breaks\n> 7 secs., 17 loose object downloads\n>\n> git status --no-breaks --no-renames\n> 1 sec., 1 loose object download\n\nThis patch doesn't add a --no-breaks option and it doesn't exist\npreviously, so adding it to the commit message serves to confuse\nrather than help. I'd just drop the last two of these (and redo the\ntiming for --no-renames assuming you are built on\nem/status-rename-config).\n\n> Add a new config status.renames setting to enable turning off rename detection\n> during status. This setting will default to the value of diff.renames.\n>\n> Add a new config status.renamelimit setting to to enable bounding the time spent\n> finding out inexact renames during status. This setting will default to the\n> value of diff.renamelimit.\n\nIt may be worth mentioning that these config settings also affect 'git\ncommit' (and it does, in my testing, which I think is a good thing).\n\n> Add status --no-renames command line option that enables overriding the config\n> setting from the command line. Add --find-renames[=] to enable detecting\n> renames and optionally setting the similarity index from the command line.\n\nThe command line options are specific to 'git status'. I don't really\nhave a strong opinion on whether they should also be added to\ngit-commit; I suspect users would be more likely to use the config\noptions in order to set it once and forget about it and that users\nwould be more likely to want to override their config setting for\nstatus than for commit.\n\n> Note: I removed the --no-breaks command line option from the original patch as\n> it will no longer be needed once the default has been changed [1] to turn it off.\n>\n> [1] https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@gmail.com/\n\nI'd just drop these lines from the commit message, and instead mention\nthat your patch depends on em/status-rename-config.\n\n> Original-Patch-by: Alejandro Pauly \n> Signed-off-by: Ben Peart \n> ---\n>\n> Notes:\n> Base Ref: master\n\nThis patch does not apply to master; it has conflicts.\n\n> Web-Diff: https://github.com/benpeart/git/commit/823212725b\n\nThis web diff shows em/status-rename-config as the parent commit, not\nmaster. Since your commit message mentions you want the change to\nbreak detection provided by that series, just listing it as the\nexplicit base seems like the right way to go.\n\n> ### Interdiff (v1..v2):\n\nThanks.\n\n> + if ((intptr_t)rename_score_arg != -1) {\n> + s.detect_rename = DIFF_DETECT_RENAME;\n\nI'd still prefer this was a\n if (s.detect_rename < DIFF_DETECT_RENAME)\n s.detect_rename = DIFF_DETECT_RENAME;\n\nIf a user specifies they are willing to pay for copy detection, but\nthen just passes --find-renames=40% because they want to find more\nrenames, it seems odd to disable copy detection to me.\n\n> +++ b/t/t7525-status-rename.sh\n\nTestcases look good. It'd be nice to also add a few testcases where\ncopy detection is turned on -- in particular, I'd like to see one with\n--find-renames=$DIFFERENT_THAN_DEFAULT being passed when\nmerge.renames=copies.\n\n\n> +test_expect_success 'setup' '\n> + echo 1 >original &&\n> + git add . &&\n> + git commit -m\"Adding original file.\" &&\n> + mv original renamed &&\n> + echo 2 >> renamed &&\n> + git add .\n> +'\n\n\n> +cat >.gitignore <<\\EOF\n> +.gitignore\n> +expect*\n> +actual*\n> +EOF\n\nCan this just be included in the setup?\n\n\nEverything else in the patch looked good to me.\n"},{"id":"347291","messageId":"bc81823e-7b7d-c516-dfc2-cd47bedb5a5a@gmail.com","threadId":"48442","inReplyTo":"CABPp-BGE6RXv3ka8wGXruFjk3W=kDEDJ6zpH3t5=_CGSTONCHQ@mail.gmail.com","subject":"Re: [PATCH v2] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-10T19:09:23Z","receivedAt":"2018-05-10T19:09:29Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/10/2018 12:19 PM, Elijah Newren wrote:\n> Hi Ben,\n> \n> On Thu, May 10, 2018 at 7:16 AM, Ben Peart wrote:\n>> After performing a merge that has conflicts, git status will by default attempt\n>> to detect renames which causes many objects to be examined. In a virtualized\n>> repo, those objects do not exist locally so the rename logic triggers them to be\n>> fetched from the server. This results in the status call taking hours to\n>> complete on very large repos. Even in a small repo (the GVFS repo) turning off\n>> break and rename detection has a significant impact:\n> \n> It'd be nice if you could show that impact by comparing 'git status'\n> to 'git status --no-renames', for some repo. Showing only the latter\n> gives us no way to assess the impact.\n> \n\nGiven the example perf impact is arbitrary (the actual example that \ntriggered this patch took status from 2+ hours to seconds) and can't be \nreplicated using the current performance tools in git, I'm just going \ndrop the specific numbers. I believe the patch is worth while just to \ngive users the flexibility to control these behaviors.\n\n>> git status --no-renames:\n>> 31 secs., 105 loose object downloads\n>>\n>> git status --no-breaks\n>> 7 secs., 17 loose object downloads\n>>\n>> git status --no-breaks --no-renames\n>> 1 sec., 1 loose object download\n> \n> This patch doesn't add a --no-breaks option and it doesn't exist\n> previously, so adding it to the commit message serves to confuse\n> rather than help. I'd just drop the last two of these (and redo the\n> timing for --no-renames assuming you are built on\n> em/status-rename-config).\n> \n\nOK\n\n>> Add a new config status.renames setting to enable turning off rename detection\n>> during status. This setting will default to the value of diff.renames.\n>>\n>> Add a new config status.renamelimit setting to to enable bounding the time spent\n>> finding out inexact renames during status. This setting will default to the\n>> value of diff.renamelimit.\n> \n> It may be worth mentioning that these config settings also affect 'git\n> commit' (and it does, in my testing, which I think is a good thing).\n> \n\nI agree this is a good thing as the other status settings behave the \nsame way. I'll update the documentation to reflect this as well.\n\n>> Add status --no-renames command line option that enables overriding the config\n>> setting from the command line. Add --find-renames[=] to enable detecting\n>> renames and optionally setting the similarity index from the command line.\n> \n> The command line options are specific to 'git status'. I don't really\n> have a strong opinion on whether they should also be added to\n> git-commit; I suspect users would be more likely to use the config\n> options in order to set it once and forget about it and that users\n> would be more likely to want to override their config setting for\n> status than for commit.\n> \n>> Note: I removed the --no-breaks command line option from the original patch as\n>> it will no longer be needed once the default has been changed [1] to turn it off.\n>>\n>> [1] https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@gmail.com/\n> \n> I'd just drop these lines from the commit message, and instead mention\n> that your patch depends on em/status-rename-config.\n> \n\nOK\n\n>> + if ((intptr_t)rename_score_arg != -1) {\n>> + s.detect_rename = DIFF_DETECT_RENAME;\n> \n> I'd still prefer this was a\n> if (s.detect_rename < DIFF_DETECT_RENAME)\n> s.detect_rename = DIFF_DETECT_RENAME;\n> \n> If a user specifies they are willing to pay for copy detection, but\n> then just passes --find-renames=40% because they want to find more\n> renames, it seems odd to disable copy detection to me.\n> \n\nI agree and will change it. It is unfortunate this will behave \ndifferently than it does with merge. Fixing the merge behavior to match \nis outside the scope of this patch.\n\n>> +++ b/t/t7525-status-rename.sh\n> \n> Testcases look good. It'd be nice to also add a few testcases where\n> copy detection is turned on -- in particular, I'd like to see one with\n> --find-renames=$DIFFERENT_THAN_DEFAULT being passed when\n> merge.renames=copies.\n> \n\nOK. I also added tests to verify the settings correctly impact commit.\n\n> \n>> +test_expect_success 'setup' '\n>> + echo 1 >original &&\n>> + git add . &&\n>> + git commit -m\"Adding original file.\" &&\n>> + mv original renamed &&\n>> + echo 2 >> renamed &&\n>> + git add .\n>> +'\n> \n> \n>> +cat >.gitignore <<\\EOF\n>> +.gitignore\n>> +expect*\n>> +actual*\n>> +EOF\n> \n> Can this just be included in the setup?\n> \n\nOK\n\n> \n> Everything else in the patch looked good to me.\n> \n"},{"id":"347316","messageId":"CABPp-BGL2Fcnj0QNu+D3wfOU5q8MuizUGJyzrEc7FXy9Q9aA_A@mail.gmail.com","threadId":"48442","inReplyTo":"bc81823e-7b7d-c516-dfc2-cd47bedb5a5a@gmail.com","subject":"Re: [PATCH v2] add status config and command line options for rename detection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-10T22:31:58Z","receivedAt":"2018-05-10T22:32:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOn Thu, May 10, 2018 at 12:09 PM, Ben Peart wrote:\n> On 5/10/2018 12:19 PM, Elijah Newren wrote:\n>> On Thu, May 10, 2018 at 7:16 AM, Ben Peart \n>> wrote:\n\n> Given the example perf impact is arbitrary (the actual example that\n> triggered this patch took status from 2+ hours to seconds) and can't be\n> replicated using the current performance tools in git, I'm just going drop\n> the specific numbers. I believe the patch is worth while just to give users\n> the flexibility to control these behaviors.\n\nYour parenthetical statement of timing going from hours to seconds I\nthink would be great; I don't think we need precise numbers.\n\n>>> + if ((intptr_t)rename_score_arg != -1) {\n>>> + s.detect_rename = DIFF_DETECT_RENAME;\n>>\n>>\n>> I'd still prefer this was a\n>> if (s.detect_rename < DIFF_DETECT_RENAME)\n>> s.detect_rename = DIFF_DETECT_RENAME;\n>>\n>> If a user specifies they are willing to pay for copy detection, but\n>> then just passes --find-renames=40% because they want to find more\n>> renames, it seems odd to disable copy detection to me.\n>>\n>\n> I agree and will change it. It is unfortunate this will behave differently\n> than it does with merge. Fixing the merge behavior to match is outside the\n> scope of this patch.\n\nI agree that changing merge is outside the scope of this patch, but\nI'm curious what change you have in mind for it to \"make it match\".\nIn particular, merge-recursive.c already has (or will shortly have)\n+ if (opts.detect_rename > DIFF_DETECT_RENAME)\n+ opts.detect_rename = DIFF_DETECT_RENAME;\nfrom your commit 85b460305ce7 (\"merge: add merge.renames config\nsetting\", 2018-05-02), so I'm not sure why we'd want to carefully\npropagate a larger value for o->{diff,merge}_detect_rename prior to\nthis point. If it's just \"future proofing\" because you suspect that\ncopy information could be useful to the merging algorithm and we'll\neventually get rid of these two lines of code, then I could get behind\nsuch a change, though color me skeptical that copy information would\never turn out to be useful in that context.\n\nThe one place copy detection does make sense inside a merge is for the\ndiffstat shown at the end (from builtin/merge.c), but it currently\nisn't controlled by any configuration setting at all. When it is\nhooked up, it'd probably store the value separately from\nmerge-recursive's internal o->{diff,merge}_detect_rename anyway,\nbecause builtin/merge.c's diffstat should be controlled by the\nrelevant confiig settings and flags (merge.renames, diff.renames,\n-Xfind-renames, etc.) regardless of which merge strategy (recursive,\nresolve, octopus, ours, ort) is employed. And when that is hooked up,\nI agree with you that it should look like what you've done with\nstatus.renames here. In fact, if you'd like to take a crack at it, I\nthink you'd do a great job. :-) If not, it's on my list of things to\ndo.\n\n>> Testcases look good. It'd be nice to also add a few testcases where\n>> copy detection is turned on -- in particular, I'd like to see one with\n>> --find-renames=$DIFFERENT_THAN_DEFAULT being passed when\n>> merge.renames=copies.\n>>\n>\n> OK. I also added tests to verify the settings correctly impact commit.\n\nNice!\n"},{"id":"347324","messageId":"xmqqfu2zhs3m.fsf@gitster-ct.c.googlers.com","threadId":"48442","inReplyTo":"CABPp-BGE6RXv3ka8wGXruFjk3W=kDEDJ6zpH3t5=_CGSTONCHQ@mail.gmail.com","subject":"Re: [PATCH v2] add status config and command line options for rename detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-11T01:57:17Z","receivedAt":"2018-05-11T01:57:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> Note: I removed the --no-breaks command line option from the original patch as\n>> it will no longer be needed once the default has been changed [1] to turn it off.\n>>\n>> [1] https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@gmail.com/\n>\n> I'd just drop these lines from the commit message, and instead mention\n> that your patch depends on em/status-rename-config.\n>\n>> Original-Patch-by: Alejandro Pauly \n>> Signed-off-by: Ben Peart \n>> ---\n\nOther things seem to have been resolved between you two already, so\nI'll only comment on a minor tangent here.\n\n>> Notes:\n>> Base Ref: master\n>\n> This patch does not apply to master; it has conflicts.\n>\n>> Web-Diff: https://github.com/benpeart/git/commit/823212725b\n\nAs Git is distributed, unlike tags that are meant to be global among\nproject participants by convention, a branch name can never be used\nas a trustable base among developers. Your 'master' branch may\npoint at a different commit from mine, and my 'master' branch today\nmay point at a different commit from mine yesterday.\n\nI've seen patches that used a similar note below the three-dash line\nthat named an exact commit object name. That is a lot more reliable\nway to convey the information necessary to consturct the exact state\nthe contributor worked on.\n\n> This web diff shows em/status-rename-config as the parent commit, not\n> master. Since your commit message mentions you want the change to\n> break detection provided by that series, just listing it as the\n> explicit base seems like the right way to go.\n\nThanks for digging. That would work well, too.\n"},{"id":"347338","messageId":"xmqqr2mig0gm.fsf@gitster-ct.c.googlers.com","threadId":"48442","inReplyTo":"20180510141621.9668-1-benpeart@microsoft.com","subject":"Re: [PATCH v2] add status config and command line options for rename detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-11T06:39:37Z","receivedAt":"2018-05-11T06:39:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> Documentation/config.txt | 10 ++++\n> Documentation/git-status.txt | 10 ++++\n> builtin/commit.c | 41 ++++++++++++++++\n> diff.c | 2 +-\n> diff.h | 1 +\n> t/t7525-status-rename.sh | 90 ++++++++++++++++++++++++++++++++++++\n> wt-status.c | 12 +++++\n> wt-status.h | 4 +-\n> 8 files changed, 168 insertions(+), 2 deletions(-)\n> create mode 100644 t/t7525-status-rename.sh\n\nI'll mark the new script as executable (otherwise the test will not\neven start).\n\n"},{"id":"347353","messageId":"276c981b-3b6b-4034-7aaa-dbfcba4ae3f1@gmail.com","threadId":"48442","inReplyTo":"CABPp-BGL2Fcnj0QNu+D3wfOU5q8MuizUGJyzrEc7FXy9Q9aA_A@mail.gmail.com","subject":"Re: [PATCH v2] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-11T12:50:27Z","receivedAt":"2018-05-11T12:50:33Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/10/2018 6:31 PM, Elijah Newren wrote:\n> Hi Ben,\n> \n> On Thu, May 10, 2018 at 12:09 PM, Ben Peart wrote:\n>> On 5/10/2018 12:19 PM, Elijah Newren wrote:\n>>> On Thu, May 10, 2018 at 7:16 AM, Ben Peart \n>>> wrote:\n> \n>> Given the example perf impact is arbitrary (the actual example that\n>> triggered this patch took status from 2+ hours to seconds) and can't be\n>> replicated using the current performance tools in git, I'm just going drop\n>> the specific numbers. I believe the patch is worth while just to give users\n>> the flexibility to control these behaviors.\n> \n> Your parenthetical statement of timing going from hours to seconds I\n> think would be great; I don't think we need precise numbers.\n> \n>>>> + if ((intptr_t)rename_score_arg != -1) {\n>>>> + s.detect_rename = DIFF_DETECT_RENAME;\n>>>\n>>>\n>>> I'd still prefer this was a\n>>> if (s.detect_rename < DIFF_DETECT_RENAME)\n>>> s.detect_rename = DIFF_DETECT_RENAME;\n>>>\n>>> If a user specifies they are willing to pay for copy detection, but\n>>> then just passes --find-renames=40% because they want to find more\n>>> renames, it seems odd to disable copy detection to me.\n>>>\n>>\n>> I agree and will change it. It is unfortunate this will behave differently\n>> than it does with merge. Fixing the merge behavior to match is outside the\n>> scope of this patch.\n> \n> I agree that changing merge is outside the scope of this patch, but\n> I'm curious what change you have in mind for it to \"make it match\".\n> In particular, merge-recursive.c already has (or will shortly have)\n> + if (opts.detect_rename > DIFF_DETECT_RENAME)\n> + opts.detect_rename = DIFF_DETECT_RENAME;\n> from your commit 85b460305ce7 (\"merge: add merge.renames config\n> setting\", 2018-05-02), \n\nThis is a good point that I missed. With that recent change to merge, \nit no longer matters that the settings parsing code caps detect_rename \nat DIFF_DETECT_RENAME because it will cap it later anyway so there is no \nneed to change the merge option behavior.\n\n> The one place copy detection does make sense inside a merge is for the\n> diffstat shown at the end (from builtin/merge.c), but it currently\n> isn't controlled by any configuration setting at all. When it is\n> hooked up, it'd probably store the value separately from\n> merge-recursive's internal o->{diff,merge}_detect_rename anyway,\n> because builtin/merge.c's diffstat should be controlled by the\n> relevant confiig settings and flags (merge.renames, diff.renames,\n> -Xfind-renames, etc.) regardless of which merge strategy (recursive,\n> resolve, octopus, ours, ort) is employed. And when that is hooked up,\n> I agree with you that it should look like what you've done with\n> status.renames here. In fact, if you'd like to take a crack at it, I\n> think you'd do a great job. :-) If not, it's on my list of things to\n> do.\n> \n\nThanks but I'll leave that to you. :) I have a large backlog of patches \nI would like to see pushed through the mailing list into master. We've \nbeen sitting on this one for over a year. If the current rate is any \nindication, it will take man years to get caught up.\n\n>>> Testcases look good. It'd be nice to also add a few testcases where\n>>> copy detection is turned on -- in particular, I'd like to see one with\n>>> --find-renames=$DIFFERENT_THAN_DEFAULT being passed when\n>>> merge.renames=copies.\n>>>\n>>\n>> OK. I also added tests to verify the settings correctly impact commit.\n> \n> Nice!\n> \n"},{"id":"347354","messageId":"20180511125623.6068-1-benpeart@microsoft.com","threadId":"48442","inReplyTo":"20180509144213.18032-1-benpeart@microsoft.com","subject":"[PATCH v3] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-11T12:56:39Z","receivedAt":"2018-05-11T12:56:47Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"After performing a merge that has conflicts git status will, by default,\nattempt to detect renames which causes many objects to be examined. In a\nvirtualized repo, those objects do not exist locally so the rename logic\ntriggers them to be fetched from the server. This results in the status call\ntaking hours to complete on very large repos vs seconds with this patch.\n\nAdd a new config status.renames setting to enable turning off rename detection\nduring status and commit. This setting will default to the value of\ndiff.renames.\n\nAdd a new config status.renamelimit setting to to enable bounding the time\nspent finding out inexact renames during status and commit. This setting will\ndefault to the value of diff.renamelimit.\n\nAdd --no-renames command line option to status that enables overriding the\nconfig setting from the command line. Add --find-renames[=] command line\noption to status that enables detecting renames and optionally setting the\nsimilarity index.\n\nThis patch depends on em/status-rename-config\n\nOriginal-Patch-by: Alejandro Pauly \nSigned-off-by: Ben Peart \n---\n\nNotes:\n Base Ref: commit dc6b1d92ca9c0c538daa244e3910bb8b2a50d959 (em/status-rename-config)\n Web-Diff: https://github.com/benpeart/git/commit/5bac43610b\n Checkout: git fetch https://github.com/benpeart/git status-renames-v3 && git checkout 5bac43610b\n \n ### Interdiff (v2..v3):\n \n diff --git a/Documentation/config.txt b/Documentation/config.txt\n index 9c8eca05b1..88884f1ead 100644\n --- a/Documentation/config.txt\n +++ b/Documentation/config.txt\n @@ -3120,14 +3120,16 @@ status.displayCommentPrefix::\n \tDefaults to false.\n \n status.renameLimit::\n -\tThe number of files to consider when performing rename detection;\n -\tif not specified, defaults to the value of diff.renameLimit.\n +\tThe number of files to consider when performing rename detection\n +\tin linkgit:git-status[1] and linkgit:git-commit[1]. Defaults to\n +\tthe value of diff.renameLimit.\n \n status.renames::\n -\tWhether and how Git detects renames. If set to \"false\",\n -\trename detection is disabled. If set to \"true\", basic rename\n -\tdetection is enabled. If set to \"copies\" or \"copy\", Git will\n -\tdetect copies, as well. Defaults to the value of diff.renames.\n +\tWhether and how Git detects renames in linkgit:git-status[1] and\n +\tlinkgit:git-commit[1] . If set to \"false\", rename detection is\n +\tdisabled. If set to \"true\", basic rename detection is enabled.\n +\tIf set to \"copies\" or \"copy\", Git will detect copies, as well.\n +\tDefaults to the value of diff.renames.\n \n status.showStash::\n \tIf set to true, linkgit:git-status[1] will display the number of\n diff --git a/builtin/commit.c b/builtin/commit.c\n index db886277f4..b50e33ef48 100644\n --- a/builtin/commit.c\n +++ b/builtin/commit.c\n @@ -1373,6 +1373,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tif (no_renames != -1)\n \t\ts.detect_rename = !no_renames;\n \tif ((intptr_t)rename_score_arg != -1) {\n +\t\tif (s.detect_rename < DIFF_DETECT_RENAME)\n \t\t\ts.detect_rename = DIFF_DETECT_RENAME;\n \t\tif (rename_score_arg)\n \t\t\ts.rename_score = parse_rename_score(&rename_score_arg);\n diff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\n old mode 100644\n new mode 100755\n index 311df8038a..ef8b1b3078\n --- a/t/t7525-status-rename.sh\n +++ b/t/t7525-status-rename.sh\n @@ -10,14 +10,13 @@ test_expect_success 'setup' '\n \tgit commit -m\"Adding original file.\" &&\n \tmv original renamed &&\n \techo 2 >> renamed &&\n -\tgit add .\n -'\n -\n -cat >.gitignore <<\\EOF\n +\tgit add . &&\n +\tcat >.gitignore <<-\\EOF\n \t.gitignore\n \texpect*\n \tactual*\n \tEOF\n +'\n \n test_expect_success 'status no-options' '\n \tgit status >actual &&\n @@ -63,7 +62,18 @@ test_expect_success 'status status.renames=true' '\n \ttest_i18ngrep \"renamed:\" actual\n '\n \n -test_expect_success 'status config overriden' '\n +test_expect_success 'commit honors status.renames=false' '\n +\tgit -c status.renames=false commit --dry-run >actual &&\n +\ttest_i18ngrep \"deleted:\" actual &&\n +\ttest_i18ngrep \"new file:\" actual\n +'\n +\n +test_expect_success 'commit honors status.renames=true' '\n +\tgit -c status.renames=true commit --dry-run >actual &&\n +\ttest_i18ngrep \"renamed:\" actual\n +'\n +\n +test_expect_success 'status config overridden' '\n \tgit -c status.renames=true status --no-renames >actual &&\n \ttest_i18ngrep \"deleted:\" actual &&\n \ttest_i18ngrep \"new file:\" actual\n @@ -87,4 +97,17 @@ test_expect_success 'status score=01%' '\n \ttest_i18ngrep \"renamed:\" actual\n '\n \n +test_expect_success 'copies not overridden by find-rename' '\n +\tcp renamed copy &&\n +\tgit add copy &&\n +\n +\tgit -c status.renames=copies status -M=01% >actual &&\n +\ttest_i18ngrep \"copied:\" actual &&\n +\ttest_i18ngrep \"renamed:\" actual &&\n +\n +\tgit -c status.renames=copies status --find-rename=01% >actual &&\n +\ttest_i18ngrep \"copied:\" actual &&\n +\ttest_i18ngrep \"renamed:\" actual\n +'\n +\n test_done\n \n ### Patches\n\n Documentation/config.txt | 12 ++++\n Documentation/git-status.txt | 10 ++++\n builtin/commit.c | 42 +++++++++++++\n diff.c | 2 +-\n diff.h | 1 +\n t/t7525-status-rename.sh | 113 +++++++++++++++++++++++++++++++++++\n wt-status.c | 12 ++++\n wt-status.h | 4 +-\n 8 files changed, 194 insertions(+), 2 deletions(-)\n create mode 100755 t/t7525-status-rename.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2659153cb3..88884f1ead 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3119,6 +3119,18 @@ status.displayCommentPrefix::\n \tbehavior of linkgit:git-status[1] in Git 1.8.4 and previous.\n \tDefaults to false.\n \n+status.renameLimit::\n+\tThe number of files to consider when performing rename detection\n+\tin linkgit:git-status[1] and linkgit:git-commit[1]. Defaults to\n+\tthe value of diff.renameLimit.\n+\n+status.renames::\n+\tWhether and how Git detects renames in linkgit:git-status[1] and\n+\tlinkgit:git-commit[1] . If set to \"false\", rename detection is\n+\tdisabled. If set to \"true\", basic rename detection is enabled.\n+\tIf set to \"copies\" or \"copy\", Git will detect copies, as well.\n+\tDefaults to the value of diff.renames.\n+\n status.showStash::\n \tIf set to true, linkgit:git-status[1] will display the number of\n \tentries currently stashed away.\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex c16e27e63d..c4467ffb98 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -135,6 +135,16 @@ ignored, then the directory is not shown, but all contents are shown.\n \tDisplay or do not display detailed ahead/behind counts for the\n \tbranch relative to its upstream branch. Defaults to true.\n \n+--renames::\n+--no-renames::\n+\tTurn on/off rename detection regardless of user configuration.\n+\tSee also linkgit:git-diff[1] `--no-renames`.\n+\n+--find-renames[=]::\n+\tTurn on rename detection, optionally setting the similarity\n+\tthreshold.\n+\tSee also linkgit:git-diff[1] `--find-renames`.\n+\n ...::\n \tSee the 'pathspec' entry in linkgit:gitglossary[7].\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5240f11225..b50e33ef48 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -143,6 +143,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)\n+{\n+\tconst char **value = opt->value;\n+\tif (arg != NULL && *arg == '=')\n+\t\targ = arg + 1;\n+\n+\t*value = arg;\n+\treturn 0;\n+}\n+\n static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head()))\n@@ -1259,11 +1269,31 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\t\treturn error(_(\"Invalid untracked files mode '%s'\"), v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"diff.renamelimit\")) {\n+\t\tif (s->rename_limit == -1)\n+\t\t\ts->rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renamelimit\")) {\n+\t\ts->rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"diff.renames\")) {\n+\t\tif (s->detect_rename == -1)\n+\t\t\ts->detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renames\")) {\n+\t\ts->detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_ui_config(k, v, NULL);\n }\n \n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n+\tstatic int no_renames = -1;\n+\tstatic const char *rename_score_arg = (const char *)-1;\n \tstatic struct wt_status s;\n \tint fd;\n \tstruct object_id oid;\n@@ -1297,6 +1327,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t N_(\"ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)\"),\n \t\t PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n \t\tOPT_COLUMN(0, \"column\", &s.colopts, N_(\"list untracked files in columns\")),\n+\t\tOPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n+\t\t{ OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n+\t\t N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n+\t\t PARSE_OPT_OPTARG, opt_parse_rename_score },\n \t\tOPT_END(),\n \t};\n \n@@ -1336,6 +1370,14 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \ts.ignore_submodule_arg = ignore_submodule_arg;\n \ts.status_format = status_format;\n \ts.verbose = verbose;\n+\tif (no_renames != -1)\n+\t\ts.detect_rename = !no_renames;\n+\tif ((intptr_t)rename_score_arg != -1) {\n+\t\tif (s.detect_rename < DIFF_DETECT_RENAME)\n+\t\t\ts.detect_rename = DIFF_DETECT_RENAME;\n+\t\tif (rename_score_arg)\n+\t\t\ts.rename_score = parse_rename_score(&rename_score_arg);\n+\t}\n \n \twt_status_collect(&s);\n \ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..5dfc24aa6d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n-static int git_config_rename(const char *var, const char *value)\n+int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn DIFF_DETECT_RENAME;\ndiff --git a/diff.h b/diff.h\nindex d29560f822..dedac472ca 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -324,6 +324,7 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n extern void diff_setup_done(struct diff_options *);\n+extern int git_config_rename(const char *var, const char *value);\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\nnew file mode 100755\nindex 0000000000..ef8b1b3078\n--- /dev/null\n+++ b/t/t7525-status-rename.sh\n@@ -0,0 +1,113 @@\n+#!/bin/sh\n+\n+test_description='git status rename detection options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo 1 >original &&\n+\tgit add . &&\n+\tgit commit -m\"Adding original file.\" &&\n+\tmv original renamed &&\n+\techo 2 >> renamed &&\n+\tgit add . &&\n+\tcat >.gitignore <<-\\EOF\n+\t.gitignore\n+\texpect*\n+\tactual*\n+\tEOF\n+'\n+\n+test_expect_success 'status no-options' '\n+\tgit status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status --no-renames' '\n+\tgit status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames false' '\n+\tgit -c diff.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames true' '\n+\tgit -c diff.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides diff.renames false' '\n+\tgit -c diff.renames=true -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides from diff.renames true' '\n+\tgit -c diff.renames=false -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status status.renames=false' '\n+\tgit -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status status.renames=true' '\n+\tgit -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'commit honors status.renames=false' '\n+\tgit -c status.renames=false commit --dry-run >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'commit honors status.renames=true' '\n+\tgit -c status.renames=true commit --dry-run >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status config overridden' '\n+\tgit -c status.renames=true status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=100%' '\n+\tgit status -M=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual &&\n+\n+\tgit status --find-rename=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=01%' '\n+\tgit status -M=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual &&\n+\n+\tgit status --find-rename=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'copies not overridden by find-rename' '\n+\tcp renamed copy &&\n+\tgit add copy &&\n+\n+\tgit -c status.renames=copies status -M=01% >actual &&\n+\ttest_i18ngrep \"copied:\" actual &&\n+\ttest_i18ngrep \"renamed:\" actual &&\n+\n+\tgit -c status.renames=copies status --find-rename=01% >actual &&\n+\ttest_i18ngrep \"copied:\" actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 32f3bcaebd..172f07cbb0 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -138,6 +138,9 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_stash = 0;\n \ts->ahead_behind_flags = AHEAD_BEHIND_UNSPECIFIED;\n \ts->display_comment_prefix = 0;\n+\ts->detect_rename = -1;\n+\ts->rename_score = -1;\n+\ts->rename_limit = -1;\n }\n \n static void wt_longstatus_print_unmerged_header(struct wt_status *s)\n@@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \t}\n \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n \trev.diffopt.format_callback_data = s;\n+\trev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n }\n@@ -625,6 +631,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n }\n@@ -982,6 +991,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\ndiff --git a/wt-status.h b/wt-status.h\nindex 430770b854..1673d146fa 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -89,7 +89,9 @@ struct wt_status {\n \tint show_stash;\n \tint hints;\n \tenum ahead_behind_flags ahead_behind_flags;\n-\n+\tint detect_rename;\n+\tint rename_score;\n+\tint rename_limit;\n \tenum wt_status_format status_format;\n \tunsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */\n \n\nbase-commit: dc6b1d92ca9c0c538daa244e3910bb8b2a50d959\n-- \n2.17.0.windows.1\n\n"},{"id":"347359","messageId":"CABPp-BFLUVB-gMparKd98v=dxBkmRdDRWCEkGweJ0fO-83DPWw@mail.gmail.com","threadId":"48442","inReplyTo":"20180511125623.6068-1-benpeart@microsoft.com","subject":"Re: [PATCH v3] add status config and command line options for rename detection","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-05-11T14:33:42Z","receivedAt":"2018-05-11T14:33:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Ben,\n\nOn Fri, May 11, 2018 at 5:56 AM, Ben Peart wrote:\n> After performing a merge that has conflicts git status will, by default,\n> attempt to detect renames which causes many objects to be examined. In a\n> virtualized repo, those objects do not exist locally so the rename logic\n> triggers them to be fetched from the server. This results in the status call\n> taking hours to complete on very large repos vs seconds with this patch.\n>\n> Add a new config status.renames setting to enable turning off rename detection\n> during status and commit. This setting will default to the value of\n> diff.renames.\n>\n> Add a new config status.renamelimit setting to to enable bounding the time\n> spent finding out inexact renames during status and commit. This setting will\n> default to the value of diff.renamelimit.\n>\n> Add --no-renames command line option to status that enables overriding the\n> config setting from the command line. Add --find-renames[=] command line\n> option to status that enables detecting renames and optionally setting the\n> similarity index.\n\nAny chance I could get you to re-wrap this at a smaller column width?\nDoesn't fit in my (80-char) terminal when I run `git log`; a couple\nlines run over by a couple characters. (Sorry for not noticing this\nearlier)\n\n> This patch depends on em/status-rename-config\n\nI'd leave this line for the notes. It's useful information now, but\nwon't be to someone looking at the commit a year from now, so it\nprobably doesn't belong in the commit message.\n\n\nWith those two changes:\n Reviewed-by: Elijah Newren \n"},{"id":"347363","messageId":"20180511153842.3368-1-benpeart@microsoft.com","threadId":"48442","inReplyTo":"20180509144213.18032-1-benpeart@microsoft.com","subject":"[PATCH v4] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-05-11T15:38:58Z","receivedAt":"2018-05-11T15:39:07Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"After performing a merge that has conflicts git status will, by default,\nattempt to detect renames which causes many objects to be examined. In a\nvirtualized repo, those objects do not exist locally so the rename logic\ntriggers them to be fetched from the server. This results in the status call\ntaking hours to complete on very large repos vs seconds with this patch.\n\nAdd a new config status.renames setting to enable turning off rename\ndetection during status and commit. This setting will default to the value\nof diff.renames.\n\nAdd a new config status.renamelimit setting to to enable bounding the time\nspent finding out inexact renames during status and commit. This setting\nwill default to the value of diff.renamelimit.\n\nAdd --no-renames command line option to status that enables overriding the\nconfig setting from the command line. Add --find-renames[=] command line\noption to status that enables detecting renames and optionally setting the\nsimilarity index.\n\nReviewed-by: Elijah Newren \nOriginal-Patch-by: Alejandro Pauly \nSigned-off-by: Ben Peart \n---\n\nNotes:\n Base Ref: commit dc6b1d92ca9c0c538daa244e3910bb8b2a50d959 (em/status-rename-config)\n Web-Diff: https://github.com/benpeart/git/commit/95974d512b\n Checkout: git fetch https://github.com/benpeart/git status-renames-v4 && git checkout 95974d512b\n \n ### Interdiff (v3..v4):\n \n ### Patches\n\n Documentation/config.txt | 12 ++++\n Documentation/git-status.txt | 10 ++++\n builtin/commit.c | 42 +++++++++++++\n diff.c | 2 +-\n diff.h | 1 +\n t/t7525-status-rename.sh | 113 +++++++++++++++++++++++++++++++++++\n wt-status.c | 12 ++++\n wt-status.h | 4 +-\n 8 files changed, 194 insertions(+), 2 deletions(-)\n create mode 100755 t/t7525-status-rename.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2659153cb3..88884f1ead 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3119,6 +3119,18 @@ status.displayCommentPrefix::\n \tbehavior of linkgit:git-status[1] in Git 1.8.4 and previous.\n \tDefaults to false.\n \n+status.renameLimit::\n+\tThe number of files to consider when performing rename detection\n+\tin linkgit:git-status[1] and linkgit:git-commit[1]. Defaults to\n+\tthe value of diff.renameLimit.\n+\n+status.renames::\n+\tWhether and how Git detects renames in linkgit:git-status[1] and\n+\tlinkgit:git-commit[1] . If set to \"false\", rename detection is\n+\tdisabled. If set to \"true\", basic rename detection is enabled.\n+\tIf set to \"copies\" or \"copy\", Git will detect copies, as well.\n+\tDefaults to the value of diff.renames.\n+\n status.showStash::\n \tIf set to true, linkgit:git-status[1] will display the number of\n \tentries currently stashed away.\ndiff --git a/Documentation/git-status.txt b/Documentation/git-status.txt\nindex c16e27e63d..c4467ffb98 100644\n--- a/Documentation/git-status.txt\n+++ b/Documentation/git-status.txt\n@@ -135,6 +135,16 @@ ignored, then the directory is not shown, but all contents are shown.\n \tDisplay or do not display detailed ahead/behind counts for the\n \tbranch relative to its upstream branch. Defaults to true.\n \n+--renames::\n+--no-renames::\n+\tTurn on/off rename detection regardless of user configuration.\n+\tSee also linkgit:git-diff[1] `--no-renames`.\n+\n+--find-renames[=]::\n+\tTurn on rename detection, optionally setting the similarity\n+\tthreshold.\n+\tSee also linkgit:git-diff[1] `--find-renames`.\n+\n ...::\n \tSee the 'pathspec' entry in linkgit:gitglossary[7].\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 5240f11225..b50e33ef48 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -143,6 +143,16 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int opt_parse_rename_score(const struct option *opt, const char *arg, int unset)\n+{\n+\tconst char **value = opt->value;\n+\tif (arg != NULL && *arg == '=')\n+\t\targ = arg + 1;\n+\n+\t*value = arg;\n+\treturn 0;\n+}\n+\n static void determine_whence(struct wt_status *s)\n {\n \tif (file_exists(git_path_merge_head()))\n@@ -1259,11 +1269,31 @@ static int git_status_config(const char *k, const char *v, void *cb)\n \t\t\treturn error(_(\"Invalid untracked files mode '%s'\"), v);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(k, \"diff.renamelimit\")) {\n+\t\tif (s->rename_limit == -1)\n+\t\t\ts->rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renamelimit\")) {\n+\t\ts->rename_limit = git_config_int(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"diff.renames\")) {\n+\t\tif (s->detect_rename == -1)\n+\t\t\ts->detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(k, \"status.renames\")) {\n+\t\ts->detect_rename = git_config_rename(k, v);\n+\t\treturn 0;\n+\t}\n \treturn git_diff_ui_config(k, v, NULL);\n }\n \n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n+\tstatic int no_renames = -1;\n+\tstatic const char *rename_score_arg = (const char *)-1;\n \tstatic struct wt_status s;\n \tint fd;\n \tstruct object_id oid;\n@@ -1297,6 +1327,10 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\t N_(\"ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)\"),\n \t\t PARSE_OPT_OPTARG, NULL, (intptr_t)\"all\" },\n \t\tOPT_COLUMN(0, \"column\", &s.colopts, N_(\"list untracked files in columns\")),\n+\t\tOPT_BOOL(0, \"no-renames\", &no_renames, N_(\"do not detect renames\")),\n+\t\t{ OPTION_CALLBACK, 'M', \"find-renames\", &rename_score_arg,\n+\t\t N_(\"n\"), N_(\"detect renames, optionally set similarity index\"),\n+\t\t PARSE_OPT_OPTARG, opt_parse_rename_score },\n \t\tOPT_END(),\n \t};\n \n@@ -1336,6 +1370,14 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \ts.ignore_submodule_arg = ignore_submodule_arg;\n \ts.status_format = status_format;\n \ts.verbose = verbose;\n+\tif (no_renames != -1)\n+\t\ts.detect_rename = !no_renames;\n+\tif ((intptr_t)rename_score_arg != -1) {\n+\t\tif (s.detect_rename < DIFF_DETECT_RENAME)\n+\t\t\ts.detect_rename = DIFF_DETECT_RENAME;\n+\t\tif (rename_score_arg)\n+\t\t\ts.rename_score = parse_rename_score(&rename_score_arg);\n+\t}\n \n \twt_status_collect(&s);\n \ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..5dfc24aa6d 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -177,7 +177,7 @@ static int parse_submodule_params(struct diff_options *options, const char *valu\n \treturn 0;\n }\n \n-static int git_config_rename(const char *var, const char *value)\n+int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn DIFF_DETECT_RENAME;\ndiff --git a/diff.h b/diff.h\nindex d29560f822..dedac472ca 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -324,6 +324,7 @@ extern int git_diff_ui_config(const char *var, const char *value, void *cb);\n extern void diff_setup(struct diff_options *);\n extern int diff_opt_parse(struct diff_options *, const char **, int, const char *);\n extern void diff_setup_done(struct diff_options *);\n+extern int git_config_rename(const char *var, const char *value);\n \n #define DIFF_DETECT_RENAME\t1\n #define DIFF_DETECT_COPY\t2\ndiff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\nnew file mode 100755\nindex 0000000000..ef8b1b3078\n--- /dev/null\n+++ b/t/t7525-status-rename.sh\n@@ -0,0 +1,113 @@\n+#!/bin/sh\n+\n+test_description='git status rename detection options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo 1 >original &&\n+\tgit add . &&\n+\tgit commit -m\"Adding original file.\" &&\n+\tmv original renamed &&\n+\techo 2 >> renamed &&\n+\tgit add . &&\n+\tcat >.gitignore <<-\\EOF\n+\t.gitignore\n+\texpect*\n+\tactual*\n+\tEOF\n+'\n+\n+test_expect_success 'status no-options' '\n+\tgit status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status --no-renames' '\n+\tgit status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames false' '\n+\tgit -c diff.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames inherits from diff.renames true' '\n+\tgit -c diff.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides diff.renames false' '\n+\tgit -c diff.renames=true -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status.renames overrides from diff.renames true' '\n+\tgit -c diff.renames=false -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status status.renames=false' '\n+\tgit -c status.renames=false status >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status status.renames=true' '\n+\tgit -c status.renames=true status >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'commit honors status.renames=false' '\n+\tgit -c status.renames=false commit --dry-run >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'commit honors status.renames=true' '\n+\tgit -c status.renames=true commit --dry-run >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'status config overridden' '\n+\tgit -c status.renames=true status --no-renames >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=100%' '\n+\tgit status -M=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual &&\n+\n+\tgit status --find-rename=100% >actual &&\n+\ttest_i18ngrep \"deleted:\" actual &&\n+\ttest_i18ngrep \"new file:\" actual\n+'\n+\n+test_expect_success 'status score=01%' '\n+\tgit status -M=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual &&\n+\n+\tgit status --find-rename=01% >actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_expect_success 'copies not overridden by find-rename' '\n+\tcp renamed copy &&\n+\tgit add copy &&\n+\n+\tgit -c status.renames=copies status -M=01% >actual &&\n+\ttest_i18ngrep \"copied:\" actual &&\n+\ttest_i18ngrep \"renamed:\" actual &&\n+\n+\tgit -c status.renames=copies status --find-rename=01% >actual &&\n+\ttest_i18ngrep \"copied:\" actual &&\n+\ttest_i18ngrep \"renamed:\" actual\n+'\n+\n+test_done\ndiff --git a/wt-status.c b/wt-status.c\nindex 32f3bcaebd..172f07cbb0 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -138,6 +138,9 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_stash = 0;\n \ts->ahead_behind_flags = AHEAD_BEHIND_UNSPECIFIED;\n \ts->display_comment_prefix = 0;\n+\ts->detect_rename = -1;\n+\ts->rename_score = -1;\n+\ts->rename_limit = -1;\n }\n \n static void wt_longstatus_print_unmerged_header(struct wt_status *s)\n@@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n \t}\n \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n \trev.diffopt.format_callback_data = s;\n+\trev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_files(&rev, 0);\n }\n@@ -625,6 +631,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n \trun_diff_index(&rev, 1);\n }\n@@ -982,6 +991,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n+\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n+\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n \trev.diffopt.file = s->fp;\n \trev.diffopt.close_file = 0;\n \t/*\ndiff --git a/wt-status.h b/wt-status.h\nindex 430770b854..1673d146fa 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -89,7 +89,9 @@ struct wt_status {\n \tint show_stash;\n \tint hints;\n \tenum ahead_behind_flags ahead_behind_flags;\n-\n+\tint detect_rename;\n+\tint rename_score;\n+\tint rename_limit;\n \tenum wt_status_format status_format;\n \tunsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */\n \n\nbase-commit: dc6b1d92ca9c0c538daa244e3910bb8b2a50d959\n-- \n2.17.0.windows.1\n\n"},{"id":"347434","messageId":"20180512080437.GA16679@esm","threadId":"48442","inReplyTo":"20180511125623.6068-1-benpeart@microsoft.com","subject":"Re: [PATCH v3] add status config and command line options for rename detection","fromName":"Eckhard Maaß","fromEmail":"eckhard.s.maass@googlemail.com","sentAt":"2018-05-12T08:04:37Z","receivedAt":"2018-05-12T08:04:44Z","isPatch":true,"sender":{"key":"eckhard.s.maass@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/21984134?v=4"},"body":"On Fri, May 11, 2018 at 12:56:39PM +0000, Ben Peart wrote:\n> After performing a merge that has conflicts git status will, by default,\n> attempt to detect renames which causes many objects to be examined. In a\n> virtualized repo, those objects do not exist locally so the rename logic\n> triggers them to be fetched from the server. This results in the status call\n> taking hours to complete on very large repos vs seconds with this patch.\n\nI see where your need comes from, but as you based this on my little\npatch one can achieve this already with tweaking diff.renames itself. I\ndo wonder why there is a special need for the status command here. And\nif there is, I personally would like it more in a style that you could\ntake all the options provided by diff.*-configuration and prefix that\nwith status, eg status.diff.renames = true. What do you think? If you\nreally only need this for merges, maybe a more specialised option is\ncalled for that only kicks in when there is a merge going on?\n\nI would like that status behaves as similar as possible to\ndiff/show/log. Special options will pull away from that again - passing\n-m to show or log will lead to the same performance issues, correct?\nCould it be feasible to impose an overall time limit on the detection?\n\nAnd after writing this I wonder what were your experience with just\ntweaking renameLimit - setting it very low should have helped the\nfetching from server part already, shouldn't it?\n\n> Add --no-renames command line option to status that enables overriding the\n> config setting from the command line. Add --find-renames[=] command line\n> option to status that enables detecting renames and optionally setting the\n> similarity index.\n\nWould it be reasonable to extend this so that we just use the same\nmachinery for parsing command line options for the diffcore options and\npass this along? It seems to me that git status wants the same init as\ndiff/show/log has anyway. But I like the direction towards passing more\ncommand line options to the git status command. \n\n> static void wt_longstatus_print_unmerged_header(struct wt_status *s)\n> @@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n> \t}\n> \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n> \trev.diffopt.format_callback_data = s;\n> +\trev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n> +\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n> +\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n> \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n> \trun_diff_files(&rev, 0);\n> }\n> @@ -625,6 +631,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n> +\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n> +\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n> \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n> \trun_diff_index(&rev, 1);\n> }\n> @@ -982,6 +991,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n> +\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n> +\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n> \trev.diffopt.file = s->fp;\n> \trev.diffopt.close_file = 0;\n> \t/*\n\nSomehow I am inclined that those should be factored out to a common\nmethod if the rest of the patch stays as it is.\n\nGreetings,\nEckhard\n"},{"id":"347594","messageId":"d306e8bf-1847-ca81-16ac-fa99ee3f6755@gmail.com","threadId":"48442","inReplyTo":"20180512080437.GA16679@esm","subject":"Re: [PATCH v3] add status config and command line options for rename detection","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-05-14T12:57:45Z","receivedAt":"2018-05-14T12:57:53Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 5/12/2018 4:04 AM, Eckhard Maaß wrote:\n> On Fri, May 11, 2018 at 12:56:39PM +0000, Ben Peart wrote:\n>> After performing a merge that has conflicts git status will, by default,\n>> attempt to detect renames which causes many objects to be examined. In a\n>> virtualized repo, those objects do not exist locally so the rename logic\n>> triggers them to be fetched from the server. This results in the status call\n>> taking hours to complete on very large repos vs seconds with this patch.\n> \n> I see where your need comes from, but as you based this on my little\n> patch one can achieve this already with tweaking diff.renames itself. I\n> do wonder why there is a special need for the status command here\n\nThe rename detection feature is nice and we'd like to leave it on \nwhenever possible. The performance issues only occur when in the middle \nof a merge - normal status commands behave reasonably. As a result, we \ndon't want to just turn it off completely by setting diff.renames.\n\nUntil we come up with a more elegant solution, we currently turn it off \ncompletely for merge via the new merge settings and then intercept calls \nto status and if there is a MERGE_HEAD we turn it off for status just \nfor that specific call. I view this as a temporary solution so would \nnot want to put that logic into git proper as it is quite specific to \nwhen running git on a virtualized repo.\n\n> if there is, I personally would like it more in a style that you could\n> take all the options provided by diff.*-configuration and prefix that\n> with status, eg status.diff.renames = true. What do you think? If you\n> really only need this for merges, maybe a more specialised option is\n> called for that only kicks in when there is a merge going on?\n> \n> I would like that status behaves as similar as possible to\n> diff/show/log. Special options will pull away from that again - passing\n> -m to show or log will lead to the same performance issues, correct?\n> Could it be feasible to impose an overall time limit on the detection?\n> \n\nI agree that they should behave as similar as possible which is why all \nthe new settings default to the diff setting when not explicitly set. I \nbelieve this is a good model - if you don't do anything special you get \nthe default/same behavior but if you know and need special behavior, you \nnow have that option.\n\n> And after writing this I wonder what were your experience with just\n> tweaking renameLimit - setting it very low should have helped the\n> fetching from server part already, shouldn't it?\n> \n>> Add --no-renames command line option to status that enables overriding the\n>> config setting from the command line. Add --find-renames[=] command line\n>> option to status that enables detecting renames and optionally setting the\n>> similarity index.\n> \n> Would it be reasonable to extend this so that we just use the same\n> machinery for parsing command line options for the diffcore options and\n> pass this along? It seems to me that git status wants the same init as\n> diff/show/log has anyway. But I like the direction towards passing more\n> command line options to the git status command.\n> \n\nI agree that it is unfortunate that diff/merge/status all parse and deal \nwith config settings differently. I'd be happy to see someone tackle \nthat and move the code to a single, coherent model but that is beyond \nthe scope of this patch.\n\n>> static void wt_longstatus_print_unmerged_header(struct wt_status *s)\n>> @@ -592,6 +595,9 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)\n>> \t}\n>> \trev.diffopt.format_callback = wt_status_collect_changed_cb;\n>> \trev.diffopt.format_callback_data = s;\n>> +\trev.diffopt.detect_rename = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n>> +\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n>> +\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n>> \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n>> \trun_diff_files(&rev, 0);\n>> }\n>> @@ -625,6 +631,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n>> +\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n>> +\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n>> \tcopy_pathspec(&rev.prune_data, &s->pathspec);\n>> \trun_diff_index(&rev, 1);\n>> }\n>> @@ -982,6 +991,9 @@ 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 = s->detect_rename >= 0 ? s->detect_rename : rev.diffopt.detect_rename;\n>> +\trev.diffopt.rename_limit = s->rename_limit >= 0 ? s->rename_limit : rev.diffopt.rename_limit;\n>> +\trev.diffopt.rename_score = s->rename_score >= 0 ? s->rename_score : rev.diffopt.rename_score;\n>> \trev.diffopt.file = s->fp;\n>> \trev.diffopt.close_file = 0;\n>> \t/*\n> \n> Somehow I am inclined that those should be factored out to a common\n> method if the rest of the patch stays as it is.\n> \n\nI debated that as well but given the logic is so simple, opted to stick \nwith this. I also debated whether it would be clearer in the form:\n\nif (s->detect_rename >= 0)\n\trev.diffopt.detect_rename = s->detect_rename;\n\nBut decided git contributes are used to seeing dense code :) and this \nstyle better matched what I saw in the merge settings.\n\n> Greetings,\n> Eckhard\n> \n"}]}