{"thread":{"id":"53876","subject":"[PATCH] git-mv: improve error message for conflicted file","startedAt":"2020-07-17T23:24:59Z","lastAt":"2020-07-20T18:28:27Z","messageCount":11,"participants":["Chris Torek via GitGitGadget","Eric Sunshine","Junio C Hamano","Chris Torek","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401716","messageId":"pull.678.git.1595028293855.gitgitgadget@gmail.com","threadId":"53876","inReplyTo":null,"subject":"[PATCH] git-mv: improve error message for conflicted file","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-17T23:24:53Z","receivedAt":"2020-07-17T23:24:59Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek <chris.torek@gmail.com>\n\n'git mv' has always complained about renaming a conflicted\nfile, as it cannot handle multiple index entries for one file.\nHowever, the error message it uses has been the same as the\none for an untracked file:\n\n    fatal: not under version control, src=...\n\nwhich is patently wrong.  Distinguish the two cases and\nadd a test to make sure we produce the correct message.\n\nSigned-off-by: Chris Torek <chris.torek@gmail.com>\n---\n    git-mv: improve error message for conflicted file\n    \n    'git mv' has always complained about renaming a conflicted file, as it\n    cannot handle multiple index entries for one file. However, the error\n    message it uses has been the same as the one for an untracked file:\n    \n    fatal: not under version control, src=...\n    \n    which is patently wrong. Distinguish the two cases and add a test to\n    make sure we produce the correct message.\n    \n    Signed-off-by: Chris Torek chris.torek@gmail.com [chris.torek@gmail.com]\n    \n    \n    ------------------------------------------------------------------------\n    \n    A small note on the test: I originally had a different message text, so\n    the grep is slightly more general than necessary here. This leaves room\n    for a better message, though; I'm not sure mine is that great.\n    \n    The error messages in general from 'git mv' could probably stand a lot\n    of cleanup. This is just a minimal fix for some particularly bad\n    behavior.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-678%2Fchris3torek%2Fgit-mv-message-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-678/chris3torek/git-mv-message-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/678\n\n builtin/mv.c  | 15 ++++++++++++---\n t/t7001-mv.sh | 18 ++++++++++++++++++\n 2 files changed, 30 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex be15ba7044..7dff121629 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -220,9 +220,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \t\t\t\targc += last - first;\n \t\t\t}\n-\t\t} else if (cache_name_pos(src, length) < 0)\n-\t\t\tbad = _(\"not under version control\");\n-\t\telse if (lstat(dst, &st) == 0 &&\n+\t\t} else if (cache_name_pos(src, length) < 0) {\n+\t\t\t/*\n+\t\t\t * This occurs for both untracked files *and*\n+\t\t\t * files that are in merge-conflict state, so\n+\t\t\t * let's distinguish between those two.\n+\t\t\t */\n+\t\t\tstruct cache_entry *ce = cache_file_exists(src, length, ignore_case);\n+\t\t\tif (ce == NULL)\n+\t\t\t\tbad = _(\"not under version control\");\n+\t\t\telse\n+\t\t\t\tbad = _(\"must resolve merge conflict first\");\n+\t\t} else if (lstat(dst, &st) == 0 &&\n \t\t\t (!ignore_case || strcasecmp(src, dst))) {\n \t\t\tbad = _(\"destination exists\");\n \t\t\tif (force) {\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 36b50d0b4c..b4974f9385 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -248,6 +248,24 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '\n \n rm -f dirty dirty2\n \n+# NB: This test is about the error message\n+# as well as the failure.\n+test_expect_success 'git mv error on conflicted file' '\n+\trm -fr .git &&\n+\tgit init &&\n+\ttouch conflicted &&\n+\tcfhash=$(git hash-object -w conflicted) &&\n+\tgit update-index --index-info <<-EOF &&\n+\t$(printf \"0 $cfhash 0\\tconflicted\\n\")\n+\t$(printf \"100644 $cfhash 1\\tconflicted\\n\")\n+\tEOF\n+\n+\ttest_must_fail git mv conflicted newname 2>actual &&\n+\ttest_i18ngrep \"merge.conflict\" actual\n+'\n+\n+rm -f conflicted\n+\n test_expect_success 'git mv should overwrite symlink to a file' '\n \n \trm -fr .git &&\n\nbase-commit: b6a658bd00c9c29e07f833cabfc0ef12224e277a\n-- \ngitgitgadget\n"},{"id":"401717","messageId":"CAPig+cQaqg7MbyNZakuWVzezhPCXu=LonCVAw_p13c=0YNBdPw@mail.gmail.com","threadId":"53876","inReplyTo":"pull.678.git.1595028293855.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-17T23:47:18Z","receivedAt":"2020-07-17T23:47:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 17, 2020 at 7:25 PM Chris Torek via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> 'git mv' has always complained about renaming a conflicted\n> file, as it cannot handle multiple index entries for one file.\n> However, the error message it uses has been the same as the\n> one for an untracked file:\n>\n>     fatal: not under version control, src=...\n>\n> which is patently wrong.  Distinguish the two cases and\n> add a test to make sure we produce the correct message.\n>\n> Signed-off-by: Chris Torek <chris.torek@gmail.com>\n> ---\n\nA few nits below...\n\n> diff --git a/builtin/mv.c b/builtin/mv.c\n> @@ -220,9 +220,18 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n> +               } else if (cache_name_pos(src, length) < 0) {\n> +                       /*\n> +                        * This occurs for both untracked files *and*\n> +                        * files that are in merge-conflict state, so\n> +                        * let's distinguish between those two.\n> +                        */\n> +                       struct cache_entry *ce = cache_file_exists(src, length, ignore_case);\n> +                       if (ce == NULL)\n> +                               bad = _(\"not under version control\");\n> +                       else\n> +                               bad = _(\"must resolve merge conflict first\");\n\nStyle: write `!ce` rather than `ce == NULL`:\n\n    if (!ce)\n        bad = _(\"not under version control\");\n    else\n        bad = _(\"must resolve merge conflict first\");\n\nor reverse the arms and skip the `!` altogether:\n\n    if (ce)\n        bad = _(\"must resolve merge conflict first\");\n    else\n        bad = _(\"not under version control\");\n\nOr even:\n\n   bad = ce ? _(\"must resolve merge conflict first\") : _(\"not under\nversion control\");\n\nthough it's subjective whether that is more readable.\n\nAs for bikeshedding the message itself, perhaps:\n\n    _(\"conflicted\");\n\nThough, perhaps that's too succinct.\n\n> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\n> @@ -248,6 +248,24 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '\n> +test_expect_success 'git mv error on conflicted file' '\n> +       rm -fr .git &&\n> +       git init &&\n> +       touch conflicted &&\n\nIf the timestamp of the file is not relevant to the test -- as is the\ncase here -- then we avoid using `touch`. Instead:\n\n    >conflicted &&\n\n> +       cfhash=$(git hash-object -w conflicted) &&\n> +       git update-index --index-info <<-EOF &&\n> +       $(printf \"0 $cfhash 0\\tconflicted\\n\")\n> +       $(printf \"100644 $cfhash 1\\tconflicted\\n\")\n> +       EOF\n\nThis can probably be written more easily and clearly like this:\n\n    git update-index --index-info <<-EOF &&\n    0 $cfhash 0    conflicted\n    100644 $cfhash 1    conflicted\n    EOF\n\nThat is, use literal TABs and let the here-doc provide the newlines.\nAlternately, you could take advantage of the q_to_tab() function to\nconvert literal \"Q\" to TAB:\n\n    q_to_tab <<-EOF | git update-index --index-info &&\n    0 $cfhash 0Qconflicted\n    100644 $cfhash 1Qconflicted\n    EOF\n\n> +       test_must_fail git mv conflicted newname 2>actual &&\n> +       test_i18ngrep \"merge.conflict\" actual\n> +'\n> +\n> +rm -f conflicted\n\nI realize that this test script is already filled with this sort of\nthing where actions are performed outside of tests, however, these\ndays we frown upon that, and there really isn't a good reason to avoid\ntaking care of this clean up via the modern idiom of using\ntest_when_finished(), which you would call immediately after creating\nthe file in the test. So:\n\n    ...\n    >conflicted &&\n    test_when_finished \"rm -f conflicted\" &&\n    ...\n"},{"id":"401718","messageId":"xmqqeep9d6tm.fsf@gitster.c.googlers.com","threadId":"53876","inReplyTo":"pull.678.git.1595028293855.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-18T00:07:17Z","receivedAt":"2020-07-18T00:07:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> -\t\t} else if (cache_name_pos(src, length) < 0)\n> -\t\t\tbad = _(\"not under version control\");\n> -\t\telse if (lstat(dst, &st) == 0 &&\n> +\t\t} else if (cache_name_pos(src, length) < 0) {\n> +\t\t\t/*\n> +\t\t\t * This occurs for both untracked files *and*\n> +\t\t\t * files that are in merge-conflict state, so\n> +\t\t\t * let's distinguish between those two.\n> +\t\t\t */\n> +\t\t\tstruct cache_entry *ce = cache_file_exists(src, length, ignore_case);\n> +\t\t\tif (ce == NULL)\n> +\t\t\t\tbad = _(\"not under version control\");\n> +\t\t\telse\n> +\t\t\t\tbad = _(\"must resolve merge conflict first\");\n\n\nThe original did not care about the cache entry itself, and that is\nwhy cache_name_pos() was used.  Now you care what cache entry is at\nthat position, running both calls is quite wasteful.\n\nWould it work better to declare \"struct cache_entry *ce\" in the\nscope that surrounds this if/elseif cascade and then rewrite this\npart more like so:\n\n\t} else if (!(ce = cache_file_exists(...)) {\n\t\tbad = _(\"not tracked\");\n\t} else if (ce_stage(ce)) {\n        \tbad = _(\"conflicted\");\n\t} else if (lstat(...)) { ...\n\n"},{"id":"401721","messageId":"CAPx1GvduDZw5pmfZHACDGZsMR5YYDowLw6+az+oL6oWLvDyCFA@mail.gmail.com","threadId":"53876","inReplyTo":"CAPig+cQaqg7MbyNZakuWVzezhPCXu=LonCVAw_p13c=0YNBdPw@mail.gmail.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-07-18T01:35:09Z","receivedAt":"2020-07-18T01:35:23Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Fri, Jul 17, 2020 at 4:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> Style: write `!ce` rather than `ce == NULL`:\n\nOK, but I'll go with Junio's suggestion of getting `ce` once and\nthen checking `ce_staged` anyway.  (I'm used to a different\nstyle guide that frowns on `if (ce)` and `if (!ce)`...)\n\n> As for bikeshedding the message itself, perhaps:\n>\n>     _(\"conflicted\");\n>\n> Though, perhaps that's too succinct.\n\nMaybe.  Succinct is usually good though.\n\n> > +       touch conflicted &&\n>\n> If the timestamp of the file is not relevant to the test -- as is the\n> case here -- then we avoid using `touch`. Instead:\n>\n>     >conflicted &&\n\nOK.\n\n> ... use literal TABs and let the here-doc provide the newlines.\n\nI personally hate depending on literal tabs, as they're really\nhard to see.  If q_to_tab() is OK I'll use that.\n\n> I realize that this test script is already filled with this sort of\n> thing where actions are performed outside of tests, however, these\n> days we frown upon that, and there really isn't a good reason to avoid\n> taking care of this clean up via the modern idiom of using\n> test_when_finished(), which you would call immediately after creating\n> the file in the test. So:\n>\n>     ...\n>     >conflicted &&\n>     test_when_finished \"rm -f conflicted\" &&\n>     ...\n\nIndeed, that's where I copied it from.\n\nShould I clean up other instances of separated-out `rm -f`s\nin this file in a separate commit?\n\nChris\n"},{"id":"401722","messageId":"CABPp-BGDp_SjJKvi+XVd6KvRLA5PVsK4xBLPvBxAimDft+0M9g@mail.gmail.com","threadId":"53876","inReplyTo":"xmqqeep9d6tm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-07-18T02:00:22Z","receivedAt":"2020-07-18T02:00:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 17, 2020 at 5:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Chris Torek via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > -             } else if (cache_name_pos(src, length) < 0)\n> > -                     bad = _(\"not under version control\");\n> > -             else if (lstat(dst, &st) == 0 &&\n> > +             } else if (cache_name_pos(src, length) < 0) {\n> > +                     /*\n> > +                      * This occurs for both untracked files *and*\n> > +                      * files that are in merge-conflict state, so\n> > +                      * let's distinguish between those two.\n> > +                      */\n> > +                     struct cache_entry *ce = cache_file_exists(src, length, ignore_case);\n> > +                     if (ce == NULL)\n> > +                             bad = _(\"not under version control\");\n> > +                     else\n> > +                             bad = _(\"must resolve merge conflict first\");\n>\n>\n> The original did not care about the cache entry itself, and that is\n> why cache_name_pos() was used.  Now you care what cache entry is at\n> that position, running both calls is quite wasteful.\n>\n> Would it work better to declare \"struct cache_entry *ce\" in the\n> scope that surrounds this if/elseif cascade and then rewrite this\n> part more like so:\n>\n>         } else if (!(ce = cache_file_exists(...)) {\n>                 bad = _(\"not tracked\");\n>         } else if (ce_stage(ce)) {\n>                 bad = _(\"conflicted\");\n>         } else if (lstat(...)) { ...\n\nOr, even better, make ce_stage(ce) not be an error; see\nhttps://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.\n\nI hadn't gotten around to it yet, but it's still on my radar.\n\n(That said, I've obviously taken a really long time to get to it, so\nimproving the error message as an interim step is perfectly fine.)\n"},{"id":"401726","messageId":"CAPig+cQpUu2UO-+jWn1nTaDykWnxwuEitzVB7PnW2SS_b7V8Hg@mail.gmail.com","threadId":"53876","inReplyTo":"CAPx1GvduDZw5pmfZHACDGZsMR5YYDowLw6+az+oL6oWLvDyCFA@mail.gmail.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-18T06:55:37Z","receivedAt":"2020-07-18T06:55:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jul 17, 2020 at 9:35 PM Chris Torek <chris.torek@gmail.com> wrote:\n> On Fri, Jul 17, 2020 at 4:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > ... use literal TABs and let the here-doc provide the newlines.\n>\n> I personally hate depending on literal tabs, as they're really\n> hard to see. If q_to_tab() is OK I'll use that.\n\nq_to_tab() is a good choice.\n\n> > I realize that this test script is already filled with this sort of\n> > thing where actions are performed outside of tests, however, these\n> > days we frown upon that, and there really isn't a good reason to avoid\n> > taking care of this clean up via the modern idiom of using\n> > test_when_finished(), which you would call immediately after creating\n> > the file in the test. So:\n>\n> Indeed, that's where I copied it from.\n>\n> Should I clean up other instances of separated-out `rm -f`s\n> in this file in a separate commit?\n\nIn general, as a reviewer, I don't mind seeing a patch or two cleaning\nup style and other violations, however, the magnitude of the fixes\nthis script needs is quite significant and would end up requiring a\nfair number of patches. As such, I'm not particularly eager to see the\nimprovement made by this patch -- which is nicely standalone --\nweighed down by a lengthy series of patches which aren't really\nrelated to it.\n\nIf cleaning up the t7001 test script is something you might be\ninterested in doing, then making it a separate patch series would be\nmore palatable. A scan of the script reveals the following problems,\nthough there may be others:\n\n* old style:\n\n    test_expect_success \\\n        'title' \\\n        'body line 1 &&\n        body line 2'\n\n  should become:\n\n    test_expect_success 'title' '\n        body line 1 &&\n        body line 2\n    '\n\n* test bodies should be indented with TAB, not spaces\n\n* some tests use a deprecated style in which there are unnecessary\n  blank lines after the opening quote of the test body and before the\n  closing quote; these blanks lines should be removed\n\n* style for `cd` in subshell is:\n\n    (\n        cd foo &&\n        ...\n    ) &&\n\n  not:\n\n    (cd foo &&\n        ...\n    ) &&\n\n* there should be no whitespace after redirect operators, so:\n\n    foo > actual &&\n\n  should become:\n\n    foo >actual &&\n\n* tests 'cd' around and expect other tests to know the current\n  directory and 'cd' relative to that; instead, any test which uses\n  'cd' should do so in a subshell to ensure the current directory is\n  restored by the time the test ends:\n\n    test_expect_success 'title' '\n        something &&\n        (\n            cd somewhere &&\n            something-else\n        )\n    '\n\n  Alternately, it may be possible to take advantage of `-C` if\n  `something-else` is a `git` command:\n\n    test_expect_success 'title' '\n        something &&\n        git -C somewhere foo\n    '\n\n* use `>` rather than `touch` to create an empty file when the\n  timestamp isn't relevant to the test\n\n* cleanup code outside of tests should be moved into the test and\n  scheduled for execution via test_when_finished()\n\n* there are several standalone \"clean up\" tests which invoke `git\n  reset --hard` which should be folded into the tests for which they\n  are cleaning up\n\n* multiple commands on one line:\n\n    mkdir foo && >foo/bar && git add foo/bar &&\n\n  should be split across multiple lines:\n\n    mkdir foo &&\n    >foo/bar &&\n    git add foo/bar &&\n\n* at least one test incorrectly uses single quotes within the body of\n  the test which itself is contained within single quotes; when\n  quoting is needed inside a test body, it should be using double\n  quotes instead; however, in this case, the quotes aren't even\n  needed, so:\n\n    git commit -m 'initial' &&\n\n  can just become:\n\n    git commit -m initial &&\n\n* take advantage of here-docs, so:\n\n    { echo other/a.txt; echo other/b.txt; } >expect &&\n\n  can be expressed more cleanly as:\n\n    cat >expect <<-\\EOF &&\n    other/a.txt\n    other/b.txt\n    EOF\n\n* use `test` rather than `[`\n\n* optional: rename the setup test 'prepare reference tree' to 'setup'\n\n* optional modernization: use test_path_exists() and cousins instead\n  of `test -f`, etc.\n\n* optional: avoid `git` command upstream of a pipe since the pipe will\n  swallow its exit code, thus a crash won't necessarily be noticed\n\n* optional: it's unusual for tests to blast the test's \".git\"\n  directory and recreate it with `git init`, however, a number of\n  tests in this script do so; for tests which really require a new\n  repository, the more common approach is to use test_create_repo() to\n  create a new repository into which the test can `cd` (in a subshell)\n  without disturbing the repository used by the other tests in the\n  script\n\nA few of the above fixes can probably be combined into a single patch,\nin particular, the style fixes in the first four bullet points. Each\nremaining bullet point, however, probably deserves its own patch\n(including the one about removing whitespace after a redirect\noperator).\n"},{"id":"401731","messageId":"xmqq8sfgd8cu.fsf@gitster.c.googlers.com","threadId":"53876","inReplyTo":"CABPp-BGDp_SjJKvi+XVd6KvRLA5PVsK4xBLPvBxAimDft+0M9g@mail.gmail.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-18T17:46:25Z","receivedAt":"2020-07-18T17:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Or, even better, make ce_stage(ce) not be an error; see\n> https://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.\n\nHmph, I actually am not convinced with that one, even though I know\nI wrote it, I do not think I had thought things through before I\ndid.  It may make sense in a narrow special case where there is a\nsingle entry at a higher stage to rename it to the destination path\nand drop it down to stage #0, but I do not think of a good behaviour\nfor more general cases.\n\nWhat should happen, for example, if two or more entries at higher\nstages exist for the path being moved?  Renaming all of them to the\ndestination path without changing their stage number may make more\nsense [*1*].  At least, that solution still lets the user to choose\nobject at which stage [*2*] to use in the final resolution.\n\nNow, if we define that \"git mv src dst\", when 'src' is a conflicted\npath, moves the working tree file src to dst at the same time moves\nall the higher stage entries for 'src' to 'dst' while retaining\ntheir stage number, what does the degenerate case of having only one\nsuch entry look like?  You start from the state where you have\n$that_path at stage #3, \"git mv $that_path $over_there\" would put\nyou in the state where you have the same contents at $over_there and\nat stage #3.  If you want to just take \"their\" contents as the\nresolution, \"git add $over_there\" after doing that \"git mv\" would\nlet you record the resolution, so it does not look too bad.\n\nIt would also be a handy way to recover from a mistake made by\n\"directory level rename\" heuristics.  Instead of resolving the\ncontent-level conflicts at the wrong path and then moving that\nresolved result to the right path, you can first correct the wrong\npath by moving the conflicted whole to the right path and then\nperform the content-level conflict resolution.  The advantage of the\nlatter is obvious.  You have to do two things (rename and edit) and\nwith the former way of doing 'edit' first and then 'rename', after\nresolving the conflict and adding the result at the wrong path, \"git\nls-files -u\" or \"git status\" no longer help you remember that the\npath still needs to be moved.  Instead, you can move first and \"git\nls-files -u\" would still remember that even after the move you still\nneed to deal with content-level conflicts.\n\nAnyway, I think the \"separate missing entry and conflicted entry\nwhen issuing an error message\" is a strict improvement.\n\n\n[Footnotes]\n\n*1* Of course, the \"D/F conflicts are issue only among the entries\n    at the same stage\" and other usual rules apply when we check if\n    the move of these higher stage entries is possible.  I am not\n    sure if the low-level API functions like rename_index_entry_at()\n    are prepared to deal with higher stage entries, though, so this\n    may be a nontrivial amount of new work.  It is unclear to me if\n    it is worth doing.\n\n*2* Actually, \"the object at stage #1 for conflicted path P\" is a\n    wrong thing to say.  The index is designed to hold multiple\n    stage #1 entries at the same path so that it can express the\n    case where there are more than one common ancestors, and\n    multiple stage #3 entries to express an octopus merge in\n    progress.  The notation to name the object in the index punts\n    and does not let you say \"git diff :1.0:path :1.1:path\" to\n    compare the first and the second common ancestor versions of\n    path, though it probably should if we wanted to be consistent.\n\n"},{"id":"401747","messageId":"CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com","threadId":"53876","inReplyTo":"xmqq8sfgd8cu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-07-19T01:48:08Z","receivedAt":"2020-07-19T01:48:23Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jul 18, 2020 at 10:46 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > Or, even better, make ce_stage(ce) not be an error; see\n> > https://lore.kernel.org/git/xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com/.\n>\n> Hmph, I actually am not convinced with that one, even though I know\n> I wrote it, I do not think I had thought things through before I\n> did.  It may make sense in a narrow special case where there is a\n> single entry at a higher stage to rename it to the destination path\n> and drop it down to stage #0, but I do not think of a good behaviour\n> for more general cases.\n>\n> What should happen, for example, if two or more entries at higher\n> stages exist for the path being moved?  Renaming all of them to the\n> destination path without changing their stage number may make more\n> sense [*1*].  At least, that solution still lets the user to choose\n> object at which stage [*2*] to use in the final resolution.\n>\n> Now, if we define that \"git mv src dst\", when 'src' is a conflicted\n> path, moves the working tree file src to dst at the same time moves\n> all the higher stage entries for 'src' to 'dst' while retaining\n> their stage number, what does the degenerate case of having only one\n> such entry look like?  You start from the state where you have\n> $that_path at stage #3, \"git mv $that_path $over_there\" would put\n> you in the state where you have the same contents at $over_there and\n> at stage #3.  If you want to just take \"their\" contents as the\n> resolution, \"git add $over_there\" after doing that \"git mv\" would\n> let you record the resolution, so it does not look too bad.\n>\n> It would also be a handy way to recover from a mistake made by\n> \"directory level rename\" heuristics.  Instead of resolving the\n> content-level conflicts at the wrong path and then moving that\n> resolved result to the right path, you can first correct the wrong\n> path by moving the conflicted whole to the right path and then\n> perform the content-level conflict resolution.  The advantage of the\n> latter is obvious.  You have to do two things (rename and edit) and\n> with the former way of doing 'edit' first and then 'rename', after\n> resolving the conflict and adding the result at the wrong path, \"git\n> ls-files -u\" or \"git status\" no longer help you remember that the\n> path still needs to be moved.  Instead, you can move first and \"git\n> ls-files -u\" would still remember that even after the move you still\n> need to deal with content-level conflicts.\n\nOh, good, I actually like the idea of not auto-resolving better; I was\nalways a bit uncomfortable with it.  And now that you brought up this\ncase, there's another good case I can think of too: D/F conflicts that\narise from a rename/add-like situation, but where the add is a new\ndirectory rather than a new file.  In such a case, there can be three\nhigher order stages for the file due to content conflicts carried with\nthe rename and auto-resolving them when renaming doesn't make any\nsense.  So, yeah, keeping the higher order stages but just renaming\nthe path seems like a nice improvement for the user.\n\n> Anyway, I think the \"separate missing entry and conflicted entry\n> when issuing an error message\" is a strict improvement.\n\nAgreed.\n\n> [Footnotes]\n>\n> *1* Of course, the \"D/F conflicts are issue only among the entries\n>     at the same stage\" and other usual rules apply when we check if\n>     the move of these higher stage entries is possible.  I am not\n>     sure if the low-level API functions like rename_index_entry_at()\n>     are prepared to deal with higher stage entries, though, so this\n>     may be a nontrivial amount of new work.  It is unclear to me if\n>     it is worth doing.\n\nYeah, if it were trivial, I would have just done it back when I\nmentioned it a couple years ago.  I think it's still worth doing, but\nI agree it might need some new low-level API function(s) or some\nmodifications to some existing one(s).\n\n> *2* Actually, \"the object at stage #1 for conflicted path P\" is a\n>     wrong thing to say.  The index is designed to hold multiple\n>     stage #1 entries at the same path so that it can express the\n>     case where there are more than one common ancestors, and\n>     multiple stage #3 entries to express an octopus merge in\n>     progress.\n\nReally?  Wow.  I never had any clue that this was possible and never\nran across it.  Is it documented anywhere?\n\n>     The notation to name the object in the index punts\n>     and does not let you say \"git diff :1.0:path :1.1:path\" to\n>     compare the first and the second common ancestor versions of\n>     path, though it probably should if we wanted to be consistent.\n"},{"id":"401753","messageId":"xmqqr1t89gi8.fsf@gitster.c.googlers.com","threadId":"53876","inReplyTo":"CABPp-BGJdwpwhQUp4Wa4bKBp4hQFB9OM3N1FXH7SzY0mvLDa7Q@mail.gmail.com","subject":"Re: [PATCH] git-mv: improve error message for conflicted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-19T06:16:15Z","receivedAt":"2020-07-19T06:16:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Really?  Wow.  I never had any clue that this was possible and never\n> ran across it.  Is it documented anywhere?\n\nProbably a test script for \"read-tree -m\" has a fairly detailed and\ncomprehensive table of possible scenarios.  \n\nMultiple stage #1 entries were introduced to primarily support the\nprotection against \"criss-cross merge\" that confuses the history and\nit is used by the resolve backend, IIRC.  \n\nThe implementation of octopus we have does not actually use multiple\nstage #3 entries but just does many two-parent merges in sequence.\nIt started its life as a POC & stop-gap implementation while waiting\nfor the \"real thing\" that merges with multiple stage #3 entries, but\nit turned out to be good enough for a relatively rare (and now\ndiscouraged mostly due to bisection efficiency reasons) style of\nmerge and left in that shape until now.\n\n"},{"id":"401761","messageId":"pull.678.v2.git.1595225873014.gitgitgadget@gmail.com","threadId":"53876","inReplyTo":"pull.678.git.1595028293855.gitgitgadget@gmail.com","subject":"[PATCH v2] git-mv: improve error message for conflicted file","fromName":"Chris Torek via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-07-20T06:17:52Z","receivedAt":"2020-07-20T06:17:58Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"From: Chris Torek <chris.torek@gmail.com>\n\n'git mv' has always complained about renaming a conflicted\nfile, as it cannot handle multiple index entries for one file.\nHowever, the error message it uses has been the same as the\none for an untracked file:\n\n    fatal: not under version control, src=...\n\nwhich is patently wrong.  Distinguish the two cases and\nadd a test to make sure we produce the correct message.\n\nSigned-off-by: Chris Torek <chris.torek@gmail.com>\n---\n    git-mv: improve error message for conflicted file\n    \n    'git mv' has always complained about renaming a conflicted file, as it\n    cannot handle multiple index entries for one file. However, the error\n    message it uses has been the same as the one for an untracked file:\n    \n    fatal: not under version control, src=...\n    \n    which is patently wrong. Distinguish the two cases and add a test to\n    make sure we produce the correct message.\n    \n    Signed-off-by: Chris Torek chris.torek@gmail.com [chris.torek@gmail.com]\n    \n    \n    ------------------------------------------------------------------------\n    \n    Tests updated, and took Junio's suggestion to reduce the cache lookup to\n    one call.\n    \n    I put in the shortened \"conflicted\" here but did not shorten the\n    existing \"not under version control\" message (to minimize the visible\n    and translations-required changes).\n    \n    I like the idea of renaming all stages and keeping them at their current\n    stages, but that's too much for this patch.\n    \n    I'll be traveling next week and not sure if I will get to any followups\n    for a while.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-678%2Fchris3torek%2Fgit-mv-message-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-678/chris3torek/git-mv-message-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/678\n\nRange-diff vs v1:\n\n 1:  0b617e74f7 ! 1:  f2e251a7ea git-mv: improve error message for conflicted file\n     @@ Commit message\n          Signed-off-by: Chris Torek <chris.torek@gmail.com>\n      \n       ## builtin/mv.c ##\n     +@@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)\n     + \tstruct stat st;\n     + \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n     + \tstruct lock_file lock_file = LOCK_INIT;\n     ++\tstruct cache_entry *ce;\n     + \n     + \tgit_config(git_default_config, NULL);\n     + \n      @@ builtin/mv.c: int cmd_mv(int argc, const char **argv, const char *prefix)\n       \t\t\t\t}\n       \t\t\t\targc += last - first;\n       \t\t\t}\n      -\t\t} else if (cache_name_pos(src, length) < 0)\n     --\t\t\tbad = _(\"not under version control\");\n     ++\t\t} else if (!(ce = cache_file_exists(src, length, ignore_case))) {\n     + \t\t\tbad = _(\"not under version control\");\n      -\t\telse if (lstat(dst, &st) == 0 &&\n     -+\t\t} else if (cache_name_pos(src, length) < 0) {\n     -+\t\t\t/*\n     -+\t\t\t * This occurs for both untracked files *and*\n     -+\t\t\t * files that are in merge-conflict state, so\n     -+\t\t\t * let's distinguish between those two.\n     -+\t\t\t */\n     -+\t\t\tstruct cache_entry *ce = cache_file_exists(src, length, ignore_case);\n     -+\t\t\tif (ce == NULL)\n     -+\t\t\t\tbad = _(\"not under version control\");\n     -+\t\t\telse\n     -+\t\t\t\tbad = _(\"must resolve merge conflict first\");\n     ++\t\t} else if (ce_stage(ce)) {\n     ++\t\t\tbad = _(\"conflicted\");\n      +\t\t} else if (lstat(dst, &st) == 0 &&\n       \t\t\t (!ignore_case || strcasecmp(src, dst))) {\n       \t\t\tbad = _(\"destination exists\");\n     @@ t/t7001-mv.sh: test_expect_success 'git mv should not change sha1 of moved cache\n      +test_expect_success 'git mv error on conflicted file' '\n      +\trm -fr .git &&\n      +\tgit init &&\n     -+\ttouch conflicted &&\n     -+\tcfhash=$(git hash-object -w conflicted) &&\n     -+\tgit update-index --index-info <<-EOF &&\n     -+\t$(printf \"0 $cfhash 0\\tconflicted\\n\")\n     -+\t$(printf \"100644 $cfhash 1\\tconflicted\\n\")\n     ++\t>conflict &&\n     ++\ttest_when_finished \"rm -f conflict\" &&\n     ++\tcfhash=$(git hash-object -w conflict) &&\n     ++\tq_to_tab <<-EOF | git update-index --index-info &&\n     ++\t0 $cfhash 0Qconflict\n     ++\t100644 $cfhash 1Qconflict\n      +\tEOF\n      +\n     -+\ttest_must_fail git mv conflicted newname 2>actual &&\n     -+\ttest_i18ngrep \"merge.conflict\" actual\n     ++\ttest_must_fail git mv conflict newname 2>actual &&\n     ++\ttest_i18ngrep \"conflicted\" actual\n      +'\n     -+\n     -+rm -f conflicted\n      +\n       test_expect_success 'git mv should overwrite symlink to a file' '\n       \n\n\n builtin/mv.c  |  7 +++++--\n t/t7001-mv.sh | 17 +++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex be15ba7044..7dac714af9 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -132,6 +132,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tstruct stat st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n \tstruct lock_file lock_file = LOCK_INIT;\n+\tstruct cache_entry *ce;\n \n \tgit_config(git_default_config, NULL);\n \n@@ -220,9 +221,11 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t}\n \t\t\t\targc += last - first;\n \t\t\t}\n-\t\t} else if (cache_name_pos(src, length) < 0)\n+\t\t} else if (!(ce = cache_file_exists(src, length, ignore_case))) {\n \t\t\tbad = _(\"not under version control\");\n-\t\telse if (lstat(dst, &st) == 0 &&\n+\t\t} else if (ce_stage(ce)) {\n+\t\t\tbad = _(\"conflicted\");\n+\t\t} else if (lstat(dst, &st) == 0 &&\n \t\t\t (!ignore_case || strcasecmp(src, dst))) {\n \t\t\tbad = _(\"destination exists\");\n \t\t\tif (force) {\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex 36b50d0b4c..c978b6dee4 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -248,6 +248,23 @@ test_expect_success 'git mv should not change sha1 of moved cache entry' '\n \n rm -f dirty dirty2\n \n+# NB: This test is about the error message\n+# as well as the failure.\n+test_expect_success 'git mv error on conflicted file' '\n+\trm -fr .git &&\n+\tgit init &&\n+\t>conflict &&\n+\ttest_when_finished \"rm -f conflict\" &&\n+\tcfhash=$(git hash-object -w conflict) &&\n+\tq_to_tab <<-EOF | git update-index --index-info &&\n+\t0 $cfhash 0Qconflict\n+\t100644 $cfhash 1Qconflict\n+\tEOF\n+\n+\ttest_must_fail git mv conflict newname 2>actual &&\n+\ttest_i18ngrep \"conflicted\" actual\n+'\n+\n test_expect_success 'git mv should overwrite symlink to a file' '\n \n \trm -fr .git &&\n\nbase-commit: ae46588be0cd730430dded4491246dfb4eac5557\n-- \ngitgitgadget\n"},{"id":"401776","messageId":"xmqqh7u29h2z.fsf@gitster.c.googlers.com","threadId":"53876","inReplyTo":"pull.678.v2.git.1595225873014.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] git-mv: improve error message for conflicted file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-20T18:28:20Z","receivedAt":"2020-07-20T18:28:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Chris Torek via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     I put in the shortened \"conflicted\" here but did not shorten the\n>     existing \"not under version control\" message (to minimize the visible\n>     and translations-required changes).\n\nIt does not matter all that much but the message in your original\nfor the new case looked better and worse at the same time ;-)\n\n\"not under version control\" is a statement of fact that does not\nhint what the user may want to do with that information.  Your\noriginal for the new case gave that hint (i.e. \"must resolve first\")\nbut the new \"the path is unmerged\" (I think 'unmerged' is a more\nproper term for this than 'conflicted'; see gitglossary[7]) stops at\nstating fact without giving further hint,and in that sense the\nmessages are consistent with each other.\n\nWe could shoot for consistency in the opposite direction, by making\n\"not under version control\" could instead say \"must add first\".  But\nthat leads to a fruitless comparison between \"'git add' then 'git\nmv'\" and \"plain 'mv' then 'git add'\".  For \"git mv\", \"must resolve\nfirst\" may be the only sane option right now, so it probably is OK.\n\nSo, after having thought the above through, I tend to (slightly)\nprefer to stop at stating fact, perhaps \"the path is unmerged\" or\nsomething to match \"not under version control\".\n\n>     I like the idea of renaming all stages and keeping them at their current\n>     stages, but that's too much for this patch.\n\nI totally agree, and I am not 100% convinced that the \"rename all at\ntheir current stage\" gives a better end-user experience.  For one\nthing, I suspect that it would still have to fail depending on how\nthe destination path and paths that conflict with it are populated\nin the index, and that may make even harder-to-explain error case.\n\n>     I'll be traveling next week and not sure if I will get to any followups\n>     for a while.\n\nThat is perfectly fine.  We are in pre-release freeze and this patch\nwon't go anywhere until the end of the month.\n\nThanks for contributing, and safe travels.\n"}]}