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

2013-03-05 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread 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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread Junio C Hamano 
Jeff King p...@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.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread Junio C Hamano 
Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

 Jeff King p...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread Junio C Hamano 
Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-05 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-03-04 Thread 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 gits...@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