From: Sebastien Marie <semarie@online.fr>
Subject: Re: fix staging of multiple files with -p
To: gameoftrees@openbsd.org
Date: Sun, 27 Oct 2019 14:21:22 +0100

On Thu, Oct 24, 2019 at 06:18:19PM +0200, Stefan Sperling wrote:
> There's a bug in stage -p where it fails if multiple files are involved.
> 
[...]
>
> 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?

the rational is good. the diff seems ok too.

ok semarie@
 
> 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
> 

-- 
Sebastien Marie