Re: [PATCH v3] remote: qualify "git pull" advice for non-upstream compareBranches

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +		if (use_divergence_advice && advice_enabled(ADVICE_STATUS_HINTS)) {
> +			if (push_remote_name && push_branch_name)
> +				strbuf_addf(sb,
> +					_("  (use \"git pull %s %s\" if you want to integrate the remote branch with yours)\n"),
> +					push_remote_name, push_branch_name);
> +			else
> +				strbuf_addstr(sb,
> +					_("  (use \"git pull\" if you want to integrate the remote branch with yours)\n"));

Here is where "git pull" is suggested as a fallback when the history
is diverged (e.g., you pushed and then you rebased).

> +		}
>  	}
>  }
>  
> @@ -2355,6 +2369,8 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  		int ours, theirs, cmp;
>  		int is_upstream, is_push;
>  		unsigned flags = 0;
> +		const char *push_remote_name = NULL;
> +		const char *push_branch_name = NULL;
>  
>  		full_ref = resolve_compare_branch(branch,
>  						  branches.items[i].string);
> @@ -2396,13 +2412,25 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
>  		if (reported)
>  			strbuf_addstr(sb, "\n");
>  
> -		if (is_upstream)
> +		if (is_upstream || is_push)
>  			flags |= ENABLE_ADVICE_PULL;
> -		if (is_push)
> -			flags |= ENABLE_ADVICE_PUSH;
>  		if (show_divergence_advice && is_upstream)
>  			flags |= ENABLE_ADVICE_DIVERGENCE;
> +		if (is_push) {
> +			flags |= ENABLE_ADVICE_PUSH;
> +			if (!upstream_ref || strcmp(upstream_ref, full_ref)) {
> +				push_remote_name = pushremote_for_branch(branch, NULL);

Here we _know_ that our repository has separate upstream and
push/publish repositories.  But we may not be able to "qualify" it
in the following "if" statement, in which case ...

> +				if (push_remote_name &&
> +				    skip_prefix(full_ref, "refs/remotes/", &push_branch_name) &&
> +				    skip_prefix(push_branch_name, push_remote_name, &push_branch_name) &&
> +				    *push_branch_name == '/')
> +					push_branch_name++;
> +				else
> +					push_remote_name = NULL;

... we assign NULL to push_remote_name to "punt".

> +			}
> +		}

Which means that this call to the helper function cannot distinguish
between the case where we were in "push" and pushing to the upstream
(i.e., "git pull" without extra arguments is perfectly a sensible
suggestion) and the case where we were in "push", diverged, and
triangular (i.e., "git pull" with or without extra arguments is not
an appropriate thing to suggest) but we cannot exactly tell what is
going on.

Shoudln't the "punt" case refrain from suggesting "git pull"?

>  		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
> +					 push_remote_name, push_branch_name,
>  					 abf, flags);
>  		reported = 1;
>  
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 0242b5bf7a..b613aba33a 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -646,4 +646,82 @@ test_expect_success 'status.compareBranches with remapped push and upstream remo
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'status.compareBranches behind both upstream and push' '
> +	test_config -C test push.default current &&
> +	test_config -C test remote.pushDefault origin &&
> +	test_config -C test status.compareBranches "@{upstream} @{push}" &&
> +	git -C test checkout -b feature13 upstream/main &&
> +	(cd test && advance work13) &&
> +	git -C test push origin &&
> +	git -C test branch --set-upstream-to upstream/ahead &&
> +	git -C test reset --hard HEAD^ &&
> +	git -C test status >actual &&
> +	cat >expect <<-EOF &&
> +	On branch feature13
> +	Your branch is behind ${SQ}upstream/ahead${SQ} by 1 commit, and can be fast-forwarded.
> +	  (use "git pull" to update your local branch)
> +
> +	Your branch is behind ${SQ}origin/feature13${SQ} by 1 commit, and can be fast-forwarded.
> +	  (use "git pull origin feature13" to update your local branch)
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'status.compareBranches with remapped push and behind push branch' '
> +	test_config -C test remote.pushDefault origin &&
> +	test_config -C test remote.origin.push refs/heads/feature14:refs/heads/remapped14 &&
> +	test_config -C test status.compareBranches "@{push}" &&
> +	git -C test checkout -b feature14 upstream/main &&
> +	(cd test && advance work14) &&
> +	git -C test push &&
> +	git -C test reset --hard HEAD^ &&
> +	git -C test status >actual &&
> +	cat >expect <<-EOF &&
> +	On branch feature14
> +	Your branch is behind ${SQ}origin/remapped14${SQ} by 1 commit, and can be fast-forwarded.
> +	  (use "git pull origin remapped14" to update your local branch)
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'status.compareBranches with behind push branch and no upstream' '
> +	test_config -C test push.default current &&
> +	test_config -C test remote.pushDefault origin &&
> +	test_config -C test status.compareBranches "@{push}" &&
> +	git -C test checkout --no-track -b feature15 upstream/main &&
> +	(cd test && advance work15) &&
> +	git -C test push origin &&
> +	git -C test reset --hard HEAD^ &&
> +	git -C test status >actual &&
> +	cat >expect <<-EOF &&
> +	On branch feature15
> +	Your branch is behind ${SQ}origin/feature15${SQ} by 1 commit, and can be fast-forwarded.
> +	  (use "git pull origin feature15" to update your local branch)
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'status.compareBranches behind upstream-equals-push suggests plain pull' '
> +	test_config -C test status.compareBranches "@{upstream} @{push}" &&
> +	git -C test checkout -b feature16 origin/main &&
> +	(cd test && advance work16) &&
> +	git -C test push origin HEAD:main &&
> +	git -C test reset --hard HEAD^ &&
> +	git -C test status >actual &&
> +	cat >expect <<-EOF &&
> +	On branch feature16
> +	Your branch is behind ${SQ}origin/main${SQ} by 1 commit, and can be fast-forwarded.
> +	  (use "git pull" to update your local branch)
> +
> +	nothing to commit, working tree clean
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_done
>
> base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf