"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:

> 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.

> >     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".

Thanks. It's a good point, but maybe it's implicit that nothing
happens when nothing can happen?

> Shall we mark it for 'next' now?
>
> Thanks for working on this.

Yes please, thanks for all the help!


Harald