git-mv: improve error message for conflicted file

From: Chris Torek <[email protected]>

'git mv' has always complained about renaming a conflicted file, as it cannot handle multiple index entries for one file. However, the error message it uses has been the same as the one for an untracked file:

fatal: not under version control, src=...

which is patently wrong. Distinguish the two cases and add a test to make sure we produce the correct message.

---
git-mv: improve error message for conflicted file

'git mv' has always complained about renaming a conflicted file, as it cannot handle multiple index entries for one file. However, the error message it uses has been the same as the one for an untracked file:

fatal: not under version control, src=...

which is patently wrong. Distinguish the two cases and add a test to make sure we produce the correct message.

------------------------------------------------------------------------

A small note on the test: I originally had a different message text, so the grep is slightly more general than necessary here. This leaves room for a better message, though; I'm not sure mine is that great.

The error messages in general from 'git mv' could probably stand a lot of cleanup. This is just a minimal fix for some particularly bad behavior.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-678%2Fchris3torek%2Fgit-mv-message-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-678/chris3torek/git-mv-message-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/678

 builtin/mv.c  | 15 ++++++++++++---
 t/t7001-mv.sh | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

index be15ba7044..7dff121629 100644
@@ -220,9 +220,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 				argc += last - first;
 			}
-		} else if (cache_name_pos(src, length) < 0)
-			bad = _("not under version control");
-		else if (lstat(dst, &st) == 0 &&
+		} else if (cache_name_pos(src, length) < 0) {
+			/*
+			 * This occurs for both untracked files *and*
+			 * files that are in merge-conflict state, so
+			 * let's distinguish between those two.
+			 */
+			struct cache_entry *ce = cache_file_exists(src, length, ignore_case);
+			if (ce == NULL)
+				bad = _("not under version control");
+			else
+				bad = _("must resolve merge conflict first");
+		} else if (lstat(dst, &st) == 0 &&
 			 (!ignore_case || strcasecmp(src, dst))) {
 			bad = _("destination exists");
 			if (force) {

index 36b50d0b4c..b4974f9385 100755
@@ -248,6 +248,24 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '
 
 rm -f dirty dirty2
 
+# NB: This test is about the error message
+# as well as the failure.
+test_expect_success 'git mv error on conflicted file' '
+	rm -fr .git &&
+	git init &&
+	touch conflicted &&
+	cfhash=$(git hash-object -w conflicted) &&
+	git update-index --index-info <<-EOF &&
+	$(printf "0 $cfhash 0\tconflicted\n")
+	$(printf "100644 $cfhash 1\tconflicted\n")
+	EOF
+
+	test_must_fail git mv conflicted newname 2>actual &&
+	test_i18ngrep "merge.conflict" actual
+'
+
+rm -f conflicted
+
 test_expect_success 'git mv should overwrite symlink to a file' '
 
 	rm -fr .git &&

base-commit: b6a658bd00c9c29e07f833cabfc0ef12224e277a

gitgitgadget

A few nits below...

    if (!ce)
        bad = _("not under version control");
    else
        bad = _("must resolve merge conflict first");

    if (ce)
        bad = _("must resolve merge conflict first");
    else
        bad = _("not under version control");

    _("conflicted");

    git update-index --index-info <<-EOF &&
    0 $cfhash 0    conflicted
    100644 $cfhash 1    conflicted
    EOF

    q_to_tab <<-EOF | git update-index --index-info &&
    0 $cfhash 0Qconflicted
    100644 $cfhash 1Qconflicted
    EOF

    ...
    >conflicted &&
    test_when_finished "rm -f conflicted" &&
    ...

OK, but I'll go with Junio's suggestion of getting ce once and then checking ce_staged anyway. (I'm used to a different style guide that frowns on if (ce) and if (!ce)...)

Maybe. Succinct is usually good though.

OK.

I personally hate depending on literal tabs, as they're really hard to see. If q_to_tab() is OK I'll use that.

Indeed, that's where I copied it from.

Should I clean up other instances of separated-out rm -fs in this file in a separate commit?

Chris

q_to_tab() is a good choice.

In general, as a reviewer, I don't mind seeing a patch or two cleaning up style and other violations, however, the magnitude of the fixes this script needs is quite significant and would end up requiring a fair number of patches. As such, I'm not particularly eager to see the improvement made by this patch -- which is nicely standalone -- weighed down by a lengthy series of patches which aren't really related to it.

If cleaning up the t7001 test script is something you might be interested in doing, then making it a separate patch series would be more palatable. A scan of the script reveals the following problems, though there may be others:

* old style:

    test_expect_success \
        'title' \
        'body line 1 &&
        body line 2'

should become:

test_expect_success 'title' '
body line 1 &&
body line 2
'

* test bodies should be indented with TAB, not spaces

* some tests use a deprecated style in which there are unnecessary blank lines after the opening quote of the test body and before the closing quote; these blanks lines should be removed

* style for cd in subshell is:

    (
        cd foo &&
        ...
    ) &&

not:

    (cd foo &&
        ...
    ) &&

* there should be no whitespace after redirect operators, so:

    foo > actual &&

should become:

    foo >actual &&

* tests 'cd' around and expect other tests to know the current directory and 'cd' relative to that; instead, any test which uses 'cd' should do so in a subshell to ensure the current directory is restored by the time the test ends:

    test_expect_success 'title' '
        something &&
        (
            cd somewhere &&
            something-else
        )
    '

Alternately, it may be possible to take advantage of -C if something-else is a git command:

test_expect_success 'title' '
something &&
git -C somewhere foo
'

* use > rather than touch to create an empty file when the timestamp isn't relevant to the test

* cleanup code outside of tests should be moved into the test and scheduled for execution via test_when_finished()

* there are several standalone "clean up" tests which invoke git reset --hard which should be folded into the tests for which they are cleaning up

* multiple commands on one line:

    mkdir foo && >foo/bar && git add foo/bar &&

should be split across multiple lines:

    mkdir foo &&
    >foo/bar &&
    git add foo/bar &&

* at least one test incorrectly uses single quotes within the body of the test which itself is contained within single quotes; when quoting is needed inside a test body, it should be using double quotes instead; however, in this case, the quotes aren't even needed, so:

    git commit -m 'initial' &&

can just become:

    git commit -m initial &&

* take advantage of here-docs, so:

    { echo other/a.txt; echo other/b.txt; } >expect &&

can be expressed more cleanly as:

    cat >expect <<-\EOF &&
    other/a.txt
    other/b.txt
    EOF

* use test rather than [

* optional: rename the setup test 'prepare reference tree' to 'setup'

* optional modernization: use test_path_exists() and cousins instead of test -f, etc.

* optional: avoid git command upstream of a pipe since the pipe will swallow its exit code, thus a crash won't necessarily be noticed

* optional: it's unusual for tests to blast the test's ".git" directory and recreate it with git init, however, a number of tests in this script do so; for tests which really require a new repository, the more common approach is to use test_create_repo() to create a new repository into which the test can cd (in a subshell) without disturbing the repository used by the other tests in the script

A few of the above fixes can probably be combined into a single patch, in particular, the style fixes in the first four bullet points. Each remaining bullet point, however, probably deserves its own patch (including the one about removing whitespace after a redirect operator).

Or, even better, make ce_stage(ce) not be an error; see
https://lore.kernel.org/git/[email protected]/.

I hadn't gotten around to it yet, but it's still on my radar.

(That said, I've obviously taken a really long time to get to it, so improving the error message as an interim step is perfectly fine.)

Hmph, I actually am not convinced with that one, even though I know I wrote it, I do not think I had thought things through before I did. It may make sense in a narrow special case where there is a single entry at a higher stage to rename it to the destination path and drop it down to stage #0, but I do not think of a good behaviour for more general cases.

What should happen, for example, if two or more entries at higher stages exist for the path being moved? Renaming all of them to the destination path without changing their stage number may make more sense [*1*]. At least, that solution still lets the user to choose object at which stage [*2*] to use in the final resolution.

Now, if we define that "git mv src dst", when 'src' is a conflicted path, moves the working tree file src to dst at the same time moves all the higher stage entries for 'src' to 'dst' while retaining their stage number, what does the degenerate case of having only one such entry look like? You start from the state where you have $that_path at stage #3, "git mv $that_path $over_there" would put you in the state where you have the same contents at $over_there and at stage #3. If you want to just take "their" contents as the resolution, "git add $over_there" after doing that "git mv" would let you record the resolution, so it does not look too bad.

It would also be a handy way to recover from a mistake made by "directory level rename" heuristics. Instead of resolving the content-level conflicts at the wrong path and then moving that resolved result to the right path, you can first correct the wrong path by moving the conflicted whole to the right path and then perform the content-level conflict resolution. The advantage of the latter is obvious. You have to do two things (rename and edit) and with the former way of doing 'edit' first and then 'rename', after resolving the conflict and adding the result at the wrong path, "git ls-files -u" or "git status" no longer help you remember that the path still needs to be moved. Instead, you can move first and "git ls-files -u" would still remember that even after the move you still need to deal with content-level conflicts.

Anyway, I think the "separate missing entry and conflicted entry when issuing an error message" is a strict improvement.

[Footnotes]

*1* Of course, the "D/F conflicts are issue only among the entries
at the same stage" and other usual rules apply when we check if the move of these higher stage entries is possible. I am not sure if the low-level API functions like rename_index_entry_at() are prepared to deal with higher stage entries, though, so this may be a nontrivial amount of new work. It is unclear to me if it is worth doing.

*2* Actually, "the object at stage #1 for conflicted path P" is a
wrong thing to say. The index is designed to hold multiple stage #1 entries at the same path so that it can express the case where there are more than one common ancestors, and multiple stage #3 entries to express an octopus merge in progress. The notation to name the object in the index punts and does not let you say "git diff :1.0:path :1.1:path" to compare the first and the second common ancestor versions of path, though it probably should if we wanted to be consistent.

Oh, good, I actually like the idea of not auto-resolving better; I was always a bit uncomfortable with it. And now that you brought up this case, there's another good case I can think of too: D/F conflicts that arise from a rename/add-like situation, but where the add is a new directory rather than a new file. In such a case, there can be three higher order stages for the file due to content conflicts carried with the rename and auto-resolving them when renaming doesn't make any sense. So, yeah, keeping the higher order stages but just renaming the path seems like a nice improvement for the user.

Agreed.

Yeah, if it were trivial, I would have just done it back when I mentioned it a couple years ago. I think it's still worth doing, but I agree it might need some new low-level API function(s) or some modifications to some existing one(s).

Really? Wow. I never had any clue that this was possible and never ran across it. Is it documented anywhere?

Probably a test script for "read-tree -m" has a fairly detailed and comprehensive table of possible scenarios.

Multiple stage #1 entries were introduced to primarily support the protection against "criss-cross merge" that confuses the history and it is used by the resolve backend, IIRC.

The implementation of octopus we have does not actually use multiple stage #3 entries but just does many two-parent merges in sequence. It started its life as a POC & stop-gap implementation while waiting for the "real thing" that merges with multiple stage #3 entries, but it turned out to be good enough for a relatively rare (and now discouraged mostly due to bisection efficiency reasons) style of merge and left in that shape until now.

From: Chris Torek <[email protected]>

'git mv' has always complained about renaming a conflicted file, as it cannot handle multiple index entries for one file. However, the error message it uses has been the same as the one for an untracked file:

fatal: not under version control, src=...

which is patently wrong. Distinguish the two cases and add a test to make sure we produce the correct message.

---
git-mv: improve error message for conflicted file

'git mv' has always complained about renaming a conflicted file, as it cannot handle multiple index entries for one file. However, the error message it uses has been the same as the one for an untracked file:

fatal: not under version control, src=...

which is patently wrong. Distinguish the two cases and add a test to make sure we produce the correct message.

------------------------------------------------------------------------

Tests updated, and took Junio's suggestion to reduce the cache lookup to one call.

I put in the shortened "conflicted" here but did not shorten the existing "not under version control" message (to minimize the visible and translations-required changes).

I like the idea of renaming all stages and keeping them at their current stages, but that's too much for this patch.

I'll be traveling next week and not sure if I will get to any followups for a while.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-678%2Fchris3torek%2Fgit-mv-message-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-678/chris3torek/git-mv-message-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/678

Range-diff vs v1:

1: 0b617e74f7 ! 1: f2e251a7ea git-mv: improve error message for conflicted file

     @@ Commit message
          Signed-off-by: Chris Torek <[email protected]>
      
       ## builtin/mv.c ##
     +@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
     + 	struct stat st;
     + 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
     + 	struct lock_file lock_file = LOCK_INIT;
     ++	struct cache_entry *ce;
     + 
     + 	git_config(git_default_config, NULL);
     + 
      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)
       				}
       				argc += last - first;
       			}
      -		} else if (cache_name_pos(src, length) < 0)
     --			bad = _("not under version control");
     ++		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
     + 			bad = _("not under version control");
      -		else if (lstat(dst, &st) == 0 &&
     -+		} else if (cache_name_pos(src, length) < 0) {
     -+			/*
     -+			 * This occurs for both untracked files *and*
     -+			 * files that are in merge-conflict state, so
     -+			 * let's distinguish between those two.
     -+			 */
     -+			struct cache_entry *ce = cache_file_exists(src, length, ignore_case);
     -+			if (ce == NULL)
     -+				bad = _("not under version control");
     -+			else
     -+				bad = _("must resolve merge conflict first");
     ++		} else if (ce_stage(ce)) {
     ++			bad = _("conflicted");
      +		} else if (lstat(dst, &st) == 0 &&
       			 (!ignore_case || strcasecmp(src, dst))) {
       			bad = _("destination exists");
     @@ t/t7001-mv.sh: test_expect_success 'git mv should not change sha1 of moved cache
      +test_expect_success 'git mv error on conflicted file' '
      +	rm -fr .git &&
      +	git init &&
     -+	touch conflicted &&
     -+	cfhash=$(git hash-object -w conflicted) &&
     -+	git update-index --index-info <<-EOF &&
     -+	$(printf "0 $cfhash 0\tconflicted\n")
     -+	$(printf "100644 $cfhash 1\tconflicted\n")
     ++	>conflict &&
     ++	test_when_finished "rm -f conflict" &&
     ++	cfhash=$(git hash-object -w conflict) &&
     ++	q_to_tab <<-EOF | git update-index --index-info &&
     ++	0 $cfhash 0Qconflict
     ++	100644 $cfhash 1Qconflict
      +	EOF
      +
     -+	test_must_fail git mv conflicted newname 2>actual &&
     -+	test_i18ngrep "merge.conflict" actual
     ++	test_must_fail git mv conflict newname 2>actual &&
     ++	test_i18ngrep "conflicted" actual
      +'
     -+
     -+rm -f conflicted
      +
       test_expect_success 'git mv should overwrite symlink to a file' '

 builtin/mv.c  |  7 +++++--
 t/t7001-mv.sh | 17 +++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

index be15ba7044..7dac714af9 100644
@@ -132,6 +132,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 	struct stat st;
 	struct string_list src_for_dst = STRING_LIST_INIT_NODUP;
 	struct lock_file lock_file = LOCK_INIT;
+	struct cache_entry *ce;
 
 	git_config(git_default_config, NULL);
 

@@ -220,9 +221,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
 				}
 				argc += last - first;
 			}
