From: Stefan Sperling <stsp@stsp.name>
Subject: fix staging of multiple files with -p
To: gameoftrees@openbsd.org
Date: Thu, 24 Oct 2019 18:18:19 +0200

There's a bug in stage -p where it fails if multiple files are involved.

I had staged changes (M in 2nd column) for the following files:

MM sys/dev/pci/if_iwm.c
MM sys/dev/pci/if_iwmreg.h
 M sys/dev/pci/if_iwmvar.h

Then I edited if_iwm.c and ran 'got stage -p' again to stage these edits.
But instead of the expected prompt for my if_iwm.c changes I got this:

  got: sys/dev/pci/if_iwmvar.h: no changes to stage

It is correct that if_iwmvar.h has no further changes to stage, but
another file does. So there is no good reason to stop the program.

It worked once I explicitly specified if_iwm.c as an argument.
Transcript showing how the above happened:

-----------------------------------------------
@@ -381,6 +381,7 @@ struct iwm_softc {
        int                     ba_start;
        int                     ba_tid;
        uint16_t                ba_ssn;
+       uint16_t                ba_winsize;

        /* Task for HT protection updates. */
        struct task             htprot_task;
-----------------------------------------------
M  sys/dev/pci/if_iwmvar.h (change 1 of 1)
stage this change? [y/n/q] y
$ got diff -s | less
$ vi if_iwm.c
$ got stage -p
got: sys/dev/pci/if_iwmvar.h: no changes to stage
$ got stage -p if_iwm.c
-----------------------------------------------
@@ -903,6 +903,7 @@ iwm_notif_intr(struct iwm_softc *sc)
 iwm_write_prph(struct iwm_softc *sc, uint32_t addr, uint32_t val)
 {
        iwm_nic_assert_locked(sc);
+       printf("%s: io[0x%x] = 0x%x\n", __func__, addr, val);
        IWM_WRITE(sc,
            IWM_HBUS_TARG_PRPH_WADDR, ((addr & 0x000fffff) | (3 << 24)));
        IWM_BARRIER_WRITE(sc);
-----------------------------------------------
M  sys/dev/pci/if_iwm.c (change 1 of 11)
stage this change? [y/n/q] n

Based on the above:

1) If a file has no changes to stage in the -p case, move on to the
next file instead of reporting an error.

2) If there is nothing to stage (with or without -p), print an error
and use an exit code != 0. The related test failed to take the exit
code into account, fix this.

ok?

