[PATCH v2 0/4] push --follow-tags

[PATCH v2 0/4] push --follow-tags

[Junio C Hamano]

The primary change since the last round is that it pushes out only
annotated tags that are missing from the other side.

Junio C Hamano (4):
  commit.c: add clear_commit_marks_many()
  commit.c: add in_merge_bases_many()
  commit.c: use clear_commit_marks_many() in in_merge_bases_many()
  push: --follow-tags

 Documentation/git-push.txt |  8 +++-
 builtin/push.c             |  2 +
 commit.c                   | 42 ++++++++++++++------
 commit.h                   |  2 +
 remote.c                   | 99 ++++++++++++++++++++++++++++++++++++++++++++++
 remote.h                   |  3 +-
 t/t5516-fetch-push.sh      | 73 ++++++++++++++++++++++++++++++++++
 transport.c                |  2 +
 transport.h                |  1 +
 9 files changed, 218 insertions(+), 14 deletions(-)

-- 
1.8.2-rc2-194-g549a9ef

[PATCH v2 1/4] commit.c: add clear_commit_marks_many()

[Junio C Hamano]

clear_commit_marks(struct commit *, unsigned) only can clear flag
bits starting from a single commit; introduce an API to allow
feeding an array of commits, so that flag bits can be cleared from
commits reachable from any of them with a single traversal.

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

diff --git a/commit.c b/commit.c
index e8eb0ae..4757e50 100644
--- a/commit.c
+++ b/commit.c
@@ -463,14 +463,23 @@ static void clear_commit_marks_1(struct commit_list **plist,
 	}
 }
 
-void clear_commit_marks(struct commit *commit, unsigned int mark)
+void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark)
 {
 	struct commit_list *list = NULL;
-	commit_list_insert(commit, &list);
+
+	while (nr--) {
+		commit_list_insert(*commit, &list);
+		commit++;
+	}
 	while (list)
 		clear_commit_marks_1(&list, pop_commit(&list), mark);
 }
 
