On Wed, Nov 03, 2021 at 04:36:16PM -0400, Josh Rickmar wrote:
> The modifications necessary to test_rebase_ancestry_check should cover
> this, right?

Yes, that should be good enough.

A few comments inline:

> diff refs/heads/main refs/heads/noop-rebase
> blob - 488cd9f35722bed936af6e7d9e2c6aa77ea2a3d8
> blob + f21c34a4f3f2dd8d6fc850ebc957f8683898e50b
> --- got/got.c
> +++ got/got.c
> @@ -9147,12 +9147,41 @@ cmd_rebase(int argc, char *argv[])
>  				goto done;
>  			error = NULL;
>  		} else {
> -			static char msg[128];
> -			snprintf(msg, sizeof(msg),
> -			    "%s is already based on %s",
> +			struct got_pathlist_head paths;
> +			char *id_str;

id_str should be freed. Moving it as close as possible to the
point where it is used makes this easier since it avoids having to
free it in all the error paths of other functions calls in-between.

You could declare and allocate id_str within the if-block that
prints "Updated to ..." and then free(id_str) right after the
printf call. Check out the "Forwarding %s to commit ..." case
which does the same thing.

> +
> +			printf("%s is already based on %s\n",
>  			    got_ref_get_name(branch),
>  			    got_worktree_get_head_ref_name(worktree));
> -			error = got_error_msg(GOT_ERR_SAME_BRANCH, msg);
> +			error = got_object_id_str(&id_str,
> +			    branch_head_commit_id);
> +			if (error)
> +				goto done;
> +			error = switch_head_ref(branch, branch_head_commit_id,
> +			    worktree, repo);
> +			if (error)
> +				goto done;
> +			error = got_worktree_set_base_commit_id(worktree, repo,
> +			    branch_head_commit_id);
> +			if (error)
> +				goto done;
> +			error = got_pathlist_append(&merged_paths, "", NULL);

s/merged_paths/paths/ ?

> +			if (error)
> +				goto done;
> +			TAILQ_INIT(&paths);
> +			error = got_worktree_checkout_files(worktree,
> +			    &merged_paths, repo, update_progress, &upa,

Same here?

> +			    check_cancelled, NULL);
> +			got_pathlist_free(&paths);
> +			if (error)
> +				goto done;
> +			if (upa.did_something) {
> +				printf("Updated to %s: %s\n",
> +				    got_worktree_get_head_ref_name(worktree),
> +				    id_str);
> +			} else
> +				printf("Already up-to-date\n");
> +			print_update_progress_stats(&upa);
>  			goto done;
>  		}
>  		error = got_worktree_rebase_prepare(&new_base_branch,
> blob - b17666eaf0ae899543adfa66236bdfd3eb49e92d
> blob + c629d068ddf4ae78f0a2184ea064823c4c1926de
> --- regress/cmdline/rebase.sh
> +++ regress/cmdline/rebase.sh
> @@ -243,11 +243,18 @@ test_rebase_ancestry_check() {
>  	(cd $testroot/repo && git checkout -q -b newbranch)
>  	echo "modified delta on branch" > $testroot/repo/gamma/delta
>  	git_commit $testroot/repo -m "committing to delta on newbranch"
> +	local newbranch_id=`git_show_head $testroot/repo`
>  
>  	(cd $testroot/wt && got rebase newbranch > $testroot/stdout \
>  		2> $testroot/stderr)
>  
> -	echo -n > $testroot/stdout.expected
> +	echo "refs/heads/newbranch is already based on refs/heads/master" \
> +		> $testroot/stdout.expected
> +	echo "Switching work tree from refs/heads/master to refs/heads/newbranch" \
> +		>> $testroot/stdout.expected
> +	echo "U  gamma/delta" >> $testroot/stdout.expected
> +	echo "Updated to refs/heads/newbranch: ${newbranch_id}" \
> +		>> $testroot/stdout.expected
>  	cmp -s $testroot/stdout.expected $testroot/stdout
>  	ret="$?"
>  	if [ "$ret" != "0" ]; then
> @@ -256,8 +263,7 @@ test_rebase_ancestry_check() {
>  		return 1
>  	fi
>  
> -	echo "got: refs/heads/newbranch is already based on refs/heads/master" \
> -		> $testroot/stderr.expected
> +	echo -n > $testroot/stderr.expected
>  	cmp -s $testroot/stderr.expected $testroot/stderr
>  	ret="$?"
>  	if [ "$ret" != "0" ]; then
> 
>