Local tag killer

Local tag killer

[Michael Haggerty]

A colleague of mine discovered, the hard way, that

    git fetch --tags --prune $REMOTE

deletes all local tags that are not present on that particular remote.
To me this seems a dangerous and poorly-documented interaction of
features and arguably a bug.

Granted, it might not be such a good idea to use local tags, as it is
all to easy to push them inadvertently and then it is difficult to
remove them permanently from a shared upstream repository because other
people might have fetched them and in turn inadvertently re-push them.

But the fact that combining two options, each of which seems safe and
reasonable for daily use, results in the death of local tags unrelated
to the remote is unexpected [1].  Also remember that the "--prune"
feature can be turned on permanently via "git config" using
"fetch.prune" or "remote.$REMOTE.prune".

Moreover, the documentation is misleading on this point:

> -p::
> --prune::
> 	After fetching, remove any remote-tracking branches which
> 	no longer exist	on the remote.

It is a stretch for references under refs/tags/ to be called
"remote-tracking branches", even if they exist as the target of the
refspec "refs/tags/*:refs/tags/*" that is implicitly added by the --tags
option.

I suggest that --prune should not touch references under refs/tags/
regardless of whether they appear on the right side of explicit or
implicit refspecs.  If pruning tags is deemed to be essential, then
there should be a specific option ("--prune-tags"?) to request it.


When looking into this, I found a test in t5510 that appears to want to
verify this very behavior:

> test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '
> 	cd "$D" &&
> 	git clone . prune-tags &&
> 	cd prune-tags &&
> 	git fetch origin refs/heads/master:refs/tags/sometag &&
> 
> 	git fetch --prune --tags origin &&
> 	git rev-parse origin/master &&
> 	test_must_fail git rev-parse somebranch
> '

However, the last line seems to contain a copy-paste error and should
presumably have s/somebranch/sometag/.

Michael

[1] It would be as if "git clean" had two options "--ammonia" and
"--bleach" :-)

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

Re: Local tag killer

[Junio C Hamano]

> When looking into this, I found a test in t5510 that appears to want to
> verify this very behavior:
>
>> test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '

The title tells me that it wants to make sure when pruning tags it does
not touch remote-tracking branches under refs/remotes/origin/, I think.

>>       cd "$D" &&
>>       git clone . prune-tags &&
>>       cd prune-tags &&
>>       git fetch origin refs/heads/master:refs/tags/sometag &&
>>
>>       git fetch --prune --tags origin &&
>>       git rev-parse origin/master &&
>>       test_must_fail git rev-parse somebranch
>> '
>
> However, the last line seems to contain a copy-paste error and should
> presumably have s/somebranch/sometag/.

I agree that somebranch must be sometag, as it wants to make sure
that --prune --tags removes the tag the other side does not have.

I also agree that the documentation is misstated; "remote-tracking branch"
may have been a convenient and well understood phrase for whoever wrote
that part, but the --prune is designed to cull extra refs in the
hierarchies into
which refs would be fetched if counterparts existed on the other side, so
culling tags that do not exist on the remote side should also be described.

Re: Local tag killer

[Junio C Hamano]

Junio C Hamano <gitster-vger@pobox.com> writes:

> I also agree that the documentation is misstated; "remote-tracking branch"
> may have been a convenient and well understood phrase for whoever wrote
> that part, but the --prune is designed to cull extra refs in the
> hierarchies into
> which refs would be fetched if counterparts existed on the other side, so
> culling tags that do not exist on the remote side should also be described.

(gleaning-leftovers mode)


 Documentation/fetch-options.txt | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index ba1fe49..a6c581b 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -41,8 +41,20 @@ ifndef::git-pull[]
 
 -p::
 --prune::
-	After fetching, remove any remote-tracking branches which
-	no longer exist	on the remote.
+
+	After fetching, remove any local ref that was not updated
+	only because the remote ref that was supposed to update it
+	was missing.
++
+For example, `git fetch origin refs/heads/*:refs/remotes/origin/*`
+tries to update local `refs/remotes/origin/frotz` if `origin` has
+`refs/heads/frotz`.  With this option, `refs/remotes/origin/frotz`
+will be removed from our repository if `origin` does not have
+`refs/heads/frotz`.
++
+This is used to remove remote-tracking branches which no longer
+exist on the remote.
+
 endif::git-pull[]
 
 ifdef::git-pull[]

Re: Local tag killer

[Michael Haggerty]

On 09/21/2013 12:51 AM, Junio C Hamano wrote:
> Junio C Hamano <gitster-vger@pobox.com> writes:
> 
>> I also agree that the documentation is misstated; "remote-tracking branch"
>> may have been a convenient and well understood phrase for whoever wrote
>> that part, but the --prune is designed to cull extra refs in the
>> hierarchies into
>> which refs would be fetched if counterparts existed on the other side, so
>> culling tags that do not exist on the remote side should also be described.
> 
> (gleaning-leftovers mode)

Thanks for following up on this with your proposed documentation patch.
 I have been researching and experimenting, and still find the use of
fetch confusing with respect to tags.  I think the problem is primarily
that the behavior is awkward, and that it would be better to change the
behavior than to document the awkward behavior.

I must have read an old version of the documentation, from which it
seemed that "git fetch --tags" fetches all tags from the remote *in
addition to* the references and tags that would otherwise be fetched.
This seems like a handy and safe feature, and I wish that this were
indeed the effect of "--tags".

But I see that the documentation for "--tags" has been changed and now
states explicitly that "--tags" is equivalent to specifying
"refs/tags/*:refs/tags/*" on the command line, overriding any configured
refspecs.  This doesn't seem like useful behavior; why would I want to
fetch tags from a remote without also updating the configured refspecs?
 And contrariwise, how can I fetch the configured refspecs *and* all
tags at the same time in a single fetch?

OK, one way to do it is to configure an explicit refspec for fetching
the tags:

[remote "origin"]
	url = [...]
	fetch = +refs/heads/*:refs/remotes/origin/*
	fetch = refs/tags/*:refs/tags/*

[Here is one oddity: even if the tags refspec doesn't have a "+" prefix,
"git fetch" will do non-ff updates to tags, presumably because of the
implicit tag-fetching behavior.]

But if I use this configuration and type "git fetch --prune", then any
local tags that are not present on the remote will be killed.

In short, when local tags are in use, or tags that are in one remote but
not another [1], then the current Git implementation makes it impossible to

- Configure "fetch.prune" or "remote.$REMOTE.prune" without preventing
the use of "fetch --tags"

- Configure default fetching of all tags (via a refspec or via
remote.$REMOTE.tagopt) without preventing the use of "fetch --prune"

- Configure "fetch.prune" or "remote.$REMOTE.prune" and the default
fetching of all tags (via a refspec or via remote.$REMOTE.tagopt) at the
same time.

This is unfortunate.

I think it would be preferable if "--prune" would *not* affect tags, and
if there were an extra option like "--prune-tags" that would have to be
used explicitly to cause tags to be pruned.  Would somebody object to
such a change?

Michael

[1] In fact, the scenario that bit my colleague was as follows: he had
just built a software release, which creates a new commit and a release
tag.  When he tried to push the commit, there was a non-ff failure due
to an upstream change.  So he ran "git fetch --prune" to get the
upstream change, and this caused the release tag (which hadn't been
pushed yet) to be lost.

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

Re: Local tag killer

[John Szakmeister]

On Sat, Sep 21, 2013 at 2:42 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 09/21/2013 12:51 AM, Junio C Hamano wrote:
>> Junio C Hamano <gitster-vger@pobox.com> writes:
>>
>>> I also agree that the documentation is misstated; "remote-tracking branch"
>>> may have been a convenient and well understood phrase for whoever wrote
>>> that part, but the --prune is designed to cull extra refs in the
>>> hierarchies into
>>> which refs would be fetched if counterparts existed on the other side, so
>>> culling tags that do not exist on the remote side should also be described.
>>
>> (gleaning-leftovers mode)
>
> Thanks for following up on this with your proposed documentation patch.
>  I have been researching and experimenting, and still find the use of
> fetch confusing with respect to tags.  I think the problem is primarily
> that the behavior is awkward, and that it would be better to change the
> behavior than to document the awkward behavior.

I agree with this sentiment.  I've never liked how `--tags` operates.

> I must have read an old version of the documentation, from which it
> seemed that "git fetch --tags" fetches all tags from the remote *in
> addition to* the references and tags that would otherwise be fetched.
> This seems like a handy and safe feature, and I wish that this were
> indeed the effect of "--tags".

Me too.

> But I see that the documentation for "--tags" has been changed and now
> states explicitly that "--tags" is equivalent to specifying
> "refs/tags/*:refs/tags/*" on the command line, overriding any configured
> refspecs.  This doesn't seem like useful behavior; why would I want to
> fetch tags from a remote without also updating the configured refspecs?
>  And contrariwise, how can I fetch the configured refspecs *and* all
> tags at the same time in a single fetch?
>
> OK, one way to do it is to configure an explicit refspec for fetching
> the tags:
>
> [remote "origin"]
>         url = [...]
>         fetch = +refs/heads/*:refs/remotes/origin/*
>         fetch = refs/tags/*:refs/tags/*
>
> [Here is one oddity: even if the tags refspec doesn't have a "+" prefix,
> "git fetch" will do non-ff updates to tags, presumably because of the
> implicit tag-fetching behavior.]
>
> But if I use this configuration and type "git fetch --prune", then any
> local tags that are not present on the remote will be killed.
>
> In short, when local tags are in use, or tags that are in one remote but
> not another [1], then the current Git implementation makes it impossible to
>
> - Configure "fetch.prune" or "remote.$REMOTE.prune" without preventing
> the use of "fetch --tags"
>
> - Configure default fetching of all tags (via a refspec or via
> remote.$REMOTE.tagopt) without preventing the use of "fetch --prune"
>
> - Configure "fetch.prune" or "remote.$REMOTE.prune" and the default
> fetching of all tags (via a refspec or via remote.$REMOTE.tagopt) at the
> same time.
>
> This is unfortunate.
>
> I think it would be preferable if "--prune" would *not* affect tags, and
> if there were an extra option like "--prune-tags" that would have to be
> used explicitly to cause tags to be pruned.  Would somebody object to
> such a change?

I, personally, think what you outline makes more sense.  Also, I'm
curious if `git remote update -p $REMOTE` suffers from the same
problem, if the remote was added with the `--tags` option.

-John

Re: Local tag killer

[Jeff King]

On Sat, Sep 21, 2013 at 08:42:26AM +0200, Michael Haggerty wrote:

> I think it would be preferable if "--prune" would *not* affect tags, and
> if there were an extra option like "--prune-tags" that would have to be
> used explicitly to cause tags to be pruned.  Would somebody object to
> such a change?

I think most of this problem is the way that we fetch tags straight into
the refs/tags hierarchy. You would not do:

  [remote "origin"]
  fetch = +refs/heads/*:refs/heads/*
  prune = true

unless you wanted to be a pure-mirror, because you would hose your local
changes any time you fetched. But that is _exactly_ what we do with a
refs/tags/*:refs/tags/* fetch.

If we instead moved to a default fetch refspec more like:

  [remote "origin"]
  fetch = +refs/*:refs/remotes/origin/refs/*

Then everything would Just Work. If you prune what the other side has
locally, that's fine. All you're doing is pruning your view of what he
has, not anything you've done locally.

The tricky part is tweaking the lookup rules so that "origin/master"
still works, and that looking for "v1.0" checks both refs/tags and
refs/remotes/*/refs/tags. And of course managing backwards
compatibility. :)

In the meantime, I'd almost be tempted to say that "--prune" should
refuse to work when we are touching anything outside of refs/remotes/.
But that would make true mirrors fail, who do want to munge their local
refs/heads/. You'd need some way to say "no, really, it's OK to prune".
Maybe let remote.*.prune be "remotes", "always", or "none", and "true"
maps to "remotes"? That's not backwards compatible, but it would be much
safer.

-Peff

Re: Local tag killer

[Marc Branchaud]

On 13-09-24 03:51 AM, Jeff King wrote:
> On Sat, Sep 21, 2013 at 08:42:26AM +0200, Michael Haggerty wrote:
>
>> I think it would be preferable if "--prune" would *not* affect tags, and
>> if there were an extra option like "--prune-tags" that would have to be
>> used explicitly to cause tags to be pruned.  Would somebody object to
>> such a change?
>
> I think most of this problem is the way that we fetch tags straight into
> the refs/tags hierarchy. You would not do:
>
>    [remote "origin"]
>    fetch = +refs/heads/*:refs/heads/*
>    prune = true
>
> unless you wanted to be a pure-mirror, because you would hose your local
> changes any time you fetched. But that is _exactly_ what we do with a
> refs/tags/*:refs/tags/* fetch.
>
> If we instead moved to a default fetch refspec more like:
>
>    [remote "origin"]
>    fetch = +refs/*:refs/remotes/origin/refs/*

I'm all for such a change.

You no doubt recall the lengthy discussion about remote ref namespaces 
back in 2011 [1].  That arose while planning for 1.8, but my feeble 
recollection is that the change was considered too disruptive.  It seems 
2.0 would be a better home for such work.

		M.

[1] 
http://thread.gmane.org/gmane.comp.version-control.git/165799/focus=166729

Re: Local tag killer

[Jeff King]

On Tue, Sep 24, 2013 at 09:22:32AM -0400, Marc Branchaud wrote:

> >If we instead moved to a default fetch refspec more like:
> >
> >   [remote "origin"]
> >   fetch = +refs/*:refs/remotes/origin/refs/*
> 
> I'm all for such a change.
> 
> You no doubt recall the lengthy discussion about remote ref
> namespaces back in 2011 [1].  That arose while planning for 1.8, but
> my feeble recollection is that the change was considered too
> disruptive.  It seems 2.0 would be a better home for such work.

I do recall the discussion, though I did not review all of the
complications before writing this most recent mail.

I assume there are backwards compatibility issues lurking, and we may
even need a config switch to flip between old-style and new-style modes
(and leave it set to old-style at first, wait a while for people to have
a git that understands both, and then flip the default to new-style).

However, none of that is for Git 2.0. I do not think we have an exact
date set for Git 2.0, but we already have several switches ready to be
flipped.  I think Junio's plan was to do it sooner rather than later,
and not try to cram a bunch of last-minute compatibility breakages in.

-Peff

Re: Local tag killer

[Nicolas Pitre]

On Tue, 24 Sep 2013, Jeff King wrote:

> On Sat, Sep 21, 2013 at 08:42:26AM +0200, Michael Haggerty wrote:
> 
> > I think it would be preferable if "--prune" would *not* affect tags, and
> > if there were an extra option like "--prune-tags" that would have to be
> > used explicitly to cause tags to be pruned.  Would somebody object to
> > such a change?
> 
> I think most of this problem is the way that we fetch tags straight into
> the refs/tags hierarchy. You would not do:
> 
>   [remote "origin"]
>   fetch = +refs/heads/*:refs/heads/*
>   prune = true
> 
> unless you wanted to be a pure-mirror, because you would hose your local
> changes any time you fetched. But that is _exactly_ what we do with a
> refs/tags/*:refs/tags/* fetch.
> 
> If we instead moved to a default fetch refspec more like:
> 
>   [remote "origin"]
>   fetch = +refs/*:refs/remotes/origin/refs/*
> 
> Then everything would Just Work. If you prune what the other side has
> locally, that's fine. All you're doing is pruning your view of what he
> has, not anything you've done locally.
> 
> The tricky part is tweaking the lookup rules so that "origin/master"
> still works, and that looking for "v1.0" checks both refs/tags and
> refs/remotes/*/refs/tags. And of course managing backwards
> compatibility. :)

Cheers !!!

I remember participating to a discussion about this like 2.5 years ago:

http://news.gmane.org/group/gmane.comp.version-control.git/thread=165799

The flat tag namespace remains my major annoyance with git IMHO.


Nicolas

Re: Local tag killer

[Michael Haggerty]

