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

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

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

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Enable ENABLE_ADVICE_PULL for push-branch comparisons too, not just
> the upstream entry, so the "use git pull" hint prints when the local
> branch is behind its push branch.
>
> Spell out "git pull <remote> <branch>" so running the suggested
> command actually pulls the ref the user was told about; plain
> "git pull" would fetch the upstream instead.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>     remote: qualify "git pull" advice for non-upstream branches
>     
>      * Don't suggest git pull when we have no good command to suggest.
>      * New test for this. Asserts the behind line shows with no follow-up
>        advice.

Very well written.  

What does not happen in the "punt" case (the first bullet point
above) may deserve to be given in the commit log message, but
otherwise it is very clear what the change wanted to do to the
future readers of "git log".  

>  remote.c                 |  48 +++++++++++++++----
>  t/t6040-tracking-info.sh | 100 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 140 insertions(+), 8 deletions(-)

The code changes look correct, and the new tests checks the new
suggestion as well as the "punt" case, which is good.

Shall we mark it for 'next' now?

Thanks for working on this.

>> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 0242b5bf7a..91cbb8775d 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -646,4 +646,104 @@ 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
> +'

A good test that clearly shows how @{push} is described ;-)

> +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
> +'

OK.