Re: [PATCH] git-mv: improve error message for conflicted file
From: Eric Sunshine <hidden>
Date: 2020-07-17 23:47:33
On Fri, Jul 17, 2020 at 7:25 PM Chris Torek via GitGitGadget [off-list ref] wrote:
'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.
Signed-off-by: Chris Torek <redacted>
---A few nits below...
quoted hunk ↗ jump to hunk
diff --git a/builtin/mv.c b/builtin/mv.c@@ -220,9 +220,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix) + } 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");
Style: write `!ce` rather than `ce == NULL`:
if (!ce)
bad = _("not under version control");
else
bad = _("must resolve merge conflict first");
or reverse the arms and skip the `!` altogether:
if (ce)
bad = _("must resolve merge conflict first");
else
bad = _("not under version control");
Or even:
bad = ce ? _("must resolve merge conflict first") : _("not under
version control");
though it's subjective whether that is more readable.
As for bikeshedding the message itself, perhaps:
_("conflicted");
Though, perhaps that's too succinct.
quoted hunk ↗ jump to hunk
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh@@ -248,6 +248,24 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' ' +test_expect_success 'git mv error on conflicted file' ' + rm -fr .git && + git init && + touch conflicted &&
If the timestamp of the file is not relevant to the test -- as is the
case here -- then we avoid using `touch`. Instead:
>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
This can probably be written more easily and clearly like this:
git update-index --index-info <<-EOF &&
0 $cfhash 0 conflicted
100644 $cfhash 1 conflicted
EOF
That is, use literal TABs and let the here-doc provide the newlines.
Alternately, you could take advantage of the q_to_tab() function to
convert literal "Q" to TAB:
q_to_tab <<-EOF | git update-index --index-info &&
0 $cfhash 0Qconflicted
100644 $cfhash 1Qconflicted
EOF
+ test_must_fail git mv conflicted newname 2>actual && + test_i18ngrep "merge.conflict" actual +' + +rm -f conflicted
I realize that this test script is already filled with this sort of
thing where actions are performed outside of tests, however, these
days we frown upon that, and there really isn't a good reason to avoid
taking care of this clean up via the modern idiom of using
test_when_finished(), which you would call immediately after creating
the file in the test. So:
...
>conflicted &&
test_when_finished "rm -f conflicted" &&
...