{"thread":{"id":"30313","subject":"[PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","startedAt":"2012-04-23T22:45:21Z","lastAt":"2012-04-24T19:06:48Z","messageCount":7,"participants":["Christopher Tiwald","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"189918","messageId":"1335221121-36664-1-git-send-email-christiwald@gmail.com","threadId":"30313","inReplyTo":null,"subject":"[PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Christopher Tiwald","fromEmail":"christiwald@gmail.com","sentAt":"2012-04-23T22:45:21Z","receivedAt":"2012-04-23T22:45:21Z","isPatch":true,"sender":{"key":"christiwald@gmail.com","avatar":"https://avatars.githubusercontent.com/u/667276?v=4"},"body":"Suppose a user configured a local branch to track an upstream branch by\na different name or didn't set an upstream branch at all. In these\ncases, issuing 'git pull' without specifying a remote repository or\nrefspec can be dangerous. In the first case, 'git pull --rebase' could\nrewrite published history. In the second, 'git pull' without argument\nwill fail.\n\nModify 'git push's non-fast-forward advice to account for these cases.\nInstruct users who push a non-fast-forward update to their current\nbranch to 'git pull <repository> <refspec>' when the branch is untracked\nor tracks to a different repo or refspec then the one they specified.\nOtherwise, instruct users to 'git pull'. Make both types of advice\nconfigurable, so that users who disable one won't disable the other on\naccident. Finally, offer users who configure a branch for octopus\nmerges, i.e. where 'branch->merge_nr > 1', the simple 'git pull' advice.\n\nSigned-off-by: Christopher Tiwald <christiwald@gmail.com>\n---\nSent this out a while back [1] but I think it went unnoticed in the\n'push default' discussion. I wanted to wait until ct/advise-push-default\nhit master before resending.\n\nThis patch clarifies the 'git pull' advice offered when a push to the\ncurrent branch fails for a non-fast-forward error. I think the patch is\nreasonable up to the 'push default' change, possibly longer, but I'm not\ntotally happy with \"pushNonFFCurrentUntracked\" and \"pushNonFFCurrentTracked\".\nI think they're both too long as variables and prohibit usability. I'm at\na loss for shorter names that convey as much information.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/194175/focus=195142\n--\nChristopher Tiwald\n\n\n\n Documentation/config.txt |    9 +++++++--\n advice.c                 |    6 ++++--\n advice.h                 |    3 ++-\n builtin/push.c           |   48 +++++++++++++++++++++++++++++++++++++++++-----\n 4 files changed, 56 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex fb386ab..fd72120 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -141,9 +141,14 @@ advice.*::\n \t\tSet this variable to 'false' if you want to disable\n \t\t'pushNonFFCurrent', 'pushNonFFDefault', and\n \t\t'pushNonFFMatching' simultaneously.\n-\tpushNonFFCurrent::\n+\tpushNonFFCurrentUntracked::\n \t\tAdvice shown when linkgit:git-push[1] fails due to a\n-\t\tnon-fast-forward update to the current branch.\n+\t\tnon-fast-forward update to the current branch and that\n+\t\tbranch doesn't match the tracked remote and refspec.\n+\tpushNonFFCurrentTracked::\n+\t\tAdvice shown when linkgit:git-push[1] fails due to a\n+\t\tnon-fast-forward update to the current branch and that\n+\t\tbranch matches the tracked remote and refspec.\n \tpushNonFFDefault::\n \t\tAdvice to set 'push.default' to 'upstream' or 'current'\n \t\twhen you ran linkgit:git-push[1] and pushed 'matching\ndiff --git a/advice.c b/advice.c\nindex a492eea..828a41b 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -1,7 +1,8 @@\n #include \"cache.h\"\n \n int advice_push_nonfastforward = 1;\n-int advice_push_non_ff_current = 1;\n+int advice_push_non_ff_current_untracked = 1;\n+int advice_push_non_ff_current_tracked = 1;\n int advice_push_non_ff_default = 1;\n int advice_push_non_ff_matching = 1;\n int advice_status_hints = 1;\n@@ -15,7 +16,8 @@ static struct {\n \tint *preference;\n } advice_config[] = {\n \t{ \"pushnonfastforward\", &advice_push_nonfastforward },\n-\t{ \"pushnonffcurrent\", &advice_push_non_ff_current },\n+\t{ \"pushnonffcurrentuntracked\", &advice_push_non_ff_current_untracked },\n+\t{ \"pushnonffcurrenttracked\", &advice_push_non_ff_current_tracked },\n \t{ \"pushnonffdefault\", &advice_push_non_ff_default },\n \t{ \"pushnonffmatching\", &advice_push_non_ff_matching },\n \t{ \"statushints\", &advice_status_hints },\ndiff --git a/advice.h b/advice.h\nindex f3cdbbf..c18809f 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -4,7 +4,8 @@\n #include \"git-compat-util.h\"\n \n extern int advice_push_nonfastforward;\n-extern int advice_push_non_ff_current;\n+extern int advice_push_non_ff_current_untracked;\n+extern int advice_push_non_ff_current_tracked;\n extern int advice_push_non_ff_default;\n extern int advice_push_non_ff_matching;\n extern int advice_status_hints;\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 6936713..e6614e9 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -134,12 +134,18 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t}\n }\n \n-static const char message_advice_pull_before_push[] =\n+static const char message_advice_tracked_pull_before_push[] =\n \tN_(\"Updates were rejected because the tip of your current branch is behind\\n\"\n \t   \"its remote counterpart. Merge the remote changes (e.g. 'git pull')\\n\"\n \t   \"before pushing again.\\n\"\n \t   \"See the 'Note about fast-forwards' in 'git push --help' for details.\");\n \n+static const char message_advice_untracked_pull_before_push[] =\n+\tN_(\"Updates were rejected because the tip of your current branch is behind\\n\"\n+\t   \"its remote counterpart. Merge the remote changes to your local branch\\n\"\n+\t   \"(e.g. 'git pull <repository> <refspec>') before pushing again.\\n\"\n+\t   \"See the 'Note about fast-forwards' in 'git push --help' for details.\");\n+\n static const char message_advice_use_upstream[] =\n \tN_(\"Updates were rejected because a pushed branch tip is behind its remote\\n\"\n \t   \"counterpart. If you did not intend to push that branch, you may want to\\n\"\n@@ -152,11 +158,20 @@ static const char message_advice_checkout_pull_push[] =\n \t   \"(e.g. 'git pull') before pushing again.\\n\"\n \t   \"See the 'Note about fast-forwards' in 'git push --help' for details.\");\n \n-static void advise_pull_before_push(void)\n+static void advise_tracked_pull_before_push(void)\n+{\n+\tif (!advice_push_non_ff_current_tracked ||\n+\t    !advice_push_nonfastforward)\n+\t\treturn;\n+\tadvise(_(message_advice_tracked_pull_before_push));\n+}\n+\n+static void advise_untracked_pull_before_push(void)\n {\n-\tif (!advice_push_non_ff_current || !advice_push_nonfastforward)\n+\tif (!advice_push_non_ff_current_untracked ||\n+\t    !advice_push_nonfastforward)\n \t\treturn;\n-\tadvise(_(message_advice_pull_before_push));\n+\tadvise(_(message_advice_untracked_pull_before_push));\n }\n \n static void advise_use_upstream(void)\n@@ -177,6 +192,16 @@ static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n \tint nonfastforward;\n+\tstruct branch *branch;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tbranch = branch_get(NULL);\n+\n+\tif (branch) {\n+\t\tstrbuf_addstr(&buf, transport->remote->name);\n+\t\tstrbuf_addstr(&buf, \"/\");\n+\t\tstrbuf_addstr(&buf, branch->name);\n+\t}\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n \n@@ -201,7 +226,18 @@ static int push_with_options(struct transport *transport, int flags)\n \tdefault:\n \t\tbreak;\n \tcase NON_FF_HEAD:\n-\t\tadvise_pull_before_push();\n+\t\t/* Branches configured for octopus merges should advise\n+\t\t * just 'git pull' */\n+\t\tif (branch->remote_name &&\n+\t\t    branch->merge &&\n+\t\t    branch->merge_nr == 1 &&\n+\t\t    !strcmp(transport->remote->name, branch->remote_name) &&\n+\t\t    !strcmp(strbuf_detach(&buf, NULL),\n+\t\t\t    prettify_refname(branch->merge[0]->dst))) {\n+\t\t\tadvise_tracked_pull_before_push();\n+\t\t}\n+\t\telse\n+\t\t\tadvise_untracked_pull_before_push();\n \t\tbreak;\n \tcase NON_FF_OTHER:\n \t\tif (default_matching_used)\n@@ -211,6 +247,8 @@ static int push_with_options(struct transport *transport, int flags)\n \t\tbreak;\n \t}\n \n+\tstrbuf_release(&buf);\n+\n \treturn 1;\n }\n \n-- \n1.7.10.228.g7061d\n"},{"id":"189926","messageId":"xmqqvckpho0a.fsf@junio.mtv.corp.google.com","threadId":"30313","inReplyTo":"1335221121-36664-1-git-send-email-christiwald@gmail.com","subject":"Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-24T02:17:25Z","receivedAt":"2012-04-24T02:17:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christopher Tiwald <christiwald@gmail.com> writes:\n\n> Suppose a user configured a local branch to track an upstream branch by\n> a different name or didn't set an upstream branch at all. In these\n> cases, issuing 'git pull' without specifying a remote repository or\n> refspec can be dangerous. In the first case, 'git pull --rebase' could\n> rewrite published history. In the second, 'git pull' without argument\n> will fail.\n\nThe latter case of stopping without causing damage is hardly dangerous,\nso I'll ignore that for now, but I am not sure what you mean by the\nformer.  A \"devel\" branch has \"master\" from \"origin\" (or whatever\nbranch.devel.remote is set) as its upstream (i.e. \"devel\" and \"master\"\nare different strings).  \"git pull\" will then fetch \"master\" from the\nother side, and either merge that into \"devel\" or rebuild \"devel\" on top\nof it if you gave \"--rebase\".\n\nBut if you used \"master\", not \"devel\", as the name of your local branch,\nI do not see anything changes.  You may have published the tip of\n\"master\" to a third repository before doing \"pull --rebase\", and you may\nbe rebasing the history leading to that commit.  Even if there is no\nthird repository, your \"master\" may be pushed to some other branch of\n\"origin\", so the story is the same.  If your counter-argument is \"but\nbut but I will never ever push my 'master' to names other than 'master'\nat 'origin'\", then in your original settings where your local branch is\ncalled \"devel\", you will never ever push you 'devel' to branches other\nthan 'master' at 'origin', exactly because its upstream is set to\n'master'.\n\nSo what makes it dangerous is the use of \"--rebase\", if anything, isn't\nit?  It does not seem to have much to do with how the local branches are\nnamed.\n"},{"id":"189929","messageId":"xmqqr4vdhnfh.fsf@junio.mtv.corp.google.com","threadId":"30313","inReplyTo":"1335221121-36664-1-git-send-email-christiwald@gmail.com","subject":"Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-24T02:29:54Z","receivedAt":"2012-04-24T02:29:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christopher Tiwald <christiwald@gmail.com> writes:\n\n> @@ -177,6 +192,16 @@ static int push_with_options(struct transport *transport, int flags)\n>  {\n>  \tint err;\n>  \tint nonfastforward;\n> +\tstruct branch *branch;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\n> +\tbranch = branch_get(NULL);\n> +\n> +\tif (branch) {\n> +\t\tstrbuf_addstr(&buf, transport->remote->name);\n> +\t\tstrbuf_addstr(&buf, \"/\");\n> +\t\tstrbuf_addstr(&buf, branch->name);\n> +\t}\n\nThe \"buf\" is a horrible name for a variable that is used to hold states\nin a long haul that has to span multiple hunks in a patch.  Please name\nit after what the value means.\n\n> @@ -201,7 +226,18 @@ static int push_with_options(struct transport *transport, int flags)\n>  \tdefault:\n>  \t\tbreak;\n>  \tcase NON_FF_HEAD:\n> -\t\tadvise_pull_before_push();\n> +\t\t/* Branches configured for octopus merges should advise\n> +\t\t * just 'git pull' */\n> +\t\tif (branch->remote_name &&\n> +\t\t    branch->merge &&\n> +\t\t    branch->merge_nr == 1 &&\n> +\t\t    !strcmp(transport->remote->name, branch->remote_name) &&\n> +\t\t    !strcmp(strbuf_detach(&buf, NULL),\n> +\t\t\t    prettify_refname(branch->merge[0]->dst))) {\n\nWhy detach?  buf_to_be_renamed_more_sanely.buf, perhaps?\n\nIs comparison between whatever buf has and the result of prettify safe\nand sane?  After all, prettify is a random abbreviation that is meant\nfor human consumption, assuming that the reader is intelligent enough to\nguess \"Ah, it must be a tag\" when she sees \"v1.0\" which is the result of\nstripping the leading \"refs/tags/\" out.\n\nThis part should be using straight strcmp against branch->merge[0]->dst,\nwhich means buf_to_be_renamed_more_sanely in the earlier hunk needs to\nbe computing what's tracked and merged correctly using the configured\nrefspec mapping, perhaps?\n"},{"id":"189933","messageId":"20120424045844.GA41274@gmail.com","threadId":"30313","inReplyTo":"xmqqvckpho0a.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Christopher Tiwald","fromEmail":"christiwald@gmail.com","sentAt":"2012-04-24T04:58:44Z","receivedAt":"2012-04-24T04:58:44Z","isPatch":true,"sender":{"key":"christiwald@gmail.com","avatar":"https://avatars.githubusercontent.com/u/667276?v=4"},"body":"On Mon, Apr 23, 2012 at 07:17:25PM -0700, Junio C Hamano wrote:\n> So what makes it dangerous is the use of \"--rebase\", if anything, isn't\n> it?  It does not seem to have much to do with how the local branches are\n> named.\n\nAfter thinking about this argument, there might be a deeper problem with\nmy reasoning. Take the workflow you describe. In the \"devel tracks to origin\nmaster\" workflow, this patch would advise 'git pull <repository> <refspec>'.\nThe advice misses the point of setting the upstream branch. Worse, the\nadvice is broken if the user issues 'git pull origin devel' and no 'devel'\nbranch exists on origin or the 'devel' branch is simply out of date (as\nmight occur if the user pushes between a personal remote clone of a\nshared repo and the shared repo itself with different frequency).\n\nMaybe the solution here is to ditch the $dest_ref and $dest_remote\nmatching entirely and just touch the one case I _know_ the advice could\ndo better: git should advise 'git pull <repo> <refspec>' or something\nlike \"consider setting an upstream branch and pulling before pushing\nagain\" when branch->merge doesn't exist at all. I like the former\nbecause it's simpler as an end user and doesn't require enforcing a\nsetting he or she may not understand.\n\nI think that might be the way to go. I approached this from a specific\nworkflow assumption. In retrospect, I can't divine the motivation of\nmerge configurations well enough to avoid bad advice.\n\n--\nChristopher Tiwald\n"},{"id":"189939","messageId":"vpqipgpehlk.fsf@bauges.imag.fr","threadId":"30313","inReplyTo":"1335221121-36664-1-git-send-email-christiwald@gmail.com","subject":"Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-04-24T07:04:07Z","receivedAt":"2012-04-24T07:04:07Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Christopher Tiwald <christiwald@gmail.com> writes:\n\n> +\t\t/* Branches configured for octopus merges should advise\n> +\t\t * just 'git pull' */\n\nStyle: Git usually writes these comments \n\n/*\n * like this\n */\n\n> +\t\tif (branch->remote_name &&\n> +\t\t    branch->merge &&\n> +\t\t    branch->merge_nr == 1 &&\n> +\t\t    !strcmp(transport->remote->name, branch->remote_name) &&\n> +\t\t    !strcmp(strbuf_detach(&buf, NULL),\n> +\t\t\t    prettify_refname(branch->merge[0]->dst))) {\n> +\t\t\tadvise_tracked_pull_before_push();\n> +\t\t}\n> +\t\telse\n> +\t\t\tadvise_untracked_pull_before_push();\n\nIsn't this doing the opposite of what the comment is saying about\noctopus merge, i.e. if branch->merge_nr > 1, call\nadvise_untracked_pull_before_push() which will advise 'git pull <remote>\n<branch>'?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"189965","messageId":"20120424122149.GB41274@gmail.com","threadId":"30313","inReplyTo":"vpqipgpehlk.fsf@bauges.imag.fr","subject":"Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Christopher Tiwald","fromEmail":"christiwald@gmail.com","sentAt":"2012-04-24T12:21:49Z","receivedAt":"2012-04-24T12:21:49Z","isPatch":true,"sender":{"key":"christiwald@gmail.com","avatar":"https://avatars.githubusercontent.com/u/667276?v=4"},"body":"On Tue, Apr 24, 2012 at 09:04:07AM +0200, Matthieu Moy wrote:\n> > +\t\tif (branch->remote_name &&\n> > +\t\t    branch->merge &&\n> > +\t\t    branch->merge_nr == 1 &&\n> > +\t\t    !strcmp(transport->remote->name, branch->remote_name) &&\n> > +\t\t    !strcmp(strbuf_detach(&buf, NULL),\n> > +\t\t\t    prettify_refname(branch->merge[0]->dst))) {\n> > +\t\t\tadvise_tracked_pull_before_push();\n> > +\t\t}\n> > +\t\telse\n> > +\t\t\tadvise_untracked_pull_before_push();\n> \n> Isn't this doing the opposite of what the comment is saying about\n> octopus merge, i.e. if branch->merge_nr > 1, call\n> advise_untracked_pull_before_push() which will advise 'git pull <remote>\n> <branch>'?\n\nAh yes. The logic is wrong for the octopus case. That's easy to fix, but\nI'm considering ditching the matching entirely per my response to\nJunio's concerns. I think it might do more harm then good.\n\n--\nChristopher Tiwald\n"},{"id":"189982","messageId":"xmqqehrdgd9z.fsf@junio.mtv.corp.google.com","threadId":"30313","inReplyTo":"20120424045844.GA41274@gmail.com","subject":"Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-24T19:06:48Z","receivedAt":"2012-04-24T19:06:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christopher Tiwald <christiwald@gmail.com> writes:\n\n> I think that might be the way to go. I approached this from a specific\n> workflow assumption. In retrospect, I can't divine the motivation of\n> merge configurations well enough to avoid bad advice.\n\nI am very sympathetic to your underlying motivation to avoid telling\nthem to \"perform a git pull to integrate the history from the other side\nbefore you push\" and then getting misunderstood as if you told them to\nLITERALLY type \"git pull<RETURN>\".  Depending on how the branch the user\nwanted to push, the approach needed to update its history so that\ncontains the history from the other side will be different, and you need\nto have everything configured correctly for your case to be able to type\n\"git pull<RETURN>\" literally and get the right result.  If you were trying\nto push one-shot into somewhere you do not usually push to, it is very\nlikely that you would need to say \"git pull $there $that\", and there is\nno canned \"Type this LITERALLY to continue\" recipe that is appropriate\nin the advice message.\n\nPerhaps a safer way out is to phrase the advice message in such a way\nthat it is crystal clear to anybody halfway intelligent that there is\nnothing the user can cut and paste from there?\n"}]}