On 09/26/2013 12:54 AM, Nicolas Pitre wrote:
> On Tue, 24 Sep 2013, Jeff King wrote:
>> I think most of this problem is the way that we fetch tags straight into
>> the refs/tags hierarchy. You would not do:
>>
>>   [remote "origin"]
>>   fetch = +refs/heads/*:refs/heads/*
>>   prune = true
>>
>> unless you wanted to be a pure-mirror, because you would hose your local
>> changes any time you fetched. But that is _exactly_ what we do with a
>> refs/tags/*:refs/tags/* fetch.
>>
>> If we instead moved to a default fetch refspec more like:
>>
>>   [remote "origin"]
>>   fetch = +refs/*:refs/remotes/origin/refs/*
>>
>> Then everything would Just Work. [...]
> 
> I remember participating to a discussion about this like 2.5 years ago:
> 
> http://news.gmane.org/group/gmane.comp.version-control.git/thread=165799
> 
> The flat tag namespace remains my major annoyance with git IMHO.

I just reviewed that old thread to determine its relevance to the
present discussion.  For the benefit of the other readers, here is a
summary of the main points that I got out of it.

The main proposal under discussion was that of Johan Herland:

    http://article.gmane.org/gmane.comp.version-control.git/165885

Nicolas made the two best arguments for the necessity of
separate tag namespaces per remote in *some* form:

> The extraordinary misfeature of the tag namespace at the moment
> comes from the fact that whenever you add a remote repo to fetch,
> and do fetch it, then your flat tag namespace gets polluted with all
> the tags the remote might have.  If you decide to delete some of
> those remote branches, the tags that came with it are still there
> and indistinguishable from other tags making it a real pain to sort
> out.
>
> -- http://article.gmane.org/gmane.comp.version-control.git/166108

and

> Let's take the OpenOffice vs LibreOffice as an example.  What if I
> want both in my repository so I can easily perform diffs between
> those independent branches?  They may certainly end up producing
> releases with the same version numbers (same tag name) but different
> content (different tag references).
>
> -- http://article.gmane.org/gmane.comp.version-control.git/166749

Other discussion and open issues regarding a ref namespace reorg:

* What exactly would be the ambiguity rules for references with the same
  name that appear in multiple remotes' namespaces?

  * Are references to two annotated tags considered the same if they
    refer to the same SHA-1, even if the annotated tags are different?
    What about an annotated vs an unannotated tag?  The consensus
    seemed to be "no".

  * Do they depend on how the reference is being used?  Yes, sometimes
    only a SHA-1 is needed, in which case multiple agreeing references
    shouldn't be a problem.  Other times the DWIM caller needs the
    full refname (e.g., "git push" pushes to different locations
    depending on whether the source is a branch or tag), in which case
    the rules would have to be more nuanced.

  * Should the same ambiguity rules be applied to other references
    (e.g., branches)?

  * What if a branch and a tag have the same name?

    * Nicolas Pitre suggested that usually they should be accepted if
      they have the same value, and if the refname matters then the
      branch should take precedence (with a warning).

    * Peff pointed out that currently dwim_ref prefers tags, but that
      Junio has said that that behavior was arbitrary [and by
      implication could be changed].  He suggested:

      > For dwim_ref, it prefers the tag and issues a warning. For
      > git-push, it complains about the ambiguity and dies. For git
      > checkout, we prefer the head. For git-tag, we prefer the tag
      > (though I think that only matters for "git tag -d").
      >
      > -- http://article.gmane.org/gmane.comp.version-control.git/166290

* What should "name-rev", "describe", "--decorate" output?  See
  discussion here:

      http://article.gmane.org/gmane.comp.version-control.git/165911

* "fetch" should probably warn if it ends up fetching a tag with the
  same name (according to the refname disambiguation rules) but value
  that conflicts with an existing tag in a different namespace.

* Do we need some pathspec modifier (e.g., "~") to specify that the
  corresponding references should be auto-followed in the manner
  currently done for refs/tags/*?  Or is auto-following maybe not
  needed at all anymore?:

      http://article.gmane.org/gmane.comp.version-control.git/160726

  Junio thought, and Johan agreed, that tag auto-following should still
  be done for repositories that use the old ref namespace format.  But
  perhaps this could be special-cased via a config setting rather than
  built into the refspec syntax.

* How would somebody (e.g., an interim maintainer) suck down tags from
  a project into his own refs/tags/* namespace?  (Would it even be
  necessary?)  Should there be a tool for this?  [It seems to me that
  something like

      git fetch . refs/remotes/origin/tags/*:refs/tags/*

  would do the trick, as long as pruning were turned off.]

* What special handling (if any) is required for
  refs/remotes/$REMOTE/HEAD?

  * According to Junio, HEAD is meant to indicate which branch is the
    "main" branch of the remote.  It is not transferred via the
    protocol, but rather guessed at by the client's "clone" process:

        http://article.gmane.org/gmane.comp.version-control.git/166694
        http://article.gmane.org/gmane.comp.version-control.git/166740

* How would this help somebody who wants to fetch content from multiple
  projects (e.g., git, gitk, gitgui) into a single repo?  There might
  be tags with the same names but very different meanings, and it would
  be awkward if there were ambiguity warnings all over the place.
  [Would it work to configure the fetching repo something like

  [remote "gitk-origin"]
          fetch = refs/tags/*:refs/remotes/gitk-origin/tags/gitk/*

  and to refer to a hypothetical gitk tag "v1.2.3" as "gitk/1.2.3"?
  Admittedly this is somewhat ambiguous with the proposed DWIM pattern
  <REMOTE>/<TAGNAME>.]

* It might be nice to have a command like

      git push $REMOTE --interactive

  that allows the user to choose interactively which branches/tags to
  push

  -- http://article.gmane.org/gmane.comp.version-control.git/166700

I hope that saves somebody the time of reading the whole thread
(though admittedly my summary is not especially short either).


As far as I can tell, the division of tags into remote-specific
namespaces would be another way of preventing the problem of tags being
pruned too aggressively.  But given that such a big change would be a
huge development effort, implementing something like the following
might be a quicker fix and would not conflict with a hypothetical
future ref namespace reorganization:

1. Limit "git fetch --prune" to only pruning references that are under
   refs/remotes/*

2. Add a new option --prune-tags that removes the above limitation

3. And the above two changes would make this one possible: Change the
   meaning of the --tags option to mean "fetch all tags *in addition
   to* (rather than *instead of*) the references that would otherwise
   be fetched".

Comments?

@Johan, I know that you were working on the ref-namespace issue at
GitMerge.  Did your work get anywhere?  Are you still working on it?
Have you documented somewhere any new insights that you have gained
about the problem space?

Michael

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

Re: Local tag killer

[Johan Herland]

On Sat, Sep 28, 2013 at 2:20 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> I just reviewed that old thread to determine its relevance to the
> present discussion.  For the benefit of the other readers, here is a
> summary of the main points that I got out of it.

I want to thank you immensely for the summary below. It really helps me
clear my own thoughts on this topic, and is an excellent base for
discussing how to advance on it.

> The main proposal under discussion was that of Johan Herland:
>
>     http://article.gmane.org/gmane.comp.version-control.git/165885
>
> Nicolas made the two best arguments for the necessity of
> separate tag namespaces per remote in *some* form:
>
>> The extraordinary misfeature of the tag namespace at the moment
>> comes from the fact that whenever you add a remote repo to fetch,
>> and do fetch it, then your flat tag namespace gets polluted with all
>> the tags the remote might have.  If you decide to delete some of
>> those remote branches, the tags that came with it are still there
>> and indistinguishable from other tags making it a real pain to sort
>> out.
>>
>> -- http://article.gmane.org/gmane.comp.version-control.git/166108
>
> and
>
>> Let's take the OpenOffice vs LibreOffice as an example.  What if I
>> want both in my repository so I can easily perform diffs between
>> those independent branches?  They may certainly end up producing
>> releases with the same version numbers (same tag name) but different
>> content (different tag references).
>>
>> -- http://article.gmane.org/gmane.comp.version-control.git/166749

I'd also like to mention my initial motivation for the proposal: a
natural way to organize other types of remote refs (notes, replace
refs, etc.). The separate tag namespace came about as a natural
(and IMHO quite useful) consequence of the proposed reorganization
of refs/remotes/*.

> Other discussion and open issues regarding a ref namespace reorg:
>
> * What exactly would be the ambiguity rules for references with the same
>   name that appear in multiple remotes' namespaces?
>
>   * Are references to two annotated tags considered the same if they
>     refer to the same SHA-1, even if the annotated tags are different?
>     What about an annotated vs an unannotated tag?  The consensus
>     seemed to be "no".
>
>   * Do they depend on how the reference is being used?  Yes, sometimes
>     only a SHA-1 is needed, in which case multiple agreeing references
>     shouldn't be a problem.  Other times the DWIM caller needs the
>     full refname (e.g., "git push" pushes to different locations
>     depending on whether the source is a branch or tag), in which case
>     the rules would have to be more nuanced.

Could we try to classify all ref lookups as either ref _name_ lookups
(in which case only a single, matching full refname is acceptable), or
ref _value_ lookups (in which case multiple matching names are allowed,
as long as they all point to the same SHA-1)? There are some complicated
cases (e.g. describe) which needs more thought, but if we can agree on
a mechanism for dealing with all the simpler cases, that might help
inform how to deal with the complicated ones.

>   * Should the same ambiguity rules be applied to other references
>     (e.g., branches)?

IMHO, yes.

>   * What if a branch and a tag have the same name?

IMHO, it depends on the context of the lookup. Some commands (e.g.
branch, tag) are clearly only interested in one ref type, and should
not care about other ref types at all.

For other lookups (e.g. rev-list, rev-parse), IMHO it depends on
whether they're looking up ref _names_ or ref _values_. In the former
case, ambiguity is always an error, while in the latter case it's not,
provided they agree on the SHA-1 (although maybe warning about
ambiguity might be appropriate in some contexts).

>     * Nicolas Pitre suggested that usually they should be accepted if
>       they have the same value, and if the refname matters then the
>       branch should take precedence (with a warning).
>
>     * Peff pointed out that currently dwim_ref prefers tags, but that
>       Junio has said that that behavior was arbitrary [and by
>       implication could be changed].  He suggested:
>
>       > For dwim_ref, it prefers the tag and issues a warning. For
>       > git-push, it complains about the ambiguity and dies. For git
>       > checkout, we prefer the head. For git-tag, we prefer the tag
>       > (though I think that only matters for "git tag -d").
>       >
>       > -- http://article.gmane.org/gmane.comp.version-control.git/166290
>
> * What should "name-rev", "describe", "--decorate" output?  See
>   discussion here:
>
>       http://article.gmane.org/gmane.comp.version-control.git/165911

AFAICS, "name-rev" and "--decorate" are the simple cases: Use the
shortest possible name which is still completely unambiguous in the
current repo.

For "describe", we want to use a tag name with no prefix (i.e. only
"$tag", not "$remote/$tag" or "$remote/tags/$tag" etc.). We also
probably want to prefer tags from some remotes over tags from other
remotes. Maybe something like:

  git describe --tags-from $remote

where the default behavior (i.e. given no --tags-from) would be to
assume

  --tags-from $(branch.$current.remote)
  --tags-from origin
  --tags-from . # local tags

or some combination of those.

> * "fetch" should probably warn if it ends up fetching a tag with the
>   same name (according to the refname disambiguation rules) but value
>   that conflicts with an existing tag in a different namespace.
>
> * Do we need some pathspec modifier (e.g., "~") to specify that the
>   corresponding references should be auto-followed in the manner
>   currently done for refs/tags/*?  Or is auto-following maybe not
>   needed at all anymore?:
>
>       http://article.gmane.org/gmane.comp.version-control.git/160726
>
>   Junio thought, and Johan agreed, that tag auto-following should still
>   be done for repositories that use the old ref namespace format.  But
>   perhaps this could be special-cased via a config setting rather than
>   built into the refspec syntax.

When using separate remote tag namespaces, I believe tag auto-following
is no longer needed. Instead the default refspec would simply fetch all
tags into the remote tag namespace.

As for the config setting, I have thought much about whether it is
possible to introduce remote ref namespaces while retaining backwards
compatibility, all _without_ having an explicit config switch. However,
I have more or less arrived at the conclusion that a "clean break" with
a config switch that selects either old or new ref layouts (and
associated semantics/behavior) is preferable.

> * How would somebody (e.g., an interim maintainer) suck down tags from
>   a project into his own refs/tags/* namespace?  (Would it even be
>   necessary?)

I'm not convinced it would be necessary. I have yet to see a case where
a (suitably unambiguous) remote tag would not fulfill the same purpose
as the equivalent local tag. The only exception is for dealing with
ambiguous remote tags, where a local tag could be created to serve as a
tie-breaker.

>   Should there be a tool for this?  [It seems to me that something like
>
>       git fetch . refs/remotes/origin/tags/*:refs/tags/*
>
>   would do the trick, as long as pruning were turned off.]

AFAICS, that fetch command should work (and should IMHO be unaffected
by fetch.prune or remote.$remote.prune in the configuration).

> * What special handling (if any) is required for
>   refs/remotes/$REMOTE/HEAD?
>
>   * According to Junio, HEAD is meant to indicate which branch is the
>     "main" branch of the remote.  It is not transferred via the
>     protocol, but rather guessed at by the client's "clone" process:
>
>         http://article.gmane.org/gmane.comp.version-control.git/166694
>         http://article.gmane.org/gmane.comp.version-control.git/166740

IMHO, if we want HEAD to accurately represent the "main" branch of the
remote, then we must also extend the protocol to contain it, so that it
does not go stale or suffer at the mercy of "guesswork".

> * How would this help somebody who wants to fetch content from multiple
>   projects (e.g., git, gitk, gitgui) into a single repo?  There might
>   be tags with the same names but very different meanings, and it would
>   be awkward if there were ambiguity warnings all over the place.
>   [Would it work to configure the fetching repo something like
>
>   [remote "gitk-origin"]
>           fetch = refs/tags/*:refs/remotes/gitk-origin/tags/gitk/*
>
>   and to refer to a hypothetical gitk tag "v1.2.3" as "gitk/1.2.3"?
>   Admittedly this is somewhat ambiguous with the proposed DWIM pattern
>   <REMOTE>/<TAGNAME>.]

Only if you also had a remote called "gitk". ;)

An alternative way to solve the problem of many ambiguity warnings:
If we define the rules so that local tags always override remote tags,
you could simply fetch the tags from your preferred remote into your
local tag namespace (as discussed above).

Personally, I would rather set up the configuration like this:

  [remote "gitk"]
          fetch = refs/tags/*:refs/remotes/gitk/tags/*

(i.e. keeping the default refspec) and then use "gitk/v1.2.3",
"git/v.1.2.3", "gitgui/v1.2.3" to disambiguate between the tags.

> * It might be nice to have a command like
>
>       git push $REMOTE --interactive
>
>   that allows the user to choose interactively which branches/tags to
>   push
>
>   -- http://article.gmane.org/gmane.comp.version-control.git/166700

I like that idea, but I think it's somewhat peripheral to the ref
namespace discussion.

> I hope that saves somebody the time of reading the whole thread
> (though admittedly my summary is not especially short either).
>
>
> As far as I can tell, the division of tags into remote-specific
> namespaces would be another way of preventing the problem of tags being
> pruned too aggressively.  But given that such a big change would be a
> huge development effort, implementing something like the following
> might be a quicker fix and would not conflict with a hypothetical
> future ref namespace reorganization:
>
> 1. Limit "git fetch --prune" to only pruning references that are under
>    refs/remotes/*
>
> 2. Add a new option --prune-tags that removes the above limitation
>
> 3. And the above two changes would make this one possible: Change the
>    meaning of the --tags option to mean "fetch all tags *in addition
>    to* (rather than *instead of*) the references that would otherwise
>    be fetched".

Agreed. The ref namespace topic clearly dwarfs the issue that started
this thread.

> @Johan, I know that you were working on the ref-namespace issue at
> GitMerge.  Did your work get anywhere? Are you still working on it?

I posted a couple of patch series dealing with _some_ preliminary
issues and adding some initial behavior changes. The first one was
parked in 'pu':

  https://git.kernel.org/cgit/git/git.git/commit/?h=pu&id=07e56ed

but has since been stuck in limbo, since we decided to roll it into
the larger series:

  http://article.gmane.org/gmane.comp.version-control.git/225137

The larger series was last posted here:

  http://thread.gmane.org/gmane.comp.version-control.git/223981

Since then, regrettably, not much has happened. I've now rebased the
series (still unfinished) onto v1.8.4 (no conflicts, still passes all
tests), and pushed it to my GitHub account:

  https://github.com/jherland/git/commits/peers-resurrect

That said, there are probably things in that series I'd like to revise,
e.g. introducing the config switch controlling new vs. old behavior as
early as possible, and reverting back to using refs/remotes/* instead
of refs/peers/*.

> Have you documented somewhere any new insights that you have gained
> about the problem space?

I think this (hideously long) reply should cover it...


Have fun! :)

...Johan

--
Johan Herland, <johan@herland.net>
www.herland.net

Re: Local tag killer

[Michael Haggerty]

On 09/28/2013 11:42 PM, Johan Herland wrote:
> On Sat, Sep 28, 2013 at 2:20 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>> Nicolas made the two best arguments for the necessity of
>> separate tag namespaces per remote in *some* form:
>> [...]
> 
> I'd also like to mention my initial motivation for the proposal: a
> natural way to organize other types of remote refs (notes, replace
> refs, etc.). The separate tag namespace came about as a natural
> (and IMHO quite useful) consequence of the proposed reorganization
> of refs/remotes/*.

ACK.

>> Other discussion and open issues regarding a ref namespace reorg:
>>
>> * What exactly would be the ambiguity rules for references with the same
>>   name that appear in multiple remotes' namespaces?
>>
>>   * Are references to two annotated tags considered the same if they
>>     refer to the same SHA-1, even if the annotated tags are different?
>>     What about an annotated vs an unannotated tag?  The consensus
>>     seemed to be "no".
>>
>>   * Do they depend on how the reference is being used?  Yes, sometimes
>>     only a SHA-1 is needed, in which case multiple agreeing references
>>     shouldn't be a problem.  Other times the DWIM caller needs the
>>     full refname (e.g., "git push" pushes to different locations
>>     depending on whether the source is a branch or tag), in which case
>>     the rules would have to be more nuanced.
> 
> Could we try to classify all ref lookups as either ref _name_ lookups
> (in which case only a single, matching full refname is acceptable), or
> ref _value_ lookups (in which case multiple matching names are allowed,
> as long as they all point to the same SHA-1)? There are some complicated
> cases (e.g. describe) which needs more thought, but if we can agree on
> a mechanism for dealing with all the simpler cases, that might help
> inform how to deal with the complicated ones.

Yes, name vs. value lookups is a useful distinction.

> [...]
>> * How would somebody (e.g., an interim maintainer) suck down tags from
>>   a project into his own refs/tags/* namespace?  (Would it even be
>>   necessary?)
> 
> I'm not convinced it would be necessary. I have yet to see a case where
> a (suitably unambiguous) remote tag would not fulfill the same purpose
> as the equivalent local tag. The only exception is for dealing with
> ambiguous remote tags, where a local tag could be created to serve as a
> tie-breaker.

I guess I was wondering how the interim maintainer would get Junio's
tags into his public repo (which he would want to do, so that users can
get everything from a single clone).

I think that the new version of "git push --tags" should *not* push all
tags from all remotes; it should push only refs/tags, like now.  So I
was thinking that the interim maintainer would want to import Junio's
tags into his own namespace, then

    git push --tags $URL

But I guess it would be cleaner just to push using an explicit refspec:

    git push $URL 'refs/remotes/origin/tags/*:refs/tags/*'

>> [...]
>> * How would this help somebody who wants to fetch content from multiple
>>   projects (e.g., git, gitk, gitgui) into a single repo?  There might
>>   be tags with the same names but very different meanings, and it would
>>   be awkward if there were ambiguity warnings all over the place.
>>   [Would it work to configure the fetching repo something like
>>
>>   [remote "gitk-origin"]
>>           fetch = refs/tags/*:refs/remotes/gitk-origin/tags/gitk/*
>>
>>   and to refer to a hypothetical gitk tag "v1.2.3" as "gitk/1.2.3"?
>>   Admittedly this is somewhat ambiguous with the proposed DWIM pattern
>>   <REMOTE>/<TAGNAME>.]
> 
> Only if you also had a remote called "gitk". ;)

True.

> An alternative way to solve the problem of many ambiguity warnings:
> If we define the rules so that local tags always override remote tags,
> you could simply fetch the tags from your preferred remote into your
> local tag namespace (as discussed above).
> 
> Personally, I would rather set up the configuration like this:
> 
>   [remote "gitk"]
>           fetch = refs/tags/*:refs/remotes/gitk/tags/*
> 
> (i.e. keeping the default refspec) and then use "gitk/v1.2.3",
> "git/v.1.2.3", "gitgui/v1.2.3" to disambiguate between the tags.

But if there were more than one remote providing gitk tags, it would be
difficult to grab a tag without caring where it came from.  And where
would I create a local gitk-scope tag?

I wonder whether remotes.group could sensibly be used to group remotes
into logical groups for value lookups:

    [remotes]
            gitk = gitk-origin
            gitk = second-gitk-repo

Then DWIM could be taught to seek "gitk/foo" under
"refs/remotes/gitk-origin/tags/foo" and
"refs/remotes/second-gitk-repo/tags/foo" in addition to
"refs/tags/gitk/foo" (insisting, of course, that if more than one of
these are present that they are all consistent).

Remote groups might also be used to configure the remotes that describe
considers when describing a commit:

    [remotes]
            describe = junio
            describe = jrn

or maybe (using the above config)

    git describe --remote-group=gitk

>> [...]
>> @Johan, I know that you were working on the ref-namespace issue at
>> GitMerge.  Did your work get anywhere? Are you still working on it?
> 
> I posted [...]

Thanks for your comments, and for the status update!

Michael

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

Re: Local tag killer

[Johan Herland]

On Sun, Sep 29, 2013 at 6:29 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> I wonder whether remotes.group could sensibly be used to group remotes
> into logical groups for value lookups:
>
>     [remotes]
>             gitk = gitk-origin
>             gitk = second-gitk-repo
>
> Then DWIM could be taught to seek "gitk/foo" under
> "refs/remotes/gitk-origin/tags/foo" and
> "refs/remotes/second-gitk-repo/tags/foo" in addition to
> "refs/tags/gitk/foo" (insisting, of course, that if more than one of
> these are present that they are all consistent).

This is an interesting idea. AFAICS, remotes.<group> is currently only
used by "git remote update" and "git fetch". According to git-remote(1)
it's used like this:

    [git remote] update
        Fetch updates for a named set of remotes in the repository
        as defined by remotes.<group>. If a named group is not
        specified on the command line, the configuration parameter
        remotes.default will be used; if remotes.default is not
        defined, all remotes which do not have the configuration
        parameter remote.<name>.skipDefaultUpdate set to true will
        be updated. (See git-config(1)).

I believe this would work well when extended to ref lookup as well:

 - Defining remotes.$group allows you to lookup refs across the grouped
   remotes by using the "$group/<ref>" syntax, as you describe above.

 - If remotes.default is defined, ref lookup happens by default across
   only those remotes, i.e. "$tag" will be sought under refs/tags/$tag
   and then refs/remotes/$remote/tags/$tag for each $remote in
   remotes.default.

 - If remotes.default is not defined, ref lookup happens across all
   remotes. This is analogous to what happens with tags today; they
   are all dumped into refs/tags/* and lookup considers all of them.

> Remote groups might also be used to configure the remotes that describe
> considers when describing a commit:
>
>     [remotes]
>             describe = junio
>             describe = jrn
>
> or maybe (using the above config)
>
>     git describe --remote-group=gitk

Hmm. I'd like to apply the same rules here, to stay consistent:

 - "git describe --from=$remote1 --from=$remote2" considers tags
   from refs/remotes/$remote1/tags/* and refs/remotes/$remote2/tags/*

 - "git describe --from=$group" considers tags from
   refs/remotes/$remote/tags/* for each $remote in $group

 - "git describe" considers tags from all remotes mentioned in
   remotes.default, or _all_ remotes is remotes.default is unset.

Additionally:

 - "git describe" (without --from) also considers (and prefers?) local
   tags.

 - "git describe --from=foo" does NOT consider local tags, but will
   also consider them if "--from=." is used.


...Johan

--
Johan Herland, <johan@herland.net>
www.herland.net

Re: Local tag killer

[Marc Branchaud]

On 13-09-29 12:29 AM, Michael Haggerty wrote:
> On 09/28/2013 11:42 PM, Johan Herland wrote:
>> On Sat, Sep 28, 2013 at 2:20 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> [...]
>>> * How would somebody (e.g., an interim maintainer) suck down tags from
>>>   a project into his own refs/tags/* namespace?  (Would it even be
>>>   necessary?)
>>
>> I'm not convinced it would be necessary. I have yet to see a case where
>> a (suitably unambiguous) remote tag would not fulfill the same purpose
>> as the equivalent local tag. The only exception is for dealing with
>> ambiguous remote tags, where a local tag could be created to serve as a
>> tie-breaker.
> 
> I guess I was wondering how the interim maintainer would get Junio's
> tags into his public repo (which he would want to do, so that users can
> get everything from a single clone).
> 
> I think that the new version of "git push --tags" should *not* push all
> tags from all remotes; it should push only refs/tags, like now.  So I
> was thinking that the interim maintainer would want to import Junio's
> tags into his own namespace, then
> 
>     git push --tags $URL
> 
> But I guess it would be cleaner just to push using an explicit refspec:
> 
>     git push $URL 'refs/remotes/origin/tags/*:refs/tags/*'

(Thanks for the awesome summary!)

You seem to be considering the case of an interim maintainer who, prior to
becoming the maintainer, has his own clone of the "official" repo and now
wants to make his clone the "official" repo, perhaps only temporarily.

But what if the interim maintainer has his own tags in his clone?  What if,
after he is no longer the interim maintainer, he wants to remove the
"official" tags from his clone's local namespace?  The interim maintainer
might also have his own branches in his clone, which shouldn't be part of an
"official" repo.

It seems to me that an interim maintainer would be wise to simply mirror the
"official" repo and publish with that mirror.  So the gymnastics you're
contemplating don't seem necessary to me.

>>> [...]
>>> * How would this help somebody who wants to fetch content from multiple
>>>   projects (e.g., git, gitk, gitgui) into a single repo?  There might
>>>   be tags with the same names but very different meanings, and it would
>>>   be awkward if there were ambiguity warnings all over the place.

Why would there be ambiguity warnings?  The fetch command shouldn't issue any
warnings, since all the remotes' names get safely tucked away in distinct
namespaces.

Are we talking about DWIM warnings?  Aside from git-describe I don't see why
such warnings would be a problem.  To DWIM-resolve a tag name look in
refs/tags/* and refs/remotes/*/tags/* -- much like it's done for branches
already.  If a tag name has multiple matches then it's ambiguous.  Git could
be clever and check for matching SHA1 values, but why bother?  It almost
seems like a disservice to silently disambiguate such names.  I would think a
user would prefer to know about any possible ambiguities, rather than have
some suddenly appear (and maybe also disappear).

And as for git-describe, I think it should only consider remote namespaces
when asked to via a command-line option or configuration setting (again
following the behaviour of git-branch).  Let whoever implements such an
option define whatever disambiguation rules they like.  Although I'd suggest
that the first implementation be very simple so that we can learn how people
expect it to work.  We may just find that users don't want git-describe to
consider remote namespaces at all.

Aside from that, I guess I just don't see a problem with using the existing
branch name disambiguation rules.  Am I missing something?

		M.

Re: Local tag killer

[Nicolas Pitre]

On Mon, 30 Sep 2013, Marc Branchaud wrote:

> Why would there be ambiguity warnings?  The fetch command shouldn't issue any
> warnings, since all the remotes' names get safely tucked away in distinct
> namespaces.
> 
> Are we talking about DWIM warnings?  Aside from git-describe I don't see why
> such warnings would be a problem.  To DWIM-resolve a tag name look in
> refs/tags/* and refs/remotes/*/tags/* -- much like it's done for branches
> already.  If a tag name has multiple matches then it's ambiguous.  Git could
> be clever and check for matching SHA1 values, but why bother?  It almost
> seems like a disservice to silently disambiguate such names.  I would think a
> user would prefer to know about any possible ambiguities, rather than have
> some suddenly appear (and maybe also disappear).

Consider that I have in my Linux kernel tree:

- a remote branch corresponding to Linus' master tree

- multiple remote branches corresponding to Linux stable branches

- a remote for linux-next which is a repo constantly being rebased

Now all those repositories share the mainline tags from Linus' repo and 
they add some more of they own which are not shared.  So if they all 
have a v3.11 tag that resolve to the same SHA1, then there is 
effectively no ambiguity at all and git should not warn at all.

*However* if one of those v3.11 tags does not agree with the others 
_then_ I want to be warned about it.

So having multiple matching tags that do resolve to the same SHA1 across 
different remote repositories _is_ the norm and should work 
transparently.


Nicolas

Re: Local tag killer

[Marc Branchaud]

On 13-09-30 11:52 AM, Nicolas Pitre wrote:
> On Mon, 30 Sep 2013, Marc Branchaud wrote:
> 
>> Why would there be ambiguity warnings?  The fetch command shouldn't issue any
>> warnings, since all the remotes' names get safely tucked away in distinct
>> namespaces.
>>
>> Are we talking about DWIM warnings?  Aside from git-describe I don't see why
>> such warnings would be a problem.  To DWIM-resolve a tag name look in
>> refs/tags/* and refs/remotes/*/tags/* -- much like it's done for branches
>> already.  If a tag name has multiple matches then it's ambiguous.  Git could
>> be clever and check for matching SHA1 values, but why bother?  It almost
>> seems like a disservice to silently disambiguate such names.  I would think a
>> user would prefer to know about any possible ambiguities, rather than have
>> some suddenly appear (and maybe also disappear).
> 
> Consider that I have in my Linux kernel tree:
> 
> - a remote branch corresponding to Linus' master tree
> 
> - multiple remote branches corresponding to Linux stable branches
> 
> - a remote for linux-next which is a repo constantly being rebased
> 
> Now all those repositories share the mainline tags from Linus' repo and 
> they add some more of they own which are not shared.  So if they all 
> have a v3.11 tag that resolve to the same SHA1, then there is 
> effectively no ambiguity at all and git should not warn at all.

Thanks, this example helps very much.

> *However* if one of those v3.11 tags does not agree with the others 
> _then_ I want to be warned about it.

Hmmm.  What behaviour would you like if you also had some non-Linux remote,
say for some driver code or something, that also had a v3.11 tag?  I presume
you want commands like
	git checkout -b my-topic v3.11
to do the Right Thing, but what's the right thing for you here?

> So having multiple matching tags that do resolve to the same SHA1 across 
> different remote repositories _is_ the norm and should work 
> transparently.

My suggestion for your example is that if some remote's tags are so
important/useful then you're better off importing them into your local tag
namespace (e.g. "git fetch Linus refs/tags/*:refs/tags/*").  By making the
remote's tags local, you're expressly telling git that they should be
considered for DWIMery, git-describe, etc.

I feel this approach lets us avoid having to somehow teach git which remote's
"v3.11" tags are important enough to merit an ambiguity warning and which
aren't.  Plus you get what I think you want, which is the current behaviour.

If this works for you, maybe it gives us a way forward:  All the DWIM
machinery should only consider the local namespace, perhaps with options to
explicitly ask it to expand its search, and we leave it up to the user to
specify which remotes' namespaces should be imported into the local namespace?

		M.

Re: Local tag killer

[Nicolas Pitre]

On Mon, 30 Sep 2013, Marc Branchaud wrote:

> On 13-09-30 11:52 AM, Nicolas Pitre wrote:
> > Consider that I have in my Linux kernel tree:
> > 
> > - a remote branch corresponding to Linus' master tree
> > 
> > - multiple remote branches corresponding to Linux stable branches
> > 
> > - a remote for linux-next which is a repo constantly being rebased
> > 
> > Now all those repositories share the mainline tags from Linus' repo and 
> > they add some more of they own which are not shared.  So if they all 
> > have a v3.11 tag that resolve to the same SHA1, then there is 
> > effectively no ambiguity at all and git should not warn at all.
> 
> Thanks, this example helps very much.
> 
> > *However* if one of those v3.11 tags does not agree with the others 
> > _then_ I want to be warned about it.
> 
> Hmmm.  What behaviour would you like if you also had some non-Linux remote,
> say for some driver code or something, that also had a v3.11 tag?

I want git to complain and bail out, maybe suggesting that I should use 
"driver_something/tag/v3.11" to disambiguate the tag.

> I presume
> you want commands like
> 	git checkout -b my-topic v3.11
> to do the Right Thing, but what's the right thing for you here?

git itself can't know it.  So the best git could do is to list 
conflicting tags with the shortest path that makes them unambiguous and 
suggest that I try again with one of them.

> > So having multiple matching tags that do resolve to the same SHA1 across 
> > different remote repositories _is_ the norm and should work 
> > transparently.
> 
> My suggestion for your example is that if some remote's tags are so
> important/useful then you're better off importing them into your local tag
> namespace (e.g. "git fetch Linus refs/tags/*:refs/tags/*").  By making the
> remote's tags local, you're expressly telling git that they should be
> considered for DWIMery, git-describe, etc.

Sure, it is probably a good thing semantically to give priority to local 
tags when they exist. However...

> I feel this approach lets us avoid having to somehow teach git which remote's
> "v3.11" tags are important enough to merit an ambiguity warning and which
> aren't.  Plus you get what I think you want, which is the current behaviour.

But I disagree here.  Most people simply won't care about local tags 
since the remote tags are sufficient for what they need.  And if they 
have multiple remotes in their repository then it is most likely to be 
different forks of the same project sharing mostly the same tags, and 
where those tags diverge then they're most likely to have different tag 
names as well.  So in the large majority of the cases, this v3.11 tag 
will come from one or more remotes and they will refer to the same SHA1, 
so it ought to just work without any special fetch.  Also, if I refer to 
v3.11.1 which is a tag that only exists in one of the remote branches 
and not in Linus' remote then it ought to just work as well.  That is 
more inline with the current _usage_ behavior even if the flat namespace 
is otherwise a nightmare to sort out when managing remotes.

Furthermore, git already has some code to detect refname ambiguities:

$ git init && echo "foo" > foo.txt && git add foo.txt
$ git commit -m "foo" && git tag foo && git branch foo && git log foo
warning: refname 'foo' is ambiguous.

So adding the extra step to lookup all possible tags and make sure they 
resolve to the same SHA1 should be a logical extension to what's already 
there.

Again, in the cases where there is actually a SHA1 conflict between all 
possible tags that match a tag short-end then listing them and asking the 
user to be more explicit is the right thing to do.  But that should be a 
very rare case in practice, and designing for making this case easy is 
the wrong approach.

Instead, the common case of multiple remotes with duplicated tag names 
referring to the same thing _and/or_ multiple remotes with distinct tags 
names is what should be made easy to use with no extra steps.


Nicolas

Re: Local tag killer

[Marc Branchaud]

On 13-09-30 04:08 PM, Nicolas Pitre wrote:
> On Mon, 30 Sep 2013, Marc Branchaud wrote:
> 
>> On 13-09-30 11:52 AM, Nicolas Pitre wrote:
>>> Consider that I have in my Linux kernel tree:
>>>
>>> - a remote branch corresponding to Linus' master tree
>>>
>>> - multiple remote branches corresponding to Linux stable branches
>>>
>>> - a remote for linux-next which is a repo constantly being rebased
>>>
>>> Now all those repositories share the mainline tags from Linus' repo and 
>>> they add some more of they own which are not shared.  So if they all 
>>> have a v3.11 tag that resolve to the same SHA1, then there is 
>>> effectively no ambiguity at all and git should not warn at all.
>>
>> Thanks, this example helps very much.
>>
>>> *However* if one of those v3.11 tags does not agree with the others 
>>> _then_ I want to be warned about it.
>>
>> Hmmm.  What behaviour would you like if you also had some non-Linux remote,
>> say for some driver code or something, that also had a v3.11 tag?
> 
> I want git to complain and bail out, maybe suggesting that I should use 
> "driver_something/tag/v3.11" to disambiguate the tag.
> 
>> I presume
>> you want commands like
>> 	git checkout -b my-topic v3.11
>> to do the Right Thing, but what's the right thing for you here?
> 
> git itself can't know it.  So the best git could do is to list 
> conflicting tags with the shortest path that makes them unambiguous and 
> suggest that I try again with one of them.
> 
>>> So having multiple matching tags that do resolve to the same SHA1 across 
>>> different remote repositories _is_ the norm and should work 
>>> transparently.
>>
>> My suggestion for your example is that if some remote's tags are so
>> important/useful then you're better off importing them into your local tag
>> namespace (e.g. "git fetch Linus refs/tags/*:refs/tags/*").  By making the
>> remote's tags local, you're expressly telling git that they should be
>> considered for DWIMery, git-describe, etc.
> 
> Sure, it is probably a good thing semantically to give priority to local 
> tags when they exist. However...
> 
>> I feel this approach lets us avoid having to somehow teach git which remote's
>> "v3.11" tags are important enough to merit an ambiguity warning and which
>> aren't.  Plus you get what I think you want, which is the current behaviour.
> 
> But I disagree here.  Most people simply won't care about local tags 
> since the remote tags are sufficient for what they need.

Good point -- I see where my suggestion was wrong.  I think it's worthwhile
to make sure that bare tag names "just work" after a simple clone.  Git's
DWIM code already does this for branch names, and it makes sense to extend
that to other ref types in remote namespaces.

> And if they 
> have multiple remotes in their repository then it is most likely to be 
> different forks of the same project sharing mostly the same tags, and 
> where those tags diverge then they're most likely to have different tag 
> names as well.

I disagree about the "most likely" part, but it's only a niggle.  I agree
with the overall point that disambiguation through SHA1 comparison makes sense.

> So in the large majority of the cases, this v3.11 tag 
> will come from one or more remotes and they will refer to the same SHA1, 
> so it ought to just work without any special fetch.  Also, if I refer to 
> v3.11.1 which is a tag that only exists in one of the remote branches 
> and not in Linus' remote then it ought to just work as well.  That is 
> more inline with the current _usage_ behavior even if the flat namespace 
> is otherwise a nightmare to sort out when managing remotes.

Agreed.

> Furthermore, git already has some code to detect refname ambiguities:
> 
> $ git init && echo "foo" > foo.txt && git add foo.txt
> $ git commit -m "foo" && git tag foo && git branch foo && git log foo
> warning: refname 'foo' is ambiguous.
> 
> So adding the extra step to lookup all possible tags and make sure they 
> resolve to the same SHA1 should be a logical extension to what's already 
> there.
> 
> Again, in the cases where there is actually a SHA1 conflict between all 
> possible tags that match a tag short-end then listing them and asking the 
> user to be more explicit is the right thing to do.  But that should be a 
> very rare case in practice, and designing for making this case easy is 
> the wrong approach.
> 
> Instead, the common case of multiple remotes with duplicated tag names 
> referring to the same thing _and/or_ multiple remotes with distinct tags 
> names is what should be made easy to use with no extra steps.

Again, I don't think that's the common case.  I think it's just as likely for
there to be multiple remotes with duplicate tag names that refer to different
objects.  However, SHA1-disambiguation covers all these cases.

		M.

Re: Local tag killer

[Nicolas Pitre]

On Mon, 30 Sep 2013, Marc Branchaud wrote:

> On 13-09-30 04:08 PM, Nicolas Pitre wrote:
> > Again, in the cases where there is actually a SHA1 conflict between all 
> > possible tags that match a tag short-end then listing them and asking the 
> > user to be more explicit is the right thing to do.  But that should be a 
> > very rare case in practice, and designing for making this case easy is 
> > the wrong approach.
> > 
> > Instead, the common case of multiple remotes with duplicated tag names 
> > referring to the same thing _and/or_ multiple remotes with distinct tags 
> > names is what should be made easy to use with no extra steps.
> 
> Again, I don't think that's the common case.  I think it's just as likely for
> there to be multiple remotes with duplicate tag names that refer to different
> objects.

Why do you say so?  I'm curious to know what kind of work flow would do 
that in practice.

At least for typical Linux kernel workflows what I said above is true.


Nicolas

Re: Local tag killer

[Jeff King]

On Mon, Sep 30, 2013 at 06:44:09PM -0400, Nicolas Pitre wrote:

> > Again, I don't think that's the common case.  I think it's just as likely for
> > there to be multiple remotes with duplicate tag names that refer to different
> > objects.
> 
> Why do you say so?  I'm curious to know what kind of work flow would do 
> that in practice.
> 
> At least for typical Linux kernel workflows what I said above is true.

I could image if you are fetching from a bunch of coworkers that several
people might reuse a common name like "start" or "tmp" for different
purposes.

But I think the behavior you've described handles that quite naturally.
If there is one "start", or if they all match, it is unambiguous. If
there are multiple matches, git says "which one did you mean?" and you
can say "bob/start" or "alice/start" to disambiguate. Anything else
would be a guess.

If _you_ have a refs/tags/start, then I think that should unambiguously
take precedence over that of your coworkers. That way your coworkers
cannot pollute the lookup of items in your own namespace.

-Peff

Re: Local tag killer

[Marc Branchaud]

On 13-09-30 06:44 PM, Nicolas Pitre wrote:
> On Mon, 30 Sep 2013, Marc Branchaud wrote:
>
>> On 13-09-30 04:08 PM, Nicolas Pitre wrote:
>>> Again, in the cases where there is actually a SHA1 conflict between all
>>> possible tags that match a tag short-end then listing them and asking the
>>> user to be more explicit is the right thing to do.  But that should be a
>>> very rare case in practice, and designing for making this case easy is
>>> the wrong approach.
>>>
>>> Instead, the common case of multiple remotes with duplicated tag names
>>> referring to the same thing _and/or_ multiple remotes with distinct tags
>>> names is what should be made easy to use with no extra steps.
>>
>> Again, I don't think that's the common case.  I think it's just as likely for
>> there to be multiple remotes with duplicate tag names that refer to different
>> objects.
>
> Why do you say so?  I'm curious to know what kind of work flow would do
> that in practice.

The use case I have in mind is where a project makes use of other 
projects, for example an application that uses some libraries.  The 
application's repository could contain the full histories of the 
libraries, each subtree-merged into a different directory.

So maybe that's not so common these days, but the current flat tag 
namespace makes it pretty much impractical.

		M.

Re: Local tag killer

[Nicolas Pitre]

On Mon, 30 Sep 2013, Marc Branchaud wrote:

> On 13-09-30 06:44 PM, Nicolas Pitre wrote:
> > On Mon, 30 Sep 2013, Marc Branchaud wrote:
> > 
> > > On 13-09-30 04:08 PM, Nicolas Pitre wrote:
> > > > Again, in the cases where there is actually a SHA1 conflict between all
> > > > possible tags that match a tag short-end then listing them and asking
> > > > the
> > > > user to be more explicit is the right thing to do.  But that should be a
> > > > very rare case in practice, and designing for making this case easy is
> > > > the wrong approach.
> > > > 
> > > > Instead, the common case of multiple remotes with duplicated tag names
> > > > referring to the same thing _and/or_ multiple remotes with distinct tags
> > > > names is what should be made easy to use with no extra steps.
> > > 
> > > Again, I don't think that's the common case.  I think it's just as likely
> > > for
> > > there to be multiple remotes with duplicate tag names that refer to
> > > different
> > > objects.
> > 
> > Why do you say so?  I'm curious to know what kind of work flow would do
> > that in practice.
> 
> The use case I have in mind is where a project makes use of other projects,
> for example an application that uses some libraries.  The application's
> repository could contain the full histories of the libraries, each
> subtree-merged into a different directory.
> 
> So maybe that's not so common these days, but the current flat tag namespace
> makes it pretty much impractical.

But with my proposal, you'd get a message saying that the tag "baz" is 
ambigous, and that you'd have to use either "libfoo/baz" or 
"libbar/baz".

The current flat namespace makes many things virtually impractical 
indeed, even with the kernel workflow I described.


Nicolas

Re: Local tag killer

[Marc Branchaud]

On 13-09-30 11:28 PM, Nicolas Pitre wrote:
>
> But with my proposal, you'd get a message saying that the tag "baz" is
> ambigous, and that you'd have to use either "libfoo/baz" or
> "libbar/baz".

Yes, and that's good.  I agree with your proposal.  Sorry if that wasn't 
clear.

		M.

[PATCH 00/15] Change semantics of "fetch --tags"

[Michael Haggerty]

This is my proposed fix for the "local tag killer" problem that I
reported recently [1].

There are three main things changed by this patch series:

1. "git fetch --tags" causes tags to be fetched *in addition to* any
   other refspecs that are configured for the remote, rather than
   *instead of*.  I believe this is more useful behavior.  It is also
   consistent with the documentation as it was written before it was
   disambiguated in 1.8.0.3.

2. "git fetch --prune" only prunes references that match an explicit
   refspec (either from the command line or from the
   remote.<name>.fetch configuration.  In particular, using "--prune"
   and "--tag" together do *not* make tags subject to pruning.  (Tags
   can still be pruned if the user specifies an explicit refspec
   "refs/tags/*:refs/tags/*".)

3. Previously, if the user invoked one of the following commands with
   --no-prune, the --no-prune option was not passed to the "git fetch"
   subprocesses that they invoked to do their work:

       git fetch --all
       git fetch --multiple
       git fetch --recurse-submodules
       git remote update

   If fetch.prune or remote.<name>.prune were set to true, this could
   result in unwanted reference pruning.  The last commit in the
   series fixes this bug and should not be controversial.

I had originally planned to solve the "local tag killer" problem by
adding a new configuration option to define which reference namespaces
were subject to pruning (e.g.,
remote.<name>.pruneRef="refs/remotes/*").  I may yet submit that patch
series as a separate feature.  But while working on it I hit on the
present solution, which I think is simpler and more elegant (albeit a
bit less flexible).

Changes (1) and (2) introduce behavior changes, but I think that they
are improvements and that the resulting backwards-incompatibility is
acceptable:

Change (1) means that "git fetch --tags <remote>" without any
additional refspec arguments will fetch more references than it did
before.  But I don't think it is very useful to want to fetch tags
without fetching other configured references, so I think it is OK [2].

Change (2) means that using "git fetch --tags --prune" will *not*
prune tags.  (This is the whole point of the change!)  As discussed in
the mailing list, it is usually bad policy to prune tags, because tags
for the local repository and for all remote repositories currently
share a single namespace, "refs/tags/*".  Therefore, pruning tags
based on information from a single remote risks pruning local tags or
tags that have been obtained from another remote.  The main exception,
when one probably *does* want to prune tags, is when fetching into a
mirror clone.  But mirror clones have
"remote.<name>.fetch=+refs/*:refs/*", and so even after this change
tags will be subject to pruning when fetching into a mirror clone.

The only other place I can find that does reference pruning is "git
remote prune", but that codepath didn't respect remote.<name>.tagopt
anyway and therefore it *didn't* prune tags unless they were part of
an explicit refspec; i.e., this codepath already behaved the "new" way
that other pruning codepaths now behave.

Patches 1-9 are just preliminary cleanup and documentation
improvements.

Patch 10 implements change (1) described above.

Patch 11 implements change (2).

Patches 12-14 are some more minor cleanups.

Patch 15 implements change (3).

[1] http://article.gmane.org/gmane.comp.version-control.git/234723

[2] Indeed, I bet that most scripts that invoke "git fetch --tags
    <remote>" also invoke a plain "git fetch" immediately before or
    after to get the rest of the references.

Michael Haggerty (15):
  t5510: use the correct tag name in test
  t5510: prepare test refs more straightforwardly
  t5510: check that "git fetch --prune --tags" does not prune branches
  api-remote.txt: correct section "struct refspect"
  get_ref_map(): rename local variables
  ref_remove_duplicates(): avoid redundant bisection
  ref_remove_duplicates(): simplify function
  ref_remove_duplicates(): improve documentation comment
  builtin/fetch.c: reorder function definitions
  fetch --tags: fetch tags *in addition to* other stuff
  fetch --prune: prune only based on explicit refspecs
  query_refspecs(): move some constants out of the loop
  builtin/remote.c: reorder function definitions
  builtin/remote.c:update(): use struct argv_array
  fetch, remote: properly convey --no-prune options to subprocesses

 Documentation/config.txt                 |   2 +-
 Documentation/fetch-options.txt          |  21 ++-
 Documentation/technical/api-remote.txt   |  20 +--
 builtin/fetch.c                          | 253 +++++++++++++++----------------
 builtin/remote.c                         | 196 ++++++++++++------------
 git-pull.sh                              |   2 +-
 remote.c                                 |  44 +++---
 remote.h                                 |   9 +-
 t/t5510-fetch.sh                         |  36 ++++-
 t/t5515/fetch.br-unconfig_--tags_.._.git |   1 +
 t/t5515/fetch.master_--tags_.._.git      |   1 +
 t/t5525-fetch-tagopt.sh                  |  23 ++-
 12 files changed, 322 insertions(+), 286 deletions(-)

-- 
1.8.4

[PATCH 01/15] t5510: use the correct tag name in test

[Michael Haggerty]

Fix an apparent copy-paste error: A few lines earlier, a tag
"refs/tags/sometag" is created.  Check for the (non-)existence of that
tag, not "somebranch", which is otherwise never mentioned in the
script.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t5510-fetch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 1f0f8e6..c5e5dfc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -121,7 +121,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br
 
 	git fetch --prune --tags origin &&
 	git rev-parse origin/master &&
-	test_must_fail git rev-parse somebranch
+	test_must_fail git rev-parse sometag
 '
 
 test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
-- 
1.8.4

[PATCH 02/15] t5510: prepare test refs more straightforwardly

[Michael Haggerty]

"git fetch" was being used with contrived refspecs to create tags and
remote-tracking branches in test repositories in preparation for the
actual tests.  This is obscure and also makes one wonder whether this
is indeed just preparation or whether some side-effect of "git fetch"
is being tested.

So use the more straightforward commands "git tag" / "git update-ref"
when preparing branches in test repositories.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t5510-fetch.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index c5e5dfc..08d8dbb 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -88,7 +88,7 @@ test_expect_success 'fetch --prune on its own works as expected' '
 	cd "$D" &&
 	git clone . prune &&
 	cd prune &&
-	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune origin &&
 	test_must_fail git rev-parse origin/extrabranch
@@ -98,7 +98,7 @@ test_expect_success 'fetch --prune with a branch name keeps branches' '
 	cd "$D" &&
 	git clone . prune-branch &&
 	cd prune-branch &&
-	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune origin master &&
 	git rev-parse origin/extrabranch
@@ -117,7 +117,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br
 	cd "$D" &&
 	git clone . prune-tags &&
 	cd prune-tags &&
-	git fetch origin refs/heads/master:refs/tags/sometag &&
+	git tag sometag master &&
 
 	git fetch --prune --tags origin &&
 	git rev-parse origin/master &&
@@ -128,7 +128,7 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo
 	cd "$D" &&
 	git clone . prune-tags-branch &&
 	cd prune-tags-branch &&
-	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
+	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune --tags origin master &&
 	git rev-parse origin/extrabranch
-- 
1.8.4

[PATCH 03/15] t5510: check that "git fetch --prune --tags" does not prune branches

[Michael Haggerty]

"git fetch --prune --tags" is currently interpreted as follows:

* "--tags" is equivalent to specifying a refspec
  "refs/tags/*:refs/tags/*", and supersedes any default refspecs
  configured via remote.$REMOTE.fetch.

* "--prune" only operates on the refspecs being fetched.

Therefore, "git fetch --prune --tags" prunes tags in refs/tags/* but
does not fetch or prune other references.  The fact that this command
does not prune references outside of refs/tags/* was previously
untested.  So add a test that verifies the status quo.

However, the status quo is surprising, so it will be changed later in
this patch series.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t5510-fetch.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 08d8dbb..8328be1 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -118,9 +118,13 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br
 	git clone . prune-tags &&
 	cd prune-tags &&
 	git tag sometag master &&
+	# Create what looks like a remote-tracking branch from an earlier
+	# fetch that has since been deleted from the remote:
+	git update-ref refs/remotes/origin/fake-remote master &&
 
 	git fetch --prune --tags origin &&
 	git rev-parse origin/master &&
+	git rev-parse origin/fake-remote &&
 	test_must_fail git rev-parse sometag
 '
 
-- 
1.8.4

[PATCH 04/15] api-remote.txt: correct section "struct refspect"

[Michael Haggerty]

* Replace reference to function parse_ref_spec() with references to
  functions parse_fetch_refspec() and parse_push_refspec().

* Correct description of src and dst: they *do* include the '*'
  characters.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/technical/api-remote.txt | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt
index 4be8776..5d245aa 100644
--- a/Documentation/technical/api-remote.txt
+++ b/Documentation/technical/api-remote.txt
@@ -58,16 +58,16 @@ default remote, given the current branch and configuration.
 struct refspec
 --------------
 
-A struct refspec holds the parsed interpretation of a refspec. If it
-will force updates (starts with a '+'), force is true. If it is a
-pattern (sides end with '*') pattern is true. src and dest are the two
-sides (if a pattern, only the part outside of the wildcards); if there
-is only one side, it is src, and dst is NULL; if sides exist but are
-empty (i.e., the refspec either starts or ends with ':'), the
-corresponding side is "".
-
-This parsing can be done to an array of strings to give an array of
-struct refpsecs with parse_ref_spec().
+A struct refspec holds the parsed interpretation of a refspec.  If it
+will force updates (starts with a '+'), force is true.  If it is a
+pattern (sides end with '*') pattern is true.  src and dest are the
+two sides (including '*' characters if present); if there is only one
+side, it is src, and dst is NULL; if sides exist but are empty (i.e.,
+the refspec either starts or ends with ':'), the corresponding side is
+"".
+
+An array of strings can be parsed into an array of struct refspecs
+using parse_fetch_refspec() or parse_push_refspec().
 
 remote_find_tracking(), given a remote and a struct refspec with
 either src or dst filled out, will fill out the other such that the
-- 
1.8.4

[PATCH 05/15] get_ref_map(): rename local variables

[Michael Haggerty]

Rename "refs" -> "refspecs" and "ref_count" -> "refspec_count" to
reduce confusion, because they describe an array of "struct refspec",
as opposed to the "struct ref" objects that are also used in this
function.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index bd7a101..2248abf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -165,8 +165,8 @@ static void find_non_local_tags(struct transport *transport,
 			struct ref ***tail);
 
 static struct ref *get_ref_map(struct transport *transport,
-			       struct refspec *refs, int ref_count, int tags,
-			       int *autotags)
+			       struct refspec *refspecs, int refspec_count,
+			       int tags, int *autotags)
 {
 	int i;
 	struct ref *rm;
@@ -175,12 +175,12 @@ static struct ref *get_ref_map(struct transport *transport,
 
 	const struct ref *remote_refs = transport_get_remote_refs(transport);
 
-	if (ref_count || tags == TAGS_SET) {
+	if (refspec_count || tags == TAGS_SET) {
 		struct ref **old_tail;
 
-		for (i = 0; i < ref_count; i++) {
-			get_fetch_map(remote_refs, &refs[i], &tail, 0);
-			if (refs[i].dst && refs[i].dst[0])
+		for (i = 0; i < refspec_count; i++) {
+			get_fetch_map(remote_refs, &refspecs[i], &tail, 0);
+			if (refspecs[i].dst && refspecs[i].dst[0])
 				*autotags = 1;
 		}
 		/* Merge everything on the command line, but not --tags */
