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

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> When "git status" reports the local branch is behind the push
> branch, the advice suggested a bare "git pull". That follows the
> upstream, which may live on a different remote, so emit
> "git pull <remote> <branch>" instead.

Hmph, shouldn't this be done conditionally, though?  Most new users
follow the recommended pattern to set branch.<name>.merge so that
"git pull" would do the right thing for them, I presume, even when
they are using triangular workflow to push to a different remote
than the remote they pull from, so the new and more verbose message
would not help the users any more than the existing message, right?

Can the code tell the situation where the extra part of the message
would help and give it only then?

> Hmph, shouldn't this be done conditionally, though?  Most new users
> follow the recommended pattern to set branch.<name>.merge so that
> "git pull" would do the right thing for them, I presume, even when
> they are using triangular workflow to push to a different remote
> than the remote they pull from, so the new and more verbose message
> would not help the users any more than the existing message, right?
>
> Can the code tell the situation where the extra part of the message
> would help and give it only then?

Yes, that's a good idea.


Harald

"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