v2wt-status: use rename settings from init_diff_ui_defaults

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

Signed-off-by: Eckhard S. Maaß <eckhard.s.maass@gmail.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
---
 builtin/commit.c       |  2 +-
 t/t4001-diff-rename.sh | 12 ++++++++++++
 wt-status.c            |  4 ----
 3 files changed, 13 insertions(+), 5 deletions(-)

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

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.

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

On Tue, May 01, 2018 at 01:00:54PM +0200, Ævar Arnfjörð Bjarmason wrote:

Sorry for not making this too explicit: I traced wt-status.c to its beginning in c91f0d92ef ("git-commit.sh: convert run_status to a C builtin", 2006-09-08). Here I lost track of other changes - but the commit you gave is earlier also has rename detection hard coded in the status command. Should I add this as a starting point instead of the commit mentioned so far? And this also seems like the very beginning of git status.

The point I wanted to make, is that git show showed you renaming of files out of the box whereas other commands did not. At least I remember this form 5+ years ago.

This changed with 5404c116aa, so that the out of the box behaviour is the same, but this is more a coincidence that the hard coded flag is the same as the default configuration value.

My comment was targeted at the hard coded rename detection flag - the other two just have seem to pile up on that and I wanted to clean them up, too. Maybe a phrasing like "While at it, also remove the two other hard coded values concerning rename detection in git status." is better? Is my intent clearer now?

Greetings, Eckhard

"Eckhard S. Maaß" <eckhard.s.maass@googlemail.com> wrote:

My understanding is that the succession of events went stg like:

1) invent the rename detection, but consider it experimental
   hence don't activate it by default;

2) add commands using the rename detection, and since it works
   well, use it by default;

3) activate rename detection by default for diff.

The next logical step is what you patch does indeed.

That init_diff_ui_defaults() should indeed have been before git_config() from the beginning. My bad, I'm the one who misplaced it apparently :-(.

This "break_opt = 0" deserves a mention in the commit message IMHO. I'm not 100% sure it's a good change actually.

break_opt is normally controlled by "-B/--break-rewrites". I'm not sure why it was set to 0.

On Tue, May 01, 2018 at 01:09:06PM +0200, Matthieu Moy wrote:

Should I have done this "bug fix" in a separate commit or mention it in the commit message?

Hm, what problems do you see here? The purpose of my patch is have the same kind of output from git status and, after commit, git show. I cannot find a good reason for git status to behave there differently, but I am interested to see where I am wrong.

On the other hand, one could a configuration option to the diff.* family for controlling that toggle also.

Greetings, Eckhard

"Eckhard Maaß" <eckhard.s.maass@googlemail.com>:

I'm fine with it as-is. Before your "fix", the config was ignored because overwritten by init_diff_ui_defaults() after reading the config, so effect of your change is indeed what the commit message describes.

I'm often thinking aloud while reviewing, don't take my comments as objections.

I don't see any "problem", I *think* your change is good, but I can't fully convince myself that it is without further explanation.

Unlike the other two, this option has no corresponding configuration variable, so the "let the config" argument doesn't apply. For "git status", there's actually not even a command-line option. So, this assignment removed, there's no way in the user-interface to re-enable the previous behavior. *If* there was a good reason to get "break_opt = 0", then your patch is breaking it.

Unfortunately, the commit introducing it doesn't help much: f714fb8 (Enable rewrite as well as rename detection in git-status, 2007-12-02) is just a one-liner message with a one-liner patch.

But actually, I never used -B/--break-rewrites, and writting this message I tried to get a case where -B would make a difference and I'm not even able to find one. So, as someone who never understood the real point of -B, I'm not sure I'm qualified to juge on what's the best default ;-).

On Tue, May 1, 2018 at 5:23 AM, Matthieu Moy <git@matthieu-moy.fr> wrote:

In git.git, just make non-sensical changes like the following (a normal rename, and a break-rename, for comparison):

    git mv oidset.c another-file.c
    echo "// Modifying normally renamed file for fun" >>another-file.c
    git rm merge.c
    git mv object.c merge.c
    echo "// Random change to break-rename file" >>merge.c
    git add merge.c another-file.c

Now compare, git before Eckhard's change:

$ /usr/bin/git status
HEAD detached at v2.17.0
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

    renamed:    oidset.c -> another-file.c
    renamed:    object.c -> merge.c

and git after Eckhard's change:

$ git status
HEAD detached at v2.17.0
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

    renamed:    oidset.c -> another-file.c
    modified:   merge.c
    deleted:    object.c

Which is better? Well, gut reaction only looking at the above output folks would probably say the former is. However, compare the output to this:

$ git diff --name-status HEAD R094 oidset.c another-file.c M merge.c D object.c

git status and git diff are inconsistent for no good reason. We can instruct diff to behave the same as old status, of course:

$ git diff --name-status -B HEAD R094 oidset.c another-file.c R099 object.c merge.c

...but why does the user have to instruct diff to get the same default behavior they get from status? I'll note here that log and show have the same default as diff.

I'm not certain what the default should be, but I do believe that it
should be consistent between these commands.  I lean towards
considering break detection being on by default a good thing, but
there are some interesting issues to address:
  - there is no knob to adjust break detection for status, only for
diff/log/show/etc.
  - folks have a knob to turn break detection on (for diff/log/show),
but not one to turn it off
  - for status, break detection makes no sense if rename detection is off.
  - for diff/log/show, break detection provides almost no value if
rename detection is off (only a dissimilarity index), suggesting that
if user turns rename detection off and doesn't explicitly ask for
break detection, then it's a waste of time to have it be on
  - merge-recursive would break horribly right now if someone turned
break detection on for it.  Turning it on might be a good long term
goal, but it's not an easy change.

So, where does that leave us?  My opinion is
  - these commands should be consistent.  Eckhard's patch makes them so.
  - we might want to move towards break detection being on as the
default.  That's a couple patch series (one for everything but
merge-recursive, and a separate much bigger series for
merge-recursive).

But I can see others saying we should leave things inconsistent until we can fix the other commands to use break detection as the default. So...thoughts?

Elijah

Elijah Newren <newren@gmail.com> writes:

Many of the issues in the above list are surmountable. A new option could be added to "status" to enable break or "diff" family to disable it if we really wanted to. A new "rewritten" state can be added alongside with "modified" to "status" output.

A more serious issue around "-B" is this one:

    https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/

Even though the message is back from 2015 and asks users not to use -B/-M together for anything critical "for now", the issue has not been resolved and the same bug remains with us in the current code.

In the longer term, I suspect that it might make sense to have an option to let users choose among "I do not want to have anything to do with -B", "I always want -B when I ask for -M" and "I always want -B whether I ask for -M". But unfortunately the latter two with the current codebase is an unacceptably risky/broken choice.

On Tue, May 1, 2018 at 4:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

Very interesting; I didn't know that break detection and rename detection were unsafe to use together.

I also just realized that in addition to status being inconsistent with diff/log/show, it was also inconsistent with itself -- it handles staged and unstaged changes differently. (wt_status_collect_changes_worktree() had different settings for break detection than wt_status_collect_changes_index().)

Eckhard, can you add some comments to your commit message mentioning the email pointed to by Junio about break detection and rename detection being unsafe to use together, as well as the inconsistencies in break detection between different commands? That may help future readers understand why break detection was turned off for wt_status_collect_changes_index().

On 5/1/2018 8:08 PM, Elijah Newren wrote:

Wow, lots of inconsistent behaviors here with diff/merge/status and the various options being used. Let me add another one:

I've been sitting on another patch that we've been using internally for some time that enables us to turn off rename and break detection for status via config settings and new command-line options.

The issue that triggered the creation of the patch was that if someone ran status while in a merge conflict state, the status would take a very long time. Turning off rename and break detection "fixed" the problem.

I was waiting for some of these inflight changes to settle down and get accepted before I started another patch series but I thought I should at least let everyone know about this additional issue that will need to be addressed.

On Tue, May 01, 2018 at 05:08:27PM -0700, Elijah Newren wrote:

I will work on that.

Greetings, Eckhard

If you do something like

    - git add .
    - git status
    - git commit
    - git show (or git diff HEAD)

one would expect to have analogous output from git status and git show (or similar diff-related programs). This is generally not the case, as git status has hard coded values for diff related options.

With this commit the hard coded settings are dropped from the status command in favour for values provided by git_diff_ui_config.

What follows are some remarks on the concrete options which were hard coded in git status:

diffopt.detect_rename

Since the very beginning of git status in a3e870f2e2 ("Add "commit" helper script", 2005-05-30), git status always used rename detection, whereas with commands like show and log one had to activate it with a command line option. After 5404c116aa ("diff: activate diff.renames by default", 2016-02-25) the default behaves the same by coincidence, but changing diff.renames to other values can break the consistency between git status and other commands again. With this commit one control the same default behaviour with diff.renames.

diffopt.rename_limit

Similarly one has the option diff.renamelimit to adjust this limit for all commands but git status. With this commit git status will also honor those.

diffopt.break_opt

Unlike the other two options this cannot be configured by a configuration option yet. This commit will also change the default behaviour to not use break rewrites. But as rename detection is most likely on, this is dangerous to be activated anyway as one can see here:

    https://public-inbox.org/git/xmqqegqaahnh.fsf@gitster.dls.corp.google.com/

Signed-off-by: Eckhard S. Maaß <eckhard.s.maass@gmail.com>
Reviewed-by: Elijah Newren <newren@gmail.com>
---

Hello,

Hopefully I have addressed all issues that have come up so far.

Greetings, Eckhard

 builtin/commit.c       |  2 +-
 t/t4001-diff-rename.sh | 12 ++++++++++++
 wt-status.c            |  4 ----
 3 files changed, 13 insertions(+), 5 deletions(-)

On Fri, May 4, 2018 at 4:12 AM, Eckhard S. Maaß <eckhard.s.maass@googlemail.com> wrote:

<snip>

Looks good to me, thanks.

Elijah

On Tue, May 01, 2018 at 02:23:51PM +0200, Matthieu Moy wrote:

No worries, I was wondering while writing the patch to extract it - the init should be changed to the appropriate location even if there is consensus to leave all the other knobs as they are, shouldn't it?

Greetings, Eckhard