remote: qualify "git pull" advice for non-upstream branches

From: Harald Nordgren <[email protected]>

When "git status" reports the local branch is behind or has diverged from 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.

Also enable the pull and divergence advice for push-branch comparisons; they were previously only set for the upstream.

---
remote: qualify "git pull" advice for non-upstream branches

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2301%2FHaraldNordgren%2Fstatus-pull-advice-qualified-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2301/HaraldNordgren/status-pull-advice-qualified-v1
Pull-Request: https://github.com/git/git/pull/2301

 remote.c                 | 47 +++++++++++++++++++++++++++++++---------
 t/t6040-tracking-info.sh | 41 +++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 10 deletions(-)

index a664cd166a..d1e09079cb 100644
@@ -2267,6 +2267,8 @@ static void format_branch_comparison(struct strbuf *sb,
 				     bool up_to_date,
 				     int ours, int theirs,
 				     const char *branch_name,
+				     const char *push_remote_name,
+				     const char *push_branch_name,
 				     enum ahead_behind_flags abf,
 				     unsigned flags)
 {

@@ -2302,9 +2304,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			branch_name, theirs);
-		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" to update your local branch)\n"));
+		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS)) {
+			if (push_remote_name && push_branch_name)
+				strbuf_addf(sb,
+					_("  (use \"git pull %s %s\" to update your local branch)\n"),
+					push_remote_name, push_branch_name);
+			else
+				strbuf_addstr(sb,
+					_("  (use \"git pull\" to update your local branch)\n"));
+		}
 	} else {
 		strbuf_addf(sb,
 			Q_("Your branch and '%s' have diverged,\n"

@@ -2315,9 +2323,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			branch_name, ours, theirs);
-		if (use_divergence_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" if you want to integrate the remote branch with yours)\n"));
+		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"));
+		}
 	}
 }
 

@@ -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,24 @@ 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)
+			if (show_divergence_advice)
+				flags |= ENABLE_ADVICE_DIVERGENCE;
+		}
+		if (is_push) {
 			flags |= ENABLE_ADVICE_PUSH;
-		if (show_divergence_advice && is_upstream)
-			flags |= ENABLE_ADVICE_DIVERGENCE;
+			push_remote_name = pushremote_for_branch(branch, NULL);
+			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;
+		}
 		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
+					 push_remote_name, push_branch_name,
 					 abf, flags);
 		reported = 1;
 

base-commit: 29bd7ed5127255713c1ac2f43b7c6f257d7b4594

gitgitgadget

From: Harald Nordgren <[email protected]>

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.

Also enable the pull advice for push-branch comparisons; it was previously only set for the upstream.

---
remote: qualify "git pull" advice for non-upstream branches

Remove the message only when diverged from push-branch, because it was in the way when I used it now on my real branch.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2301%2FHaraldNordgren%2Fstatus-pull-advice-qualified-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2301/HaraldNordgren/status-pull-advice-qualified-v2
Pull-Request: https://github.com/git/git/pull/2301

Range-diff vs v1:

1: 0a06883cc8 ! 1: 1f06873f82 remote: qualify "git pull" advice for non-upstream branches
@@ Metadata
## Commit message ##
remote: qualify "git pull" advice for non-upstream branches

- When "git status" reports the local branch is behind or has diverged
- from the push branch, the advice suggested a bare "git pull". That
- follows the upstream, which may live on a different remote, so emit
+ 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.

- Also enable the pull and divergence advice for push-branch
- comparisons; they were previously only set for the upstream.
+ Also enable the pull advice for push-branch comparisons; it was
+ previously only set for the upstream.

