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

From: Elijah Newren <newren@gmail.com>
Date: May 10, 2018, 16:19 UTC
Message-ID: <CABPp-BGE6RXv3ka8wGXruFjk3W=kDEDJ6zpH3t5=_CGSTONCHQ@mail.gmail.com>
In-Reply-To: <20180510141621.9668-1-benpeart@microsoft.com>

Hi Ben,

On Thu, May 10, 2018 at 7:16 AM, Ben Peart <Ben.Peart@microsoft.com> wrote:

Show 6 quoted lines

> 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.

Show 8 quoted lines

> 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).

Show 6 quoted lines

> 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.

Show 6 quoted lines

> 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.

Show 8 quoted lines

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

Show 5 quoted lines

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

Can this just be included in the setup?

Everything else in the patch looked good to me.