[PATCH 1/2] commit: add in_merge_bases_many()

[PATCH 1/2] commit: add in_merge_bases_many()

[Junio C Hamano]

Similar to in_merge_bases(commit, other) that returns true when
commit is an ancestor (i.e. in the merge bases between the two) of
the other commit, in_merge_bases_many(commit, n_other, other[])
checks if commit is an ancestor of any of the other[] commits.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 24 ++++++++++++++++++------
 commit.h |  1 +
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index e8eb0ae..3a201e0 100644
--- a/commit.c
+++ b/commit.c
@@ -852,25 +852,37 @@ int is_descendant_of(struct commit *commit, struct commit_list *with_commit)
 }
 
 /*
- * Is "commit" an ancestor of (i.e. reachable from) the "reference"?
+ * Is "commit" an ancestor of one of the "reference"s?
  */
-int in_merge_bases(struct commit *commit, struct commit *reference)
+int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit **reference)
 {
 	struct commit_list *bases;
-	int ret = 0;
+	int ret = 0, i;
 
-	if (parse_commit(commit) || parse_commit(reference))
+	if (parse_commit(commit))
 		return ret;
+	for (i = 0; i < nr_reference; i++)
+		if (parse_commit(reference[i]))
+			return ret;
 
-	bases = paint_down_to_common(commit, 1, &reference);
+	bases = paint_down_to_common(commit, nr_reference, reference);
 	if (commit->object.flags & PARENT2)
 		ret = 1;
 	clear_commit_marks(commit, all_flags);
-	clear_commit_marks(reference, all_flags);
+	for (i = 0; i < nr_reference; i++)
+		clear_commit_marks(reference[i], all_flags);
 	free_commit_list(bases);
 	return ret;
 }
 
+/*
+ * Is "commit" an ancestor of (i.e. reachable from) the "reference"?
+ */
+int in_merge_bases(struct commit *commit, struct commit *reference)
+{
+	return in_merge_bases_many(commit, 1, &reference);
+}
+
 struct commit_list *reduce_heads(struct commit_list *heads)
 {
 	struct commit_list *p;
diff --git a/commit.h b/commit.h
index b6ad8f3..d5b9712 100644
--- a/commit.h
+++ b/commit.h
@@ -170,6 +170,7 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,
 
 int is_descendant_of(struct commit *, struct commit_list *);
 int in_merge_bases(struct commit *, struct commit *);
+int in_merge_bases_many(struct commit *, int, struct commit **);
 
 extern int interactive_add(int argc, const char **argv, const char *prefix, int patch);
 extern int run_add_interactive(const char *revision, const char *patch_mode,
-- 
1.8.2-rc2-182-g857a7fa

[PATCH 2/2] push: --follow-tag

[Junio C Hamano]

The new option "--follow-tag" tells "git push" to push tags that are
missing from the other side and that can be reached by the history
that is otherwise pushed out.  For example, if you are using the
"simple", "current", or "upstream" push, you would ordinarily push
the history leading to the commit at your current HEAD and nothing
else.  With this option, you would also push all tags that can be
reached from that commit to the other side.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The previous and this were built on 'maint', and may have
   trivial conflicts with 'master' and upwards.

   This is primarily to scratch my own itch; after tagging an rc or
   final release, I've been doing

   	git push k.org v1.8.2
        git push k.org

   and the first step can easily be forgotten.  With

	git push k.org --follow-tag

   I no longer should have to worry about that.  It will push out
   whatever would be pushed out without --follow-tag, and also tags
   that are missing from my k.org repository that point into the
   history being pushed out.

   I'd certainly need help with phrasing in the documentation, by
   the way.

 Documentation/git-push.txt |  8 +++-
 builtin/push.c             |  2 +
 remote.c                   | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 remote.h                   |  3 +-
 t/t5516-fetch-push.sh      | 73 ++++++++++++++++++++++++++++++++++++
 transport.c                |  2 +
 transport.h                |  1 +
 7 files changed, 179 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 8b637d3..5020662 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
 SYNOPSIS
 --------
 [verse]
-'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
+'git push' [--all | --mirror | --tags] [--follow-tag] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
 	   [<repository> [<refspec>...]]
 
@@ -111,6 +111,12 @@ no `push.default` configuration variable is set.
 	addition to refspecs explicitly listed on the command
 	line.
 
+--follow-tag::
+	Push all the refs that would be pushed without this option,
+	and also push the refs under `refs/tags` that are missing
+	from the remote but are reachable from the refs that would
+	be pushed without this option.
+
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
 	Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..08e3db1 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -399,6 +399,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "progress", &progress, N_("force progress reporting")),
 		OPT_BIT(0, "prune", &flags, N_("prune locally removed refs"),
 			TRANSPORT_PUSH_PRUNE),
+		OPT_BIT(0, "follow-tag", &flags, N_("push missing but relevant tags"),
+			TRANSPORT_PUSH_FOLLOW_TAG),
 		OPT_END()
 	};
 