@@ remote.c: int format_tracking_info(struct branch *branch, struct strbuf *sb,
strbuf_addstr(sb, "\n");

- if (is_upstream)
-+ if (is_upstream || is_push) {
++ if (is_upstream || is_push)
flags |= ENABLE_ADVICE_PULL;
- if (is_push)
-+ if (show_divergence_advice)
-+ flags |= ENABLE_ADVICE_DIVERGENCE;
-+ }
+- flags |= ENABLE_ADVICE_PUSH;
+ if (show_divergence_advice && is_upstream)
+ flags |= ENABLE_ADVICE_DIVERGENCE;
+ if (is_push) {
- flags |= ENABLE_ADVICE_PUSH;
-- if (show_divergence_advice && is_upstream)
-- flags |= ENABLE_ADVICE_DIVERGENCE;
++ flags |= ENABLE_ADVICE_PUSH;
+ push_remote_name = pushremote_for_branch(branch, NULL);
+ if (push_remote_name &&
+ skip_prefix(full_ref, "refs/remotes/", &push_branch_name) &&
@@ remote.c: int format_tracking_info(struct branch *branch, struct strbuf *sb,

## t/t6040-tracking-info.sh ##
-@@ t/t6040-tracking-info.sh: test_expect_success 'status.compareBranches with diverged push branch' ' -
- Your branch and ${SQ}origin/feature8${SQ} have diverged,
- and have 1 and 1 different commits each, respectively.
-+ (use "git pull origin feature8" if you want to integrate the remote branch with yours)
-
- nothing to commit, working tree clean
- EOF
@@ t/t6040-tracking-info.sh: test_expect_success 'status.compareBranches with remapped push and upstream remo
test_cmp expect actual
'

 remote.c                 | 44 ++++++++++++++++++++++++++++++++--------
 t/t6040-tracking-info.sh | 40 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 9 deletions(-)

index a664cd166a..e096fdb674 100644
@@ -2267,6 +2267,8 @@ static void format_branch_comparison(struct strbuf *sb,
 				     bool up_to_date,
 				     int ours, int theirs,
 				     const char *branch_name,
+				     const char *push_remote_name,
+				     const char *push_branch_name,
 				     enum ahead_behind_flags abf,
 				     unsigned flags)
 {

@@ -2302,9 +2304,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			branch_name, theirs);
-		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" to update your local branch)\n"));
+		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS)) {
+			if (push_remote_name && push_branch_name)
+				strbuf_addf(sb,
+					_("  (use \"git pull %s %s\" to update your local branch)\n"),
+					push_remote_name, push_branch_name);
+			else
+				strbuf_addstr(sb,
+					_("  (use \"git pull\" to update your local branch)\n"));
+		}
 	} else {
 		strbuf_addf(sb,
 			Q_("Your branch and '%s' have diverged,\n"

@@ -2315,9 +2323,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			branch_name, ours, theirs);
-		if (use_divergence_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" if you want to integrate the remote branch with yours)\n"));
+		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"));
+		}
 	}
 }
 

@@ -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,23 @@ 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;
+			push_remote_name = pushremote_for_branch(branch, NULL);
+			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;
+		}
 		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
+					 push_remote_name, push_branch_name,
 					 abf, flags);
 		reported = 1;
 

index 0242b5bf7a..b686bf356a 100755
@@ -646,4 +646,44 @@ test_expect_success 'status.compareBranches with remapped push and upstream remo
 	test_cmp expect actual
 '
 
+test_expect_success 'status.compareBranches with behind push branch suggests qualified pull' '
+	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 reset --hard HEAD^ &&
+	git -C test status >actual &&
+	cat >expect <<-EOF &&
+	On branch feature13
+	Your branch is up to date with ${SQ}upstream/main${SQ}.
+
+	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_done

base-commit: 59ff4886a579f4bc91e976fe18590b9ae02c7a08

gitgitgadget

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

From: Harald Nordgren <[email protected]>

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.

---
remote: qualify "git pull" advice for non-upstream branches

* Only suggest git pull <remote> <branch> when plain git pull wouldn't do the right thing.
* Tests: when upstream and push are the same ref, the message stays plain git pull.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2301%2FHaraldNordgren%2Fstatus-pull-advice-qualified-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2301/HaraldNordgren/status-pull-advice-qualified-v3
Pull-Request: https://github.com/git/git/pull/2301

Range-diff vs v2:

1: 1f06873f82 ! 1: 3703be9aac remote: qualify "git pull" advice for non-upstream branches

     @@ Metadata
      Author: Harald Nordgren <[email protected]>
      
       ## Commit message ##
     -    remote: qualify "git pull" advice for non-upstream branches
     +    remote: qualify "git pull" advice for non-upstream compareBranches
      
     -    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.
     +    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.
      
     -    Also enable the pull advice for push-branch comparisons; it was
     -    previously only set for the upstream.
     +    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 <[email protected]>
      
     @@ remote.c: int format_tracking_info(struct branch *branch, struct strbuf *sb,
       			flags |= ENABLE_ADVICE_DIVERGENCE;
      +		if (is_push) {
      +			flags |= ENABLE_ADVICE_PUSH;
     -+			push_remote_name = pushremote_for_branch(branch, NULL);
     -+			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;
     ++			if (!upstream_ref || strcmp(upstream_ref, full_ref)) {
     ++				push_remote_name = pushremote_for_branch(branch, NULL);
     ++				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;
     ++			}
      +		}
       		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
      +					 push_remote_name, push_branch_name,
     @@ t/t6040-tracking-info.sh: test_expect_success 'status.compareBranches with remap
       	test_cmp expect actual
       '
       
     -+test_expect_success 'status.compareBranches with behind push branch suggests qualified pull' '
     ++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 up to date with ${SQ}upstream/main${SQ}.
     ++	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)
     @@ t/t6040-tracking-info.sh: test_expect_success 'status.compareBranches with remap
      +	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

 remote.c                 | 46 +++++++++++++++++++-----
 t/t6040-tracking-info.sh | 78 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 9 deletions(-)

index a664cd166a..2b82f6b312 100644
@@ -2267,6 +2267,8 @@ static void format_branch_comparison(struct strbuf *sb,
 				     bool up_to_date,
 				     int ours, int theirs,
 				     const char *branch_name,
+				     const char *push_remote_name,
+				     const char *push_branch_name,
 				     enum ahead_behind_flags abf,
 				     unsigned flags)
 {

@@ -2302,9 +2304,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			branch_name, theirs);
-		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" to update your local branch)\n"));
+		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS)) {
+			if (push_remote_name && push_branch_name)
+				strbuf_addf(sb,
+					_("  (use \"git pull %s %s\" to update your local branch)\n"),
+					push_remote_name, push_branch_name);
+			else
+				strbuf_addstr(sb,
+					_("  (use \"git pull\" to update your local branch)\n"));
+		}
 	} else {
 		strbuf_addf(sb,
 			Q_("Your branch and '%s' have diverged,\n"

@@ -2315,9 +2323,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			branch_name, ours, theirs);
-		if (use_divergence_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" if you want to integrate the remote branch with yours)\n"));
+		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"));
+		}
 	}
 }
 

@@ -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);
+				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;
+			}
+		}
 		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
+					 push_remote_name, push_branch_name,
 					 abf, flags);
 		reported = 1;
 

base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf

gitgitgadget

From: Harald Nordgren <[email protected]>

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.

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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2301%2FHaraldNordgren%2Fstatus-pull-advice-qualified-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2301/HaraldNordgren/status-pull-advice-qualified-v4
Pull-Request: https://github.com/git/git/pull/2301

Range-diff vs v3:

1: 3703be9aac ! 1: ef54dacb07 remote: qualify "git pull" advice for non-upstream compareBranches

     @@ remote.c: int format_tracking_info(struct branch *branch, struct strbuf *sb,
       		full_ref = resolve_compare_branch(branch,
       						  branches.items[i].string);
      @@ remote.c: int format_tracking_info(struct branch *branch, struct strbuf *sb,
     - 		if (reported)
     - 			strbuf_addstr(sb, "\n");
       
     --		if (is_upstream)
     -+		if (is_upstream || is_push)
     + 		if (is_upstream)
       			flags |= ENABLE_ADVICE_PULL;
      -		if (is_push)
      -			flags |= ENABLE_ADVICE_PUSH;
     @@ remote.c: int format_tracking_info(struct branch *branch, struct strbuf *sb,
      +				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 == '/') {
      +					push_branch_name++;
     -+				else
     ++					flags |= ENABLE_ADVICE_PULL;
     ++				} else {
      +					push_remote_name = NULL;
     ++				}
     ++			} else {
     ++				flags |= ENABLE_ADVICE_PULL;
      +			}
      +		}
       		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
     @@ t/t6040-tracking-info.sh: test_expect_success 'status.compareBranches with remap
      +	EOF
      +	test_cmp expect actual
      +'
     ++
     ++test_expect_success 'status.compareBranches suppresses advice when push tracking ref is unconventional' '
     ++	test_config -C test push.default current &&
     ++	test_config -C test remote.imported.url ../. &&
     ++	test_config -C test remote.imported.fetch "+refs/heads/*:refs/imported/imported/*" &&
     ++	test_config -C test branch.feature17.pushRemote imported &&
     ++	test_config -C test status.compareBranches "@{push}" &&
     ++	git -C test fetch imported &&
     ++	git -C test checkout --no-track -b feature17 refs/imported/imported/main &&
     ++	(cd test && advance work17) &&
     ++	git -C test push imported HEAD:feature17 &&
     ++	git -C test fetch imported &&
     ++	git -C test reset --hard HEAD^ &&
     ++	git -C test status >actual &&
     ++	cat >expect <<-EOF &&
     ++	On branch feature17
     ++	Your branch is behind ${SQ}imported/imported/feature17${SQ} by 1 commit, and can be fast-forwarded.
     ++
     ++	nothing to commit, working tree clean
     ++	EOF
     ++	test_cmp expect actual
     ++'
      +
       test_done

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

index 24a8118d25..193e1dd1f1 100644
@@ -2268,6 +2268,8 @@ static void format_branch_comparison(struct strbuf *sb,
 				     bool up_to_date,
 				     int ours, int theirs,
 				     const char *branch_name,
+				     const char *push_remote_name,
+				     const char *push_branch_name,
 				     enum ahead_behind_flags abf,
 				     unsigned flags)
 {

@@ -2303,9 +2305,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "and can be fast-forwarded.\n",
 			   theirs),
 			branch_name, theirs);
-		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" to update your local branch)\n"));
+		if (use_pull_advice && advice_enabled(ADVICE_STATUS_HINTS)) {
+			if (push_remote_name && push_branch_name)
+				strbuf_addf(sb,
+					_("  (use \"git pull %s %s\" to update your local branch)\n"),
+					push_remote_name, push_branch_name);
+			else
+				strbuf_addstr(sb,
+					_("  (use \"git pull\" to update your local branch)\n"));
+		}
 	} else {
 		strbuf_addf(sb,
 			Q_("Your branch and '%s' have diverged,\n"

@@ -2316,9 +2324,15 @@ static void format_branch_comparison(struct strbuf *sb,
 			       "respectively.\n",
 			   ours + theirs),
 			branch_name, ours, theirs);
-		if (use_divergence_advice && advice_enabled(ADVICE_STATUS_HINTS))
-			strbuf_addstr(sb,
-				_("  (use \"git pull\" if you want to integrate the remote branch with yours)\n"));
+		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"));
+		}
 	}
 }
 

@@ -2356,6 +2370,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);

@@ -2399,11 +2415,27 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb,
 
 		if (is_upstream)
 			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);
+				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++;
+					flags |= ENABLE_ADVICE_PULL;
+				} else {
+					push_remote_name = NULL;
+				}
+			} else {
+				flags |= ENABLE_ADVICE_PULL;
+			}
+		}
 		format_branch_comparison(sb, !cmp, ours, theirs, short_ref,
+					 push_remote_name, push_branch_name,
 					 abf, flags);
 		reported = 1;
 

base-commit: aec3f587505a472db67e9462d0702e7d463a449d

gitgitgadget

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

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.

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

Yes please, thanks for all the help!

Harald