-- 
1.8.4

[PATCH 06/15] ref_remove_duplicates(): avoid redundant bisection

[Michael Haggerty]

The old code called string_list_lookup(), and if that failed called
string_list_insert(), thus doing the bisection search through the
string list twice in the latter code path.

Instead, just call string_list_insert() right away.  If an entry for
that peer reference name already existed, then its util pointer is
always non-NULL.

Of course this doesn't change the fact that the repeated
string_list_insert() calls make the function scale like O(N^2) if the
input reference list is not already approximately sorted.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

The O(N^2) algorithm mentioned above essentially boils down to

    for each reference:
        insert reference into string_list (sorted)

Because the insertion requires later array entries to be shifted to
the right, the algorithm scales like O(N^2) if the entries are not
already approximately in order.

I can think of three easy ways to fix this:

* Use a hash map instead of a sorted string_list to record which
  entries have already been seen.

* If the order of the result doesn't matter and it doesn't matter
  *which* of the duplicates is retained, then insert all of the names
  into the string_list unsorted, then sort them all in one go, then
  iterate through looking for duplicates and stringing the survivors
  together in a new linked list.

* If the order is important, or it is important that the *first* of
  duplicates be retained, then iterate through the linked list twice.
  The first time, just add the entries to the string_list.  Then sort
  the string_list using a stable sort (e.g., git_qsort()).  Then
  iterate through the linked list a second time as in the current
  code.