+void clear_commit_marks(struct commit *commit, unsigned int mark)
+{
+	clear_commit_marks_many(1, &commit, mark);
+}
+
 void clear_commit_marks_for_object_array(struct object_array *a, unsigned mark)
 {
 	struct object *object;
@@ -797,8 +806,7 @@ struct commit_list *get_merge_bases_many(struct commit *one,
 	if (!result || !result->next) {
 		if (cleanup) {
 			clear_commit_marks(one, all_flags);
-			for (i = 0; i < n; i++)
-				clear_commit_marks(twos[i], all_flags);
+			clear_commit_marks_many(n, twos, all_flags);
 		}
 		return result;
 	}
@@ -816,8 +824,7 @@ struct commit_list *get_merge_bases_many(struct commit *one,
 	free_commit_list(result);
 
 	clear_commit_marks(one, all_flags);
-	for (i = 0; i < n; i++)
-		clear_commit_marks(twos[i], all_flags);
+	clear_commit_marks_many(n, twos, all_flags);
 
 	cnt = remove_redundant(rslt, cnt);
 	result = NULL;
diff --git a/commit.h b/commit.h
index b6ad8f3..b997eea 100644
--- a/commit.h
+++ b/commit.h
@@ -134,6 +134,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list,
 struct commit *pop_commit(struct commit_list **stack);
 
 void clear_commit_marks(struct commit *commit, unsigned int mark);
+void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark);
 void clear_commit_marks_for_object_array(struct object_array *a, unsigned mark);
 
 /*
-- 
1.8.2-rc2-194-g549a9ef

Re: [PATCH v2 1/4] commit.c: add clear_commit_marks_many()

[Jeff King]

On Tue, Mar 05, 2013 at 02:47:16PM -0800, Junio C Hamano wrote:

> clear_commit_marks(struct commit *, unsigned) only can clear flag
> bits starting from a single commit; introduce an API to allow
> feeding an array of commits, so that flag bits can be cleared from
> commits reachable from any of them with a single traversal.

Out of curiosity, is that actually measurably more efficient?

Since we stop traversing a commit's parents when we see it does not have
any of the flags we are clearing, we should visit most commits once. The
exception is ones that we hit by multiple paths. But that should
be the same whether it is done as part of a single traversal or
multiple; in each case, we hit the commit and say "Oh, already cleared;
do not add it to the parents list".

So I would expect it to have little to no impact on performance.  I
still think it is a sane interface change; I was just curious from the
commit message whether there could be a performance impact.

-Peff

Re: [PATCH v2 1/4] commit.c: add clear_commit_marks_many()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 05, 2013 at 02:47:16PM -0800, Junio C Hamano wrote:
>
>> clear_commit_marks(struct commit *, unsigned) only can clear flag
>> bits starting from a single commit; introduce an API to allow
>> feeding an array of commits, so that flag bits can be cleared from
>> commits reachable from any of them with a single traversal.
>
> Out of curiosity, is that actually measurably more efficient?
>
> Since we stop traversing a commit's parents when we see it does not have
> any of the flags we are clearing, we should visit most commits once. The
> exception is ones that we hit by multiple paths. But that should
> be the same whether it is done as part of a single traversal or
> multiple; in each case, we hit the commit and say "Oh, already cleared;
> do not add it to the parents list".
>
> So I would expect it to have little to no impact on performance.  I
> still think it is a sane interface change; I was just curious from the
> commit message whether there could be a performance impact.

I agree that the log message should end with "with a single API
call." to clarify that this is more about eliminating the code
overhead (the caller having to loop over an array of things) than
about runtime overhead.

[PATCH v2 2/4] commit.c: 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 4757e50..d12e799 100644
--- a/commit.c
+++ b/commit.c
@@ -859,25 +859,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 "references"?
  */
-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 b997eea..5057f14 100644
--- a/commit.h
+++ b/commit.h
@@ -171,6 +171,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-194-g549a9ef

[PATCH v2 3/4] commit.c: use clear_commit_marks_many() in in_merge_bases_many()

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index d12e799..b4512ab 100644
--- a/commit.c
+++ b/commit.c
@@ -876,8 +876,7 @@ int in_merge_bases_many(struct commit *commit, int nr_reference, struct commit *
 	if (commit->object.flags & PARENT2)
 		ret = 1;
 	clear_commit_marks(commit, all_flags);
-	for (i = 0; i < nr_reference; i++)
-		clear_commit_marks(reference[i], all_flags);
+	clear_commit_marks_many(nr_reference, reference, all_flags);
 	free_commit_list(bases);
 	return ret;
 }
-- 
1.8.2-rc2-194-g549a9ef

[PATCH v2 4/4] push: --follow-tags

[Junio C Hamano]

The new option "--follow-tags" tells "git push" to push annotated
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 annotated tags that can be reached from that commit to
the other side.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-push.txt |  8 +++-
 builtin/push.c             |  2 +
 remote.c                   | 99 ++++++++++++++++++++++++++++++++++++++++++++++
 remote.h                   |  3 +-
 t/t5516-fetch-push.sh      | 73 ++++++++++++++++++++++++++++++++++
 transport.c                |  2 +
 transport.h                |  1 +
 7 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 8b637d3..40377ba 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-tags] [-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-tags::
+	Push all the refs that would be pushed without this option,
+	and also push annotated tags in `refs/tags` that are missing
+	from the remote but are pointing at committish that are
+	reachable from the refs being pushed.
+
 --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..34a8271 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-tags", &flags, N_("push missing but relevant tags"),
+			TRANSPORT_PUSH_FOLLOW_TAGS),
 		OPT_END()
 	};
 
diff --git a/remote.c b/remote.c
index ca1f8f2..f035af3 100644
--- a/remote.c
+++ b/remote.c
@@ -1195,6 +1195,101 @@ 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, const unsigned char *sha1)
+{
+	struct commit *commit;
+
+	if (is_null_sha1(sha1))
+		return;
+	commit = lookup_commit_reference_gently(sha1, 1);
+	if (!commit || (commit->object.flags & TMP_MARK))
+		return;
+	commit->object.flags |= TMP_MARK;
+	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 know they would have at the end of
+	 * this push, 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->new_sha1);
+		else
+			add_to_tips(&sent_tips, ref->old_sha1);
+		if (!prefixcmp(ref->name, "refs/tags/"))
+			string_list_append(&dst_tag, ref->name);
+	}
+	clear_commit_marks_many(sent_tips.nr, sent_tips.tip, TMP_MARK);
+
+	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; /* not a tag */
+		if (string_list_has_string(&dst_tag, ref->name))
+			continue; /* they already have it */
+		if (sha1_object_info(ref->new_sha1, NULL) != OBJ_TAG)
+			continue; /* be conservative */
+		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 an ancestor of any of the sent_tips needs 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 +1352,10 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	free_name:
 		free(dst_name);
 	}
