{"thread":{"id":"51490","subject":"Merge: \"git rm bla\": \"bla: needs merge\", but still removes file \"bla\"","startedAt":"2019-07-17T08:05:11Z","lastAt":"2019-07-18T21:43:28Z","messageCount":6,"participants":["Ulrich Windl","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"379004","messageId":"5D2ED50D020000A100032435@gwsmtp.uni-regensburg.de","threadId":"51490","inReplyTo":null,"subject":"Merge: \"git rm bla\": \"bla: needs merge\", but still removes file \"bla\"","fromName":"Ulrich Windl","fromEmail":"ulrich.windl@rz.uni-regensburg.de","sentAt":"2019-07-17T07:58:05Z","receivedAt":"2019-07-17T08:05:11Z","isPatch":false,"sender":{"key":"ulrich.windl@rz.uni-regensburg.de","avatar":null},"body":"Hi!\n\nI just had \"an interesting case\" for a merge with conflicts:\nThe merge re-introduced a file that had been renamed (say old one is \"bla\", and the new one is \"foo\").\nAfter merging the changes from bla into foo, I added foo, trying to remove bla:\n> git add foo\n> git rm bla\nbla: needs merge\nrm 'bla'\n\nThe interesting part is that the file was still removed, while claiming \"needs merge\" before:\n> git rm bla\nfatal: pathspec 'bla' did not match any files\n\n(git-2.12.3-27.14.1.x86_64 from SLES12 SP4)\n\nRegards,\nUlrich\n\n\n"},{"id":"379021","messageId":"xmqqd0i81ui4.fsf@gitster-ct.c.googlers.com","threadId":"51490","inReplyTo":"5D2ED50D020000A100032435@gwsmtp.uni-regensburg.de","subject":"Re: Merge: \"git rm bla\": \"bla: needs merge\", but still removes file \"bla\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-17T16:44:03Z","receivedAt":"2019-07-17T16:44:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ulrich Windl\" <Ulrich.Windl@rz.uni-regensburg.de> writes:\n\n> I just had \"an interesting case\" for a merge with conflicts:\n> The merge re-introduced a file that had been renamed (say old one is \"bla\", and the new one is \"foo\").\n> After merging the changes from bla into foo, I added foo, trying to remove bla:\n>> git add foo\n>> git rm bla\n> bla: needs merge\n> rm 'bla'\n\nYeah, I think I've known about this for a long time.  We have an\ninternal call to \"update-index --refresh\" before starting to remove,\nbecause we need to know which path is up-to-date wrt the index, and\nthe machinery for refreshing the index by default gives the \"needs\nmerge\" message.  We never bothered to squelch it.\n\nPerhaps this one-liner would be a sufficient fix.\n\n\n\n builtin/rm.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 2eacda42b4..19ce95a901 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -273,7 +273,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD,\n \t\t       prefix, argv);\n-\trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n+\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &pathspec, NULL, NULL);\n \n \tseen = xcalloc(pathspec.nr, 1);\n \n"},{"id":"379030","messageId":"xmqqims0z99w.fsf_-_@gitster-ct.c.googlers.com","threadId":"51490","inReplyTo":"xmqqd0i81ui4.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] rm: resolving by removal is not a warning-worthy event","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-17T20:38:35Z","receivedAt":"2019-07-17T20:38:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When resolving a conflict on a path in favor of removing it, using\n\"git rm\" on it is the standard way to do so.  The user however is\ngreeted with a \"needs merge\" message during that operation:\n\n\t$ git merge side-branch\n\t$ edit conflicted-path-1\n\t$ git add conflicted-path-1\n\t$ git rm conflicted-path-2\n\tconflicted-path-2: needs merge\n\trm 'conflicted-path-2'\n\nThe removal by \"git rm\" does get performed, but an uninitiated user\nmay find it confusing, \"needs merge? so I need to resolve conflict\nbefore being able to remove it???\"\n\nThe message is coming from \"update-index --refresh\" that is called\ninternally to make sure \"git rm\" knows which paths are clean and\nwhich paths are dirty, in order to prevent removal of paths modified\nrelative to the index without the \"-f\" option.  We somehow ended up\nnot squelching this message which seeped through to the UI surface.\n\nUse the same mechanism used by \"git commit\", \"git describe\", etc. to\nsquelch the message.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/rm.c  |  2 +-\n t/t3600-rm.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex 65b448ef8e..b63c86ae92 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -272,7 +272,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tparse_pathspec(&pathspec, 0,\n \t\t       PATHSPEC_PREFER_CWD,\n \t\t       prefix, argv);\n-\trefresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);\n+\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &pathspec, NULL, NULL);\n \n \tseen = xcalloc(pathspec.nr, 1);\n \ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex b8fbdefcdc..5aae78ccc4 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -251,6 +251,19 @@ test_expect_success 'choking \"git rm\" should not let it die with cruft' '\n \ttest_path_is_missing .git/index.lock\n '\n \n+test_expect_success 'Resolving by removal is not a warning-worthy event' '\n+\tgit reset -q --hard &&\n+\ttest_when_finished \"rm -f .git/index.lock msg && git reset -q --hard\" &&\n+\tqfwfq=$(echo qfwfq | git hash-object -w --stdin) &&\n+\tfor stage in 1 2 3\n+\tdo\n+\t\techo \"100644 $qfwfq $stage\tqfwfq\"\n+\tdone | git update-index --index-info &&\n+\tgit rm qfwfq >msg &&\n+\ttest_i18ngrep ! \"needs merge\" msg &&\n+\ttest_must_fail git ls-files -s --error-unmatch qfwfq\n+'\n+\n test_expect_success 'rm removes subdirectories recursively' '\n \tmkdir -p dir/subdir/subsubdir &&\n \techo content >dir/subdir/subsubdir/file &&\n-- \n2.22.0-653-g37fc7794bc\n\n"},{"id":"379079","messageId":"20190718202656.GC22038@sigill.intra.peff.net","threadId":"51490","inReplyTo":"xmqqims0z99w.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH] rm: resolving by removal is not a warning-worthy event","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-18T20:26:56Z","receivedAt":"2019-07-18T20:26:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 17, 2019 at 01:38:35PM -0700, Junio C Hamano wrote:\n\n> When resolving a conflict on a path in favor of removing it, using\n> \"git rm\" on it is the standard way to do so.  The user however is\n> greeted with a \"needs merge\" message during that operation:\n> \n> \t$ git merge side-branch\n> \t$ edit conflicted-path-1\n> \t$ git add conflicted-path-1\n> \t$ git rm conflicted-path-2\n> \tconflicted-path-2: needs merge\n> \trm 'conflicted-path-2'\n> \n> The removal by \"git rm\" does get performed, but an uninitiated user\n> may find it confusing, \"needs merge? so I need to resolve conflict\n> before being able to remove it???\"\n> \n> The message is coming from \"update-index --refresh\" that is called\n> internally to make sure \"git rm\" knows which paths are clean and\n> which paths are dirty, in order to prevent removal of paths modified\n> relative to the index without the \"-f\" option.  We somehow ended up\n> not squelching this message which seeped through to the UI surface.\n> \n> Use the same mechanism used by \"git commit\", \"git describe\", etc. to\n> squelch the message.\n\nNicely explained, and the patch makes sense.\n\n> +test_expect_success 'Resolving by removal is not a warning-worthy event' '\n> +\tgit reset -q --hard &&\n> +\ttest_when_finished \"rm -f .git/index.lock msg && git reset -q --hard\" &&\n> +\tqfwfq=$(echo qfwfq | git hash-object -w --stdin) &&\n\nI'd have called this \"$blob\" for my own sanity in typing later lines,\nbut OK. :)\n\n> +\tdo\n> +\t\techo \"100644 $qfwfq $stage\tqfwfq\"\n> +\tdone | git update-index --index-info &&\n> +\tgit rm qfwfq >msg &&\n> +\ttest_i18ngrep ! \"needs merge\" msg &&\n\nShould we capture stderr from \"git rm\", too, to cover all bases?\n\n-Peff\n"},{"id":"379081","messageId":"xmqq8ssvxd9w.fsf@gitster-ct.c.googlers.com","threadId":"51490","inReplyTo":"20190718202656.GC22038@sigill.intra.peff.net","subject":"Re: [PATCH] rm: resolving by removal is not a warning-worthy event","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-18T21:07:23Z","receivedAt":"2019-07-18T21:07:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +test_expect_success 'Resolving by removal is not a warning-worthy event' '\n>> +\tgit reset -q --hard &&\n>> +\ttest_when_finished \"rm -f .git/index.lock msg && git reset -q --hard\" &&\n>> +\tqfwfq=$(echo qfwfq | git hash-object -w --stdin) &&\n>\n> I'd have called this \"$blob\" for my own sanity in typing later lines,\n> but OK. :)\n\nOK, I can change that easily ;-)\n\n>> +\tdo\n>> +\t\techo \"100644 $qfwfq $stage\tqfwfq\"\n>> +\tdone | git update-index --index-info &&\n>> +\tgit rm qfwfq >msg &&\n>> +\ttest_i18ngrep ! \"needs merge\" msg &&\n>\n> Should we capture stderr from \"git rm\", too, to cover all bases?\n\nDo you mean\n\n\tgit rm blob >msg 2>&1\n\nbecause we could later change our mind and send \"needs merge\"\nmessage to the standard error stream?\n\n\n"},{"id":"379082","messageId":"20190718214325.GA28619@sigill.intra.peff.net","threadId":"51490","inReplyTo":"xmqq8ssvxd9w.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] rm: resolving by removal is not a warning-worthy event","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-18T21:43:25Z","receivedAt":"2019-07-18T21:43:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 18, 2019 at 02:07:23PM -0700, Junio C Hamano wrote:\n\n> >> +\tgit rm qfwfq >msg &&\n> >> +\ttest_i18ngrep ! \"needs merge\" msg &&\n> >\n> > Should we capture stderr from \"git rm\", too, to cover all bases?\n> \n> Do you mean\n> \n> \tgit rm blob >msg 2>&1\n> \n> because we could later change our mind and send \"needs merge\"\n> message to the standard error stream?\n\nYes, exactly.\n\n-Peff\n"}]}