diff a926e3a4334151b10e2b78e1fa3f2a59a44c71f2 /home/stsp/src/got
blob - 3c6178de57a898b1c3c7be0e3585e0a5c40bb6bb
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -5871,7 +5871,8 @@ check_stage_ok(void *arg, unsigned char status,
 	struct got_object_id *base_commit_idp = NULL;
 	char *in_repo_path = NULL, *p;
 
-	if (status == GOT_STATUS_UNVERSIONED)
+	if (status == GOT_STATUS_UNVERSIONED ||
+	    status == GOT_STATUS_NO_CHANGE)
 		return NULL;
 	if (status == GOT_STATUS_NONEXISTENT)
 		return got_error_set_errno(ENOENT, relpath);
@@ -5891,10 +5892,7 @@ check_stage_ok(void *arg, unsigned char status,
 		base_commit_idp = &base_commit_id;
 	}
 
-	if (status == GOT_STATUS_NO_CHANGE) {
-		err = got_error_path(ie->path, GOT_ERR_STAGE_NO_CHANGE);
-		goto done;
-	} else if (status == GOT_STATUS_CONFLICT) {
+	if (status == GOT_STATUS_CONFLICT) {
 		err = got_error_path(ie->path, GOT_ERR_STAGE_CONFLICT);
 		goto done;
 	} else if (status != GOT_STATUS_ADD &&
@@ -5925,6 +5923,7 @@ struct stage_path_arg {
 	void *status_arg;
 	got_worktree_patch_cb patch_cb;
 	void *patch_arg;
+	int staged_something;
 };
 
 static const struct got_error *
@@ -5983,6 +5982,7 @@ stage_path(void *arg, unsigned char status,
 		else
 			stage = GOT_FILEIDX_STAGE_MODIFY;
 		got_fileindex_entry_stage_set(ie, stage);
+		a->staged_something = 1;
 		if (a->status_cb == NULL)
 			break;
 		err = (*a->status_cb)(a->status_arg, GOT_STATUS_NO_CHANGE,
@@ -6007,13 +6007,13 @@ stage_path(void *arg, unsigned char status,
 		}
 		stage = GOT_FILEIDX_STAGE_DELETE;
 		got_fileindex_entry_stage_set(ie, stage);
+		a->staged_something = 1;
 		if (a->status_cb == NULL)
 			break;
 		err = (*a->status_cb)(a->status_arg, GOT_STATUS_NO_CHANGE,
 		    get_staged_status(ie), relpath, NULL, NULL, NULL);
 		break;
 	case GOT_STATUS_NO_CHANGE:
-		err = got_error_path(relpath, GOT_ERR_STAGE_NO_CHANGE);
 		break;
 	case GOT_STATUS_CONFLICT:
 		err = got_error_path(relpath, GOT_ERR_STAGE_CONFLICT);
@@ -6089,11 +6089,16 @@ got_worktree_stage(struct got_worktree *worktree,
 	spa.patch_arg = patch_arg;
 	spa.status_cb = status_cb;
 	spa.status_arg = status_arg;
+	spa.staged_something = 0;
 	TAILQ_FOREACH(pe, paths, entry) {
 		err = worktree_status(worktree, pe->path, fileindex, repo,
 		    stage_path, &spa, NULL, NULL);
 		if (err)
 			goto done;
+	}
+	if (!spa.staged_something) {
+		err = got_error(GOT_ERR_STAGE_NO_CHANGE);
+		goto done;
 	}
 
 	sync_err = sync_fileindex(fileindex, fileindex_path);
blob - ba1b5eaaf04bac104befb9a8defef1522ad1de37
file + regress/cmdline/stage.sh
--- regress/cmdline/stage.sh
+++ regress/cmdline/stage.sh
@@ -352,7 +352,7 @@ function test_double_stage {
 	(cd $testroot/wt && got add foo > /dev/null)
 	(cd $testroot/wt && got stage alpha beta foo > /dev/null)
 
-	echo "got: alpha: no changes to stage" > $testroot/stderr.expected
+	echo "got: no changes to stage" > $testroot/stderr.expected
 	(cd $testroot/wt && got stage alpha 2> $testroot/stderr)
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret="$?"
@@ -362,10 +362,39 @@ function test_double_stage {
 		return 1
 	fi
 
-	(cd $testroot/wt && got stage beta > $testroot/stdout)
+	(cd $testroot/wt && got stage beta \
+		> $testroot/stdout 2> $testroot/stderr)
 	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got stage command succeeded unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	echo -n > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
 	if [ "$ret" != "0" ]; then
-		echo "got stage command failed unexpectedly" >&2
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "got: no changes to stage" > $testroot/stderr.expected
+	(cd $testroot/wt && got stage foo 2> $testroot/stderr)
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	printf "q\n" > $testroot/patchscript
+	(cd $testroot/wt && got stage -F $testroot/patchscript -p \
+		> $testroot/stdout 2> $testroot/stderr)
+	ret="$?"
+	if [ "$ret" == "0" ]; then
+		echo "got stage command succeeded unexpectedly" >&2
 		test_done "$testroot" "1"
 		return 1
 	fi
@@ -378,7 +407,7 @@ function test_double_stage {
 		return 1
 	fi
 
-	echo "got: foo: no changes to stage" > $testroot/stderr.expected
+	echo "got: no changes to stage" > $testroot/stderr.expected
 	(cd $testroot/wt && got stage foo 2> $testroot/stderr)
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret="$?"
@@ -1421,10 +1450,10 @@ function test_stage_patch {
 	# don't stage any hunks
 	printf "n\nn\nn\n" > $testroot/patchscript
 	(cd $testroot/wt && got stage -F $testroot/patchscript -p \
-		numbers > $testroot/stdout)
+		numbers > $testroot/stdout 2> $testroot/stderr)
 	ret="$?"
-	if [ "$ret" != "0" ]; then
-		echo "got stage command failed unexpectedly" >&2
+	if [ "$ret" == "0" ]; then
+		echo "got stage command succeeded unexpectedly" >&2
 		test_done "$testroot" "1"
 		return 1
 	fi
@@ -1472,6 +1501,16 @@ EOF
 		return 1
 	fi
 
+	echo "got: no changes to stage" > $testroot/stderr.expected
+	cmp -s $testroot/stderr.expected $testroot/stderr
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stderr.expected $testroot/stderr
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+
 	(cd $testroot/wt && got status > $testroot/stdout)
 	echo "M  numbers" > $testroot/stdout.expected
 	cmp -s $testroot/stdout.expected $testroot/stdout
@@ -1968,7 +2007,7 @@ function test_stage_patch_added_twice {
 		return 1
 	fi
 
-	echo "got: epsilon/new: no changes to stage" > $testroot/stderr.expected
+	echo "got: no changes to stage" > $testroot/stderr.expected
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret="$?"
 	if [ "$ret" != "0" ]; then
@@ -2087,13 +2126,13 @@ function test_stage_patch_removed_twice {
 	(cd $testroot/wt && got stage -F $testroot/patchscript -p beta \
 		> $testroot/stdout 2> $testroot/stderr)
 	ret="$?"
-	if [ "$ret" != "0" ]; then
-		echo "got stage command failed unexpectedly" >&2
+	if [ "$ret" == "0" ]; then
+		echo "got stage command succeeded unexpectedly" >&2
 		test_done "$testroot" "$ret"
 		return 1
 	fi
 
-	echo -n > $testroot/stderr.expected
+	echo "got: no changes to stage" > $testroot/stderr.expected
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret="$?"
 	if [ "$ret" != "0" ]; then