See also my comments to commit 10.

However, I believe that the O(N^2) worst case is unlikely to bite in
most cases.  This function is called from
builtin/fetch.c:get_ref_map(), and the input list is the concatenation
of several sub-lists.  Most sublists come from resolving refspecs, and
the last comes from find_non_local_tags().  I believe that each of the
sublists is sorted.  So the O(N^2) worst-case could only occur if more
than one of the sublists is large and at least two of the large
sublists are not in the correct order relative to each other.

I tried to concoct such a test case by creating a repository with
about 20k branches and 20k tags, instrumenting
ref_remove_duplicates(), and then fetching with something like

    git fetch --prune origin refs/heads/*:refs/remotes/origin/* \
                             refs/tags/*:refs/tags/*

Indeed, the running time of ref_remove_duplicates() became significant
when the order of the refspecs was reversed, but it was still less
than a second out of a much longer command invocation.

So I didn't bother fixing the O(N^2) algorithm.  If somebody reports
real-world slowness here, we can always come back to it.

 remote.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index e9fedfa..a44e897 100644
--- a/remote.c
+++ b/remote.c
@@ -750,13 +750,15 @@ void ref_remove_duplicates(struct ref *ref_map)
 	struct string_list refs = STRING_LIST_INIT_NODUP;
 	struct string_list_item *item = NULL;
 	struct ref *prev = NULL, *next = NULL;
+
 	for (; ref_map; prev = ref_map, ref_map = next) {
 		next = ref_map->next;
 		if (!ref_map->peer_ref)
 			continue;
 
-		item = string_list_lookup(&refs, ref_map->peer_ref->name);
-		if (item) {
+		item = string_list_insert(&refs, ref_map->peer_ref->name);
+		if (item->util) {
+			/* Entry already existed */
 			if (strcmp(((struct ref *)item->util)->name,
 				   ref_map->name))
 				die("%s tracks both %s and %s",
@@ -767,11 +769,9 @@ void ref_remove_duplicates(struct ref *ref_map)
 			free(ref_map->peer_ref);
 			free(ref_map);
 			ref_map = prev; /* skip this; we freed it */
-			continue;
+		} else {
+			item->util = ref_map;
 		}
-
-		item = string_list_insert(&refs, ref_map->peer_ref->name);
-		item->util = ref_map;
 	}
 	string_list_clear(&refs, 0);
 }
-- 
1.8.4

[PATCH 07/15] ref_remove_duplicates(): simplify function

[Michael Haggerty]

* Use a dedicated variable, ref, for referring to the current item
  rather than using the ref_map pointer for this purpose.

* Use a (struct ref **) as iteration variable to avoid having to keep
  track of prev and next in addition to the pointer to the current
  item.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 remote.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/remote.c b/remote.c
index a44e897..5ade07f 100644
--- a/remote.c
+++ b/remote.c
@@ -748,29 +748,33 @@ int for_each_remote(each_remote_fn fn, void *priv)
 void ref_remove_duplicates(struct ref *ref_map)
 {
 	struct string_list refs = STRING_LIST_INIT_NODUP;
-	struct string_list_item *item = NULL;
-	struct ref *prev = NULL, *next = NULL;
+	struct ref **p;
 
-	for (; ref_map; prev = ref_map, ref_map = next) {
-		next = ref_map->next;
-		if (!ref_map->peer_ref)
+	for (p = &ref_map; *p; ) {
+		struct ref *ref = *p;
+		struct string_list_item *item;
+
+		if (!ref->peer_ref) {
+			p = &ref->next;
 			continue;
+		}
 
-		item = string_list_insert(&refs, ref_map->peer_ref->name);
+		item = string_list_insert(&refs, ref->peer_ref->name);
 		if (item->util) {
 			/* Entry already existed */
 			if (strcmp(((struct ref *)item->util)->name,
-				   ref_map->name))
+				   ref->name))
 				die("%s tracks both %s and %s",
-				    ref_map->peer_ref->name,
+				    ref->peer_ref->name,
 				    ((struct ref *)item->util)->name,
-				    ref_map->name);
-			prev->next = ref_map->next;
-			free(ref_map->peer_ref);
-			free(ref_map);
-			ref_map = prev; /* skip this; we freed it */
+				    ref->name);
+			/* item is a duplicate; remove and free it */
+			*p = ref->next;
+			free(ref->peer_ref);
+			free(ref);
 		} else {
-			item->util = ref_map;
+			item->util = ref;
+			p = &ref->next;
 		}
 	}
 	string_list_clear(&refs, 0);
-- 
1.8.4

[PATCH 08/15] ref_remove_duplicates(): improve documentation comment

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 remote.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/remote.h b/remote.h
index 131130a..40293c0 100644
--- a/remote.h
+++ b/remote.h
@@ -149,7 +149,14 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);
 int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
 
 /*
- * Removes and frees any duplicate refs in the map.
+ * Remove and free all but the first of any entries in the input list
+ * that map the same remote reference to the same local reference.  If
+ * there are two entries that map different remote references to the
+ * same local reference, die.
+ *
+ * Note that the first entry is never removed; therefore, the pointer
+ * passed in as argument still points to the head of the list after
+ * the function returns.
  */
 void ref_remove_duplicates(struct ref *ref_map);
 