diff --git a/remote.c b/remote.c
index ca1f8f2..4441944 100644
--- a/remote.c
+++ b/remote.c
@@ -1195,6 +1195,94 @@ static struct ref **tail_ref(struct ref **head)
 	return tail;
 }
 
+struct tips {
+	struct commit **tip;
+	int nr, alloc;
+};
+
+static void add_to_tips(struct tips *tips, struct ref *ref)
+{
+	struct commit *commit;
+
+	if (is_null_sha1(ref->new_sha1))
+		return;
+	commit = lookup_commit_reference_gently(ref->new_sha1, 1);
+	if (!commit)
+		return; /* not pushing a commit, which is not an error */
+	ALLOC_GROW(tips->tip, tips->nr + 1, tips->alloc);
+	tips->tip[tips->nr++] = commit;
+}
+
+static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***dst_tail)
+{
+	struct string_list dst_tag = STRING_LIST_INIT_NODUP;
+	struct string_list src_tag = STRING_LIST_INIT_NODUP;
+	struct string_list_item *item;
+	struct ref *ref;
+	struct tips sent_tips;
+
+	/*
+	 * Collect everything we are going to send
+	 * and collect all tags they have.
+	 */
+	memset(&sent_tips, 0, sizeof(sent_tips));
+	for (ref = *dst; ref; ref = ref->next) {
+		if (ref->peer_ref &&
+		    !is_null_sha1(ref->peer_ref->new_sha1))
+			add_to_tips(&sent_tips, ref->peer_ref);
+		if (!prefixcmp(ref->name, "refs/tags/"))
+			string_list_append(&dst_tag, ref->name);
+	}
+	sort_string_list(&dst_tag);
+
+	/* Collect tags they do not have. */
+	for (ref = src; ref; ref = ref->next) {
+		if (prefixcmp(ref->name, "refs/tags/"))
+			continue;
+		if (string_list_has_string(&dst_tag, ref->name))
+			continue;
+		item = string_list_append(&src_tag, ref->name);
+		item->util = ref;
+	}
+	string_list_clear(&dst_tag, 0);
+
+	/*
+	 * At this point, src_tag lists tags that are missing from
+	 * dst, and sent_tips lists the tips we are pushing (or those
+	 * that we know they already have). An element in the src_tag
+	 * that is not an ancestor of any of the sent_tips need to be
+	 * sent to the other side.
+	 */
+	if (sent_tips.nr) {
+		for_each_string_list_item(item, &src_tag) {
+			struct ref *ref = item->util;
+			struct ref *dst_ref;
+			struct commit *commit;
+
+			if (is_null_sha1(ref->new_sha1))
+				continue;
+			commit = lookup_commit_reference_gently(ref->new_sha1, 1);
+			if (!commit)
+				/* not pushing a commit, which is not an error */
+				continue;
+
+			/*
+			 * Is this tag, which they do not have, reachable from
+			 * any of the commits we are sending?
+			 */
+			if (!in_merge_bases_many(commit, sent_tips.nr, sent_tips.tip))
+				continue;
+
+			/* Add it in */
+			dst_ref = make_linked_ref(ref->name, dst_tail);
+			hashcpy(dst_ref->new_sha1, ref->new_sha1);
+			dst_ref->peer_ref = copy_ref(ref);
+		}
+	}
+	string_list_clear(&src_tag, 0);
+	free(sent_tips.tip);
+}
+
 /*
  * Given the set of refs the local repository has, the set of refs the
  * remote repository has, and the refspec used for push, determine
@@ -1257,6 +1345,10 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	free_name:
 		free(dst_name);
 	}
+
+	if (flags & MATCH_REFS_FOLLOW_TAG)
+		add_missing_tags(src, dst, &dst_tail);
+
 	if (send_prune) {
 		/* check for missing refs on the remote */
 		for (ref = *dst; ref; ref = ref->next) {
diff --git a/remote.h b/remote.h
index 251d8fd..950ab5a 100644
--- a/remote.h
+++ b/remote.h
@@ -148,7 +148,8 @@ enum match_refs_flags {
 	MATCH_REFS_NONE		= 0,
 	MATCH_REFS_ALL 		= (1 << 0),
 	MATCH_REFS_MIRROR	= (1 << 1),
-	MATCH_REFS_PRUNE	= (1 << 2)
+	MATCH_REFS_PRUNE	= (1 << 2),
+	MATCH_REFS_FOLLOW_TAG	= (1 << 3)
 };
 
 /* Reporting of tracking info */
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..4ff2eb2 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -995,4 +995,77 @@ test_expect_success 'push --prune refspec' '
 	! check_push_result $the_first_commit tmp/foo tmp/bar
 '
 
+test_expect_success 'fetch follows tags by default' '
+	mk_test heads/master &&
+	rm -fr src dst &&
+	git init src &&
+	(
+		cd src &&
+		git pull ../testrepo master &&
+		git tag -m "annotated" tag &&
+		git for-each-ref >tmp1 &&
+		(
+			cat tmp1
+			sed -n "s|refs/heads/master$|refs/remotes/origin/master|p" tmp1
+		) |
+		sort -k 3 >../expect
+	) &&
+	git init dst &&
+	(
+		cd dst &&
+		git remote add origin ../src &&
+		git config branch.master.remote origin &&
+		git config branch.master.merge refs/heads/master &&
+		git pull &&
+		git for-each-ref >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push does not follow tags by default' '
+	mk_test heads/master &&
+	rm -fr src dst &&
+	git init src &&
+	git init --bare dst &&
+	(
+		cd src &&
+		git pull ../testrepo master &&
+		git tag -m "annotated" tag &&
+		git checkout -b another &&
+		git commit --allow-empty -m "future commit" &&
+		git tag -m "future" future &&
+		git checkout master &&
+		git for-each-ref refs/heads/master >../expect &&
+		git push ../dst master
+	) &&
+	(
+		cd dst &&
+		git for-each-ref >../actual
+	) &&
+	test_cmp expect actual
+'
+
+test_expect_success 'push --follow-tag only pushes relevant tags' '
+	mk_test heads/master &&
+	rm -fr src dst &&
+	git init src &&
+	git init --bare dst &&
+	(
+		cd src &&
+		git pull ../testrepo master &&
+		git tag -m "annotated" tag &&
+		git checkout -b another &&
+		git commit --allow-empty -m "future commit" &&
+		git tag -m "future" future &&
+		git checkout master &&
+		git for-each-ref refs/heads/master refs/tags/tag >../expect
+		git push --follow-tag ../dst master
+	) &&
+	(
+		cd dst &&
+		git for-each-ref >../actual
+	) &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/transport.c b/transport.c
index b9306ef..6e2f22d 100644
--- a/transport.c
+++ b/transport.c
@@ -1059,6 +1059,8 @@ int transport_push(struct transport *transport,
 			match_flags |= MATCH_REFS_MIRROR;
 		if (flags & TRANSPORT_PUSH_PRUNE)
 			match_flags |= MATCH_REFS_PRUNE;
+		if (flags & TRANSPORT_PUSH_FOLLOW_TAG)
+			match_flags |= MATCH_REFS_FOLLOW_TAG;
 
 		if (match_push_refs(local_refs, &remote_refs,
 				    refspec_nr, refspec, match_flags)) {
diff --git a/transport.h b/transport.h
index 4a61c0c..bbebfbc 100644
--- a/transport.h
+++ b/transport.h
@@ -104,6 +104,7 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
 #define TRANSPORT_PUSH_PRUNE 128
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
+#define TRANSPORT_PUSH_FOLLOW_TAG 1024
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
-- 
1.8.2-rc2-182-g857a7fa

Re: [PATCH 2/2] push: --follow-tag

[Jeff King]

On Mon, Mar 04, 2013 at 04:54:39PM -0800, Junio C Hamano wrote:

>    This is primarily to scratch my own itch; after tagging an rc or
>    final release, I've been doing
> 
>    	git push k.org v1.8.2
>         git push k.org
> 
>    and the first step can easily be forgotten.  With
> 
> 	git push k.org --follow-tag
> 
>    I no longer should have to worry about that.  It will push out
>    whatever would be pushed out without --follow-tag, and also tags
>    that are missing from my k.org repository that point into the
>    history being pushed out.

I've run into the same problem. It's a minor annoyance, but your patch
should make it smoother.

Should this be called "--follow-tags"? That makes more sense to me, as
you are catching all tags. For consistency, should this match the naming
of git-fetch's options (or vice versa)? There we have:

  <default>: auto-follow tags

  --tags: fetch all of refs/heads/tags

  --no-tags: do not auto-follow

I think that naming has caused some confusion in the past. And there is
no way to explicitly specify the default behavior. I wonder if both
should support:

  --follow-tags: auto-follow tags

  --no-follow-tags: do not auto-follow tags

  --tags: fetch/push all of refs/heads/tags

  --no-tags: turn off auto-following, and cancel any previous --tags

The default for push should probably keep auto-follow off, though.

> +--follow-tag::
> +	Push all the refs that would be pushed without this option,
> +	and also push the refs under `refs/tags` that are missing
> +	from the remote but are reachable from the refs that would
> +	be pushed without this option.
> +

This reads OK to me, though it is a little confusing in that there are
two sets of refs being discussed, and "the refs that would be pushed
without this option" is quite a long noun phrase (that gets used twice).

You also don't define "reachable" here. Being familiar with git, I
assume it is "the commit that the refs/tag entry peels to is an ancestor
of a commit that is at the tip of a pushed ref".  Maybe that is obvious
enough that it doesn't need to be spelled out.

I think you could just drop the second "refs that would be pushed
without this option", and just say "pushed refs". That is ambiguous (is
it all pushed refs, or refs that would be pushed without this option?),
but since reachability is transitive, they are the same set (i.e., if we
add a ref due to this option, then its ancestors are already candidates,
and it does not affect the outcome).

Maybe:

  In addition to any refs that would be pushed without this option,
  also push any refs under `refs/tags` that are missing from the remote
  but are reachable from the pushed refs.

I dunno. I don't really like that version much, either.

> +	/*
> +	 * At this point, src_tag lists tags that are missing from
> +	 * dst, and sent_tips lists the tips we are pushing (or those
> +	 * that we know they already have). An element in the src_tag
> +	 * that is not an ancestor of any of the sent_tips need to be
> +	 * sent to the other side.
> +	 */
> +	if (sent_tips.nr) {
> +		for_each_string_list_item(item, &src_tag) {
> +			struct ref *ref = item->util;
> +			struct ref *dst_ref;
> +			struct commit *commit;
> +
> +			if (is_null_sha1(ref->new_sha1))
> +				continue;
> +			commit = lookup_commit_reference_gently(ref->new_sha1, 1);
> +			if (!commit)
> +				/* not pushing a commit, which is not an error */
> +				continue;

This will find anything under refs/tags, including annotated and
non-annotated tags. I wonder if it is worth making a distinction. In
many workflows, unannotated tags should not be leaked out to public
repos. But because this feature finds any reachable tags, it will push a
tag you made a long time ago as a bookmarker on some part of the history
unrelated to the release you are making now.

One obvious alternative is only to push annotated tags with this
feature. That has the downside of not matching fetch's behavior, as well
as withholding the feature from people whose workflow uses only
unannotated tags.

Another alternative would be to change the inclusion rule from
"reachable" to "points at the tip of something being sent". But then we
lose the feature that it would backfill any old tags the remote happens
to be missing.

-Peff

Re: [PATCH 2/2] push: --follow-tag

[Michael Haggerty]

On 03/05/2013 09:22 AM, Jeff King wrote:
> On Mon, Mar 04, 2013 at 04:54:39PM -0800, Junio C Hamano wrote:
> [...]
> This will find anything under refs/tags, including annotated and
> non-annotated tags. I wonder if it is worth making a distinction. In
> many workflows, unannotated tags should not be leaked out to public
> repos. But because this feature finds any reachable tags, it will push a
> tag you made a long time ago as a bookmarker on some part of the history
> unrelated to the release you are making now.
> 
> One obvious alternative is only to push annotated tags with this
> feature. That has the downside of not matching fetch's behavior, as well
> as withholding the feature from people whose workflow uses only
> unannotated tags.
> 
> Another alternative would be to change the inclusion rule from
> "reachable" to "points at the tip of something being sent". But then we
> lose the feature that it would backfill any old tags the remote happens
> to be missing.

I have no opinion on this matter, but ISTM that another obvious
alternative would be to push tags that point at *any* commits that are
being sent (not just at the tip), but not those that point to older
commits already known to the server.  This would reduce the potential
for accidental pushes of "distant" tags.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 2/2] push: --follow-tag

[Jeff King]

On Tue, Mar 05, 2013 at 12:49:57PM +0100, Michael Haggerty wrote:

> > One obvious alternative is only to push annotated tags with this
> > feature. That has the downside of not matching fetch's behavior, as well
> > as withholding the feature from people whose workflow uses only
> > unannotated tags.
> > 
> > Another alternative would be to change the inclusion rule from
> > "reachable" to "points at the tip of something being sent". But then we
> > lose the feature that it would backfill any old tags the remote happens
> > to be missing.
> 
> I have no opinion on this matter, but ISTM that another obvious
> alternative would be to push tags that point at *any* commits that are
> being sent (not just at the tip), but not those that point to older
> commits already known to the server.  This would reduce the potential
> for accidental pushes of "distant" tags.

Yeah, I think that is another sensible variant. It does not really
"backfill" in the way that Junio's patch does (e.g., if you forgot to
push out v1.6 to a remote 2 weeks ago and now you are pushing out v1.7,
Junio's patch will magically fill it in). But I do not see a huge value
in backfilling. It is anyone's guess whether you simply forgot to push
out v1.6 or whether you intended to hold it back. And I'd rather err on
the side of not-pushing because of the difficulty of retraction.

-Peff

Re: [PATCH 2/2] push: --follow-tag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I think that is another sensible variant. It does not really
> "backfill" in the way that Junio's patch does (e.g., if you forgot to
> push out v1.6 to a remote 2 weeks ago and now you are pushing out v1.7,
> Junio's patch will magically fill it in).

I may have tentatively tagged the tip of 'master' as v1.8.2 in my
private repository, started the integration testing, but may not be
confident enough to push out the branch nor the tag yet.  I may have
an experimental topic that I want to share with others before I am
done with the release to unblock them, and the topic may build on
the 'master' I may or may not want to redo before the release.

I can do so with "git push github jc/topic" (no --follow-tags).
After doing such a "you may want to start with this" push, I can
continue working on the release, and the 'master' branch may turn
out to be good to go without redoing.

A later "git push github --follow-tags master" in such a case should
send v1.8.2 out.  It is inexcuable to break it, saying "Oh, I've
seen that commit already---it is part of the previous update to
jc/topic".  That defeats the whole point of --follow-tags.

The other "tags at the tip" is slightly less problematic than
"ignore the commits the receiving end has already seen", but it will
break if I tag the tip of 'maint' as v1.8.1.6, continue working
without being able to push perhaps due to network disruption, and
have already started building towards v1.8.1.7 when the network
comes back.  'maint' may be past v1.8.1.6 and its tip won't be
pointing at that tag, but it still is missing from the public
repository.

While I agree with your goal to make it safer to use "push", both
"ignore commits that the receiving end already saw" and "only look
at the commits at the tip being sent" castrate the option too much
to the point that it is meaningless.  The whole point of asking
explicitly with the "--follow-tags" option to "push" to push out
relevant tags is, eh, to push out relevant tags.  I do not think it
is magical at all.  "backfill" is an integral part of the option.

Re: [PATCH 2/2] push: --follow-tag

[Jeff King]

On Tue, Mar 05, 2013 at 11:17:11AM -0800, Junio C Hamano wrote:

> I may have tentatively tagged the tip of 'master' as v1.8.2 in my
> private repository, started the integration testing, but may not be
> confident enough to push out the branch nor the tag yet.  I may have
> an experimental topic that I want to share with others before I am
> done with the release to unblock them, and the topic may build on
> the 'master' I may or may not want to redo before the release.
> 
> I can do so with "git push github jc/topic" (no --follow-tags).
> After doing such a "you may want to start with this" push, I can
> continue working on the release, and the 'master' branch may turn
> out to be good to go without redoing.
> 
> A later "git push github --follow-tags master" in such a case should
> send v1.8.2 out.  It is inexcuable to break it, saying "Oh, I've
> seen that commit already---it is part of the previous update to
> jc/topic".  That defeats the whole point of --follow-tags.

That depends on the specifics of the rule. If the rule is "any commit
that the server already has", then yes, it runs afoul of that problem.
But what if it is "any commit in the ref update being sent"? That is, we
would look at the range "origin/master..master" and send any tags that
point to commits in that range.

> The other "tags at the tip" is slightly less problematic than
> "ignore the commits the receiving end has already seen", but it will
> break if I tag the tip of 'maint' as v1.8.1.6, continue working
> without being able to push perhaps due to network disruption, and
> have already started building towards v1.8.1.7 when the network
> comes back.  'maint' may be past v1.8.1.6 and its tip won't be
> pointing at that tag, but it still is missing from the public
> repository.

Right, I think "tags at the tip" is too likely to miss things you did
want to send.

> While I agree with your goal to make it safer to use "push", both
> "ignore commits that the receiving end already saw" and "only look
> at the commits at the tip being sent" castrate the option too much
> to the point that it is meaningless.  The whole point of asking
> explicitly with the "--follow-tags" option to "push" to push out
> relevant tags is, eh, to push out relevant tags.  I do not think it
> is magical at all.  "backfill" is an integral part of the option.

I know, and I'm willing to accept that the right resolution is "we
should not have this feature" (or possibly "the documentation must warn
about caveats of the feature). I'm just worried about it accidentally
being misused and causing problems.

-Peff

Re: [PATCH 2/2] push: --follow-tag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Should this be called "--follow-tags"? That makes more sense to me, as
> you are catching all tags.

Perhaps.  We are sending all zero-or-more relevant tags, so I agree
that plural form is more appropriate.  I have a doubt about
"follow", though; inertia made me use "follow", but I am not sure
what we are following.  We certainly are not following tags.  If
anything, we are making tags to piggy back on the history being
pushed.

> For consistency, should this match the naming of git-fetch's
> options (or vice versa)? There we have:
>
>   <default>: auto-follow tags
>
>   --tags: fetch all of refs/heads/tags
>
>   --no-tags: do not auto-follow
>
> I think that naming has caused some confusion in the past.

"--tags" does not belong in the discussion of "auto following".  It
does not have to do with any "auto"-ness.  Renaming "--no-tags" to
"--no-follow-tags" does make sense, though.

> And there is no way to explicitly specify the default behavior. I
> wonder if both should support:
>
>   --follow-tags: auto-follow tags
>
>   --no-follow-tags: do not auto-follow tags

Yup, I like that.  Perhaps make "--no-tags" a deprecated synonym to
the latter.

>   --tags: fetch/push all of refs/heads/tags
>
>   --no-tags: turn off auto-following, and cancel any previous --tags

Sounds sensible.

> The default for push should probably keep auto-follow off, though.
>
>> +--follow-tag::
>> +	Push all the refs that would be pushed without this option,
>> +	and also push the refs under `refs/tags` that are missing
>> +	from the remote but are reachable from the refs that would
>> +	be pushed without this option.
>> +
>
> This reads OK to me, though it is a little confusing in that there are
> two sets of refs being discussed, and "the refs that would be pushed
> without this option" is quite a long noun phrase (that gets used twice).

Yes, exactly why I said I do not like the phrasing of this one.

> This will find anything under refs/tags, including annotated and
> non-annotated tags. I wonder if it is worth making a distinction. In
> many workflows, unannotated tags should not be leaked out to public
> repos. But because this feature finds any reachable tags, it will push a
> tag you made a long time ago as a bookmarker on some part of the history
> unrelated to the release you are making now.

What does the auto-follow feature of "git fetch" do currently?
Whatever we do here on the "git push" side should match it.

If somebody wants to add some form of filtering mechanism based on
the tagname (e.g. '--auto-follow-tags=v[0-9]*'), I would not have a
strong objection to it, but I think that is something we should do
on top and consistently between fetch and push.  I am not thrilled
by the idea of conflating annotated-ness and the public-ness of
tags.

Thanks.

Re: [PATCH 2/2] push: --follow-tag

[Jeff King]

On Tue, Mar 05, 2013 at 07:58:45AM -0800, Junio C Hamano wrote:

> > This will find anything under refs/tags, including annotated and
> > non-annotated tags. I wonder if it is worth making a distinction. In
> > many workflows, unannotated tags should not be leaked out to public
> > repos. But because this feature finds any reachable tags, it will push a
> > tag you made a long time ago as a bookmarker on some part of the history
> > unrelated to the release you are making now.
> 
> What does the auto-follow feature of "git fetch" do currently?
> Whatever we do here on the "git push" side should match it.

It fetches anything in refs/tags, unannotated or not. And that is
certainly a point in favor of "git push" doing the same.

But I wonder if fetching and pushing are different in that respect. You
are (usually) fetching from a public publishing point, and it is assumed
that whatever is there is useful for sharing. The only reason to limit
it is to save time transferring objects the user does not want.

But for "push", you are on the publishing side, which usually means you
need to be a little more careful. It is not just an optimization; it is
about deciding what should be shared. You do not want to accidentally
push cruft or work in progress in your private repository. I think it's
the same logic that leads us to fetch "refs/heads/*" by default, but
only push "matching" (or more recently "HEAD").

> If somebody wants to add some form of filtering mechanism based on
> the tagname (e.g. '--auto-follow-tags=v[0-9]*'), I would not have a
> strong objection to it, but I think that is something we should do
> on top and consistently between fetch and push.  I am not thrilled
> by the idea of conflating annotated-ness and the public-ness of
> tags.

I don't like it either. But I also don't want to introduce a feature
that causes people to accidentally publish cruft. It may not be a
problem in practice; I'm just thinking out loud at this point.

-Peff

Re: [PATCH 2/2] push: --follow-tag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I wonder if fetching and pushing are different in that respect. You
> are (usually) fetching from a public publishing point, and it is assumed
> that whatever is there is useful for sharing. The only reason to limit
> it is to save time transferring objects the user does not want.

There are those who have to emulate "git fetch" with a reverse "git
push" (or vice versa) due to network connection limitations, so I do
not think hardcoding such a policy decision in the direction is
necessarily a good idea.

> ... But I also don't want to introduce a feature
> that causes people to accidentally publish cruft. It may not be a
> problem in practice; I'm just thinking out loud at this point.

Likewise.

Re: [PATCH 2/2] push: --follow-tag

[Jeff King]

On Tue, Mar 05, 2013 at 10:15:20AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But I wonder if fetching and pushing are different in that respect. You
> > are (usually) fetching from a public publishing point, and it is assumed
> > that whatever is there is useful for sharing. The only reason to limit
> > it is to save time transferring objects the user does not want.
> 
> There are those who have to emulate "git fetch" with a reverse "git
> push" (or vice versa) due to network connection limitations, so I do
> not think hardcoding such a policy decision in the direction is
> necessarily a good idea.

Yeah, but I think it makes sense to optimize the defaults for the common
cases, and let people doing unusual things override the behavior via
options (or even config).

Don't get me wrong, I think there is value in the simplicity of having
the push/fetch transactions be as symmetric as possible. But given the
potentially high cost of a mistaken push (i.e., retracting published
history can be embarrassing or complicated), there's also value in safe
defaults. And I feel like we've already gone in that direction with the
default refspecs being different between fetch and push.

-Peff