{"thread":{"id":"33071","subject":"[PATCH 1/2] commit: add in_merge_bases_many()","startedAt":"2013-03-05T00:48:19Z","lastAt":"2013-03-05T19:27:52Z","messageCount":11,"participants":["Junio C Hamano","Jeff King","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"210599","messageId":"7vd2vewu24.fsf@alter.siamese.dyndns.org","threadId":"33071","inReplyTo":null,"subject":"[PATCH 1/2] commit: add in_merge_bases_many()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T00:48:19Z","receivedAt":"2013-03-05T00:48:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Similar to in_merge_bases(commit, other) that returns true when\ncommit is an ancestor (i.e. in the merge bases between the two) of\nthe other commit, in_merge_bases_many(commit, n_other, other[])\nchecks if commit is an ancestor of any of the other[] commits.\n\nSigned-off-by: Junio C Hamano \n---\n commit.c | 24 ++++++++++++++++++------\n commit.h | 1 +\n 2 files changed, 19 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex e8eb0ae..3a201e0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -852,25 +852,37 @@ int is_descendant_of(struct commit *commit, struct commit_list *with_commit)\n }\n \n /*\n- * Is \"commit\" an ancestor of (i.e. reachable from) the \"reference\"?\n+ * Is \"commit\" an ancestor of one of the \"reference\"s?\n */\n-int in_merge_bases(struct commit *commit, struct commit *reference)\n+int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference)\n {\n \tstruct commit_list *bases;\n-\tint ret = 0;\n+\tint ret = 0, i;\n \n-\tif (parse_commit(commit) || parse_commit(reference))\n+\tif (parse_commit(commit))\n \t\treturn ret;\n+\tfor (i = 0; i < nr_reference; i++)\n+\t\tif (parse_commit(reference[i]))\n+\t\t\treturn ret;\n \n-\tbases = paint_down_to_common(commit, 1, &reference);\n+\tbases = paint_down_to_common(commit, nr_reference, reference);\n \tif (commit->object.flags & PARENT2)\n \t\tret = 1;\n \tclear_commit_marks(commit, all_flags);\n-\tclear_commit_marks(reference, all_flags);\n+\tfor (i = 0; i < nr_reference; i++)\n+\t\tclear_commit_marks(reference[i], all_flags);\n \tfree_commit_list(bases);\n \treturn ret;\n }\n \n+/*\n+ * Is \"commit\" an ancestor of (i.e. reachable from) the \"reference\"?\n+ */\n+int in_merge_bases(struct commit *commit, struct commit *reference)\n+{\n+\treturn in_merge_bases_many(commit, 1, &reference);\n+}\n+\n struct commit_list *reduce_heads(struct commit_list *heads)\n {\n \tstruct commit_list *p;\ndiff --git a/commit.h b/commit.h\nindex b6ad8f3..d5b9712 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -170,6 +170,7 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,\n \n int is_descendant_of(struct commit *, struct commit_list *);\n int in_merge_bases(struct commit *, struct commit *);\n+int in_merge_bases_many(struct commit *, int, struct commit **);\n \n extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);\n extern int run_add_interactive(const char *revision, const char *patch_mode,\n-- \n1.8.2-rc2-182-g857a7fa\n"},{"id":"210600","messageId":"7v6216wtrk.fsf@alter.siamese.dyndns.org","threadId":"33071","inReplyTo":"7vd2vewu24.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] push: --follow-tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T00:54:39Z","receivedAt":"2013-03-05T00:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The new option \"--follow-tag\" tells \"git push\" to push tags that are\nmissing from the other side and that can be reached by the history\nthat is otherwise pushed out. For example, if you are using the\n\"simple\", \"current\", or \"upstream\" push, you would ordinarily push\nthe history leading to the commit at your current HEAD and nothing\nelse. With this option, you would also push all tags that can be\nreached from that commit to the other side.\n\nSigned-off-by: Junio C Hamano \n---\n * The previous and this were built on 'maint', and may have\n trivial conflicts with 'master' and upwards.\n\n This is primarily to scratch my own itch; after tagging an rc or\n final release, I've been doing\n\n \tgit push k.org v1.8.2\n git push k.org\n\n and the first step can easily be forgotten. With\n\n\tgit push k.org --follow-tag\n\n I no longer should have to worry about that. It will push out\n whatever would be pushed out without --follow-tag, and also tags\n that are missing from my k.org repository that point into the\n history being pushed out.\n\n I'd certainly need help with phrasing in the documentation, by\n the way.\n\n Documentation/git-push.txt | 8 +++-\n builtin/push.c | 2 +\n remote.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++\n remote.h | 3 +-\n t/t5516-fetch-push.sh | 73 ++++++++++++++++++++++++++++++++++++\n transport.c | 2 +\n transport.h | 1 +\n 7 files changed, 179 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 8b637d3..5020662 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects\n SYNOPSIS\n --------\n [verse]\n-'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=]\n+'git push' [--all | --mirror | --tags] [--follow-tag] [-n | --dry-run] [--receive-pack=]\n \t [--repo=] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]\n \t [ [...]]\n \n@@ -111,6 +111,12 @@ no `push.default` configuration variable is set.\n \taddition to refspecs explicitly listed on the command\n \tline.\n \n+--follow-tag::\n+\tPush all the refs that would be pushed without this option,\n+\tand also push the refs under `refs/tags` that are missing\n+\tfrom the remote but are reachable from the refs that would\n+\tbe pushed without this option.\n+\n --receive-pack=::\n --exec=::\n \tPath to the 'git-receive-pack' program on the remote\ndiff --git a/builtin/push.c b/builtin/push.c\nindex db9ba30..08e3db1 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -399,6 +399,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"progress\", &progress, N_(\"force progress reporting\")),\n \t\tOPT_BIT(0, \"prune\", &flags, N_(\"prune locally removed refs\"),\n \t\t\tTRANSPORT_PUSH_PRUNE),\n+\t\tOPT_BIT(0, \"follow-tag\", &flags, N_(\"push missing but relevant tags\"),\n+\t\t\tTRANSPORT_PUSH_FOLLOW_TAG),\n \t\tOPT_END()\n \t};\n \ndiff --git a/remote.c b/remote.c\nindex ca1f8f2..4441944 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1195,6 +1195,94 @@ static struct ref **tail_ref(struct ref **head)\n \treturn tail;\n }\n \n+struct tips {\n+\tstruct commit **tip;\n+\tint nr, alloc;\n+};\n+\n+static void add_to_tips(struct tips *tips, struct ref *ref)\n+{\n+\tstruct commit *commit;\n+\n+\tif (is_null_sha1(ref->new_sha1))\n+\t\treturn;\n+\tcommit = lookup_commit_reference_gently(ref->new_sha1, 1);\n+\tif (!commit)\n+\t\treturn; /* not pushing a commit, which is not an error */\n+\tALLOC_GROW(tips->tip, tips->nr + 1, tips->alloc);\n+\ttips->tip[tips->nr++] = commit;\n+}\n+\n+static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***dst_tail)\n+{\n+\tstruct string_list dst_tag = STRING_LIST_INIT_NODUP;\n+\tstruct string_list src_tag = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n+\tstruct ref *ref;\n+\tstruct tips sent_tips;\n+\n+\t/*\n+\t * Collect everything we are going to send\n+\t * and collect all tags they have.\n+\t */\n+\tmemset(&sent_tips, 0, sizeof(sent_tips));\n+\tfor (ref = *dst; ref; ref = ref->next) {\n+\t\tif (ref->peer_ref &&\n+\t\t !is_null_sha1(ref->peer_ref->new_sha1))\n+\t\t\tadd_to_tips(&sent_tips, ref->peer_ref);\n+\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n+\t\t\tstring_list_append(&dst_tag, ref->name);\n+\t}\n+\tsort_string_list(&dst_tag);\n+\n+\t/* Collect tags they do not have. */\n+\tfor (ref = src; ref; ref = ref->next) {\n+\t\tif (prefixcmp(ref->name, \"refs/tags/\"))\n+\t\t\tcontinue;\n+\t\tif (string_list_has_string(&dst_tag, ref->name))\n+\t\t\tcontinue;\n+\t\titem = string_list_append(&src_tag, ref->name);\n+\t\titem->util = ref;\n+\t}\n+\tstring_list_clear(&dst_tag, 0);\n+\n+\t/*\n+\t * At this point, src_tag lists tags that are missing from\n+\t * dst, and sent_tips lists the tips we are pushing (or those\n+\t * that we know they already have). An element in the src_tag\n+\t * that is not an ancestor of any of the sent_tips need to be\n+\t * sent to the other side.\n+\t */\n+\tif (sent_tips.nr) {\n+\t\tfor_each_string_list_item(item, &src_tag) {\n+\t\t\tstruct ref *ref = item->util;\n+\t\t\tstruct ref *dst_ref;\n+\t\t\tstruct commit *commit;\n+\n+\t\t\tif (is_null_sha1(ref->new_sha1))\n+\t\t\t\tcontinue;\n+\t\t\tcommit = lookup_commit_reference_gently(ref->new_sha1, 1);\n+\t\t\tif (!commit)\n+\t\t\t\t/* not pushing a commit, which is not an error */\n+\t\t\t\tcontinue;\n+\n+\t\t\t/*\n+\t\t\t * Is this tag, which they do not have, reachable from\n+\t\t\t * any of the commits we are sending?\n+\t\t\t */\n+\t\t\tif (!in_merge_bases_many(commit, sent_tips.nr, sent_tips.tip))\n+\t\t\t\tcontinue;\n+\n+\t\t\t/* Add it in */\n+\t\t\tdst_ref = make_linked_ref(ref->name, dst_tail);\n+\t\t\thashcpy(dst_ref->new_sha1, ref->new_sha1);\n+\t\t\tdst_ref->peer_ref = copy_ref(ref);\n+\t\t}\n+\t}\n+\tstring_list_clear(&src_tag, 0);\n+\tfree(sent_tips.tip);\n+}\n+\n /*\n * Given the set of refs the local repository has, the set of refs the\n * remote repository has, and the refspec used for push, determine\n@@ -1257,6 +1345,10 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \tfree_name:\n \t\tfree(dst_name);\n \t}\n+\n+\tif (flags & MATCH_REFS_FOLLOW_TAG)\n+\t\tadd_missing_tags(src, dst, &dst_tail);\n+\n \tif (send_prune) {\n \t\t/* check for missing refs on the remote */\n \t\tfor (ref = *dst; ref; ref = ref->next) {\ndiff --git a/remote.h b/remote.h\nindex 251d8fd..950ab5a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -148,7 +148,8 @@ enum match_refs_flags {\n \tMATCH_REFS_NONE\t\t= 0,\n \tMATCH_REFS_ALL \t\t= (1 << 0),\n \tMATCH_REFS_MIRROR\t= (1 << 1),\n-\tMATCH_REFS_PRUNE\t= (1 << 2)\n+\tMATCH_REFS_PRUNE\t= (1 << 2),\n+\tMATCH_REFS_FOLLOW_TAG\t= (1 << 3)\n };\n \n /* Reporting of tracking info */\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b5417cc..4ff2eb2 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -995,4 +995,77 @@ test_expect_success 'push --prune refspec' '\n \t! check_push_result $the_first_commit tmp/foo tmp/bar\n '\n \n+test_expect_success 'fetch follows tags by default' '\n+\tmk_test heads/master &&\n+\trm -fr src dst &&\n+\tgit init src &&\n+\t(\n+\t\tcd src &&\n+\t\tgit pull ../testrepo master &&\n+\t\tgit tag -m \"annotated\" tag &&\n+\t\tgit for-each-ref >tmp1 &&\n+\t\t(\n+\t\t\tcat tmp1\n+\t\t\tsed -n \"s|refs/heads/master$|refs/remotes/origin/master|p\" tmp1\n+\t\t) |\n+\t\tsort -k 3 >../expect\n+\t) &&\n+\tgit init dst &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit remote add origin ../src &&\n+\t\tgit config branch.master.remote origin &&\n+\t\tgit config branch.master.merge refs/heads/master &&\n+\t\tgit pull &&\n+\t\tgit for-each-ref >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'push does not follow tags by default' '\n+\tmk_test heads/master &&\n+\trm -fr src dst &&\n+\tgit init src &&\n+\tgit init --bare dst &&\n+\t(\n+\t\tcd src &&\n+\t\tgit pull ../testrepo master &&\n+\t\tgit tag -m \"annotated\" tag &&\n+\t\tgit checkout -b another &&\n+\t\tgit commit --allow-empty -m \"future commit\" &&\n+\t\tgit tag -m \"future\" future &&\n+\t\tgit checkout master &&\n+\t\tgit for-each-ref refs/heads/master >../expect &&\n+\t\tgit push ../dst master\n+\t) &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit for-each-ref >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'push --follow-tag only pushes relevant tags' '\n+\tmk_test heads/master &&\n+\trm -fr src dst &&\n+\tgit init src &&\n+\tgit init --bare dst &&\n+\t(\n+\t\tcd src &&\n+\t\tgit pull ../testrepo master &&\n+\t\tgit tag -m \"annotated\" tag &&\n+\t\tgit checkout -b another &&\n+\t\tgit commit --allow-empty -m \"future commit\" &&\n+\t\tgit tag -m \"future\" future &&\n+\t\tgit checkout master &&\n+\t\tgit for-each-ref refs/heads/master refs/tags/tag >../expect\n+\t\tgit push --follow-tag ../dst master\n+\t) &&\n+\t(\n+\t\tcd dst &&\n+\t\tgit for-each-ref >../actual\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex b9306ef..6e2f22d 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1059,6 +1059,8 @@ int transport_push(struct transport *transport,\n \t\t\tmatch_flags |= MATCH_REFS_MIRROR;\n \t\tif (flags & TRANSPORT_PUSH_PRUNE)\n \t\t\tmatch_flags |= MATCH_REFS_PRUNE;\n+\t\tif (flags & TRANSPORT_PUSH_FOLLOW_TAG)\n+\t\t\tmatch_flags |= MATCH_REFS_FOLLOW_TAG;\n \n \t\tif (match_push_refs(local_refs, &remote_refs,\n \t\t\t\t refspec_nr, refspec, match_flags)) {\ndiff --git a/transport.h b/transport.h\nindex 4a61c0c..bbebfbc 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -104,6 +104,7 @@ struct transport {\n #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64\n #define TRANSPORT_PUSH_PRUNE 128\n #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256\n+#define TRANSPORT_PUSH_FOLLOW_TAG 1024\n \n #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)\n #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)\n-- \n1.8.2-rc2-182-g857a7fa\n"},{"id":"210613","messageId":"20130305082204.GB13552@sigill.intra.peff.net","threadId":"33071","inReplyTo":"7v6216wtrk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T08:22:04Z","receivedAt":"2013-03-05T08:22:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2013 at 04:54:39PM -0800, Junio C Hamano wrote:\n\n> This is primarily to scratch my own itch; after tagging an rc or\n> final release, I've been doing\n> \n> \tgit push k.org v1.8.2\n> git push k.org\n> \n> and the first step can easily be forgotten. With\n> \n> \tgit push k.org --follow-tag\n> \n> I no longer should have to worry about that. It will push out\n> whatever would be pushed out without --follow-tag, and also tags\n> that are missing from my k.org repository that point into the\n> history being pushed out.\n\nI've run into the same problem. It's a minor annoyance, but your patch\nshould make it smoother.\n\nShould this be called \"--follow-tags\"? That makes more sense to me, as\nyou are catching all tags. For consistency, should this match the naming\nof git-fetch's options (or vice versa)? There we have:\n\n : auto-follow tags\n\n --tags: fetch all of refs/heads/tags\n\n --no-tags: do not auto-follow\n\nI think that naming has caused some confusion in the past. And there is\nno way to explicitly specify the default behavior. I wonder if both\nshould support:\n\n --follow-tags: auto-follow tags\n\n --no-follow-tags: do not auto-follow tags\n\n --tags: fetch/push all of refs/heads/tags\n\n --no-tags: turn off auto-following, and cancel any previous --tags\n\nThe default for push should probably keep auto-follow off, though.\n\n> +--follow-tag::\n> +\tPush all the refs that would be pushed without this option,\n> +\tand also push the refs under `refs/tags` that are missing\n> +\tfrom the remote but are reachable from the refs that would\n> +\tbe pushed without this option.\n> +\n\nThis reads OK to me, though it is a little confusing in that there are\ntwo sets of refs being discussed, and \"the refs that would be pushed\nwithout this option\" is quite a long noun phrase (that gets used twice).\n\nYou also don't define \"reachable\" here. Being familiar with git, I\nassume it is \"the commit that the refs/tag entry peels to is an ancestor\nof a commit that is at the tip of a pushed ref\". Maybe that is obvious\nenough that it doesn't need to be spelled out.\n\nI think you could just drop the second \"refs that would be pushed\nwithout this option\", and just say \"pushed refs\". That is ambiguous (is\nit all pushed refs, or refs that would be pushed without this option?),\nbut since reachability is transitive, they are the same set (i.e., if we\nadd a ref due to this option, then its ancestors are already candidates,\nand it does not affect the outcome).\n\nMaybe:\n\n In addition to any refs that would be pushed without this option,\n also push any refs under `refs/tags` that are missing from the remote\n but are reachable from the pushed refs.\n\nI dunno. I don't really like that version much, either.\n\n> +\t/*\n> +\t * At this point, src_tag lists tags that are missing from\n> +\t * dst, and sent_tips lists the tips we are pushing (or those\n> +\t * that we know they already have). An element in the src_tag\n> +\t * that is not an ancestor of any of the sent_tips need to be\n> +\t * sent to the other side.\n> +\t */\n> +\tif (sent_tips.nr) {\n> +\t\tfor_each_string_list_item(item, &src_tag) {\n> +\t\t\tstruct ref *ref = item->util;\n> +\t\t\tstruct ref *dst_ref;\n> +\t\t\tstruct commit *commit;\n> +\n> +\t\t\tif (is_null_sha1(ref->new_sha1))\n> +\t\t\t\tcontinue;\n> +\t\t\tcommit = lookup_commit_reference_gently(ref->new_sha1, 1);\n> +\t\t\tif (!commit)\n> +\t\t\t\t/* not pushing a commit, which is not an error */\n> +\t\t\t\tcontinue;\n\nThis will find anything under refs/tags, including annotated and\nnon-annotated tags. I wonder if it is worth making a distinction. In\nmany workflows, unannotated tags should not be leaked out to public\nrepos. But because this feature finds any reachable tags, it will push a\ntag you made a long time ago as a bookmarker on some part of the history\nunrelated to the release you are making now.\n\nOne obvious alternative is only to push annotated tags with this\nfeature. That has the downside of not matching fetch's behavior, as well\nas withholding the feature from people whose workflow uses only\nunannotated tags.\n\nAnother alternative would be to change the inclusion rule from\n\"reachable\" to \"points at the tip of something being sent\". But then we\nlose the feature that it would backfill any old tags the remote happens\nto be missing.\n\n-Peff\n"},{"id":"210622","messageId":"5135DBE5.6080909@alum.mit.edu","threadId":"33071","inReplyTo":"20130305082204.GB13552@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-03-05T11:49:57Z","receivedAt":"2013-03-05T11:49:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/05/2013 09:22 AM, Jeff King wrote:\n> On Mon, Mar 04, 2013 at 04:54:39PM -0800, Junio C Hamano wrote:\n> [...]\n> This will find anything under refs/tags, including annotated and\n> non-annotated tags. I wonder if it is worth making a distinction. In\n> many workflows, unannotated tags should not be leaked out to public\n> repos. But because this feature finds any reachable tags, it will push a\n> tag you made a long time ago as a bookmarker on some part of the history\n> unrelated to the release you are making now.\n> \n> One obvious alternative is only to push annotated tags with this\n> feature. That has the downside of not matching fetch's behavior, as well\n> as withholding the feature from people whose workflow uses only\n> unannotated tags.\n> \n> Another alternative would be to change the inclusion rule from\n> \"reachable\" to \"points at the tip of something being sent\". But then we\n> lose the feature that it would backfill any old tags the remote happens\n> to be missing.\n\nI have no opinion on this matter, but ISTM that another obvious\nalternative would be to push tags that point at *any* commits that are\nbeing sent (not just at the tip), but not those that point to older\ncommits already known to the server. This would reduce the potential\nfor accidental pushes of \"distant\" tags.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"210633","messageId":"7vlia1vnwq.fsf@alter.siamese.dyndns.org","threadId":"33071","inReplyTo":"20130305082204.GB13552@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T15:58:45Z","receivedAt":"2013-03-05T15:58:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Should this be called \"--follow-tags\"? That makes more sense to me, as\n> you are catching all tags.\n\nPerhaps. We are sending all zero-or-more relevant tags, so I agree\nthat plural form is more appropriate. I have a doubt about\n\"follow\", though; inertia made me use \"follow\", but I am not sure\nwhat we are following. We certainly are not following tags. If\nanything, we are making tags to piggy back on the history being\npushed.\n\n> For consistency, should this match the naming of git-fetch's\n> options (or vice versa)? There we have:\n>\n> : auto-follow tags\n>\n> --tags: fetch all of refs/heads/tags\n>\n> --no-tags: do not auto-follow\n>\n> I think that naming has caused some confusion in the past.\n\n\"--tags\" does not belong in the discussion of \"auto following\". It\ndoes not have to do with any \"auto\"-ness. Renaming \"--no-tags\" to\n\"--no-follow-tags\" does make sense, though.\n\n> And there is no way to explicitly specify the default behavior. I\n> wonder if both should support:\n>\n> --follow-tags: auto-follow tags\n>\n> --no-follow-tags: do not auto-follow tags\n\nYup, I like that. Perhaps make \"--no-tags\" a deprecated synonym to\nthe latter.\n\n> --tags: fetch/push all of refs/heads/tags\n>\n> --no-tags: turn off auto-following, and cancel any previous --tags\n\nSounds sensible.\n\n> The default for push should probably keep auto-follow off, though.\n>\n>> +--follow-tag::\n>> +\tPush all the refs that would be pushed without this option,\n>> +\tand also push the refs under `refs/tags` that are missing\n>> +\tfrom the remote but are reachable from the refs that would\n>> +\tbe pushed without this option.\n>> +\n>\n> This reads OK to me, though it is a little confusing in that there are\n> two sets of refs being discussed, and \"the refs that would be pushed\n> without this option\" is quite a long noun phrase (that gets used twice).\n\nYes, exactly why I said I do not like the phrasing of this one.\n\n> This will find anything under refs/tags, including annotated and\n> non-annotated tags. I wonder if it is worth making a distinction. In\n> many workflows, unannotated tags should not be leaked out to public\n> repos. But because this feature finds any reachable tags, it will push a\n> tag you made a long time ago as a bookmarker on some part of the history\n> unrelated to the release you are making now.\n\nWhat does the auto-follow feature of \"git fetch\" do currently?\nWhatever we do here on the \"git push\" side should match it.\n\nIf somebody wants to add some form of filtering mechanism based on\nthe tagname (e.g. '--auto-follow-tags=v[0-9]*'), I would not have a\nstrong objection to it, but I think that is something we should do\non top and consistently between fetch and push. I am not thrilled\nby the idea of conflating annotated-ness and the public-ness of\ntags.\n\nThanks.\n"},{"id":"210644","messageId":"20130305172233.GA9379@sigill.intra.peff.net","threadId":"33071","inReplyTo":"7vlia1vnwq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T17:22:33Z","receivedAt":"2013-03-05T17:22:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 07:58:45AM -0800, Junio C Hamano wrote:\n\n> > This will find anything under refs/tags, including annotated and\n> > non-annotated tags. I wonder if it is worth making a distinction. In\n> > many workflows, unannotated tags should not be leaked out to public\n> > repos. But because this feature finds any reachable tags, it will push a\n> > tag you made a long time ago as a bookmarker on some part of the history\n> > unrelated to the release you are making now.\n> \n> What does the auto-follow feature of \"git fetch\" do currently?\n> Whatever we do here on the \"git push\" side should match it.\n\nIt fetches anything in refs/tags, unannotated or not. And that is\ncertainly a point in favor of \"git push\" doing the same.\n\nBut I wonder if fetching and pushing are different in that respect. You\nare (usually) fetching from a public publishing point, and it is assumed\nthat whatever is there is useful for sharing. The only reason to limit\nit is to save time transferring objects the user does not want.\n\nBut for \"push\", you are on the publishing side, which usually means you\nneed to be a little more careful. It is not just an optimization; it is\nabout deciding what should be shared. You do not want to accidentally\npush cruft or work in progress in your private repository. I think it's\nthe same logic that leads us to fetch \"refs/heads/*\" by default, but\nonly push \"matching\" (or more recently \"HEAD\").\n\n> If somebody wants to add some form of filtering mechanism based on\n> the tagname (e.g. '--auto-follow-tags=v[0-9]*'), I would not have a\n> strong objection to it, but I think that is something we should do\n> on top and consistently between fetch and push. I am not thrilled\n> by the idea of conflating annotated-ness and the public-ness of\n> tags.\n\nI don't like it either. But I also don't want to introduce a feature\nthat causes people to accidentally publish cruft. It may not be a\nproblem in practice; I'm just thinking out loud at this point.\n\n-Peff\n"},{"id":"210647","messageId":"7vobexu30n.fsf@alter.siamese.dyndns.org","threadId":"33071","inReplyTo":"20130305172233.GA9379@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T18:15:20Z","receivedAt":"2013-03-05T18:15:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I wonder if fetching and pushing are different in that respect. You\n> are (usually) fetching from a public publishing point, and it is assumed\n> that whatever is there is useful for sharing. The only reason to limit\n> it is to save time transferring objects the user does not want.\n\nThere are those who have to emulate \"git fetch\" with a reverse \"git\npush\" (or vice versa) due to network connection limitations, so I do\nnot think hardcoding such a policy decision in the direction is\nnecessarily a good idea.\n\n> ... But I also don't want to introduce a feature\n> that causes people to accidentally publish cruft. It may not be a\n> problem in practice; I'm just thinking out loud at this point.\n\nLikewise.\n"},{"id":"210648","messageId":"20130305181850.GA10928@sigill.intra.peff.net","threadId":"33071","inReplyTo":"7vobexu30n.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T18:18:50Z","receivedAt":"2013-03-05T18:18:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 10:15:20AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But I wonder if fetching and pushing are different in that respect. You\n> > are (usually) fetching from a public publishing point, and it is assumed\n> > that whatever is there is useful for sharing. The only reason to limit\n> > it is to save time transferring objects the user does not want.\n> \n> There are those who have to emulate \"git fetch\" with a reverse \"git\n> push\" (or vice versa) due to network connection limitations, so I do\n> not think hardcoding such a policy decision in the direction is\n> necessarily a good idea.\n\nYeah, but I think it makes sense to optimize the defaults for the common\ncases, and let people doing unusual things override the behavior via\noptions (or even config).\n\nDon't get me wrong, I think there is value in the simplicity of having\nthe push/fetch transactions be as symmetric as possible. But given the\npotentially high cost of a mistaken push (i.e., retracting published\nhistory can be embarrassing or complicated), there's also value in safe\ndefaults. And I feel like we've already gone in that direction with the\ndefault refspecs being different between fetch and push.\n\n-Peff\n"},{"id":"210649","messageId":"20130305182255.GB10928@sigill.intra.peff.net","threadId":"33071","inReplyTo":"5135DBE5.6080909@alum.mit.edu","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T18:22:55Z","receivedAt":"2013-03-05T18:22:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 12:49:57PM +0100, Michael Haggerty wrote:\n\n> > One obvious alternative is only to push annotated tags with this\n> > feature. That has the downside of not matching fetch's behavior, as well\n> > as withholding the feature from people whose workflow uses only\n> > unannotated tags.\n> > \n> > Another alternative would be to change the inclusion rule from\n> > \"reachable\" to \"points at the tip of something being sent\". But then we\n> > lose the feature that it would backfill any old tags the remote happens\n> > to be missing.\n> \n> I have no opinion on this matter, but ISTM that another obvious\n> alternative would be to push tags that point at *any* commits that are\n> being sent (not just at the tip), but not those that point to older\n> commits already known to the server. This would reduce the potential\n> for accidental pushes of \"distant\" tags.\n\nYeah, I think that is another sensible variant. It does not really\n\"backfill\" in the way that Junio's patch does (e.g., if you forgot to\npush out v1.6 to a remote 2 weeks ago and now you are pushing out v1.7,\nJunio's patch will magically fill it in). But I do not see a huge value\nin backfilling. It is anyone's guess whether you simply forgot to push\nout v1.6 or whether you intended to hold it back. And I'd rather err on\nthe side of not-pushing because of the difficulty of retraction.\n\n-Peff\n"},{"id":"210652","messageId":"7v6215u05k.fsf@alter.siamese.dyndns.org","threadId":"33071","inReplyTo":"20130305182255.GB10928@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-05T19:17:11Z","receivedAt":"2013-03-05T19:17:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I think that is another sensible variant. It does not really\n> \"backfill\" in the way that Junio's patch does (e.g., if you forgot to\n> push out v1.6 to a remote 2 weeks ago and now you are pushing out v1.7,\n> Junio's patch will magically fill it in).\n\nI may have tentatively tagged the tip of 'master' as v1.8.2 in my\nprivate repository, started the integration testing, but may not be\nconfident enough to push out the branch nor the tag yet. I may have\nan experimental topic that I want to share with others before I am\ndone with the release to unblock them, and the topic may build on\nthe 'master' I may or may not want to redo before the release.\n\nI can do so with \"git push github jc/topic\" (no --follow-tags).\nAfter doing such a \"you may want to start with this\" push, I can\ncontinue working on the release, and the 'master' branch may turn\nout to be good to go without redoing.\n\nA later \"git push github --follow-tags master\" in such a case should\nsend v1.8.2 out. It is inexcuable to break it, saying \"Oh, I've\nseen that commit already---it is part of the previous update to\njc/topic\". That defeats the whole point of --follow-tags.\n\nThe other \"tags at the tip\" is slightly less problematic than\n\"ignore the commits the receiving end has already seen\", but it will\nbreak if I tag the tip of 'maint' as v1.8.1.6, continue working\nwithout being able to push perhaps due to network disruption, and\nhave already started building towards v1.8.1.7 when the network\ncomes back. 'maint' may be past v1.8.1.6 and its tip won't be\npointing at that tag, but it still is missing from the public\nrepository.\n\nWhile I agree with your goal to make it safer to use \"push\", both\n\"ignore commits that the receiving end already saw\" and \"only look\nat the commits at the tip being sent\" castrate the option too much\nto the point that it is meaningless. The whole point of asking\nexplicitly with the \"--follow-tags\" option to \"push\" to push out\nrelevant tags is, eh, to push out relevant tags. I do not think it\nis magical at all. \"backfill\" is an integral part of the option.\n"},{"id":"210653","messageId":"20130305192752.GA13335@sigill.intra.peff.net","threadId":"33071","inReplyTo":"7v6215u05k.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] push: --follow-tag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-05T19:27:52Z","receivedAt":"2013-03-05T19:27:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2013 at 11:17:11AM -0800, Junio C Hamano wrote:\n\n> I may have tentatively tagged the tip of 'master' as v1.8.2 in my\n> private repository, started the integration testing, but may not be\n> confident enough to push out the branch nor the tag yet. I may have\n> an experimental topic that I want to share with others before I am\n> done with the release to unblock them, and the topic may build on\n> the 'master' I may or may not want to redo before the release.\n> \n> I can do so with \"git push github jc/topic\" (no --follow-tags).\n> After doing such a \"you may want to start with this\" push, I can\n> continue working on the release, and the 'master' branch may turn\n> out to be good to go without redoing.\n> \n> A later \"git push github --follow-tags master\" in such a case should\n> send v1.8.2 out. It is inexcuable to break it, saying \"Oh, I've\n> seen that commit already---it is part of the previous update to\n> jc/topic\". That defeats the whole point of --follow-tags.\n\nThat depends on the specifics of the rule. If the rule is \"any commit\nthat the server already has\", then yes, it runs afoul of that problem.\nBut what if it is \"any commit in the ref update being sent\"? That is, we\nwould look at the range \"origin/master..master\" and send any tags that\npoint to commits in that range.\n\n> The other \"tags at the tip\" is slightly less problematic than\n> \"ignore the commits the receiving end has already seen\", but it will\n> break if I tag the tip of 'maint' as v1.8.1.6, continue working\n> without being able to push perhaps due to network disruption, and\n> have already started building towards v1.8.1.7 when the network\n> comes back. 'maint' may be past v1.8.1.6 and its tip won't be\n> pointing at that tag, but it still is missing from the public\n> repository.\n\nRight, I think \"tags at the tip\" is too likely to miss things you did\nwant to send.\n\n> While I agree with your goal to make it safer to use \"push\", both\n> \"ignore commits that the receiving end already saw\" and \"only look\n> at the commits at the tip being sent\" castrate the option too much\n> to the point that it is meaningless. The whole point of asking\n> explicitly with the \"--follow-tags\" option to \"push\" to push out\n> relevant tags is, eh, to push out relevant tags. I do not think it\n> is magical at all. \"backfill\" is an integral part of the option.\n\nI know, and I'm willing to accept that the right resolution is \"we\nshould not have this feature\" (or possibly \"the documentation must warn\nabout caveats of the feature). I'm just worried about it accidentally\nbeing misused and causing problems.\n\n-Peff\n"}]}