Re: [PATCH v2] add status config and command line options for rename detection

[Elijah Newren <newren@gmail.com>]

Hi Ben,

On Thu, May 10, 2018 at 7:16 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:
> After performing a merge that has conflicts, git status will by default attempt
> to detect renames which causes many objects to be examined.  In a virtualized
> repo, those objects do not exist locally so the rename logic triggers them to be
> fetched from the server. This results in the status call taking hours to
> complete on very large repos.  Even in a small repo (the GVFS repo) turning off
> break and rename detection has a significant impact:

It'd be nice if you could show that impact by comparing 'git status'
to 'git status --no-renames', for some repo.  Showing only the latter
gives us no way to assess the impact.

> git status --no-renames:
> 31 secs., 105 loose object downloads
>
> git status --no-breaks
> 7 secs., 17 loose object downloads
>
> git status --no-breaks --no-renames
> 1 sec., 1 loose object download

This patch doesn't add a --no-breaks option and it doesn't exist
previously, so adding it to the commit message serves to confuse
rather than help.  I'd just drop the last two of these (and redo the
timing for --no-renames assuming you are built on
em/status-rename-config).

> Add a new config status.renames setting to enable turning off rename detection
> during status.  This setting will default to the value of diff.renames.
>
> Add a new config status.renamelimit setting to to enable bounding the time spent
> finding out inexact renames during status.  This setting will default to the
> value of diff.renamelimit.

It may be worth mentioning that these config settings also affect 'git
commit' (and it does, in my testing, which I think is a good thing).

> Add status --no-renames command line option that enables overriding the config
> setting from the command line. Add --find-renames[=<n>] to enable detecting
> renames and optionally setting the similarity index from the command line.

The command line options are specific to 'git status'.  I don't really
have a strong opinion on whether they should also be added to
git-commit; I suspect users would be more likely to use the config
options in order to set it once and forget about it and that users
would be more likely to want to override their config setting for
status than for commit.

> Note: I removed the --no-breaks command line option from the original patch as
> it will no longer be needed once the default has been changed [1] to turn it off.
>
> [1] https://public-inbox.org/git/20180430093421.27551-2-eckhard.s.maass@gmail.com/

I'd just drop these lines from the commit message, and instead mention
that your patch depends on em/status-rename-config.

> Original-Patch-by: Alejandro Pauly <alpauly@microsoft.com>
> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
> ---
>
> Notes:
>     Base Ref: master

This patch does not apply to master; it has conflicts.

>     Web-Diff: https://github.com/benpeart/git/commit/823212725b

This web diff shows em/status-rename-config as the parent commit, not
master.  Since your commit message mentions you want the change to
break detection provided by that series, just listing it as the
explicit base seems like the right way to go.

>     ### Interdiff (v1..v2):

Thanks.

> +       if ((intptr_t)rename_score_arg != -1) {
> +               s.detect_rename = DIFF_DETECT_RENAME;

I'd still prefer this was a
        if (s.detect_rename < DIFF_DETECT_RENAME)
                s.detect_rename = DIFF_DETECT_RENAME;

If a user specifies they are willing to pay for copy detection, but
then just passes --find-renames=40% because they want to find more
renames, it seems odd to disable copy detection to me.

> +++ b/t/t7525-status-rename.sh

Testcases look good.  It'd be nice to also add a few testcases where
copy detection is turned on -- in particular, I'd like to see one with
--find-renames=$DIFFERENT_THAN_DEFAULT being passed when
merge.renames=copies.


> +test_expect_success 'setup' '
> +       echo 1 >original &&
> +       git add . &&
> +       git commit -m"Adding original file." &&
> +       mv original renamed &&
> +       echo 2 >> renamed &&
> +       git add .
> +'


> +cat >.gitignore <<\EOF
> +.gitignore
> +expect*
> +actual*
> +EOF

Can this just be included in the setup?


Everything else in the patch looked good to me.