git-mv: improve error message for conflicted file

[chris3torek]

'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

---

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.

[chris3torek]

/preview

[gitgitgadget]

Preview email sent as pull.678.git.1595011316683.gitgitgadget@gmail.com

[chris3torek]

/submit

[gitgitgadget]

Submitted as pull.678.git.1595028293855.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Chris Torek via GitGitGadget" <gitgitgadget@gmail.com> writes:

> -		} 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");


The original did not care about the cache entry itself, and that is
why cache_name_pos() was used.  Now you care what cache entry is at
that position, running both calls is quite wasteful.

Would it work better to declare "struct cache_entry *ce" in the
scope that surrounds this if/elseif cascade and then rewrite this
part more like so:

	} else if (!(ce = cache_file_exists(...)) {
		bad = _("not tracked");
	} else if (ce_stage(ce)) {
        	bad = _("conflicted");
	} else if (lstat(...)) { ...

[gitgitgadget]

On the Git mailing list, Chris Torek wrote (reply to this):

On Fri, Jul 17, 2020 at 4:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Style: write `!ce` rather than `ce == NULL`:

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

> As for bikeshedding the message itself, perhaps:
>
>     _("conflicted");
>
> Though, perhaps that's too succinct.

Maybe.  Succinct is usually good though.

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

OK.

> ... use literal TABs and let the here-doc provide the newlines.

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

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

Indeed, that's where I copied it from.

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

Chris

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Fri, Jul 17, 2020 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Chris Torek via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > -             } 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");
>
>
> The original did not care about the cache entry itself, and that is
> why cache_name_pos() was used.  Now you care what cache entry is at
> that position, running both calls is quite wasteful.
>
> Would it work better to declare "struct cache_entry *ce" in the
> scope that surrounds this if/elseif cascade and then rewrite this
> part more like so:
>
>         } else if (!(ce = cache_file_exists(...)) {
>                 bad = _("not tracked");
>         } else if (ce_stage(ce)) {
>                 bad = _("conflicted");
>         } else if (lstat(...)) { ...

Or, even better, make ce_stage(ce) not be an error; see
https://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.

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

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Fri, Jul 17, 2020 at 9:35 PM Chris Torek <chris.torek@gmail.com> wrote:
> On Fri, Jul 17, 2020 at 4:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > ... use literal TABs and let the here-doc provide the newlines.
>
> I personally hate depending on literal tabs, as they're really
> hard to see. If q_to_tab() is OK I'll use that.

q_to_tab() is a good choice.

> > 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:
>
> Indeed, that's where I copied it from.
>
> Should I clean up other instances of separated-out `rm -f`s
> in this file in a separate commit?

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

> Or, even better, make ce_stage(ce) not be an error; see
> https://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.

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.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sat, Jul 18, 2020 at 10:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > Or, even better, make ce_stage(ce) not be an error; see
> > https://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.
>
> 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.

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.

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

Agreed.

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

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

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

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

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

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

[chris3torek]

/preview

[gitgitgadget]

Preview email sent as pull.678.v2.git.1595225517467.gitgitgadget@gmail.com

[chris3torek]

/submit

[gitgitgadget]

Submitted as pull.678.v2.git.1595225873014.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

[gitgitgadget]

This branch is now known as ct/mv-unmerged-path-error.

[gitgitgadget]

This patch series was integrated into seen via git@1ea2db0.

[gitgitgadget]

This patch series was integrated into seen via git@5a94ab7.

[gitgitgadget]

This patch series was integrated into seen via git@d6c04fd.

[gitgitgadget]

This patch series was integrated into next via git@c170072.

[gitgitgadget]

This patch series was integrated into seen via git@d8f7f11.

[gitgitgadget]

This patch series was integrated into seen via git@6c8c4f7.

[gitgitgadget]

This patch series was integrated into seen via git@7b766d9.

[gitgitgadget]

This patch series was integrated into seen via git@819913f.

[gitgitgadget]

This patch series was integrated into seen via git@6568652.

[gitgitgadget]

This patch series was integrated into seen via git@7f63d06.

[gitgitgadget]

This patch series was integrated into seen via git@be2dab9.

[gitgitgadget]

This patch series was integrated into next via git@be2dab9.

[gitgitgadget]

This patch series was integrated into master via git@be2dab9.

[gitgitgadget]

Closed via be2dab9.