{"thread":{"id":"29156","subject":"[PATCH 2/2] push: --ignore-stale option","startedAt":"2011-12-13T23:35:13Z","lastAt":"2011-12-19T20:16:43Z","messageCount":11,"participants":["Junio C Hamano","Andreas Ericsson","Jeff King","Marc Branchaud"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"181097","messageId":"7vobvct58u.fsf@alter.siamese.dyndns.org","threadId":"29156","inReplyTo":null,"subject":"[PATCH 2/2] push: --ignore-stale option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-13T23:35:13Z","receivedAt":"2011-12-13T23:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you forked many local branches that are not active from a shared\n'origin' repository, it is sometimes convenient to say \"I know many of my\nbranches are stale, so do not push them, but push ones that have my own\nwork\". Using the usual 'matching refs' semantics that is designed for the\nworkflow to push into your own publishing repository is not suitable for\nthis purpose as-is, because many of your local branches are likely to have\nbeen made stale by other people pushing into the same shared repository,\ntriggering 'no fast-forward' errors.\n\nTeach a new \"--ignore-stale\" option to \"git push\" which tells it not to\npush stale refs (i.e. the commit that would have been pushed without the\noption is an ancestor of the commit that is at the destination). With this,\na lazy workflow could be like this:\n\n $ git clone <>\n $ git checkout -b topic1 origin/topic1\n $ work work work\n $ git push origin :\n $ git checkout -b topic2 origin/topic2\n $ work work work\n $ git push --ignore-stale origin :\n\nand the second push does not have to worry about other people working on\ntopic1 and updating it in the central repository while you haven't touched\nthe corresponding local branch at all.\n\nSigned-off-by: Junio C Hamano \n---\n\n * You may end up having 47 local branches, 7 of them worked on recently\n and none of them pushed yet, which requires you to remember which 7 of\n them you need to push among 47. The reason you left these 7 unpushed\n even though you checked out other branches at least 6 times after\n making the last commit on these branches is probably because you wanted\n to make sure everything is in good order, delaying the pushout as much\n as possible, which by itself is a good discipline.\n\n You however should be testing these 7 before pushing them out anyway,\n and the sane way to do so is to check one out, test it, push it, and\n iterate that sequence 7 times. If you do so, a workable alternative is\n to use the configuration to push the current branch and you do not need\n this patch series at all.\n\n Perhaps this series encourages a wrong workflow in that sense. I dunno.\n\n Documentation/git-push.txt | 10 +++++++++-\n builtin/push.c | 1 +\n builtin/send-pack.c | 7 +++++++\n cache.h | 1 +\n remote.c | 20 ++++++++++++++++++++\n t/t5516-fetch-push.sh | 31 +++++++++++++++++++++++++++++++\n transport.c | 6 ++++++\n transport.h | 1 +\n 8 files changed, 76 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex aede488..f2b5ee2 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n [verse]\n 'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=]\n \t [--repo=] [-f | --force] [-v | --verbose] [-u | --set-upstream]\n-\t [ [...]]\n+\t [--ignore-stale] [ [...]]\n \n DESCRIPTION\n -----------\n@@ -114,6 +114,14 @@ nor in any Push line of the corresponding remotes file---see below).\n \tThis flag disables the check. This can cause the\n \tremote repository to lose commits; use it with care.\n \n+--ignore-stale::\n+\tWhen the commit that is pushed is known to be an ancestor of the\n+\tcommit that is at the remote ref, exclude it from the push\n+\trequest. This can be used with the \"matching\" semantics to push\n+\tout only the branches that have your own work, when you know many\n+\tof your branches since they were forked from their upstream are\n+\tuntouched and stale.\n+\n --repo=::\n \tThis option is only relevant if no argument is\n \tpassed in the invocation. In this case, 'git push' derives the\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 35cce53..165d2be 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -261,6 +261,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT('u', \"set-upstream\", &flags, \"set upstream for git pull/status\",\n \t\t\tTRANSPORT_PUSH_SET_UPSTREAM),\n \t\tOPT_BOOLEAN(0, \"progress\", &progress, \"force progress reporting\"),\n+\t\tOPT_BIT(0, \"ignore-stale\", &flags, \"ignore stale refs\", TRANSPORT_PUSH_IGNORE_STALE),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex ec107ed..0cfc69a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -196,6 +196,11 @@ static void print_helper_status(struct ref *ref)\n \t\t\tmsg = \"up to date\";\n \t\t\tbreak;\n \n+\t\tcase REF_STATUS_STALE:\n+\t\t\tres = \"ok\";\n+\t\t\tmsg = \"ignored stale\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\t\tres = \"error\";\n \t\t\tmsg = \"non-fast forward\";\n@@ -282,6 +287,7 @@ int send_pack(struct send_pack_args *args,\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tcase REF_STATUS_UPTODATE:\n+\t\tcase REF_STATUS_STALE:\n \t\t\tcontinue;\n \t\tdefault:\n \t\t\t; /* do nothing */\n@@ -379,6 +385,7 @@ int send_pack(struct send_pack_args *args,\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\n \t\tcase REF_STATUS_UPTODATE:\n+\t\tcase REF_STATUS_STALE:\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n \t\tdefault:\ndiff --git a/cache.h b/cache.h\nindex 8c98d05..696805d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1009,6 +1009,7 @@ struct ref {\n \t\tREF_STATUS_OK,\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n \t\tREF_STATUS_REJECT_NODELETE,\n+\t\tREF_STATUS_STALE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n \t\tREF_STATUS_EXPECTING_REPORT\ndiff --git a/remote.c b/remote.c\nindex 95d7f37..9c63426 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1224,11 +1224,25 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \treturn 0;\n }\n \n+/*\n+ * Do we know if the other side has newer commit than what we are\n+ * trying to push (i.e. old_sha1 is descendant of new_sha1)? If so\n+ * just ignore the request to push this particular bref under the\n+ * \"--ignore-stale\" option.\n+ */\n+static int is_stale_push(unsigned char *old_sha1, unsigned char *new_sha1)\n+{\n+\tif (!has_sha1_file(old_sha1) || !has_sha1_file(new_sha1))\n+\t\treturn 0;\n+\treturn ref_newer(old_sha1, new_sha1);\n+}\n+\n void set_ref_status_for_push(struct ref *remote_refs, unsigned flags)\n {\n \tstruct ref *ref;\n \tint send_mirror = flags & TRANSPORT_PUSH_MIRROR;\n \tint force_update = flags & TRANSPORT_PUSH_FORCE;\n+\tint ignore_stale = flags & TRANSPORT_PUSH_IGNORE_STALE;\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tif (ref->peer_ref)\n@@ -1243,6 +1257,12 @@ void set_ref_status_for_push(struct ref *remote_refs, unsigned flags)\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (ignore_stale && !ref->deletion &&\n+\t\t is_stale_push(ref->old_sha1, ref->new_sha1)) {\n+\t\t\tref->status = REF_STATUS_STALE;\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\t/* This part determines what can overwrite what.\n \t\t * The rules are:\n \t\t *\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b69cf57..c925640 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -979,4 +979,35 @@ test_expect_success 'push --porcelain --dry-run rejected' '\n \ttest_cmp .git/foo .git/bar\n '\n \n+test_expect_success 'push --ignore-stale' '\n+\tmk_empty &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit fetch --update-head-ok .. \"refs/heads/*:refs/heads/*\" &&\n+\t\tgit checkout -b next master &&\n+\t\tgit commit --allow-empty -m \"updated next\" &&\n+\t\tgit push . next:master &&\n+\t\tgit for-each-ref >../snapshot.before\n+\t) &&\n+\tgit checkout branch1 &&\n+\tgit commit --allow-empty -m \"updated branch1\" &&\n+\ttest_must_fail git push testrepo : &&\n+\tgit fetch testrepo \"+refs/heads/*:refs/remotes/origin/*\" &&\n+\tgit push --ignore-stale testrepo : &&\n+\t(\n+\t\tcd testrepo &&\n+\t\tgit for-each-ref >../snapshot.after\n+\t) &&\n+\n+\t# branch1 must be updated and master must stay the same\n+\tgit for-each-ref refs/heads/branch1 >expect &&\n+\tgrep refs/heads/branch1 snapshot.after >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgrep refs/heads/master snapshot.before >expect &&\n+\tgrep refs/heads/master snapshot.after >actual &&\n+\ttest_cmp expect actual\n+\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex 95556da..d124d70 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -566,6 +566,7 @@ static int push_had_errors(struct ref *ref)\n \tfor (; ref; ref = ref->next) {\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_NONE:\n+\t\tcase REF_STATUS_STALE:\n \t\tcase REF_STATUS_UPTODATE:\n \t\tcase REF_STATUS_OK:\n \t\t\tbreak;\n@@ -581,6 +582,7 @@ int transport_refs_pushed(struct ref *ref)\n \tfor (; ref; ref = ref->next) {\n \t\tswitch(ref->status) {\n \t\tcase REF_STATUS_NONE:\n+\t\tcase REF_STATUS_STALE:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tbreak;\n \t\tdefault:\n@@ -690,6 +692,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \t\tprint_ref_status('=', \"[up to date]\", ref,\n \t\t\t\t\t\t ref->peer_ref, NULL, porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_STALE:\n+\t\tprint_ref_status('=', \"[stale ignored]\", ref,\n+\t\t\t\t\t\t ref->peer_ref, NULL, porcelain);\n+\t\tbreak;\n \tcase REF_STATUS_REJECT_NONFASTFORWARD:\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n \t\t\t\t\t\t \"non-fast-forward\", porcelain);\ndiff --git a/transport.h b/transport.h\nindex 059b330..5db8d23 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -102,6 +102,7 @@ struct transport {\n #define TRANSPORT_PUSH_PORCELAIN 16\n #define TRANSPORT_PUSH_SET_UPSTREAM 32\n #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n+#define TRANSPORT_PUSH_IGNORE_STALE 128\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n \n-- \n1.7.8.249.gb1b73\n"},{"id":"181098","messageId":"4EE7E466.4060406@op5.se","threadId":"29156","inReplyTo":"7vobvct58u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --ignore-stale option","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2011-12-13T23:48:54Z","receivedAt":"2011-12-13T23:48:54Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 12/14/2011 12:35 AM, Junio C Hamano wrote:\n> \n> You however should be testing these 7 before pushing them out anyway,\n> and the sane way to do so is to check one out, test it, push it, and\n> iterate that sequence 7 times. If you do so, a workable alternative is\n> to use the configuration to push the current branch and you do not need\n> this patch series at all.\n> \n> Perhaps this series encourages a wrong workflow in that sense. I dunno.\n> \n\nI for one really love it and will probably make an alias to use for most of\nmy repos. With 'maint' branches (with topics) being worked on by one team and\n'master' (with topics) being worked on by a different group, but all branches\ngenerally checked out, tracked and touched by everyone sooner or later, I've\ncome to just ignore the \"not fast-forward\" error. I've never stopped loathing\nit though, since git should just *know* that it doesn't fast-forward because\nI haven't done anything and not because I've done something (and maybe\nforgotten to push) and someone else has done something different, which would\nactually be interesting for me to know.\n\nSo big thumbs up and three loud \"hurrah\"'s for this.\n\n-- \nAndreas Ericsson andreas.ericsson@op5.se\nOP5 AB www.op5.se\nTel: +46 8-230225 Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"181213","messageId":"20111215073816.GD1327@sigill.intra.peff.net","threadId":"29156","inReplyTo":"7vobvct58u.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --ignore-stale option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-15T07:38:16Z","receivedAt":"2011-12-15T07:38:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 13, 2011 at 03:35:13PM -0800, Junio C Hamano wrote:\n\n> Teach a new \"--ignore-stale\" option to \"git push\" which tells it not to\n> push stale refs (i.e. the commit that would have been pushed without the\n> option is an ancestor of the commit that is at the destination). With this,\n> a lazy workflow could be like this:\n> \n> $ git clone <>\n> $ git checkout -b topic1 origin/topic1\n> $ work work work\n> $ git push origin :\n> $ git checkout -b topic2 origin/topic2\n> $ work work work\n> $ git push --ignore-stale origin :\n> \n> and the second push does not have to worry about other people working on\n> topic1 and updating it in the central repository while you haven't touched\n> the corresponding local branch at all.\n\nUnless the action of the people in the central repo was to rewind\nhistory, in which case you are overwriting their work. Probably\nunlikely, though.\n\nBut I still see this as solving the wrong problem, and encouraging a\ndangerous workflow. Why are you using \":\" to push matching branches if\nyou don't want to push topic1? I think a much more likely scenario is:\n\n $ git clone <>\n $ git checkout -b topic1 origin/topic1\n $ work work work\n $ : hmm, it's not ready yet\n $ git checkout -b topic2 origin/topic2\n $ work work work\n $ : looking good, let's ship it!\n $ git push ;# we default to matching!\n\nI.e., the user is not actually intending to push all matching branches,\nbut does so accidentally because \"matching\" is the default. And they\nhave accidentally just pushed non-ready work to topic1, which would\nhappen even with --ignore-stale.\n\nIOW, I am not convinced that people are actually consciously choosing\nthe workflow you outlined above, and are instead interested in\n--ignore-stale as a convenient way of guessing which branches should be\npushed during a matching push. But it's only a guess, and as I showed\nabove, it can still cause problems. I think the right solution is to\nswitch the default away from matching, which is the root cause of the\nconfusion.\n\n-Peff\n"},{"id":"181234","messageId":"4EEA329F.1020201@xiplink.com","threadId":"29156","inReplyTo":"20111215073816.GD1327@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: --ignore-stale option","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2011-12-15T17:47:11Z","receivedAt":"2011-12-15T17:47:11Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 11-12-15 02:38 AM, Jeff King wrote:\n> \n> But I still see this as solving the wrong problem, and encouraging a\n> dangerous workflow. Why are you using \":\" to push matching branches if\n> you don't want to push topic1? I think a much more likely scenario is:\n> \n> $ git clone <>\n> $ git checkout -b topic1 origin/topic1\n> $ work work work\n> $ : hmm, it's not ready yet\n> $ git checkout -b topic2 origin/topic2\n> $ work work work\n> $ : looking good, let's ship it!\n> $ git push ;# we default to matching!\n> \n> I.e., the user is not actually intending to push all matching branches,\n> but does so accidentally because \"matching\" is the default. And they\n> have accidentally just pushed non-ready work to topic1, which would\n> happen even with --ignore-stale.\n> \n> IOW, I am not convinced that people are actually consciously choosing\n> the workflow you outlined above, and are instead interested in\n> --ignore-stale as a convenient way of guessing which branches should be\n> pushed during a matching push. But it's only a guess, and as I showed\n> above, it can still cause problems. I think the right solution is to\n> switch the default away from matching, which is the root cause of the\n> confusion.\n\n100% agree.\n\nIn my experience, people are confused by any magic that happens when a local\nref name matches a remote's ref name. I find folks have a much easier time\nwith git once they embrace the notion that a remote's branches are distinct\nfrom their local clone's, and that explicitly saying\n\tpush origin :\nis the clearest way of making sure git does what you want.\n\nIt's convenient for advanced users to take advantage of the shortcuts any\nref-matching magic provides, but I find novice users tend to make mistakes as\noften as not.\n\n\t\tM.\n"},{"id":"181237","messageId":"7vmxatofvr.fsf@alter.siamese.dyndns.org","threadId":"29156","inReplyTo":"20111215073816.GD1327@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: --ignore-stale option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-15T18:21:12Z","receivedAt":"2011-12-15T18:21:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I still see this as solving the wrong problem, and encouraging a\n> dangerous workflow.\n\nYes, that is what I meant, so we are in total agreement that these people\nshould be encouraged not to use the matching semantics.\n\nHow that encouragement is made we may differ, though.\n"},{"id":"181451","messageId":"7vliq9nn0l.fsf@alter.siamese.dyndns.org","threadId":"29156","inReplyTo":"7vmxatofvr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --ignore-stale option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T05:33:46Z","receivedAt":"2011-12-19T05:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> But I still see this as solving the wrong problem, and encouraging a\n>> dangerous workflow.\n>\n> Yes, that is what I meant, so we are in total agreement that these people\n> should be encouraged not to use the matching semantics.\n>\n> How that encouragement is made we may differ, though.\n\nSo here is my idea of how best to implement such an encouragement.\n\nThe first patch is a documentation update/clean-up to prepare for the main\none which is the second.\n"},{"id":"181452","messageId":"7vhb0xnmyi.fsf_-_@alter.siamese.dyndns.org","threadId":"29156","inReplyTo":"7vliq9nn0l.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] advice: Document that they all default to true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T05:35:01Z","receivedAt":"2011-12-19T05:35:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"By definition, the default value of \"advice.*\" variables must be true and\nthey all control various additional help messages that are designed to aid\nnew users. Setting one to false is to tell Git that the user understands\nthe nature of the error and does not need the additional verbose help\nmessage.\n\nAlso fix the asciidoc markup for linkgit:git-checkout[1] in the\ndescription of the detachedHead advice by removing an excess colon.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 19 ++++++++-----------\n 1 files changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8a7d2d4..9be7106 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -115,35 +115,32 @@ in the appropriate manual page. You will find a description of non-core\n porcelain configuration variables in the respective porcelain documentation.\n \n advice.*::\n-\tWhen set to 'true', display the given optional help message.\n-\tWhen set to 'false', do not display. The configuration variables\n-\tare:\n+\tThese variables control various optional help messages designed to\n+\taid new users. All 'advice.*' variables default to 'true', and you\n+\tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n \tpushNonFastForward::\n \t\tAdvice shown when linkgit:git-push[1] refuses\n-\t\tnon-fast-forward refs. Default: true.\n+\t\tnon-fast-forward refs.\n \tstatusHints::\n \t\tDirections on how to stage/unstage/add shown in the\n \t\toutput of linkgit:git-status[1] and the template shown\n-\t\twhen writing commit messages. Default: true.\n+\t\twhen writing commit messages.\n \tcommitBeforeMerge::\n \t\tAdvice shown when linkgit:git-merge[1] refuses to\n \t\tmerge to avoid overwriting local changes.\n-\t\tDefault: true.\n \tresolveConflict::\n \t\tAdvices shown by various commands when conflicts\n \t\tprevent the operation from being performed.\n-\t\tDefault: true.\n \timplicitIdentity::\n \t\tAdvice on how to set your identity configuration when\n \t\tyour information is guessed from the system username and\n-\t\tdomain name. Default: true.\n-\n+\t\tdomain name.\n \tdetachedHead::\n-\t\tAdvice shown when you used linkgit::git-checkout[1] to\n+\t\tAdvice shown when you used linkgit:git-checkout[1] to\n \t\tmove to the detach HEAD state, to instruct how to create\n-\t\ta local branch after the fact. Default: true.\n+\t\ta local branch after the fact.\n --\n \n core.fileMode::\n-- \n1.7.8.370.gb3269\n"},{"id":"181453","messageId":"7vbor5nmsq.fsf_-_@alter.siamese.dyndns.org","threadId":"29156","inReplyTo":"7vliq9nn0l.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] push: hint to use push.default=upstream when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T05:38:29Z","receivedAt":"2011-12-19T05:38:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If you push into a shared repository that others push to, and you have\nlocal branches in your repository that matches the remote side but do not\nkeep them up-to-date, then 'matching refs' is not an appropriate default\nfor you.\n\nDetect when push failed due to non-fast-forward _and_ we did matching refs\nby default (i.e. if the user explicitly said ':' from the command line, or\nhad push.default set to 'matching', then we do not want to advise), and\ngive a hint to tell the user that the user may want to set 'push.default'\nconfiguration variable to 'upstream', if the remote repository receives\npushes from other places.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 6 ++++++\n advice.c | 2 ++\n advice.h | 1 +\n builtin/push.c | 25 +++++++++++++++++++++++++\n cache.h | 3 ++-\n environment.c | 2 +-\n 6 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9be7106..8da7063 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -141,6 +141,12 @@ advice.*::\n \t\tAdvice shown when you used linkgit:git-checkout[1] to\n \t\tmove to the detach HEAD state, to instruct how to create\n \t\ta local branch after the fact.\n+\tpushUseUpstream::\n+\t\tAdvice to set 'push.default' to 'upstream' when you ran\n+\t\tlinkgit:git-push[1] and pushed 'matching refs' by default\n+\t\t(i.e. you did not have any explicit refspec on the command\n+\t\tline, and no 'push.default' configuration was set) and it\n+\t\tresulted in a non-fast-forward error.\n --\n \n core.fileMode::\ndiff --git a/advice.c b/advice.c\nindex e02e632..9b56709 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -6,6 +6,7 @@ int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n int advice_implicit_identity = 1;\n int advice_detached_head = 1;\n+int advice_push_use_upstream = 1;\n \n static struct {\n \tconst char *name;\n@@ -17,6 +18,7 @@ static struct {\n \t{ \"resolveconflict\", &advice_resolve_conflict },\n \t{ \"implicitidentity\", &advice_implicit_identity },\n \t{ \"detachedhead\", &advice_detached_head },\n+\t{ \"pushuseupstream\", &advice_push_use_upstream },\n };\n \n void advise(const char *advice, ...)\ndiff --git a/advice.h b/advice.h\nindex e5d0af7..6bb89cd 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -9,6 +9,7 @@ extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\n extern int advice_implicit_identity;\n extern int advice_detached_head;\n+extern int advice_push_use_upstream;\n \n int git_default_advice_config(const char *var, const char *value);\n void advise(const char *advice, ...);\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 35cce53..5f8c7f4 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -9,6 +9,7 @@\n #include \"transport.h\"\n #include \"parse-options.h\"\n #include \"submodule.h\"\n+#include \"advice.h\"\n \n static const char * const push_usage[] = {\n \t\"git push [] [ [...]]\",\n@@ -24,6 +25,7 @@ static int progress;\n static const char **refspec;\n static int refspec_nr;\n static int refspec_alloc;\n+static int default_matching_used;\n \n static void add_refspec(const char *ref)\n {\n@@ -95,6 +97,9 @@ static void setup_default_push_refspecs(struct remote *remote)\n {\n \tswitch (push_default) {\n \tdefault:\n+\tcase PUSH_DEFAULT_UNSPECIFIED:\n+\t\tdefault_matching_used = 1;\n+\t\t/* fallthru */\n \tcase PUSH_DEFAULT_MATCHING:\n \t\tadd_refspec(\":\");\n \t\tbreak;\n@@ -114,6 +119,23 @@ static void setup_default_push_refspecs(struct remote *remote)\n \t}\n }\n \n+static const char *message_advice_use_upstream[] = {\n+\t\"If you are pushing into a repository that receives pushes from\",\n+\t\"repositories other than the current repository, you may want to\",\n+\t\"set 'push.default' configuration variable to 'upstream' to avoid\",\n+\t\"pushing branches you haven't worked on that others have updated.\",\n+};\n+\n+static void advise_use_upstream(void)\n+{\n+\tint i;\n+\n+\tif (!advice_push_use_upstream)\n+\t\treturn;\n+\tfor (i = 0; i < ARRAY_SIZE(message_advice_use_upstream); i++)\n+\t\tadvise(message_advice_use_upstream[i]);\n+}\n+\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n@@ -136,6 +158,9 @@ static int push_with_options(struct transport *transport, int flags)\n \n \terr |= transport_disconnect(transport);\n \n+\tif (nonfastforward && default_matching_used)\n+\t\tadvise_use_upstream();\n+\n \tif (!err)\n \t\treturn 0;\n \ndiff --git a/cache.h b/cache.h\nindex 627fb6a..6c86725 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -624,7 +624,8 @@ enum push_default_type {\n \tPUSH_DEFAULT_NOTHING = 0,\n \tPUSH_DEFAULT_MATCHING,\n \tPUSH_DEFAULT_UPSTREAM,\n-\tPUSH_DEFAULT_CURRENT\n+\tPUSH_DEFAULT_CURRENT,\n+\tPUSH_DEFAULT_UNSPECIFIED\n };\n \n extern enum branch_track git_branch_track;\ndiff --git a/environment.c b/environment.c\nindex c93b8f4..d7e6c65 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -52,7 +52,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;\n enum rebase_setup_type autorebase = AUTOREBASE_NEVER;\n-enum push_default_type push_default = PUSH_DEFAULT_MATCHING;\n+enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;\n #ifndef OBJECT_CREATION_MODE\n #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS\n #endif\n-- \n1.7.8.370.gb3269\n"},{"id":"181458","messageId":"20111219093704.GA1736@sigill.intra.peff.net","threadId":"29156","inReplyTo":"7vhb0xnmyi.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] advice: Document that they all default to true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-19T09:37:04Z","receivedAt":"2011-12-19T09:37:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 18, 2011 at 09:35:01PM -0800, Junio C Hamano wrote:\n\n> By definition, the default value of \"advice.*\" variables must be true and\n> they all control various additional help messages that are designed to aid\n> new users. Setting one to false is to tell Git that the user understands\n> the nature of the error and does not need the additional verbose help\n> message.\n> \n> Also fix the asciidoc markup for linkgit:git-checkout[1] in the\n> description of the detachedHead advice by removing an excess colon.\n\nMakes sense.\n\nAcked-by: Jeff King \n\n-Peff\n"},{"id":"181463","messageId":"20111219103746.GB1736@sigill.intra.peff.net","threadId":"29156","inReplyTo":"7vbor5nmsq.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: hint to use push.default=upstream when appropriate","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-12-19T10:37:46Z","receivedAt":"2011-12-19T10:37:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 18, 2011 at 09:38:29PM -0800, Junio C Hamano wrote:\n\n> If you push into a shared repository that others push to, and you have\n> local branches in your repository that matches the remote side but do not\n> keep them up-to-date, then 'matching refs' is not an appropriate default\n> for you.\n> \n> Detect when push failed due to non-fast-forward _and_ we did matching refs\n> by default (i.e. if the user explicitly said ':' from the command line, or\n> had push.default set to 'matching', then we do not want to advise), and\n> give a hint to tell the user that the user may want to set 'push.default'\n> configuration variable to 'upstream', if the remote repository receives\n> pushes from other places.\n\nI like the general idea. I initially had the thought \"wait, don't we\nalready complain loudly when push.default is not set?\". But it seems we\nripped that out long ago (I think I set push.default to \"matching\" to\nshut it up, and then never noticed that doing so is no longer\nnecessary).\n\nFocusing instead on when an actual (suspected) misconfiguration has\noccurred is a much better approach. It won't catch the case of \"oops,\nI'm on branch 'foo' and just pushed not-ready-to-publish work to 'bar'\".\nBut the point is not necessarily to catch every mistake, but rather\ncatch some easy ones and hopefully educate the user to update their\nsettings or modify their workflow as appropriate.\n\n> +static const char *message_advice_use_upstream[] = {\n> +\t\"If you are pushing into a repository that receives pushes from\",\n> +\t\"repositories other than the current repository, you may want to\",\n> +\t\"set 'push.default' configuration variable to 'upstream' to avoid\",\n> +\t\"pushing branches you haven't worked on that others have updated.\",\n> +};\n\nMinor English nit: I would say \"...you want to set _the_ 'push.default'\nconfiguration variable...\".\n\nThe first few lines feel a little overwhelming, as you are setting up a\nhypothetical \"if you are in situation X\" in as few words as possible,\nbut it involves the word \"repository\" three different times (to refer to\nthree different repositories). I don't think there is anything\ninaccurate in the text, or even that the same meaning could be conveyed\nmore succinctly. But I wonder if it would make sense to take a step back\nand stop worrying about the potential repository setup, and try to\ndescribe the failure more specifically.\n\nIt seems like the real problem is that they are on branch \"foo\", but the\nmatching behavior tried to push \"bar\", which didn't work. And we are\nworried that they may be surprised that we even attempted to push \"bar\"\nat all.\n\nAnd that probably happened because of the situation you describe, but we\n(and the user) don't have to think about that. We can just think about\nthe more immediate mistake of \"oops, I didn't want to push 'bar'\".\n\nWhich leads me to two ideas:\n\n 1. This is not good advice to give when pushing the _current_ branch\n failed due to non-ff. Setting push.default to \"upstream\" would\n probably yield the same error. We should probably tighten the\n condition for showing this message to when a non-HEAD branch failed\n to fast-forward.\n\n 2. The text should focus on the (possible) local misconfiguration, not\n the repo setup.\n\nSo maybe something like:\n\n By default, git pushes all branches that have a matching counterpart\n on the remote. In this case, some of your local branches were stale\n with respect to their remote counterparts. If you did not intend to\n push these branches, you may want to set the 'push.default'\n configuration variable to 'upstream' to push only the current branch.\n\nI'm not 100% happy with that text, but I think you can see the direction\nI am talking about.\n\n> static int push_with_options(struct transport *transport, int flags)\n> {\n> \tint err;\n> @@ -136,6 +158,9 @@ static int push_with_options(struct transport *transport, int flags)\n> \n> \terr |= transport_disconnect(transport);\n> \n> +\tif (nonfastforward && default_matching_used)\n> +\t\tadvise_use_upstream();\n> +\n\nHow does the location of this message interact with the existing\nmessages? It seems to come before the usual non-fast-forward advice,\nbut after the status table and \"failed to push some refs to...\" error\nmessage.\n\nE.g.:\n\n To origin\n 1065894..c6935ca master -> master\n ! [rejected] other -> other (non-fast-forward)\n error: failed to push some refs to 'origin'\n hint: If you are pushing into a repository that receives pushes from\n hint: repositories other than the current repository, you may want to\n hint: set 'push.default' configuration variable to 'upstream' to avoid\n hint: pushing branches you haven't worked on that others have updated.\n To prevent you from losing history, non-fast-forward updates were rejected\n Merge the remote changes (e.g. 'git pull') before pushing again. See the\n 'Note about fast-forwards' section of 'git push --help' for details.\n\nwhich is quite a chunk of text. If we follow my suggestion above and\nonly print this message for non-HEAD refs, then these two messages\nbecome an either/or situation, I think. If the HEAD failed to\nfast-forward, then the right advice is probably \"git pull && git push\".\nIf a non-HEAD failed, then the right advice is either \"git checkout $X\n&& git pull && git push\" or \"here's how to avoid accidentally pushing\n$X\".\n\nSo the logic would be something like:\n\n if (nonfastforward == NONFASTFORWARD_HEAD)\n advise_pull_before_push();\n else if (nonfastforward == NONFASTFORWAD_OTHER) {\n if (default_matching_used)\n advise_use_upstream();\n else\n advise_checkout_pull_push();\n }\n\nI'm not sure that the checkout-pull-push advice is really all that good,\nbut we don't have much else to offer.\n\n-Peff\n"},{"id":"181484","messageId":"7vfwggmi50.fsf@alter.siamese.dyndns.org","threadId":"29156","inReplyTo":"20111219103746.GB1736@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: hint to use push.default=upstream when appropriate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-19T20:16:43Z","receivedAt":"2011-12-19T20:16:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It seems like the real problem is that they are on branch \"foo\", but the\n> matching behavior tried to push \"bar\", which didn't work. And we are\n> worried that they may be surprised that we even attempted to push \"bar\"\n> at all.\n\nOf we may be seeing a non-fast-forward on 'foo' itself, which is your\n1. below.\n\n> And that probably happened because of the situation you describe, but we\n> (and the user) don't have to think about that. We can just think about\n> the more immediate mistake of \"oops, I didn't want to push 'bar'\".\n>\n> Which leads me to two ideas:\n>\n> 1. This is not good advice to give when pushing the _current_ branch\n> failed due to non-ff. Setting push.default to \"upstream\" would\n> probably yield the same error. We should probably tighten the\n> condition for showing this message to when a non-HEAD branch failed\n> to fast-forward.\n>\n> 2. The text should focus on the (possible) local misconfiguration, not\n> the repo setup.\n\nOK, I think we are in agreement.\n\n> So maybe something like:\n>\n> By default, git pushes all branches that have a matching counterpart\n> on the remote. In this case, some of your local branches were stale\n> with respect to their remote counterparts. If you did not intend to\n> push these branches, you may want to set the 'push.default'\n> configuration variable to 'upstream' to push only the current branch.\n>\n> I'm not 100% happy with that text, but I think you can see the direction\n> I am talking about.\n\nAs long as we can tighten the condition further to ensure that the advice\nabove is triggered only when appropriate, I actually am 100% happy with\nthat text. Seeing others do the real work for me always makes me happy ;-)\n\nIn addition to \"did we use default-matching?\", we should use \"did we get\nnon-fast-forward error on a branch that is _not_ the current one?\" instead\nof the \"did we get any non-fast-forward error?\" in my patch, and the text\nshould match the situation more-or-less exactly.\n\n> ... If we follow my suggestion above and\n> only print this message for non-HEAD refs, then these two messages\n> become an either/or situation, I think. If the HEAD failed to\n> fast-forward, then the right advice is probably \"git pull && git push\".\n> If a non-HEAD failed, then the right advice is either \"git checkout $X\n> && git pull && git push\" or \"here's how to avoid accidentally pushing\n> $X\".\n>\n> So the logic would be something like:\n>\n> if (nonfastforward == NONFASTFORWARD_HEAD)\n> advise_pull_before_push();\n> else if (nonfastforward == NONFASTFORWAD_OTHER) {\n> if (default_matching_used)\n> advise_use_upstream();\n> else\n> advise_checkout_pull_push();\n> }\n\nSounds good. Spelling things out at this detail would let others who may\nbe interested in getting their hands dirty try to come up with an updated\npatch ;-).\n\nThanks.\n"}]}