+
+	if (flags & MATCH_REFS_FOLLOW_TAGS)
+		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..a0731f1 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_TAGS	= (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..9fe4aa3 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_TAGS)
+			match_flags |= MATCH_REFS_FOLLOW_TAGS;
 
 		if (match_push_refs(local_refs, &remote_refs,
 				    refspec_nr, refspec, match_flags)) {
diff --git a/transport.h b/transport.h
index 4a61c0c..8c493f7 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_TAGS 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-194-g549a9ef

Re: [PATCH v2 0/4] push --follow-tags

[Jeff King]

On Tue, Mar 05, 2013 at 02:47:15PM -0800, Junio C Hamano wrote:

> The primary change since the last round is that it pushes out only
> annotated tags that are missing from the other side.

Like you, I have mixed feelings on treating annotated tags separately. I
don't feel like the previous discussion actually came to a conclusion.

I kind of lean towards the "if we are pushing A..B to a ref, no matter
if the remote had the objects already, consider any tag that peels to
any commit in A..B". That seems intuitive to me, and reduces the chances
of accidentally pushing crufty tags. But I admit it's just a gut
feeling.

I think the implementation would just be something like:

  for each ref we are pushing
          mark ref->old_sha1 as UNINTERESTING
          add ref->new_sha1 to pending
  traverse pending commits, marking SEEN

  for each local tag
          if tag does not exist on remote &&
             commit = lookup_commit_reference_gently(tag) &&
             commit->object.flags & SEEN
                    push tag

-Peff

Re: [PATCH v2 0/4] push --follow-tags

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 05, 2013 at 02:47:15PM -0800, Junio C Hamano wrote:
>
>> The primary change since the last round is that it pushes out only
>> annotated tags that are missing from the other side.
>
> Like you, I have mixed feelings on treating annotated tags separately. I
> don't feel like the previous discussion actually came to a conclusion.
>
> I kind of lean towards the "if we are pushing A..B to a ref,...

I rejected that approach when I started writing the initial round of
this topic, primarily because A (i.e. dst_ref->old_sha1) may not
necessarily be what the current pusher pushed out during his last
push.

The criteria for the tip of a branch to be good enough to be pushed
out and the criteria for a commit to be good enough to be tagged are
different.  You may know that a commit together with the history
behind it is solid and may be sure that you will not have to rewind
beyond that commit---that is when you _can_ push out (you do not
have to, and I usually don't, so this is not a problem for me
personally).

But at that point in time, you may still not know if the commit and
the history behind it is the good point to be tagged for a release,
or you would still need some other commits on top to polish other
areas for the tagged release is complete.

As I made a habit of not pushing anything out if I am planning to
tag in a near future (and my tagged commits are ones designed to be
tagged, not just an ordinary commit that happens to be tagged), it
is not a problem for me, but it would be for "centralized workflow"
people, where there is more pressure to publish a commit as early as
possible.  Once you know the commit is good enough so that you do
not anticipate that it will need to be rewound, you want to push it
out so that you can unblock others who want to build on it.  It is
plausible to see:

    (1) The pumpking pushes 'master' after being satisfied the
        correctness of the history that leads to it.  The pumpking
        hasn't decided if the commit is to be tagged, or needs some
        other commits on top, for the next release.  But he prefers
        to push the current state out so that he can unblock other
        people.

    (2) Another committer pulls and pushes with his changes.

    (3) The pumpking tags what he pushed out with (1), attempts a
        push, fails the ff check, pulls (2) and then pushes the
        result out with "push --follow-tags".