Subject: Re: [PATCH v2] wt-status: use rename settings from init_diff_ui_defaults

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Tue, 01 May 2018 13:00:54 +0200

On Tue, May 01 2018, Eckhard S. Maaß wrote:

> Since the very beginning, git status behaved differently for rename
> detection than other rename aware commands like git log or git show as
> it has the use of rename hard coded into it.

Can you elaborate on this? It seems initial rename detection was added
in 5c97558c9a ("[PATCH] Detect renames in diff family.", 2005-05-19) and
the first version of the status script added by Linus in a3e870f2e2
("Add "commit" helper script", 2005-05-30), and that one piggy-backs on
"diff" for rename detection.

So didn't we use diff heuristics to begin with, and then regressed? I've
only given this a skimming, but it's useful to have that sort of
historical context mentioned explicitly with commit ids.

> After 5404c116aa ("diff:
> activate diff.renames by default", 2016-02-25) the default behaves the
> same by coincidence, but a work flow like
>
>     - git add .
>     - git status
>     - git commit
>     - git show
>
> should give you the same information on renames (and/or copies if
> activated) accordingly to the diff.renames and diff.renameLimit setting.
>
> With this commit the hard coded settings are dropped from the status
> command.

It's unclear to me what this means, so the only difference between
"status" and "diff" is that the former had a hardcoded limit of 200? In
that case it was added at 100 (later adusted) in 0024a54923 ("Fix the
rename detection limit checking", 2007-09-14), so not since "the very
beginning...".

> Signed-off-by: Eckhard S. Maaß <[email-masked]>
> Reviewed-by: Elijah Newren <[email-masked]>
> ---
>  builtin/commit.c       |  2 +-
>  t/t4001-diff-rename.sh | 12 ++++++++++++
>  wt-status.c            |  4 ----
>  3 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 5571d4a3e2..5240f11225 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -161,9 +161,9 @@ static void determine_whence(struct wt_status *s)
>  static void status_init_config(struct wt_status *s, config_fn_t fn)
>  {
>  	wt_status_prepare(s);
> +	init_diff_ui_defaults();
>  	git_config(fn, s);
>  	determine_whence(s);
> -	init_diff_ui_defaults();
>  	s->hints = advice_status_hints; /* must come after git_config() */
>  }
>
> diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
> index a07816d560..bf4030371a 100755
> --- a/t/t4001-diff-rename.sh
> +++ b/t/t4001-diff-rename.sh
> @@ -138,6 +138,18 @@ test_expect_success 'favour same basenames over different ones' '
>  	test_i18ngrep "renamed: .*path1 -> subdir/path1" out
>  '
>
> +test_expect_success 'test diff.renames=true for git status' '
> +	git -c diff.renames=true status >out &&
> +	test_i18ngrep "renamed: .*path1 -> subdir/path1" out
> +'
> +
> +test_expect_success 'test diff.renames=false for git status' '
> +	git -c diff.renames=false status >out &&
> +	test_i18ngrep ! "renamed: .*path1 -> subdir/path1" out &&
> +	test_i18ngrep "new file: .*subdir/path1" out &&
> +	test_i18ngrep "deleted: .*[^/]path1" out
> +'
> +
>  test_expect_success 'favour same basenames even with minor differences' '
>  	git show HEAD:path1 | sed "s/15/16/" > subdir/path1 &&
>  	git status >out &&
> diff --git a/wt-status.c b/wt-status.c
> index 50815e5faf..32f3bcaebd 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -625,9 +625,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)
>  	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
>  	rev.diffopt.format_callback = wt_status_collect_updated_cb;
>  	rev.diffopt.format_callback_data = s;
> -	rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
> -	rev.diffopt.rename_limit = 200;
> -	rev.diffopt.break_opt = 0;
>  	copy_pathspec(&rev.prune_data, &s->pathspec);
>  	run_diff_index(&rev, 1);
>  }
> @@ -985,7 +982,6 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
>  	setup_revisions(0, NULL, &rev, &opt);
>
>  	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
> -	rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
>  	rev.diffopt.file = s->fp;
>  	rev.diffopt.close_file = 0;
>  	/*