Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Jun 03, 2022 at 11:34:48AM +0200, Omar Polo wrote:
> > found this by chance; i've staged a debug printf somewhere by accident,
> > then removed, staged the removal and... the file was still staged but
> > with an empty diff!  (it was the only edit to that file)
> > 
> > patch below tries to make stage_path slightly more clever by checking
> > the staged blob and the file blob: if they're the same we've effectively
> > unstaged a file.
> > 
> > does this makes sense?  am i trying to make 'got stage' too much clever?
> 
> Could we make this situation an error and require the user to
> run 'got unstage' instead? Or would that be too complicated?

it's just as easy i guess, see patch below.  I prefer the first
behaviour (the implicit unstage) but don't have a strong opinion.

diff 842467521f94def2d4cce96b3c39f8bbad73bd0b /home/op/w/got
blob - 8ae16da40c07f4dcb3133f9ff08328a10605722b
file + include/got_error.h
--- include/got_error.h
+++ include/got_error.h
@@ -168,6 +168,7 @@
 #define GOT_ERR_HUNK_FAILED	150
 #define GOT_ERR_PATCH_FAILED	151
 #define GOT_ERR_FILEIDX_DUP_ENTRY 152
+#define GOT_ERR_STAGE_REVERSE	153
 
 struct got_error {
         int code;
blob - 29541c0234b7f96a4638157fdd9017888571a76c
file + lib/error.c
--- lib/error.c
+++ lib/error.c
@@ -216,6 +216,7 @@ static const struct got_error got_errors[] = {
 	{ GOT_ERR_HUNK_FAILED, "hunk failed to apply" },
 	{ GOT_ERR_PATCH_FAILED, "patch failed to apply" },
 	{ GOT_ERR_FILEIDX_DUP_ENTRY, "duplicate file index entry" },
+	{ GOT_ERR_STAGE_REVERSE, "stage operation would unstage the file" },
 };
 
 static struct got_custom_error {
blob - c8e98cc3b117b9e02d199a6a46a60f02d73c2b83
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -8044,6 +8044,11 @@ stage_path(void *arg, unsigned char status,
 			break;
 		memcpy(ie->staged_blob_sha1, new_staged_blob_id->sha1,
 		    SHA1_DIGEST_LENGTH);
+		if (memcmp(ie->staged_blob_sha1, ie->blob_sha1,
+		    SHA1_DIGEST_LENGTH) == 0) {
+			err = got_error_path(relpath, GOT_ERR_STAGE_REVERSE);
+			break;
+		}
 		if (status == GOT_STATUS_ADD || staged_status == GOT_STATUS_ADD)
 			stage = GOT_FILEIDX_STAGE_ADD;
 		else
blob - 701e5bded8367b6fc13908410c4111d0e1c82210
file + regress/cmdline/stage.sh
--- regress/cmdline/stage.sh
+++ regress/cmdline/stage.sh
@@ -2154,6 +2154,54 @@ test_stage_patch_removed_twice() {
 	test_done "$testroot" "$ret"
 }
 
+test_stage_patch_reversed() {
+	local testroot=`test_init stage_patch_reversed`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo 'ALPHA' > $testroot/wt/alpha
+	(cd $testroot/wt && got stage alpha > $testroot/stdout)
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo ' M alpha' > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo 'alpha' > $testroot/wt/alpha
+	(cd $testroot/wt && got stage alpha 2> $testroot/stderr)
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "got staged command succeeded unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo 'got: alpha: stage operation would unstage the file' \
+		> $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_stage_patch_quit() {
 	local testroot=`test_init stage_patch_quit`
 
@@ -2987,6 +3035,7 @@ run_test test_stage_patch_added
 run_test test_stage_patch_added_twice
 run_test test_stage_patch_removed
 run_test test_stage_patch_removed_twice
+run_test test_stage_patch_reversed
 run_test test_stage_patch_quit
 run_test test_stage_patch_incomplete_script
 run_test test_stage_symlink