Re: [PATCH] git-mv: improve error message for conflicted file

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Jul 17, 2020 at 7:25 PM Chris Torek via GitGitGadget
<gitgitgadget@gmail.com> 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 <chris.torek@gmail.com>
> ---

A few nits below...

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

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