-- 
1.8.4

[PATCH 09/15] builtin/fetch.c: reorder function definitions

[Michael Haggerty]

Reorder function definitions to avoid the need for a forward
declaration of function find_non_local_tags().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch.c | 198 +++++++++++++++++++++++++++-----------------------------
 1 file changed, 97 insertions(+), 101 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 2248abf..61e8117 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -160,9 +160,105 @@ static void add_merge_config(struct ref **head,
 	}
 }
 
+static int add_existing(const char *refname, const unsigned char *sha1,
+			int flag, void *cbdata)
+{
+	struct string_list *list = (struct string_list *)cbdata;
+	struct string_list_item *item = string_list_insert(list, refname);
+	item->util = xmalloc(20);
+	hashcpy(item->util, sha1);
+	return 0;
+}
+
+static int will_fetch(struct ref **head, const unsigned char *sha1)
+{
+	struct ref *rm = *head;
+	while (rm) {
+		if (!hashcmp(rm->old_sha1, sha1))
+			return 1;
+		rm = rm->next;
+	}
+	return 0;
+}
+
 static void find_non_local_tags(struct transport *transport,
 			struct ref **head,
-			struct ref ***tail);
+			struct ref ***tail)
+{
+	struct string_list existing_refs = STRING_LIST_INIT_DUP;
+	struct string_list remote_refs = STRING_LIST_INIT_NODUP;
+	const struct ref *ref;
+	struct string_list_item *item = NULL;
+
+	for_each_ref(add_existing, &existing_refs);
+	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
+		if (prefixcmp(ref->name, "refs/tags/"))
+			continue;
+
+		/*
+		 * The peeled ref always follows the matching base
+		 * ref, so if we see a peeled ref that we don't want
+		 * to fetch then we can mark the ref entry in the list
+		 * as one to ignore by setting util to NULL.
+		 */
+		if (!suffixcmp(ref->name, "^{}")) {
+			if (item && !has_sha1_file(ref->old_sha1) &&
+			    !will_fetch(head, ref->old_sha1) &&
+			    !has_sha1_file(item->util) &&
+			    !will_fetch(head, item->util))
+				item->util = NULL;
+			item = NULL;
+			continue;
+		}
+
+		/*
+		 * If item is non-NULL here, then we previously saw a
+		 * ref not followed by a peeled reference, so we need
+		 * to check if it is a lightweight tag that we want to
+		 * fetch.
+		 */
+		if (item && !has_sha1_file(item->util) &&
+		    !will_fetch(head, item->util))
+			item->util = NULL;
+
+		item = NULL;
+
+		/* skip duplicates and refs that we already have */
+		if (string_list_has_string(&remote_refs, ref->name) ||
+		    string_list_has_string(&existing_refs, ref->name))
+			continue;
+
+		item = string_list_insert(&remote_refs, ref->name);
+		item->util = (void *)ref->old_sha1;
+	}
+	string_list_clear(&existing_refs, 1);
+
+	/*
+	 * We may have a final lightweight tag that needs to be
+	 * checked to see if it needs fetching.
+	 */
+	if (item && !has_sha1_file(item->util) &&
+	    !will_fetch(head, item->util))
+		item->util = NULL;
+
+	/*
+	 * For all the tags in the remote_refs string list,
+	 * add them to the list of refs to be fetched
+	 */
+	for_each_string_list_item(item, &remote_refs) {
+		/* Unless we have already decided to ignore this item... */
+		if (item->util)
+		{
+			struct ref *rm = alloc_ref(item->string);
+			rm->peer_ref = alloc_ref(item->string);
+			hashcpy(rm->old_sha1, item->util);
+			**tail = rm;
+			*tail = &rm->next;
+		}
+	}
+
+	string_list_clear(&remote_refs, 0);
+}
 
 static struct ref *get_ref_map(struct transport *transport,
 			       struct refspec *refspecs, int refspec_count,
@@ -612,106 +708,6 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)
 	return result;
 }
 
-static int add_existing(const char *refname, const unsigned char *sha1,
-			int flag, void *cbdata)
-{
-	struct string_list *list = (struct string_list *)cbdata;
-	struct string_list_item *item = string_list_insert(list, refname);
-	item->util = xmalloc(20);
-	hashcpy(item->util, sha1);
-	return 0;
-}
-
-static int will_fetch(struct ref **head, const unsigned char *sha1)
-{
-	struct ref *rm = *head;
-	while (rm) {
-		if (!hashcmp(rm->old_sha1, sha1))
-			return 1;
-		rm = rm->next;
-	}
-	return 0;
-}
-
-static void find_non_local_tags(struct transport *transport,
-			struct ref **head,
-			struct ref ***tail)
-{
-	struct string_list existing_refs = STRING_LIST_INIT_DUP;
-	struct string_list remote_refs = STRING_LIST_INIT_NODUP;
-	const struct ref *ref;
-	struct string_list_item *item = NULL;
-
-	for_each_ref(add_existing, &existing_refs);
-	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
-		if (prefixcmp(ref->name, "refs/tags/"))
-			continue;
-
-		/*
-		 * The peeled ref always follows the matching base
-		 * ref, so if we see a peeled ref that we don't want
-		 * to fetch then we can mark the ref entry in the list
-		 * as one to ignore by setting util to NULL.
-		 */
-		if (!suffixcmp(ref->name, "^{}")) {
-			if (item && !has_sha1_file(ref->old_sha1) &&
-			    !will_fetch(head, ref->old_sha1) &&
-			    !has_sha1_file(item->util) &&
-			    !will_fetch(head, item->util))
-				item->util = NULL;
-			item = NULL;
-			continue;
-		}
-
-		/*
-		 * If item is non-NULL here, then we previously saw a
-		 * ref not followed by a peeled reference, so we need
-		 * to check if it is a lightweight tag that we want to
-		 * fetch.
-		 */
-		if (item && !has_sha1_file(item->util) &&
-		    !will_fetch(head, item->util))
-			item->util = NULL;
-
-		item = NULL;
-
-		/* skip duplicates and refs that we already have */
-		if (string_list_has_string(&remote_refs, ref->name) ||
-		    string_list_has_string(&existing_refs, ref->name))
-			continue;
-
-		item = string_list_insert(&remote_refs, ref->name);
-		item->util = (void *)ref->old_sha1;
-	}
-	string_list_clear(&existing_refs, 1);
-
-	/*
-	 * We may have a final lightweight tag that needs to be
-	 * checked to see if it needs fetching.
-	 */
-	if (item && !has_sha1_file(item->util) &&
-	    !will_fetch(head, item->util))
-		item->util = NULL;
-
-	/*
-	 * For all the tags in the remote_refs string list,
-	 * add them to the list of refs to be fetched
-	 */
-	for_each_string_list_item(item, &remote_refs) {
-		/* Unless we have already decided to ignore this item... */
-		if (item->util)
-		{
-			struct ref *rm = alloc_ref(item->string);
-			rm->peer_ref = alloc_ref(item->string);
-			hashcpy(rm->old_sha1, item->util);
-			**tail = rm;
-			*tail = &rm->next;
-		}
-	}
-
-	string_list_clear(&remote_refs, 0);
-}
-
 static void check_not_current_branch(struct ref *ref_map)
 {
 	struct branch *current_branch = branch_get(NULL);
-- 
1.8.4

[PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Michael Haggerty]

Previously, fetch's "--tags" option was considered equivalent to
specifying the refspec "refs/tags/*:refs/tags/*" on the command line;
in particular, it caused the remote.<name>.refspec configuration to be
ignored.

But it is not very useful to fetch tags without also fetching other
references, whereas it *is* quite useful to be able to fetch tags *in
addition to* other references.  So change the semantics of this option
to do the latter.

If a user wants to fetch *only* tags, then it is still possible to
specifying an explicit refspec:

    git fetch <remote> 'refs/tags/*:refs/tags/*'

Please note that the documentation prior to 1.8.0.3 was ambiguous
about this aspect of "fetch --tags" behavior.  Commit

    f0cb2f137c 2012-12-14 fetch --tags: clarify documentation

made the documentation match the old behavior.  This commit changes
the documentation to match the new behavior.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---

The change to builtin/fetch.c:get_ref_map() has the side effect of
changing the order that the (struct ref) objects are listed in
ref_map.  It seems to me that this could probably only matter in the
case of duplicates.  But later ref_remove_duplicates() is called, so
duplicates are eliminated.  However, ref_remove_duplicates() always
retains the first instance of duplicates and discards the rest.  It is
conceivable that the boolean flags (which are not inspected by
ref_remove_duplicates()) could differ among the duplicates, and that
therefore changing the order of the items in the original list has the
effect of changing the flags on the items that are retained.

I don't know this code well enough to judge whether this might be a
problem.

If it is, then the correct approach is probably either to teach
ref_remove_duplicates() to ensure that the flags are also consistent
across duplicates, or to somehow combine the flags from all duplicates
into the one that is retained.  Please let me know if this is needed.

 Documentation/fetch-options.txt          |  8 +++---
 builtin/fetch.c                          | 46 +++++++++++++++++++-------------
 git-pull.sh                              |  2 +-
 t/t5510-fetch.sh                         | 22 ++++++++++++---
 t/t5515/fetch.br-unconfig_--tags_.._.git |  1 +
 t/t5515/fetch.master_--tags_.._.git      |  1 +
 t/t5525-fetch-tagopt.sh                  | 23 ++++++++++++----
 7 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index ba1fe49..0e6d2ac 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -61,11 +61,9 @@ endif::git-pull[]
 ifndef::git-pull[]
 -t::
 --tags::
-	This is a short-hand for giving `refs/tags/*:refs/tags/*`
-	refspec from the command line, to ask all tags to be fetched
-	and stored locally.  Because this acts as an explicit
-	refspec, the default refspecs (configured with the
-	remote.$name.fetch variable) are overridden and not used.
+	This is a short-hand requesting that all tags be fetched from
+	the remote in addition to whatever else is being fetched.  It
+	is similar to using the refspec `refs/tags/*:refs/tags/*`.
 
 --recurse-submodules[=yes|on-demand|no]::
 	This option controls if and under what conditions new commits of
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61e8117..7edb1ea 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -271,7 +271,7 @@ static struct ref *get_ref_map(struct transport *transport,
 
 	const struct ref *remote_refs = transport_get_remote_refs(transport);
 
-	if (refspec_count || tags == TAGS_SET) {
+	if (refspec_count) {
 		struct ref **old_tail;
 
 		for (i = 0; i < refspec_count; i++) {
@@ -279,11 +279,9 @@ static struct ref *get_ref_map(struct transport *transport,
 			if (refspecs[i].dst && refspecs[i].dst[0])
 				*autotags = 1;
 		}
-		/* Merge everything on the command line, but not --tags */
+		/* Merge everything on the command line (but not --tags) */
 		for (rm = ref_map; rm; rm = rm->next)
 			rm->fetch_head_status = FETCH_HEAD_MERGE;
-		if (tags == TAGS_SET)
-			get_fetch_map(remote_refs, tag_refspec, &tail, 0);
 
 		/*
 		 * For any refs that we happen to be fetching via command-line
@@ -334,8 +332,13 @@ static struct ref *get_ref_map(struct transport *transport,
 			tail = &ref_map->next;
 		}
 	}
-	if (tags == TAGS_DEFAULT && *autotags)
+
+	if (tags == TAGS_SET)
+		/* also fetch all tags */
+		get_fetch_map(remote_refs, tag_refspec, &tail, 0);
+	else if (tags == TAGS_DEFAULT && *autotags)
 		find_non_local_tags(transport, &ref_map, &tail);
+
 	ref_remove_duplicates(ref_map);
 
 	return ref_map;
@@ -826,31 +829,38 @@ static int do_fetch(struct transport *transport,
 		goto cleanup;
 	}
 	if (prune) {
-		/*
-		 * If --tags was specified, pretend that the user gave us
-		 * the canonical tags refspec
-		 */
+		struct refspec *prune_refspecs;
+		int prune_refspec_count;
+
+		if (ref_count) {
+			prune_refspecs = refs;
+			prune_refspec_count = ref_count;
+		} else {
+			prune_refspecs = transport->remote->fetch;
+			prune_refspec_count = transport->remote->fetch_refspec_nr;
+		}
+
 		if (tags == TAGS_SET) {
+			/*
+			 * --tags was specified.  Pretend that the user also
+			 * gave us the canonical tags refspec
+			 */
 			const char *tags_str = "refs/tags/*:refs/tags/*";
 			struct refspec *tags_refspec, *refspec;
 
 			/* Copy the refspec and add the tags to it */
-			refspec = xcalloc(ref_count + 1, sizeof(struct refspec));
+			refspec = xcalloc(prune_refspec_count + 1, sizeof(*refspec));
 			tags_refspec = parse_fetch_refspec(1, &tags_str);
-			memcpy(refspec, refs, ref_count * sizeof(struct refspec));
-			memcpy(&refspec[ref_count], tags_refspec, sizeof(struct refspec));
-			ref_count++;
+			memcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));
+			memcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));
 
-			prune_refs(refspec, ref_count, ref_map);
+			prune_refs(refspec, prune_refspec_count + 1, ref_map);
 
-			ref_count--;
 			/* The rest of the strings belong to fetch_one */
 			free_refspec(1, tags_refspec);
 			free(refspec);
-		} else if (ref_count) {
-			prune_refs(refs, ref_count, ref_map);
 		} else {
-			prune_refs(transport->remote->fetch, transport->remote->fetch_refspec_nr, ref_map);
+			prune_refs(prune_refspecs, prune_refspec_count, ref_map);
 		}
 	}
 	free_refs(ref_map);
diff --git a/git-pull.sh b/git-pull.sh
index b946fd9..dac7e1c 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -172,7 +172,7 @@ error_on_no_merge_candidates () {
 	do
 		case "$opt" in
 		-t|--t|--ta|--tag|--tags)
-			echo "Fetching tags only, you probably meant:"
+			echo "It doesn't make sense to pull tags; you probably meant:"
 			echo "  git fetch --tags"
 			exit 1
 		esac
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 8328be1..02e5901 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -113,7 +113,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	git rev-parse origin/master
 '
 
-test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '
+test_expect_success 'fetch --prune --tags prunes tags and branches' '
 	cd "$D" &&
 	git clone . prune-tags &&
 	cd prune-tags &&
@@ -124,7 +124,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br
 
 	git fetch --prune --tags origin &&
 	git rev-parse origin/master &&
-	git rev-parse origin/fake-remote &&
+	test_must_fail git rev-parse origin/fake-remote &&
 	test_must_fail git rev-parse sometag
 '
 
@@ -132,10 +132,26 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo
 	cd "$D" &&
 	git clone . prune-tags-branch &&
 	cd prune-tags-branch &&
+	git tag sometag master &&
 	git update-ref refs/remotes/origin/extrabranch master &&
 
 	git fetch --prune --tags origin master &&
-	git rev-parse origin/extrabranch
+	git rev-parse origin/extrabranch &&
+	test_must_fail git rev-parse sometag
+'
+
+test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '
+	cd "$D" &&
+	git clone . prune-tags-refspec &&
+	cd prune-tags-refspec &&
+	git tag sometag master &&
+	git update-ref refs/remotes/origin/foo/otherbranch master &&
+	git update-ref refs/remotes/origin/extrabranch master &&
+
+	git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&
+	test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&
+	git rev-parse origin/extrabranch &&
+	test_must_fail git rev-parse sometag
 '
 
 test_expect_success 'fetch tags when there is no tags' '
diff --git a/t/t5515/fetch.br-unconfig_--tags_.._.git b/t/t5515/fetch.br-unconfig_--tags_.._.git
index 1669cc4..0f70f66 100644
--- a/t/t5515/fetch.br-unconfig_--tags_.._.git
+++ b/t/t5515/fetch.br-unconfig_--tags_.._.git
@@ -1,4 +1,5 @@
 # br-unconfig --tags ../.git
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5515/fetch.master_--tags_.._.git b/t/t5515/fetch.master_--tags_.._.git
index 8a74935..ab473a6 100644
--- a/t/t5515/fetch.master_--tags_.._.git
+++ b/t/t5515/fetch.master_--tags_.._.git
@@ -1,4 +1,5 @@
 # master --tags ../.git
+0567da4d5edd2ff4bb292a465ba9e64dcad9536b		../
 6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
 8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
 22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh
index 4fbf7a1..45815f7 100755
--- a/t/t5525-fetch-tagopt.sh
+++ b/t/t5525-fetch-tagopt.sh
@@ -8,7 +8,8 @@ setup_clone () {
 	git clone --mirror . $1 &&
 	git remote add remote_$1 $1 &&
 	(cd $1 &&
-	git tag tag_$1)
+	git tag tag_$1 &&
+	git branch branch_$1)
 }
 
 test_expect_success setup '
@@ -21,21 +22,33 @@ test_expect_success setup '
 
 test_expect_success "fetch with tagopt=--no-tags does not get tag" '
 	git fetch remote_one &&
-	test_must_fail git show-ref tag_one
+	test_must_fail git show-ref tag_one &&
+	git show-ref remote_one/branch_one
 	'
 
 test_expect_success "fetch --tags with tagopt=--no-tags gets tag" '
+	(
+		cd one &&
+		git branch second_branch_one
+	) &&
 	git fetch --tags remote_one &&
-	git show-ref tag_one
+	git show-ref tag_one &&
+	git show-ref remote_one/second_branch_one
 	'
 
 test_expect_success "fetch --no-tags with tagopt=--tags does not get tag" '
 	git fetch --no-tags remote_two &&
-	test_must_fail git show-ref tag_two
+	test_must_fail git show-ref tag_two &&
+	git show-ref remote_two/branch_two
 	'
 
 test_expect_success "fetch with tagopt=--tags gets tag" '
+	(
+		cd two &&
+		git branch second_branch_two
+	) &&
 	git fetch remote_two &&
-	git show-ref tag_two
+	git show-ref tag_two &&
+	git show-ref remote_two/second_branch_two
 	'
 test_done
-- 
1.8.4

[PATCH 11/15] fetch --prune: prune only based on explicit refspecs

[Michael Haggerty]

The old behavior of "fetch --prune" was to prune whatever was being
fetched.  In particular, "fetch --prune --tags" caused tags not only
to be fetched, but also to be pruned.  This is inappropriate because
there is only one tags namespace that is shared among the local
repository and all remotes.  Therefore, if the user defines a local
tag and then runs "git fetch --prune --tags", then the local tag is
deleted.  Moreover, "--prune" and "--tags" can also be configured via
fetch.prune / remote.<name>.prune and remote.<name>.tagopt, making it
even less obvious that an invocation of "git fetch" could result in
tag lossage.

Since the command "git remote update" invokes "git fetch", it had the
same problem.

The command "git remote prune", on the other hand, disregarded the
setting of remote.<name>.tagopt, and so its behavior was inconsistent
with that of the other commands.

So the old behavior made it too easy to lose tags.  To fix this
problem, change "fetch --prune" to prune references based only on
refspecs specified explicitly by the user, either on the command line
or via remote.<name>.fetch.  Thus, tags are no longer made subject to
pruning by the --tags option or the remote.<name>.tagopt setting.

However, tags *are* still subject to pruning if they are fetched as
part of a refspec, and that is good.  For example:

