rm: resolving by removal is not a warning-worthy event
diff mbox series

Message ID xmqqims0z99w.fsf_-_@gitster-ct.c.googlers.com
State New
Headers show
Series
  • rm: resolving by removal is not a warning-worthy event
Related show

Commit Message

Junio C Hamano July 17, 2019, 8:38 p.m. UTC
When resolving a conflict on a path in favor of removing it, using
"git rm" on it is the standard way to do so.  The user however is
greeted with a "needs merge" message during that operation:

	$ git merge side-branch
	$ edit conflicted-path-1
	$ git add conflicted-path-1
	$ git rm conflicted-path-2
	conflicted-path-2: needs merge
	rm 'conflicted-path-2'

The removal by "git rm" does get performed, but an uninitiated user
may find it confusing, "needs merge? so I need to resolve conflict
before being able to remove it???"

The message is coming from "update-index --refresh" that is called
internally to make sure "git rm" knows which paths are clean and
which paths are dirty, in order to prevent removal of paths modified
relative to the index without the "-f" option.  We somehow ended up
not squelching this message which seeped through to the UI surface.

Use the same mechanism used by "git commit", "git describe", etc. to
squelch the message.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rm.c  |  2 +-
 t/t3600-rm.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Jeff King July 18, 2019, 8:26 p.m. UTC | #1
On Wed, Jul 17, 2019 at 01:38:35PM -0700, Junio C Hamano wrote:

> When resolving a conflict on a path in favor of removing it, using
> "git rm" on it is the standard way to do so.  The user however is
> greeted with a "needs merge" message during that operation:
> 
> 	$ git merge side-branch
> 	$ edit conflicted-path-1
> 	$ git add conflicted-path-1
> 	$ git rm conflicted-path-2
> 	conflicted-path-2: needs merge
> 	rm 'conflicted-path-2'
> 
> The removal by "git rm" does get performed, but an uninitiated user
> may find it confusing, "needs merge? so I need to resolve conflict
> before being able to remove it???"
> 
> The message is coming from "update-index --refresh" that is called
> internally to make sure "git rm" knows which paths are clean and
> which paths are dirty, in order to prevent removal of paths modified
> relative to the index without the "-f" option.  We somehow ended up
> not squelching this message which seeped through to the UI surface.
> 
> Use the same mechanism used by "git commit", "git describe", etc. to
> squelch the message.

Nicely explained, and the patch makes sense.

> +test_expect_success 'Resolving by removal is not a warning-worthy event' '
> +	git reset -q --hard &&
> +	test_when_finished "rm -f .git/index.lock msg && git reset -q --hard" &&
> +	qfwfq=$(echo qfwfq | git hash-object -w --stdin) &&

I'd have called this "$blob" for my own sanity in typing later lines,
but OK. :)

> +	do
> +		echo "100644 $qfwfq $stage	qfwfq"
> +	done | git update-index --index-info &&
> +	git rm qfwfq >msg &&
> +	test_i18ngrep ! "needs merge" msg &&

Should we capture stderr from "git rm", too, to cover all bases?

-Peff
Junio C Hamano July 18, 2019, 9:07 p.m. UTC | #2
Jeff King <peff@peff.net> writes:

>> +test_expect_success 'Resolving by removal is not a warning-worthy event' '
>> +	git reset -q --hard &&
>> +	test_when_finished "rm -f .git/index.lock msg && git reset -q --hard" &&
>> +	qfwfq=$(echo qfwfq | git hash-object -w --stdin) &&
>
> I'd have called this "$blob" for my own sanity in typing later lines,
> but OK. :)

OK, I can change that easily ;-)

>> +	do
>> +		echo "100644 $qfwfq $stage	qfwfq"
>> +	done | git update-index --index-info &&
>> +	git rm qfwfq >msg &&
>> +	test_i18ngrep ! "needs merge" msg &&
>
> Should we capture stderr from "git rm", too, to cover all bases?

Do you mean

	git rm blob >msg 2>&1

because we could later change our mind and send "needs merge"
message to the standard error stream?
Jeff King July 18, 2019, 9:43 p.m. UTC | #3
On Thu, Jul 18, 2019 at 02:07:23PM -0700, Junio C Hamano wrote:

> >> +	git rm qfwfq >msg &&
> >> +	test_i18ngrep ! "needs merge" msg &&
> >
> > Should we capture stderr from "git rm", too, to cover all bases?
> 
> Do you mean
> 
> 	git rm blob >msg 2>&1
> 
> because we could later change our mind and send "needs merge"
> message to the standard error stream?

Yes, exactly.

-Peff

Patch
diff mbox series

diff --git a/builtin/rm.c b/builtin/rm.c
index 65b448ef8e..b63c86ae92 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -272,7 +272,7 @@  int cmd_rm(int argc, const char **argv, const char *prefix)
 	parse_pathspec(&pathspec, 0,
 		       PATHSPEC_PREFER_CWD,
 		       prefix, argv);
-	refresh_index(&the_index, REFRESH_QUIET, &pathspec, NULL, NULL);
+	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &pathspec, NULL, NULL);
 
 	seen = xcalloc(pathspec.nr, 1);
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index b8fbdefcdc..5aae78ccc4 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -251,6 +251,19 @@  test_expect_success 'choking "git rm" should not let it die with cruft' '
 	test_path_is_missing .git/index.lock
 '
 
+test_expect_success 'Resolving by removal is not a warning-worthy event' '
+	git reset -q --hard &&
+	test_when_finished "rm -f .git/index.lock msg && git reset -q --hard" &&
+	qfwfq=$(echo qfwfq | git hash-object -w --stdin) &&
+	for stage in 1 2 3
+	do
+		echo "100644 $qfwfq $stage	qfwfq"
+	done | git update-index --index-info &&
+	git rm qfwfq >msg &&
+	test_i18ngrep ! "needs merge" msg &&
+	test_must_fail git ls-files -s --error-unmatch qfwfq
+'
+
 test_expect_success 'rm removes subdirectories recursively' '
 	mkdir -p dir/subdir/subsubdir &&
 	echo content >dir/subdir/subsubdir/file &&