[PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Christopher Tiwald]

Suppose a user configured a local branch to track an upstream branch by
a different name or didn't set an upstream branch at all. In these
cases, issuing 'git pull' without specifying a remote repository or
refspec can be dangerous. In the first case, 'git pull --rebase' could
rewrite published history. In the second, 'git pull' without argument
will fail.

Modify 'git push's non-fast-forward advice to account for these cases.
Instruct users who push a non-fast-forward update to their current
branch to 'git pull <repository> <refspec>' when the branch is untracked
or tracks to a different repo or refspec then the one they specified.
Otherwise, instruct users to 'git pull'. Make both types of advice
configurable, so that users who disable one won't disable the other on
accident. Finally, offer users who configure a branch for octopus
merges, i.e. where 'branch->merge_nr > 1', the simple 'git pull' advice.

Signed-off-by: Christopher Tiwald <christiwald@gmail.com>
---
Sent this out a while back [1] but I think it went unnoticed in the
'push default' discussion. I wanted to wait until ct/advise-push-default
hit master before resending.

This patch clarifies the 'git pull' advice offered when a push to the
current branch fails for a non-fast-forward error. I think the patch is
reasonable up to the 'push default' change, possibly longer, but I'm not
totally happy with "pushNonFFCurrentUntracked" and "pushNonFFCurrentTracked".
I think they're both too long as variables and prohibit usability. I'm at
a loss for shorter names that convey as much information.

[1] http://thread.gmane.org/gmane.comp.version-control.git/194175/focus=195142
--
Christopher Tiwald



 Documentation/config.txt |    9 +++++++--
 advice.c                 |    6 ++++--
 advice.h                 |    3 ++-
 builtin/push.c           |   48 +++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fb386ab..fd72120 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -141,9 +141,14 @@ advice.*::
 		Set this variable to 'false' if you want to disable
 		'pushNonFFCurrent', 'pushNonFFDefault', and
 		'pushNonFFMatching' simultaneously.
-	pushNonFFCurrent::
+	pushNonFFCurrentUntracked::
 		Advice shown when linkgit:git-push[1] fails due to a
-		non-fast-forward update to the current branch.
+		non-fast-forward update to the current branch and that
+		branch doesn't match the tracked remote and refspec.
+	pushNonFFCurrentTracked::
+		Advice shown when linkgit:git-push[1] fails due to a
+		non-fast-forward update to the current branch and that
+		branch matches the tracked remote and refspec.
 	pushNonFFDefault::
 		Advice to set 'push.default' to 'upstream' or 'current'
 		when you ran linkgit:git-push[1] and pushed 'matching
diff --git a/advice.c b/advice.c
index a492eea..828a41b 100644
--- a/advice.c
+++ b/advice.c
@@ -1,7 +1,8 @@
 #include "cache.h"
 
 int advice_push_nonfastforward = 1;
-int advice_push_non_ff_current = 1;
+int advice_push_non_ff_current_untracked = 1;
+int advice_push_non_ff_current_tracked = 1;
 int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
 int advice_status_hints = 1;
@@ -15,7 +16,8 @@ static struct {
 	int *preference;
 } advice_config[] = {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
-	{ "pushnonffcurrent", &advice_push_non_ff_current },
+	{ "pushnonffcurrentuntracked", &advice_push_non_ff_current_untracked },
+	{ "pushnonffcurrenttracked", &advice_push_non_ff_current_tracked },
 	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
 	{ "statushints", &advice_status_hints },
diff --git a/advice.h b/advice.h
index f3cdbbf..c18809f 100644
--- a/advice.h
+++ b/advice.h
@@ -4,7 +4,8 @@
 #include "git-compat-util.h"
 
 extern int advice_push_nonfastforward;
-extern int advice_push_non_ff_current;
+extern int advice_push_non_ff_current_untracked;
+extern int advice_push_non_ff_current_tracked;
 extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
 extern int advice_status_hints;
diff --git a/builtin/push.c b/builtin/push.c
index 6936713..e6614e9 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -134,12 +134,18 @@ static void setup_default_push_refspecs(struct remote *remote)
 	}
 }
 