* On the command line,

      git fetch --prune 'refs/tags/*:refs/tags/*'

  causes tags, and only tags, to be fetched and pruned, and is
  therefore a simple way for the user to get the equivalent of the old
  behavior of "--prune --tag".

* For a remote that was configured with the "--mirror" option, the
  configuration is set to include

      [remote "name"]
              fetch = +refs/*:refs/*

  , which causes tags to be subject to pruning along with all other
  references.  This is the behavior that will typically be desired for
  a mirror.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/config.txt        |  2 +-
 Documentation/fetch-options.txt | 15 ++++++++++++---
 builtin/fetch.c                 | 39 +++++++++------------------------------
 t/t5510-fetch.sh                | 10 +++++-----
 4 files changed, 27 insertions(+), 39 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d4d93c9..83c1700 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2086,7 +2086,7 @@ remote.<name>.vcs::
 remote.<name>.prune::
 	When set to true, fetching from this remote by default will also
 	remove any remote-tracking branches which no longer exist on the
-	remote (as if the `--prune` option was give on the command line).
+	remote (as if the `--prune` option was given on the command line).
 	Overrides `fetch.prune` settings, if any.
 
 remotes.<group>::
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 0e6d2ac..5d12219 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -41,8 +41,14 @@ ifndef::git-pull[]
 
 -p::
 --prune::
-	After fetching, remove any remote-tracking branches which
-	no longer exist	on the remote.
+	After fetching, remove any remote-tracking branches that
+	no longer exist	on the remote.  Tags are not subject to
+	pruning in the usual case that they are fetched because of the
+	--tags option or remote.<name>.tagopt.  However, if tags are
+	fetched due to an explicit refspec (either on the command line
+	or in the remote configuration, for example if the remote was
+	cloned with the --mirror option), then they are also subject
+	to pruning.
 endif::git-pull[]
 
 ifdef::git-pull[]
@@ -63,7 +69,10 @@ ifndef::git-pull[]
 --tags::
 	This is a short-hand requesting that all tags be fetched from
 	the remote in addition to whatever else is being fetched.  It
-	is similar to using the refspec `refs/tags/*:refs/tags/*`.
+	is similar to using the refspec `refs/tags/*:refs/tags/*`,
+	except that it doesn't subject tags to pruning, regardless of
+	a --prune option or the configuration settings of fetch.prune
+	or remote.<name>.prune.
 
 --recurse-submodules[=yes|on-demand|no]::
 	This option controls if and under what conditions new commits of
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7edb1ea..47b63a7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -829,38 +829,17 @@ static int do_fetch(struct transport *transport,
 		goto cleanup;
 	}
 	if (prune) {
-		struct refspec *prune_refspecs;
-		int prune_refspec_count;
-
+		/*
+		 * We only prune based on refspecs specified
+		 * explicitly (via command line or configuration); we
+		 * don't care whether --tags was specified.
+		 */
 		if (ref_count) {
-			prune_refspecs = refs;
-			prune_refspec_count = ref_count;
-		} else {
-			prune_refspecs = transport->remote->fetch;
-			prune_refspec_count = transport->remote->fetch_refspec_nr;
-		}
-
-		if (tags == TAGS_SET) {
-			/*
-			 * --tags was specified.  Pretend that the user also
-			 * gave us the canonical tags refspec
-			 */
-			const char *tags_str = "refs/tags/*:refs/tags/*";
-			struct refspec *tags_refspec, *refspec;
-
-			/* Copy the refspec and add the tags to it */
-			refspec = xcalloc(prune_refspec_count + 1, sizeof(*refspec));
-			tags_refspec = parse_fetch_refspec(1, &tags_str);
-			memcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));
-			memcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));
-
-			prune_refs(refspec, prune_refspec_count + 1, ref_map);
-
-			/* The rest of the strings belong to fetch_one */
-			free_refspec(1, tags_refspec);
-			free(refspec);
+			prune_refs(refs, ref_count, ref_map);
 		} else {
-			prune_refs(prune_refspecs, prune_refspec_count, ref_map);
+			prune_refs(transport->remote->fetch,
+				   transport->remote->fetch_refspec_nr,
+				   ref_map);
 		}
 	}
 	free_refs(ref_map);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 02e5901..5d4581d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -113,7 +113,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
 	git rev-parse origin/master
 '
 
-test_expect_success 'fetch --prune --tags prunes tags and branches' '
+test_expect_success 'fetch --prune --tags prunes branches but not tags' '
 	cd "$D" &&
 	git clone . prune-tags &&
 	cd prune-tags &&
@@ -125,10 +125,10 @@ test_expect_success 'fetch --prune --tags prunes tags and branches' '
 	git fetch --prune --tags origin &&
 	git rev-parse origin/master &&
 	test_must_fail git rev-parse origin/fake-remote &&
-	test_must_fail git rev-parse sometag
+	git rev-parse sometag
 '
 
-test_expect_success 'fetch --prune --tags with branch does not delete other remote-tracking branches' '
+test_expect_success 'fetch --prune --tags with branch does not prune other things' '
 	cd "$D" &&
 	git clone . prune-tags-branch &&
 	cd prune-tags-branch &&
@@ -137,7 +137,7 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo
 
 	git fetch --prune --tags origin master &&
 	git rev-parse origin/extrabranch &&
-	test_must_fail git rev-parse sometag
+	git rev-parse sometag
 '
 
 test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' '
@@ -151,7 +151,7 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec'
 	git fetch --prune --tags origin refs/heads/foo/*:refs/remotes/origin/foo/* &&
 	test_must_fail git rev-parse refs/remotes/origin/foo/otherbranch &&
 	git rev-parse origin/extrabranch &&
-	test_must_fail git rev-parse sometag
+	git rev-parse sometag
 '
 
 test_expect_success 'fetch tags when there is no tags' '
-- 
1.8.4

[PATCH 12/15] query_refspecs(): move some constants out of the loop

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 remote.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 5ade07f..ff5b62a 100644
--- a/remote.c
+++ b/remote.c
@@ -829,6 +829,8 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q
 {
 	int i;
 	int find_src = !query->src;
+	const char *needle = find_src ? query->dst : query->src;
+	char **result = find_src ? &query->src : &query->dst;
 
 	if (find_src && !query->dst)
 		return error("query_refspecs: need either src or dst");
@@ -837,8 +839,6 @@ static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *q
 		struct refspec *refspec = &refs[i];
 		const char *key = find_src ? refspec->dst : refspec->src;
 		const char *value = find_src ? refspec->src : refspec->dst;
-		const char *needle = find_src ? query->dst : query->src;
-		char **result = find_src ? &query->src : &query->dst;
 
 		if (!refspec->dst)
 			continue;
-- 
1.8.4

[PATCH 13/15] builtin/remote.c: reorder function definitions

[Michael Haggerty]

Reorder function definitions to remove the need for forward
declarations.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 159 +++++++++++++++++++++++++++----------------------------
 1 file changed, 78 insertions(+), 81 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 4e14891..ecedd96 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -77,9 +77,6 @@ static const char * const builtin_remote_seturl_usage[] = {
 
 static int verbose;
 
-static int show_all(void);
-static int prune_remote(const char *remote, int dry_run);
-
 static inline int postfixcmp(const char *string, const char *postfix)
 {
 	int len1 = strlen(string), len2 = strlen(postfix);
@@ -1084,6 +1081,64 @@ static int show_push_info_item(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
+static int get_one_entry(struct remote *remote, void *priv)
+{
+	struct string_list *list = priv;
+	struct strbuf url_buf = STRBUF_INIT;
+	const char **url;
+	int i, url_nr;
+
+	if (remote->url_nr > 0) {
+		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
+		string_list_append(list, remote->name)->util =
+				strbuf_detach(&url_buf, NULL);
+	} else
+		string_list_append(list, remote->name)->util = NULL;
+	if (remote->pushurl_nr) {
+		url = remote->pushurl;
+		url_nr = remote->pushurl_nr;
+	} else {
+		url = remote->url;
+		url_nr = remote->url_nr;
+	}
+	for (i = 0; i < url_nr; i++)
+	{
+		strbuf_addf(&url_buf, "%s (push)", url[i]);
+		string_list_append(list, remote->name)->util =
+				strbuf_detach(&url_buf, NULL);
+	}
+
+	return 0;
+}
+
+static int show_all(void)
+{
+	struct string_list list = STRING_LIST_INIT_NODUP;
+	int result;
+
+	list.strdup_strings = 1;
+	result = for_each_remote(get_one_entry, &list);
+
+	if (!result) {
+		int i;
+
+		sort_string_list(&list);
+		for (i = 0; i < list.nr; i++) {
+			struct string_list_item *item = list.items + i;
+			if (verbose)
+				printf("%s\t%s\n", item->string,
+					item->util ? (const char *)item->util : "");
+			else {
+				if (i && !strcmp((item - 1)->string, item->string))
+					continue;
+				printf("%s\n", item->string);
+			}
+		}
+	}
+	string_list_clear(&list, 1);
+	return result;
+}
+
 static int show(int argc, const char **argv)
 {
 	int no_query = 0, result = 0, query_flag = 0;
@@ -1246,26 +1301,6 @@ static int set_head(int argc, const char **argv)
 	return result;
 }
 
-static int prune(int argc, const char **argv)
-{
-	int dry_run = 0, result = 0;
-	struct option options[] = {
-		OPT__DRY_RUN(&dry_run, N_("dry run")),
-		OPT_END()
-	};
-
-	argc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,
-			     0);
-
-	if (argc < 1)
-		usage_with_options(builtin_remote_prune_usage, options);
-
-	for (; argc; argc--, argv++)
-		result |= prune_remote(*argv, dry_run);
-
-	return result;
-}
-
 static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
@@ -1304,6 +1339,26 @@ static int prune_remote(const char *remote, int dry_run)
 	return result;
 }
 
+static int prune(int argc, const char **argv)
+{
+	int dry_run = 0, result = 0;
+	struct option options[] = {
+		OPT__DRY_RUN(&dry_run, N_("dry run")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, NULL, options, builtin_remote_prune_usage,
+			     0);
+
+	if (argc < 1)
+		usage_with_options(builtin_remote_prune_usage, options);
+
+	for (; argc; argc--, argv++)
+		result |= prune_remote(*argv, dry_run);
+
+	return result;
+}
+
 static int get_remote_default(const char *key, const char *value, void *priv)
 {
 	if (strcmp(key, "remotes.default") == 0) {
@@ -1505,64 +1560,6 @@ static int set_url(int argc, const char **argv)
 	return 0;
 }
 
-static int get_one_entry(struct remote *remote, void *priv)
-{
-	struct string_list *list = priv;
-	struct strbuf url_buf = STRBUF_INIT;
-	const char **url;
-	int i, url_nr;
-
-	if (remote->url_nr > 0) {
-		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
-		string_list_append(list, remote->name)->util =
-				strbuf_detach(&url_buf, NULL);
-	} else
-		string_list_append(list, remote->name)->util = NULL;
-	if (remote->pushurl_nr) {
-		url = remote->pushurl;
-		url_nr = remote->pushurl_nr;
-	} else {
-		url = remote->url;
-		url_nr = remote->url_nr;
-	}
-	for (i = 0; i < url_nr; i++)
-	{
-		strbuf_addf(&url_buf, "%s (push)", url[i]);
-		string_list_append(list, remote->name)->util =
-				strbuf_detach(&url_buf, NULL);
-	}
-
-	return 0;
-}
-
-static int show_all(void)
-{
-	struct string_list list = STRING_LIST_INIT_NODUP;
-	int result;
-
-	list.strdup_strings = 1;
-	result = for_each_remote(get_one_entry, &list);
-
-	if (!result) {
-		int i;
-
-		sort_string_list(&list);
-		for (i = 0; i < list.nr; i++) {
-			struct string_list_item *item = list.items + i;
-			if (verbose)
-				printf("%s\t%s\n", item->string,
-					item->util ? (const char *)item->util : "");
-			else {
-				if (i && !strcmp((item - 1)->string, item->string))
-					continue;
-				printf("%s\n", item->string);
-			}
-		}
-	}
-	string_list_clear(&list, 1);
-	return result;
-}
-
 int cmd_remote(int argc, const char **argv, const char *prefix)
 {
 	struct option options[] = {
-- 
1.8.4

[PATCH 14/15] builtin/remote.c:update(): use struct argv_array

[Michael Haggerty]

Use struct argv_array for calling the "git fetch" subprocesses.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index ecedd96..bffe2f9 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -6,6 +6,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 #include "refs.h"
+#include "argv-array.h"
 
 static const char * const builtin_remote_usage[] = {
 	N_("git remote [-v | --verbose]"),
@@ -1376,36 +1377,36 @@ static int update(int argc, const char **argv)
 			 N_("prune remotes after fetching")),
 		OPT_END()
 	};
-	const char **fetch_argv;
-	int fetch_argc = 0;
+	struct argv_array fetch_argv = ARGV_ARRAY_INIT;
 	int default_defined = 0;
-
-	fetch_argv = xmalloc(sizeof(char *) * (argc+5));
+	int retval;
 
 	argc = parse_options(argc, argv, NULL, options, builtin_remote_update_usage,
 			     PARSE_OPT_KEEP_ARGV0);
 
-	fetch_argv[fetch_argc++] = "fetch";
+	argv_array_push(&fetch_argv, "fetch");
 
 	if (prune)
-		fetch_argv[fetch_argc++] = "--prune";
+		argv_array_push(&fetch_argv, "--prune");
 	if (verbose)
-		fetch_argv[fetch_argc++] = "-v";
-	fetch_argv[fetch_argc++] = "--multiple";
+		argv_array_push(&fetch_argv, "-v");
+	argv_array_push(&fetch_argv, "--multiple");
 	if (argc < 2)
-		fetch_argv[fetch_argc++] = "default";
+		argv_array_push(&fetch_argv, "default");
 	for (i = 1; i < argc; i++)
-		fetch_argv[fetch_argc++] = argv[i];
+		argv_array_push(&fetch_argv, argv[i]);
 
-	if (strcmp(fetch_argv[fetch_argc-1], "default") == 0) {
+	if (strcmp(fetch_argv.argv[fetch_argv.argc-1], "default") == 0) {
 		git_config(get_remote_default, &default_defined);
-		if (!default_defined)
-			fetch_argv[fetch_argc-1] = "--all";
+		if (!default_defined) {
+			argv_array_pop(&fetch_argv);
+			argv_array_push(&fetch_argv, "--all");
+		}
 	}
 
-	fetch_argv[fetch_argc] = NULL;
-
-	return run_command_v_opt(fetch_argv, RUN_GIT_CMD);
+	retval = run_command_v_opt(fetch_argv.argv, RUN_GIT_CMD);
+	argv_array_clear(&fetch_argv);
+	return retval;
 }
 
 static int remove_all_fetch_refspecs(const char *remote, const char *key)
-- 
1.8.4

[PATCH 15/15] fetch, remote: properly convey --no-prune options to subprocesses

[Michael Haggerty]

If --no-prune is passed to one of the following commands:

    git fetch --all
    git fetch --multiple
    git fetch --recurse-submodules
    git remote update

then it must also be passed to the "fetch" subprocesses that those
commands use to do their work.  Otherwise there might be a fetch.prune
or remote.<name>.prune configuration setting that causes pruning to
occur, contrary to the user's express wish.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/fetch.c  | 4 ++--
 builtin/remote.c | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 47b63a7..8711df0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -915,8 +915,8 @@ static void add_options_to_argv(struct argv_array *argv)
 {
 	if (dry_run)
 		argv_array_push(argv, "--dry-run");
-	if (prune > 0)
-		argv_array_push(argv, "--prune");
+	if (prune != -1)
+		argv_array_push(argv, prune ? "--prune" : "--no-prune");
 	if (update_head_ok)
 		argv_array_push(argv, "--update-head-ok");
 	if (force)
diff --git a/builtin/remote.c b/builtin/remote.c
index bffe2f9..f532f35 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1371,7 +1371,7 @@ static int get_remote_default(const char *key, const char *value, void *priv)
 
 static int update(int argc, const char **argv)
 {
-	int i, prune = 0;
+	int i, prune = -1;
 	struct option options[] = {
 		OPT_BOOL('p', "prune", &prune,
 			 N_("prune remotes after fetching")),
@@ -1386,8 +1386,8 @@ static int update(int argc, const char **argv)
 
 	argv_array_push(&fetch_argv, "fetch");
 
-	if (prune)
-		argv_array_push(&fetch_argv, "--prune");
+	if (prune != -1)
+		argv_array_push(&fetch_argv, prune ? "--prune" : "--no-prune");
 	if (verbose)
 		argv_array_push(&fetch_argv, "-v");
 	argv_array_push(&fetch_argv, "--multiple");
-- 
1.8.4

Re: [PATCH 00/15] Change semantics of "fetch --tags"

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> This is my proposed fix for the "local tag killer" problem that I
> reported recently [1].
>
> There are three main things changed by this patch series:
> ...

Haven't looked at any of the 1-15 messages, but the basic design to
demote "--tags" from being an explicit "refs/tags/*:refs/tags/*"
given from the command line to a more special case option sounds
very sane and solid.

With the auto-following of tags, I think it is a _mistake_ to have
"--tags" anywhere on the command line (and "tagopt" config) these
days, and I do not expect a huge fallout from incompatibility that
arises from the change in behaviour.

Thanks---looking forward to reading it through.

Re: [PATCH 02/15] t5510: prepare test refs more straightforwardly

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> "git fetch" was being used with contrived refspecs to create tags and
> remote-tracking branches in test repositories in preparation for the
> actual tests.  This is obscure and also makes one wonder whether this
> is indeed just preparation or whether some side-effect of "git fetch"
> is being tested.
>
> So use the more straightforward commands "git tag" / "git update-ref"
> when preparing branches in test repositories.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  t/t5510-fetch.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index c5e5dfc..08d8dbb 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -88,7 +88,7 @@ test_expect_success 'fetch --prune on its own works as expected' '
>  	cd "$D" &&
>  	git clone . prune &&
>  	cd prune &&
> -	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
> +	git update-ref refs/remotes/origin/extrabranch master &&

As long as you have checked that our local 'master' should be at the
same commit as the origin's 'master' at this point, I think this
change is OK.

I wouldn't call the use of "very explicit, without any room for
mistake" refspecs "contrived", though.

Re: [PATCH 04/15] api-remote.txt: correct section "struct refspect"

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> Subject: Re: [PATCH 04/15] api-remote.txt: correct section "struct refspect"

refspect???

> * Replace reference to function parse_ref_spec() with references to
>   functions parse_fetch_refspec() and parse_push_refspec().
>
> * Correct description of src and dst: they *do* include the '*'
>   characters.

 * Replace a single SP after a full-stop at the end of sentence with
   double SP as if we were back in the typewriter age.

The last one made it hard to spot what actually got changed,
though.  The updated text otherwise looks correct.

Thanks.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  Documentation/technical/api-remote.txt | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/technical/api-remote.txt b/Documentation/technical/api-remote.txt
> index 4be8776..5d245aa 100644
> --- a/Documentation/technical/api-remote.txt
> +++ b/Documentation/technical/api-remote.txt
> @@ -58,16 +58,16 @@ default remote, given the current branch and configuration.
>  struct refspec
>  --------------
>  
> -A struct refspec holds the parsed interpretation of a refspec. If it
> -will force updates (starts with a '+'), force is true. If it is a
> -pattern (sides end with '*') pattern is true. src and dest are the two
> -sides (if a pattern, only the part outside of the wildcards); if there
> -is only one side, it is src, and dst is NULL; if sides exist but are
> -empty (i.e., the refspec either starts or ends with ':'), the
> -corresponding side is "".
> -
> -This parsing can be done to an array of strings to give an array of
> -struct refpsecs with parse_ref_spec().
> +A struct refspec holds the parsed interpretation of a refspec.  If it
> +will force updates (starts with a '+'), force is true.  If it is a
> +pattern (sides end with '*') pattern is true.  src and dest are the
> +two sides (including '*' characters if present); if there is only one
> +side, it is src, and dst is NULL; if sides exist but are empty (i.e.,
> +the refspec either starts or ends with ':'), the corresponding side is
> +"".
> +
> +An array of strings can be parsed into an array of struct refspecs
> +using parse_fetch_refspec() or parse_push_refspec().
>  
>  remote_find_tracking(), given a remote and a struct refspec with
>  either src or dst filled out, will fill out the other such that the

Re: [PATCH 05/15] get_ref_map(): rename local variables

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Rename "refs" -> "refspecs" and "ref_count" -> "refspec_count" to
> reduce confusion, because they describe an array of "struct refspec",
> as opposed to the "struct ref" objects that are also used in this
> function.

Good.  In general, we'd prefer to name an array of things that are
primarily walked in the index order "thing[]", so that "thing number
3" can be spelled thing[3] (not things[3]) in the code, though.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/fetch.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index bd7a101..2248abf 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -165,8 +165,8 @@ static void find_non_local_tags(struct transport *transport,
>  			struct ref ***tail);
>  
>  static struct ref *get_ref_map(struct transport *transport,
> -			       struct refspec *refs, int ref_count, int tags,
> -			       int *autotags)
> +			       struct refspec *refspecs, int refspec_count,
> +			       int tags, int *autotags)
>  {
>  	int i;
>  	struct ref *rm;
> @@ -175,12 +175,12 @@ static struct ref *get_ref_map(struct transport *transport,
>  
>  	const struct ref *remote_refs = transport_get_remote_refs(transport);
>  
> -	if (ref_count || tags == TAGS_SET) {
> +	if (refspec_count || tags == TAGS_SET) {
>  		struct ref **old_tail;
>  
> -		for (i = 0; i < ref_count; i++) {
> -			get_fetch_map(remote_refs, &refs[i], &tail, 0);
> -			if (refs[i].dst && refs[i].dst[0])
> +		for (i = 0; i < refspec_count; i++) {
> +			get_fetch_map(remote_refs, &refspecs[i], &tail, 0);
> +			if (refspecs[i].dst && refspecs[i].dst[0])
>  				*autotags = 1;
>  		}
>  		/* Merge everything on the command line, but not --tags */