-		} else if (cache_name_pos(src, length) < 0)
+		} else if (!(ce = cache_file_exists(src, length, ignore_case))) {
 			bad = _("not under version control");
-		else if (lstat(dst, &st) == 0 &&
+		} else if (ce_stage(ce)) {
+			bad = _("conflicted");
+		} else if (lstat(dst, &st) == 0 &&
 			 (!ignore_case || strcasecmp(src, dst))) {
 			bad = _("destination exists");
 			if (force) {

index 36b50d0b4c..c978b6dee4 100755
@@ -248,6 +248,23 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '
 
 rm -f dirty dirty2
 
+# NB: This test is about the error message
+# as well as the failure.
+test_expect_success 'git mv error on conflicted file' '
+	rm -fr .git &&
+	git init &&
+	>conflict &&
+	test_when_finished "rm -f conflict" &&
+	cfhash=$(git hash-object -w conflict) &&
+	q_to_tab <<-EOF | git update-index --index-info &&
+	0 $cfhash 0Qconflict
+	100644 $cfhash 1Qconflict
+	EOF
+
+	test_must_fail git mv conflict newname 2>actual &&
+	test_i18ngrep "conflicted" actual
+'
+
 test_expect_success 'git mv should overwrite symlink to a file' '
 
 	rm -fr .git &&

base-commit: ae46588be0cd730430dded4491246dfb4eac5557

gitgitgadget

It does not matter all that much but the message in your original for the new case looked better and worse at the same time ;-)

"not under version control" is a statement of fact that does not hint what the user may want to do with that information. Your original for the new case gave that hint (i.e. "must resolve first") but the new "the path is unmerged" (I think 'unmerged' is a more proper term for this than 'conflicted'; see gitglossary[7]) stops at stating fact without giving further hint,and in that sense the messages are consistent with each other.

We could shoot for consistency in the opposite direction, by making "not under version control" could instead say "must add first". But that leads to a fruitless comparison between "'git add' then 'git mv'" and "plain 'mv' then 'git add'". For "git mv", "must resolve first" may be the only sane option right now, so it probably is OK.

So, after having thought the above through, I tend to (slightly) prefer to stop at stating fact, perhaps "the path is unmerged" or something to match "not under version control".

I totally agree, and I am not 100% convinced that the "rename all at their current stage" gives a better end-user experience. For one thing, I suspect that it would still have to fail depending on how the destination path and paths that conflict with it are populated in the index, and that may make even harder-to-explain error case.

That is perfectly fine. We are in pre-release freeze and this patch won't go anywhere until the end of the month.

Thanks for contributing, and safe travels.