-static const char message_advice_pull_before_push[] =
+static const char message_advice_tracked_pull_before_push[] =
 	N_("Updates were rejected because the tip of your current branch is behind\n"
 	   "its remote counterpart. Merge the remote changes (e.g. 'git pull')\n"
 	   "before pushing again.\n"
 	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
 
+static const char message_advice_untracked_pull_before_push[] =
+	N_("Updates were rejected because the tip of your current branch is behind\n"
+	   "its remote counterpart. Merge the remote changes to your local branch\n"
+	   "(e.g. 'git pull <repository> <refspec>') before pushing again.\n"
+	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
+
 static const char message_advice_use_upstream[] =
 	N_("Updates were rejected because a pushed branch tip is behind its remote\n"
 	   "counterpart. If you did not intend to push that branch, you may want to\n"
@@ -152,11 +158,20 @@ static const char message_advice_checkout_pull_push[] =
 	   "(e.g. 'git pull') before pushing again.\n"
 	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
 
-static void advise_pull_before_push(void)
+static void advise_tracked_pull_before_push(void)
+{
+	if (!advice_push_non_ff_current_tracked ||
+	    !advice_push_nonfastforward)
+		return;
+	advise(_(message_advice_tracked_pull_before_push));
+}
+
+static void advise_untracked_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current || !advice_push_nonfastforward)
+	if (!advice_push_non_ff_current_untracked ||
+	    !advice_push_nonfastforward)
 		return;
-	advise(_(message_advice_pull_before_push));
+	advise(_(message_advice_untracked_pull_before_push));
 }
 
 static void advise_use_upstream(void)
@@ -177,6 +192,16 @@ static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
 	int nonfastforward;
+	struct branch *branch;
+	struct strbuf buf = STRBUF_INIT;
+
+	branch = branch_get(NULL);
+
+	if (branch) {
+		strbuf_addstr(&buf, transport->remote->name);
+		strbuf_addstr(&buf, "/");
+		strbuf_addstr(&buf, branch->name);
+	}
 
 	transport_set_verbosity(transport, verbosity, progress);
 
@@ -201,7 +226,18 @@ static int push_with_options(struct transport *transport, int flags)
 	default:
 		break;
 	case NON_FF_HEAD:
-		advise_pull_before_push();
+		/* Branches configured for octopus merges should advise
+		 * just 'git pull' */
+		if (branch->remote_name &&
+		    branch->merge &&
+		    branch->merge_nr == 1 &&
+		    !strcmp(transport->remote->name, branch->remote_name) &&
+		    !strcmp(strbuf_detach(&buf, NULL),
+			    prettify_refname(branch->merge[0]->dst))) {
+			advise_tracked_pull_before_push();
+		}
+		else
+			advise_untracked_pull_before_push();
 		break;
 	case NON_FF_OTHER:
 		if (default_matching_used)
@@ -211,6 +247,8 @@ static int push_with_options(struct transport *transport, int flags)
 		break;
 	}
 
+	strbuf_release(&buf);
+
 	return 1;
 }
 
-- 
1.7.10.228.g7061d

Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Junio C Hamano]

Christopher Tiwald <christiwald@gmail.com> writes:

> Suppose a user configured a local branch to track an upstream branch by
> a different name or didn't set an upstream branch at all. In these
> cases, issuing 'git pull' without specifying a remote repository or
> refspec can be dangerous. In the first case, 'git pull --rebase' could
> rewrite published history. In the second, 'git pull' without argument
> will fail.

The latter case of stopping without causing damage is hardly dangerous,
so I'll ignore that for now, but I am not sure what you mean by the
former.  A "devel" branch has "master" from "origin" (or whatever
branch.devel.remote is set) as its upstream (i.e. "devel" and "master"
are different strings).  "git pull" will then fetch "master" from the
other side, and either merge that into "devel" or rebuild "devel" on top
of it if you gave "--rebase".

But if you used "master", not "devel", as the name of your local branch,
I do not see anything changes.  You may have published the tip of
"master" to a third repository before doing "pull --rebase", and you may
be rebasing the history leading to that commit.  Even if there is no
third repository, your "master" may be pushed to some other branch of
"origin", so the story is the same.  If your counter-argument is "but
but but I will never ever push my 'master' to names other than 'master'
at 'origin'", then in your original settings where your local branch is
called "devel", you will never ever push you 'devel' to branches other
than 'master' at 'origin', exactly because its upstream is set to
'master'.

So what makes it dangerous is the use of "--rebase", if anything, isn't
it?  It does not seem to have much to do with how the local branches are
named.

Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Junio C Hamano]

Christopher Tiwald <christiwald@gmail.com> writes:

> @@ -177,6 +192,16 @@ static int push_with_options(struct transport *transport, int flags)
>  {
>  	int err;
>  	int nonfastforward;
> +	struct branch *branch;
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	branch = branch_get(NULL);
> +
> +	if (branch) {
> +		strbuf_addstr(&buf, transport->remote->name);
> +		strbuf_addstr(&buf, "/");
> +		strbuf_addstr(&buf, branch->name);
> +	}

The "buf" is a horrible name for a variable that is used to hold states
in a long haul that has to span multiple hunks in a patch.  Please name
it after what the value means.

> @@ -201,7 +226,18 @@ static int push_with_options(struct transport *transport, int flags)
>  	default:
>  		break;
>  	case NON_FF_HEAD:
> -		advise_pull_before_push();
> +		/* Branches configured for octopus merges should advise
> +		 * just 'git pull' */
> +		if (branch->remote_name &&
> +		    branch->merge &&
> +		    branch->merge_nr == 1 &&
> +		    !strcmp(transport->remote->name, branch->remote_name) &&
> +		    !strcmp(strbuf_detach(&buf, NULL),
> +			    prettify_refname(branch->merge[0]->dst))) {

Why detach?  buf_to_be_renamed_more_sanely.buf, perhaps?

Is comparison between whatever buf has and the result of prettify safe
and sane?  After all, prettify is a random abbreviation that is meant
for human consumption, assuming that the reader is intelligent enough to
guess "Ah, it must be a tag" when she sees "v1.0" which is the result of
stripping the leading "refs/tags/" out.

This part should be using straight strcmp against branch->merge[0]->dst,
which means buf_to_be_renamed_more_sanely in the earlier hunk needs to
be computing what's tracked and merged correctly using the configured
refspec mapping, perhaps?

Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Christopher Tiwald]

On Mon, Apr 23, 2012 at 07:17:25PM -0700, Junio C Hamano wrote:
> So what makes it dangerous is the use of "--rebase", if anything, isn't
> it?  It does not seem to have much to do with how the local branches are
> named.

After thinking about this argument, there might be a deeper problem with
my reasoning. Take the workflow you describe. In the "devel tracks to origin
master" workflow, this patch would advise 'git pull <repository> <refspec>'.
The advice misses the point of setting the upstream branch. Worse, the
advice is broken if the user issues 'git pull origin devel' and no 'devel'
branch exists on origin or the 'devel' branch is simply out of date (as
might occur if the user pushes between a personal remote clone of a
shared repo and the shared repo itself with different frequency).

Maybe the solution here is to ditch the $dest_ref and $dest_remote
matching entirely and just touch the one case I _know_ the advice could
do better: git should advise 'git pull <repo> <refspec>' or something
like "consider setting an upstream branch and pulling before pushing
again" when branch->merge doesn't exist at all. I like the former
because it's simpler as an end user and doesn't require enforcing a
setting he or she may not understand.

I think that might be the way to go. I approached this from a specific
workflow assumption. In retrospect, I can't divine the motivation of
merge configurations well enough to avoid bad advice.

--
Christopher Tiwald

Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Matthieu Moy]

Christopher Tiwald <christiwald@gmail.com> writes:

> +		/* Branches configured for octopus merges should advise
> +		 * just 'git pull' */

Style: Git usually writes these comments 

/*
 * like this
 */

> +		if (branch->remote_name &&
> +		    branch->merge &&
> +		    branch->merge_nr == 1 &&
> +		    !strcmp(transport->remote->name, branch->remote_name) &&
> +		    !strcmp(strbuf_detach(&buf, NULL),
> +			    prettify_refname(branch->merge[0]->dst))) {
> +			advise_tracked_pull_before_push();
> +		}
> +		else
> +			advise_untracked_pull_before_push();

Isn't this doing the opposite of what the comment is saying about
octopus merge, i.e. if branch->merge_nr > 1, call
advise_untracked_pull_before_push() which will advise 'git pull <remote>
<branch>'?

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Christopher Tiwald]

On Tue, Apr 24, 2012 at 09:04:07AM +0200, Matthieu Moy wrote:
> > +		if (branch->remote_name &&
> > +		    branch->merge &&
> > +		    branch->merge_nr == 1 &&
> > +		    !strcmp(transport->remote->name, branch->remote_name) &&
> > +		    !strcmp(strbuf_detach(&buf, NULL),
> > +			    prettify_refname(branch->merge[0]->dst))) {
> > +			advise_tracked_pull_before_push();
> > +		}
> > +		else
> > +			advise_untracked_pull_before_push();
> 
> Isn't this doing the opposite of what the comment is saying about
> octopus merge, i.e. if branch->merge_nr > 1, call
> advise_untracked_pull_before_push() which will advise 'git pull <remote>
> <branch>'?

Ah yes. The logic is wrong for the octopus case. That's easy to fix, but
I'm considering ditching the matching entirely per my response to
Junio's concerns. I think it might do more harm then good.

--
Christopher Tiwald

Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

[Junio C Hamano]

Christopher Tiwald <christiwald@gmail.com> writes:

> I think that might be the way to go. I approached this from a specific
> workflow assumption. In retrospect, I can't divine the motivation of
> merge configurations well enough to avoid bad advice.

I am very sympathetic to your underlying motivation to avoid telling
them to "perform a git pull to integrate the history from the other side
before you push" and then getting misunderstood as if you told them to
LITERALLY type "git pull<RETURN>".  Depending on how the branch the user
wanted to push, the approach needed to update its history so that
contains the history from the other side will be different, and you need
to have everything configured correctly for your case to be able to type
"git pull<RETURN>" literally and get the right result.  If you were trying
to push one-shot into somewhere you do not usually push to, it is very
likely that you would need to say "git pull $there $that", and there is
no canned "Type this LITERALLY to continue" recipe that is appropriate
in the advice message.

Perhaps a safer way out is to phrase the advice message in such a way
that it is crystal clear to anybody halfway intelligent that there is
nothing the user can cut and paste from there?