Re: [PATCH 08/15] ref_remove_duplicates(): improve documentation comment

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Up to this point the patches all look very sensible (modulo minor
nits I sent separately).  Will come back to the rest of the topics
later.

Thanks.

> ---
>  remote.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/remote.h b/remote.h
> index 131130a..40293c0 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -149,7 +149,14 @@ int resolve_remote_symref(struct ref *ref, struct ref *list);
>  int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1);
>  
>  /*
> - * Removes and frees any duplicate refs in the map.
> + * Remove and free all but the first of any entries in the input list
> + * that map the same remote reference to the same local reference.  If
> + * there are two entries that map different remote references to the
> + * same local reference, die.
> + *
> + * Note that the first entry is never removed; therefore, the pointer
> + * passed in as argument still points to the head of the list after
> + * the function returns.
>   */
>  void ref_remove_duplicates(struct ref *ref_map);

Re: [PATCH 02/15] t5510: prepare test refs more straightforwardly

[Michael Haggerty]

On 10/23/2013 08:36 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> "git fetch" was being used with contrived refspecs to create tags and
>> remote-tracking branches in test repositories in preparation for the
>> actual tests.  This is obscure and also makes one wonder whether this
>> is indeed just preparation or whether some side-effect of "git fetch"
>> is being tested.
>>
>> So use the more straightforward commands "git tag" / "git update-ref"
>> when preparing branches in test repositories.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  t/t5510-fetch.sh | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index c5e5dfc..08d8dbb 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -88,7 +88,7 @@ test_expect_success 'fetch --prune on its own works as expected' '
>>  	cd "$D" &&
>>  	git clone . prune &&
>>  	cd prune &&
>> -	git fetch origin refs/heads/master:refs/remotes/origin/extrabranch &&
>> +	git update-ref refs/remotes/origin/extrabranch master &&
> 
> As long as you have checked that our local 'master' should be at the
> same commit as the origin's 'master' at this point, I think this
> change is OK.

It doesn't matter what the reference points at; the only point of these
tests is to check whether branches that look like stale remote-tracking
branches are pruned or not by the later command.

> I wouldn't call the use of "very explicit, without any room for
> mistake" refspecs "contrived", though.

According to Wiktionary, contrived means "unnatural, forced".

When the goal is just to create a local reference whose contents are
irrelevant, "fetch" is not the first command that comes to my mind.  It
brings a lot of unnecessary machinery to bear on such a trivial task.
Plus it is not very common in daily life to invoke "fetch" with a
complicated, asymmetic refspec like this.  Because of that it cost me a
little extra time to convince myself that the "fetch" command wasn't
trying to do something more.  In that sense it seems "contrived" to me.

Michael

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

Re: [PATCH 04/15] api-remote.txt: correct section "struct refspect"

[Michael Haggerty]

On 10/23/2013 08:43 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> Subject: Re: [PATCH 04/15] api-remote.txt: correct section "struct refspect"
> 
> refspect???

Thanks for catching the typo.

>> * Replace reference to function parse_ref_spec() with references to
>>   functions parse_fetch_refspec() and parse_push_refspec().
>>
>> * Correct description of src and dst: they *do* include the '*'
>>   characters.
> 
>  * Replace a single SP after a full-stop at the end of sentence with
>    double SP as if we were back in the typewriter age.

:-)

I agree it's archaic, but it is standard practice in English.

Also, emacs, with the default sentence-end-double-space setting, doesn't
treat punctuation followed by a single space as an end of sentence and
when reflowing text even goes to extra effort not to break a line at
such a position (because that would make it look like it *were* the end
of the sentence).

TeX also distinguishes between interword spaces and end-of-sentence
spaces, but it uses a different heuristic (which can be overridden by
explicit markup).  It also typesets them differently: end-of-sentence
spaces are a bit wider and more elastic.

Nevertheless it is probably good that there is no Unicode
END_OF_SENTENCE_SPACE character; otherwise we would never get *any* work
done for all the arguing about how to encode empty pixels :-)

> The last one made it hard to spot what actually got changed,
> though.  The updated text otherwise looks correct.

Michael

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

Re: [PATCH 05/15] get_ref_map(): rename local variables

[Michael Haggerty]

On 10/23/2013 08:45 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Rename "refs" -> "refspecs" and "ref_count" -> "refspec_count" to
>> reduce confusion, because they describe an array of "struct refspec",
>> as opposed to the "struct ref" objects that are also used in this
>> function.
> 
> Good.  In general, we'd prefer to name an array of things that are
> primarily walked in the index order "thing[]", so that "thing number
> 3" can be spelled thing[3] (not things[3]) in the code, though.

Since I didn't change singular -> plural or vice versa in this patch,
it's a bit off topic, but in case you are curious I prefer plural to
distinguish which pointers point at lists or arrays as opposed to single
objects.  This convention conveniently leaves the singular available to
name a variable that is used for a single object; for example, in a loop

    struct thing thing = things[i];

(The convention in SQL is different: tables are usually named using
singular nouns.  But that makes sense in SQL because there is not really
a way to reference a single row in a table as an aggregate, so there is
no need to reserve the singular noun for that purpose.  In fact, in
SELECT statements the table name often appears in a context that makes
it look like it does refer to a single row:

    SELECT employee.name, employee.salary FROM ...

So I think it makes sense to use different conventions in C vs. SQL.)

Michael

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

Re: [PATCH 02/15] t5510: prepare test refs more straightforwardly

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> As long as you have checked that our local 'master' should be at the
>> same commit as the origin's 'master' at this point, I think this
>> change is OK.
>
> It doesn't matter what the reference points at; the only point of these
> tests is to check whether branches that look like stale remote-tracking
> branches are pruned or not by the later command.

OK, thanks.

Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Previously, fetch's "--tags" option was considered equivalent to
> specifying the refspec "refs/tags/*:refs/tags/*" on the command line;
> in particular, it caused the remote.<name>.refspec configuration to be
> ignored.
>
> But it is not very useful to fetch tags without also fetching other
> references, whereas it *is* quite useful to be able to fetch tags *in
> addition to* other references.

True but when fetching other references, tags relevant to the
history being fetched by default should automatically follow, so the
above explains why "fetch --tags" is not a useful thing to do daily.

> So change the semantics of this option
> to do the latter.

And I am not opposed to that change in the semantics; it makes an
operation that is not so useful less annoying.

> If a user wants to fetch *only* tags, then it is still possible to
> specifying an explicit refspec:
>
>     git fetch <remote> 'refs/tags/*:refs/tags/*'
>
> Please note that the documentation prior to 1.8.0.3 was ambiguous
> about this aspect of "fetch --tags" behavior.  Commit
>
>     f0cb2f137c 2012-12-14 fetch --tags: clarify documentation
>
> made the documentation match the old behavior.  This commit changes
> the documentation to match the new behavior.

The "old behaviour as designed".  The documentation change was not
about refusing to fix a bug but the above makes it sound as if it
were a such commit.

> The change to builtin/fetch.c:get_ref_map() has the side effect of
> changing the order that the (struct ref) objects are listed in
> ref_map.  It seems to me that this could probably only matter in the
> case of duplicates.  But later ref_remove_duplicates() is called, so
> duplicates are eliminated.  However, ref_remove_duplicates() always
> retains the first instance of duplicates and discards the rest.  It is
> conceivable that the boolean flags (which are not inspected by
> ref_remove_duplicates()) could differ among the duplicates, and that
> therefore changing the order of the items in the original list has the
> effect of changing the flags on the items that are retained.
>
> I don't know this code well enough to judge whether this might be a
> problem.

> If it is, then the correct approach is probably either to teach
> ref_remove_duplicates() to ensure that the flags are also consistent
> across duplicates, or to somehow combine the flags from all duplicates
> into the one that is retained.  Please let me know if this is needed.

I do not think either of these two is sufficient if you want to fix
it; ref_remove_duplicates() needs to be taught to retain the first
one it encounters among the duplicates, not "ensure the flags are
consistent" by erroring out when they are not among duplicates, nor
"somehow combine" flags from later one to the first one.

But I suspect that, if this behaviour change were a problem, such a
configuration was a problem before this change to most people; the
order of duplicated [remote "foo"] section would not be under
control of those who only use "git remote" without using an editor
to tweak .git/config file anyway. So I do not think this regression
is too big a deal; it is something you can fix later on top.

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index ba1fe49..0e6d2ac 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -61,11 +61,9 @@ endif::git-pull[]
>  ifndef::git-pull[]
>  -t::
>  --tags::
> -	This is a short-hand for giving `refs/tags/*:refs/tags/*`
> -	refspec from the command line, to ask all tags to be fetched
> -	and stored locally.  Because this acts as an explicit
> -	refspec, the default refspecs (configured with the
> -	remote.$name.fetch variable) are overridden and not used.
> +	This is a short-hand requesting that all tags be fetched from
> +	the remote in addition to whatever else is being fetched.  It
> +	is similar to using the refspec `refs/tags/*:refs/tags/*`.

This is no longer a short-hand, is it?  There is no other way to ask
"fetch the usual stuff, and then refs/tags/*:refs/tags/* as well".

It should be sufficient for me to locally do:

    s/This is a short-hand requesting/Request/;

I think.

> diff --git a/git-pull.sh b/git-pull.sh
> index b946fd9..dac7e1c 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -172,7 +172,7 @@ error_on_no_merge_candidates () {
>  	do
>  		case "$opt" in
>  		-t|--t|--ta|--tag|--tags)
> -			echo "Fetching tags only, you probably meant:"
> +			echo "It doesn't make sense to pull tags; you probably meant:"

s/pull tags/pull all tags/; perhaps?

>  			echo "  git fetch --tags"
>  			exit 1
>  		esac
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 8328be1..02e5901 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -113,7 +113,7 @@ test_expect_success 'fetch --prune with a namespace keeps other namespaces' '
>  	git rev-parse origin/master
>  '
>  
> -test_expect_success 'fetch --prune --tags does not delete the remote-tracking branches' '
> +test_expect_success 'fetch --prune --tags prunes tags and branches' '
>  	cd "$D" &&
>  	git clone . prune-tags &&
>  	cd prune-tags &&
> @@ -124,7 +124,7 @@ test_expect_success 'fetch --prune --tags does not delete the remote-tracking br
>  
>  	git fetch --prune --tags origin &&
>  	git rev-parse origin/master &&
> -	git rev-parse origin/fake-remote &&
> +	test_must_fail git rev-parse origin/fake-remote &&

Nice.

>  	test_must_fail git rev-parse sometag
>  '
>  
> @@ -132,10 +132,26 @@ test_expect_success 'fetch --prune --tags with branch does not delete other remo
>  	cd "$D" &&
>  	git clone . prune-tags-branch &&
>  	cd prune-tags-branch &&
> +	git tag sometag master &&
>  	git update-ref refs/remotes/origin/extrabranch master &&
>  
>  	git fetch --prune --tags origin master &&
> -	git rev-parse origin/extrabranch
> +	git rev-parse origin/extrabranch &&
> +	test_must_fail git rev-parse sometag

OK, and I'd imagine this will be an issue for a later patch to
address...

> diff --git a/t/t5515/fetch.br-unconfig_--tags_.._.git b/t/t5515/fetch.br-unconfig_--tags_.._.git
> index 1669cc4..0f70f66 100644
> --- a/t/t5515/fetch.br-unconfig_--tags_.._.git
> +++ b/t/t5515/fetch.br-unconfig_--tags_.._.git
> @@ -1,4 +1,5 @@
>  # br-unconfig --tags ../.git
> +0567da4d5edd2ff4bb292a465ba9e64dcad9536b		../
>  6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
>  8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
>  22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../
> diff --git a/t/t5515/fetch.master_--tags_.._.git b/t/t5515/fetch.master_--tags_.._.git
> index 8a74935..ab473a6 100644
> --- a/t/t5515/fetch.master_--tags_.._.git
> +++ b/t/t5515/fetch.master_--tags_.._.git
> @@ -1,4 +1,5 @@
>  # master --tags ../.git
> +0567da4d5edd2ff4bb292a465ba9e64dcad9536b		../
>  6c9dec2b923228c9ff994c6cfe4ae16c12408dc5	not-for-merge	tag 'tag-master' of ../
>  8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge	tag 'tag-one' of ../
>  22feea448b023a2d864ef94b013735af34d238ba	not-for-merge	tag 'tag-one-tree' of ../

This shows us that the updated error message for "git pull --tags"
is much less likely to be given to the user, as we are much more
likely to be fetching something we would want to fetch and merge.
Good.

Thanks.

Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> ...
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>

Everything in the proposed log message made sense to me.

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d4d93c9..83c1700 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2086,7 +2086,7 @@ remote.<name>.vcs::
>  remote.<name>.prune::
>  	When set to true, fetching from this remote by default will also
>  	remove any remote-tracking branches which no longer exist on the
> -	remote (as if the `--prune` option was give on the command line).
> +	remote (as if the `--prune` option was given on the command line).

Shouldn't we stop saying "branches" here?

> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 0e6d2ac..5d12219 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -41,8 +41,14 @@ ifndef::git-pull[]
>  
>  -p::
>  --prune::
> -	After fetching, remove any remote-tracking branches which
> -	no longer exist	on the remote.
> +	After fetching, remove any remote-tracking branches that

Likewise.  This is a lot more important than the one in
remote.<name>.prune documentation, as the next sentence "Tags are
not subject to ..." implies that they fall into the same category as
what gets pruned here, i.e. "remote-tracking branches" in the above
sentence, but nobody calls refs/tags/v1.0.0 a "remote-tracking
branch" even if it came from your 'origin'.

> +	no longer exist	on the remote.  Tags are not subject to
> +	pruning in the usual case that they are fetched because of the
> +	--tags option or remote.<name>.tagopt.  

We should mention the most usual case tags are fetched, before
mentioning the case the unusual option "--tags" was used from the
command line or .tagopt configuration was used.  Namely, when the
tags are automatically followed.

> +     However, if tags are
> +	fetched due to an explicit refspec (either on the command line
> +	or in the remote configuration, for example if the remote was
> +	cloned with the --mirror option), then they are also subject
> +	to pruning.

Very good.

> @@ -63,7 +69,10 @@ ifndef::git-pull[]
>  --tags::
>  	This is a short-hand requesting that all tags be fetched from
>  	the remote in addition to whatever else is being fetched.  It
> -	is similar to using the refspec `refs/tags/*:refs/tags/*`.
> +	is similar to using the refspec `refs/tags/*:refs/tags/*`,
> +	except that it doesn't subject tags to pruning, regardless of
> +	a --prune option or the configuration settings of fetch.prune
> +	or remote.<name>.prune.

Using --tags is not similar to using refs/tags/*:refs/tags/* after
the previous patch already; "git fetch origin --tags" and "git fetch
origin refs/tags/*:refs/tags/*" are vastly different and that was
the whole point of the previous step.  And that "calling something
not so similar similar" needs to be fixed further here to clarify
that they are not similar in yet another way.

We should just lose "It is similar to using" from 10/15 and start
over, perhaps?  Add the first paragraph of the below in 10/15 and
add the rest in 11/15, or something.

	--tags::
		Request that all tags be fetched from the remote
		under the same name (i.e. `refs/tags/X` is created in
		our repository by copying their `refs/tags/X`), in
		addition to whatever is fetched by the same `git
		fetch` command without this option on the command
		line.
	+
        When `refs/tags/*` hierarchy from the remote is copied only
        because this option was given, they are not subject to be
	pruned when `--prune` option (or configuration variables
	like `fetch.prune` or `remote.<name>.prune`) is in effect.

That would make it clear that they are subject to pruning when --mirror
or an explicit refspec refs/tags/*:refs/tags/* is given, as tags are
not fetched "only because of --tags" in such cases.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 7edb1ea..47b63a7 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -829,38 +829,17 @@ static int do_fetch(struct transport *transport,
>  		goto cleanup;
>  	}
>  	if (prune) {
> -		struct refspec *prune_refspecs;
> -		int prune_refspec_count;
> -
> +		/*
> +		 * We only prune based on refspecs specified
> +		 * explicitly (via command line or configuration); we
> +		 * don't care whether --tags was specified.
> +		 */
>  		if (ref_count) {
> -			prune_refspecs = refs;
> -			prune_refspec_count = ref_count;
> -		} else {
> -			prune_refspecs = transport->remote->fetch;
> -			prune_refspec_count = transport->remote->fetch_refspec_nr;
> -		}
> -
> -		if (tags == TAGS_SET) {
> -			/*
> -			 * --tags was specified.  Pretend that the user also
> -			 * gave us the canonical tags refspec
> -			 */
> -			const char *tags_str = "refs/tags/*:refs/tags/*";
> -			struct refspec *tags_refspec, *refspec;
> -
> -			/* Copy the refspec and add the tags to it */
> -			refspec = xcalloc(prune_refspec_count + 1, sizeof(*refspec));
> -			tags_refspec = parse_fetch_refspec(1, &tags_str);
> -			memcpy(refspec, prune_refspecs, prune_refspec_count * sizeof(*refspec));
> -			memcpy(&refspec[prune_refspec_count], tags_refspec, sizeof(*refspec));
> -
> -			prune_refs(refspec, prune_refspec_count + 1, ref_map);
> -
> -			/* The rest of the strings belong to fetch_one */
> -			free_refspec(1, tags_refspec);
> -			free(refspec);
> +			prune_refs(refs, ref_count, ref_map);
>  		} else {
> -			prune_refs(prune_refspecs, prune_refspec_count, ref_map);
> +			prune_refs(transport->remote->fetch,
> +				   transport->remote->fetch_refspec_nr,
> +				   ref_map);
>  		}
>  	}

