| Message ID | pull.678.v2.git.1595225873014.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] git-mv: improve error message for conflicted file | expand |
"Chris Torek via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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). 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 like the idea of renaming all stages and keeping them at their current > stages, but that's too much for this patch. 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. > I'll be traveling next week and not sure if I will get to any followups > for a while. 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.
diff --git a/builtin/mv.c b/builtin/mv.c index be15ba7044..7dac714af9 100644 --- a/builtin/mv.c +++ b/builtin/mv.c @@ -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) { diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 36b50d0b4c..c978b6dee4 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -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 &&