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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 17, 2020, 23:47 UTC
Message-ID: <CAPig+cQaqg7MbyNZakuWVzezhPCXu=LonCVAw_p13c=0YNBdPw@mail.gmail.com>
In-Reply-To: <pull.678.git.1595028293855.gitgitgadget@gmail.com>

On Fri, Jul 17, 2020 at 7:25 PM Chris Torek via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines

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

Show 13 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

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