Nice.

Re: [PATCH 15/15] fetch, remote: properly convey --no-prune options to subprocesses

[Junio C Hamano]

I finished reading thru the remainder and all looked good (again
except minor nits I sent reviews for separately).

Thanks, will queue.

Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Michael Haggerty]

On 10/24/2013 10:51 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Previously, fetch's "--tags" option was considered equivalent to
>> specifying the refspec "refs/tags/*:refs/tags/*" on the command line;
>> in particular, it caused the remote.<name>.refspec configuration to be
>> ignored.
>>
>> But it is not very useful to fetch tags without also fetching other
>> references, whereas it *is* quite useful to be able to fetch tags *in
>> addition to* other references.
> 
> True but when fetching other references, tags relevant to the
> history being fetched by default should automatically follow, so the
> above explains why "fetch --tags" is not a useful thing to do daily.

Maybe not necessary in many scenarios, but is it harmful for the common
case of cloning from and then periodically fetching from an upstream?
ISTM that the result of the declarative --tags option

    we have all upstream tags

is easier to reason about than the history-dependent tag-following behavior

    we have all upstream tags that point to commits that were
    reachable from an upstream branch at the time of this or one
    of the previous fetches

I just noticed that this is not explained very clearly in the fetch man
page.  I will try to improve it.

>> If a user wants to fetch *only* tags, then it is still possible to
>> specifying an explicit refspec:
>>
>>     git fetch <remote> 'refs/tags/*:refs/tags/*'
>>
>> Please note that the documentation prior to 1.8.0.3 was ambiguous
>> about this aspect of "fetch --tags" behavior.  Commit
>>
>>     f0cb2f137c 2012-12-14 fetch --tags: clarify documentation
>>
>> made the documentation match the old behavior.  This commit changes
>> the documentation to match the new behavior.
> 
> The "old behaviour as designed".  The documentation change was not
> about refusing to fix a bug but the above makes it sound as if it
> were a such commit.

I didn't mean to imply this.  My point in mentioning the documentation
change was that even though the old behavior has been in effect for a
while, it was not clearly documented until quite recently.  But I guess
it is a moot point.

>> The change to builtin/fetch.c:get_ref_map() has the side effect of
>> changing the order that the (struct ref) objects are listed in
>> ref_map.  It seems to me that this could probably only matter in the
>> case of duplicates.  But later ref_remove_duplicates() is called, so
>> duplicates are eliminated.  However, ref_remove_duplicates() always
>> retains the first instance of duplicates and discards the rest.  It is
>> conceivable that the boolean flags (which are not inspected by
>> ref_remove_duplicates()) could differ among the duplicates, and that
>> therefore changing the order of the items in the original list has the
>> effect of changing the flags on the items that are retained.
>>
>> I don't know this code well enough to judge whether this might be a
>> problem.
> 
>> If it is, then the correct approach is probably either to teach
>> ref_remove_duplicates() to ensure that the flags are also consistent
>> across duplicates, or to somehow combine the flags from all duplicates
>> into the one that is retained.  Please let me know if this is needed.
> 
> I do not think either of these two is sufficient if you want to fix
> it; ref_remove_duplicates() needs to be taught to retain the first
> one it encounters among the duplicates, not "ensure the flags are
> consistent" by erroring out when they are not among duplicates, nor
> "somehow combine" flags from later one to the first one.
> 
> But I suspect that, if this behaviour change were a problem, such a
> configuration was a problem before this change to most people; the
> order of duplicated [remote "foo"] section would not be under
> control of those who only use "git remote" without using an editor
> to tweak .git/config file anyway. So I do not think this regression
> is too big a deal; it is something you can fix later on top.

I'll address your second point first: I agree that the order among
explicit refspecs couldn't really be a problem because it is already
arbitrary.  However, get_ref_map() adds references from other sources to
the result list.  For example, if there are command-line arguments, it adds

1. command-line arguments (with fetch_head_status=FETCH_HEAD_MERGE)

2. configured refspecs (with fetch_head_status=FETCH_HEAD_IGNORE)

3. if (--tags)
   then tags_refspec (with fetch_head_status=FETCH_HEAD_NOT_FOR_MERGE)
   else the result of find_non_local_tags() (with
            fetch_head_status=FETCH_HEAD_NOT_FOR_MERGE)

It could very well be that the order matters across different classes of
reference.

Regarding your first point: if it matters which of the duplicates is
chosen, then it can only matter because they differ in some other way
than their reference names, for example in their flags.  So a robust way
of de-duping them (if it is possible) would be to compare the two
records and decide which one should take precedence based on this other
information rather than based on which one happened to come earlier in
the list.  Then the list order would be immaterial and (for example) it
wouldn't be a problem to reorder the items.

I'm being very wishy-washy because I haven't yet invested the time to
learn how these lists of refs are used.  Maybe I need to do so.

I have run out of time, so I will address your comments about the
documentation changes in a separate email.

Michael

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

Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Michael Haggerty]

On 10/24/2013 10:51 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
>> index ba1fe49..0e6d2ac 100644
>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -61,11 +61,9 @@ endif::git-pull[]
>>  ifndef::git-pull[]
>>  -t::
>>  --tags::
>> -	This is a short-hand for giving `refs/tags/*:refs/tags/*`
>> -	refspec from the command line, to ask all tags to be fetched
>> -	and stored locally.  Because this acts as an explicit
>> -	refspec, the default refspecs (configured with the
>> -	remote.$name.fetch variable) are overridden and not used.
>> +	This is a short-hand requesting that all tags be fetched from
>> +	the remote in addition to whatever else is being fetched.  It
>> +	is similar to using the refspec `refs/tags/*:refs/tags/*`.
> 
> This is no longer a short-hand, is it?  There is no other way to ask
> "fetch the usual stuff, and then refs/tags/*:refs/tags/* as well".
> 
> It should be sufficient for me to locally do:
> 
>     s/This is a short-hand requesting/Request/;
> 
> I think.

Yes, that's better.

>> diff --git a/git-pull.sh b/git-pull.sh
>> index b946fd9..dac7e1c 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -172,7 +172,7 @@ error_on_no_merge_candidates () {
>>  	do
>>  		case "$opt" in
>>  		-t|--t|--ta|--tag|--tags)
>> -			echo "Fetching tags only, you probably meant:"
>> +			echo "It doesn't make sense to pull tags; you probably meant:"
> 
> s/pull tags/pull all tags/; perhaps?

Yes, that's also an improvement.

Thanks,
Michael

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

Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs

[Michael Haggerty]

On 10/24/2013 11:11 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> ...
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> 
> Everything in the proposed log message made sense to me.
> 
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index d4d93c9..83c1700 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2086,7 +2086,7 @@ remote.<name>.vcs::
>>  remote.<name>.prune::
>>  	When set to true, fetching from this remote by default will also
>>  	remove any remote-tracking branches which no longer exist on the
>> -	remote (as if the `--prune` option was give on the command line).
>> +	remote (as if the `--prune` option was given on the command line).
> 
> Shouldn't we stop saying "branches" here?
> 
>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
>> index 0e6d2ac..5d12219 100644
>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -41,8 +41,14 @@ ifndef::git-pull[]
>>  
>>  -p::
>>  --prune::
>> -	After fetching, remove any remote-tracking branches which
>> -	no longer exist	on the remote.
>> +	After fetching, remove any remote-tracking branches that
> 
> Likewise.  This is a lot more important than the one in
> remote.<name>.prune documentation, as the next sentence "Tags are
> not subject to ..." implies that they fall into the same category as
> what gets pruned here, i.e. "remote-tracking branches" in the above
> sentence, but nobody calls refs/tags/v1.0.0 a "remote-tracking
> branch" even if it came from your 'origin'.

OK, I will change both of the above from "remote-tracking branches" to
"remote-tracking references".

>> +	no longer exist	on the remote.  Tags are not subject to
>> +	pruning in the usual case that they are fetched because of the
>> +	--tags option or remote.<name>.tagopt.  
> 
> We should mention the most usual case tags are fetched, before
> mentioning the case the unusual option "--tags" was used from the
> command line or .tagopt configuration was used.  Namely, when the
> tags are automatically followed.

OK, I will change this in the next draft.

>> @@ -63,7 +69,10 @@ ifndef::git-pull[]
>>  --tags::
>>  	This is a short-hand requesting that all tags be fetched from
>>  	the remote in addition to whatever else is being fetched.  It
>> -	is similar to using the refspec `refs/tags/*:refs/tags/*`.
>> +	is similar to using the refspec `refs/tags/*:refs/tags/*`,
>> +	except that it doesn't subject tags to pruning, regardless of
>> +	a --prune option or the configuration settings of fetch.prune
>> +	or remote.<name>.prune.
> 
> Using --tags is not similar to using refs/tags/*:refs/tags/* after
> the previous patch already; "git fetch origin --tags" and "git fetch
> origin refs/tags/*:refs/tags/*" are vastly different and that was
> the whole point of the previous step.  And that "calling something
> not so similar similar" needs to be fixed further here to clarify
> that they are not similar in yet another way.
> 
> We should just lose "It is similar to using" from 10/15 and start
> over, perhaps?  Add the first paragraph of the below in 10/15 and
> add the rest in 11/15, or something.
> 
> 	--tags::
> 		Request that all tags be fetched from the remote
> 		under the same name (i.e. `refs/tags/X` is created in
> 		our repository by copying their `refs/tags/X`), in
> 		addition to whatever is fetched by the same `git
> 		fetch` command without this option on the command
> 		line.
> 	+
>         When `refs/tags/*` hierarchy from the remote is copied only
>         because this option was given, they are not subject to be
> 	pruned when `--prune` option (or configuration variables
> 	like `fetch.prune` or `remote.<name>.prune`) is in effect.
> 
> That would make it clear that they are subject to pruning when --mirror
> or an explicit refspec refs/tags/*:refs/tags/* is given, as tags are
> not fetched "only because of --tags" in such cases.

I see your point.  What do you think about the following version, which
is a bit more compact and refers the reader to --prune for the full story:

-t::
--tags::
	Fetch all tags from the remote (i.e., fetch remote tags
	`refs/tags/*` into local tags with the same name), in addition
	to whatever else would otherwise be fetched.  Using this
	option does not subject tags to pruning, even if --prune is
	used (though tags may be pruned anyway if they are also the
	destination of an explicit refspec; see '--prune').

I also want to improve the description of tag auto-following in general.
 I will send a re-rolled patch series in the next couple of days.

Thanks for your prompt and helpful advice!

Michael

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

Re: [PATCH 11/15] fetch --prune: prune only based on explicit refspecs

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 10/24/2013 11:11 PM, Junio C Hamano wrote:
> ...
>> We should just lose "It is similar to using" from 10/15 and start
>> over, perhaps?  Add the first paragraph of the below in 10/15 and
>> add the rest in 11/15, or something.
>> 
>> 	--tags::
>> 		Request that all tags be fetched from the remote
>> 		under the same name (i.e. `refs/tags/X` is created in
>> 		our repository by copying their `refs/tags/X`), in
>> 		addition to whatever is fetched by the same `git
>> 		fetch` command without this option on the command
>> 		line.
>> 	+
>>         When `refs/tags/*` hierarchy from the remote is copied only
>>         because this option was given, they are not subject to be
>> 	pruned when `--prune` option (or configuration variables
>> 	like `fetch.prune` or `remote.<name>.prune`) is in effect.
>> 
>> That would make it clear that they are subject to pruning when --mirror
>> or an explicit refspec refs/tags/*:refs/tags/* is given, as tags are
>> not fetched "only because of --tags" in such cases.
>
> I see your point.  What do you think about the following version, which
> is a bit more compact and refers the reader to --prune for the full story:
>
> -t::
> --tags::
> 	Fetch all tags from the remote (i.e., fetch remote tags
> 	`refs/tags/*` into local tags with the same name), in addition
> 	to whatever else would otherwise be fetched.  Using this
> 	option does not subject tags to pruning, even if --prune is
> 	used (though tags may be pruned anyway if they are also the
> 	destination of an explicit refspec; see '--prune').

I like the first sentence of yours better. The second sentence feels
somewhat iffy, though. --tags refs/tags/*:refs/tags/* will allow
tags to be pruned, so s/option does not/option alone does not/ needs
to be done to be precise, at least.

Thanks.

Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> True but when fetching other references, tags relevant to the
>> history being fetched by default should automatically follow, so the
>> above explains why "fetch --tags" is not a useful thing to do daily.
>
> Maybe not necessary in many scenarios, but is it harmful for the common
> case of cloning from and then periodically fetching from an upstream?

There is no mention of "harmful"; I only said "not useful". And it
comes primarily from knowing why "--tags" was introduced in the
first place; I should have said "less useful than before, ever since
we started to reliably auto-follow".

The only "harmful" part is its interaction with "--prune", which
your series nicely addresses.

> ISTM that the result of the declarative --tags option
>
>     we have all upstream tags
>
> is easier to reason about than the history-dependent tag-following behavior

I'd say "we have all the relevant tags" and "we have all the tags
the other side has" are equally valid things to wish for, depending
who you are and how your project is organized, and one is not
necessarily easy/useful than the other.  In case it was unclear, I
was not seriously advocating to deprecate/remove "--tags".

> Regarding your first point: if it matters which of the duplicates is
> chosen, then it can only matter because they differ in some other way
> than their reference names, for example in their flags.  So a robust way
> of de-duping them (if it is possible) would be to compare the two
> records and decide which one should take precedence based on this other
> information rather than based on which one happened to come earlier in
> the list.  Then the list order would be immaterial and (for example) it
> wouldn't be a problem to reorder the items.

But that changes the behaviour to those who has cared to rely on the
ordering.  With the change to first collect and then sort unstably
before deduping, the series already tells them not to rely on the
order, and two of us tentatively agreed in this discussion that it
is not likely to matter.  If later this agreement turns out to be a
mistake and there are people who *do* rely on the ordering, the only
acceptable fix for them is by making sure we restore the "first one
trumps" semantics, not by defining an alternative, arguably better,
behaviour---that is not a regression fix.

In any case, thanks for working on this topic.

Re: [PATCH 10/15] fetch --tags: fetch tags *in addition to* other stuff

[Michael Haggerty]

On 10/28/2013 08:10 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> True but when fetching other references, tags relevant to the
>>> history being fetched by default should automatically follow, so the
>>> above explains why "fetch --tags" is not a useful thing to do daily.
>>
>> Maybe not necessary in many scenarios, but is it harmful for the common
>> case of cloning from and then periodically fetching from an upstream?
> 
> There is no mention of "harmful"; I only said "not useful". And it
> comes primarily from knowing why "--tags" was introduced in the
> first place; I should have said "less useful than before, ever since
> we started to reliably auto-follow".
> 
> The only "harmful" part is its interaction with "--prune", which
> your series nicely addresses.

OK, then we are in agreement.

>> ISTM that the result of the declarative --tags option
>>
>>     we have all upstream tags
>>
>> is easier to reason about than the history-dependent tag-following behavior
> 
> I'd say "we have all the relevant tags" and "we have all the tags
> the other side has" are equally valid things to wish for, depending
> who you are and how your project is organized, and one is not
> necessarily easy/useful than the other.  In case it was unclear, I
> was not seriously advocating to deprecate/remove "--tags".

Yes, I agree that both are valid things to wish for.  I guess I was
mostly thinking about which would be a better default.

"clone" and "remote add", by default, configure the repo to fetch all
branches on the remote.  For this default setup, what are the pros and
cons of the two tag-fetching options (assuming that this patch series
has fixed the problem with tag pruning)?

Pros of auto-following:

* Doesn't require a change to the status quo

* The the user can change the refspec to be more restrictive without
having to change the tag-following option and it continues to do the
right thing.

* If the project has branches outside of refs/heads (which would not, by
default, be fetched--think continuous integration artifacts) and also
has tags pointing at those branches, the user might get unwanted
contents with "--tags", but not with auto-following.


Pros of --tags:

* Easier to understand (?) because result is not history-dependent.

* Avoids missing tags that are not directly on a branch:

      o---o---o---o    <- branch
           \
            o      <- tag

So it's not obvious that one is better than the other.  I think if I
were choosing the behavior for the first time, I would favor --tags.
But I don't think the advantage is strong enough to make it worth
changing the existing default.

>> Regarding your first point: if it matters which of the duplicates is
>> chosen, then it can only matter because they differ in some other way
>> than their reference names, for example in their flags.  So a robust way
>> of de-duping them (if it is possible) would be to compare the two
>> records and decide which one should take precedence based on this other
>> information rather than based on which one happened to come earlier in
>> the list.  Then the list order would be immaterial and (for example) it
>> wouldn't be a problem to reorder the items.
> 
> But that changes the behaviour to those who has cared to rely on the
> ordering.  With the change to first collect and then sort unstably
> before deduping, the series already tells them not to rely on the
> order, and two of us tentatively agreed in this discussion that it
> is not likely to matter.  If later this agreement turns out to be a
> mistake and there are people who *do* rely on the ordering, the only
> acceptable fix for them is by making sure we restore the "first one
> trumps" semantics, not by defining an alternative, arguably better,
> behaviour---that is not a regression fix.

Please note that the current patch series does *not* change the
algorithm to use an unstable sort; that was only a suggestion for the
future should somebody discover that the O(N^2) algorithm in this
function is a performance bottleneck.

What the old patch series *did* do was change the ordering that
get_ref_map() adds references to the list in the case of (refspec_count
&& tags == TAGS_SET) by moving the (tags == TAGS_SET) handling outside
of the "true" branch of the (refspec_count) conditional.  This had the
result that the references added by

       get_fetch_map(remote_refs, tag_refspec, &tail, 0);

came after, rather than before, the references opportunistically being
fetched with FETCH_HEAD_IGNORE status.

But I dug even deeper and found that there was a (rather obscure)
situation where the ordering change could lead to incorrect behavior,
namely if all of the following are true:

* there is a configured refspec for tags, like refs/tags/*:refs/tags/*

* the user runs fetch with the --tags option and also with an explicit
refspec on the command line to fetch a remote tag (e.g.,
refs/tags/foo:refs/heads/foo).

In that case (after this version of this patch), an entry for
refs/tags/foo:refs/heads/foo would be added to the list, then the
opportunistic "refs/tags/foo:refs/tags/foo" would be added with
FETCH_HEAD_IGNORE.  Then the --tags option would be processed, adding a
second record "refs/tags/foo:refs/tags/foo" to the list, but this time
with FETCH_HEAD_NOT_FOR_MERGE.  The call to ref_remove_duplicates()
would remove the last entry, leaving the entry with FETCH_HEAD_IGNORE
instead of the (correct) entry with FETCH_HEAD_NOT_FOR_MERGE.  The
result would be that refs/tags/foo would not be written to FETCH_HEAD.

So I will re-roll this series with a few extra patches that first, avoid
subjecting the --tags entries to the opportunistic-update machinery (it
is redundant), and also preserve the old order where the --tags entries
precede the opportunistic entries in the list.

Thanks again for your comments!
Michael

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