[BUG?] git fetch -p -t prunes all non-tag refs

Hi,

When the --prune and --tags options are given to git fetch together, all non-tag refs are pruned because only tags are looked at and when pruning it appears as if the branches have disappeared and are therefore deleted locally. Maybe this is an unintentional wanted behavior, but it should at least be documented that the combination causes this to happen.

    git-1.7.6.2-1.fc15.x86_64

Appears to still be there in master (85e9c7e) from looking at the logs.

--Ben

Ben Boeckel <mathstuf@gmail.com> writes:

I would call that a bug, and it is not limited to the use of "--tags". For example, I suspect that

    $ git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would prune remote tracking branches for "origin" other than "master".

On Mon, Sep 26, 2011 at 15:30:33 -0700, Junio C Hamano wrote:

Indeed it does. Given that, should --prune be an error when a refspec is given or when in combination with --tags?

--Ben

On Mon, 2011-09-26 at 15:30 -0700, Junio C Hamano wrote:

This should fix it (in a way). Let's agree that it's a bad idea and complain to the user. Looking at the surrounding code, I can't find anything obvious that would break, and `git fetch --prune --multiple a b` is allowed.

Patch on top of maint.

--- 8< ---
Subject: [PATCH] fetch: disallow --prune in combination with tags or refspecs

Pruning shouldn't be used when other options limit the references that should be taken into account. For example

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

Both these commands would remove references which do still exist in the remote.

Print an error and exit if prune is selected at the same time as either tags are selected or a refspec is given.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/fetch.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Carlos Martín Nieto <cmn@elego.de> writes:

That might be a reasonable short-term safety measure, but in the longer term I think we should fix it properly. We are already learning "what are the refs the remote side currently has" from the transport and the right fix ought to be to use that original information, not the version filtered for the use of the primary objective of fetch, which is to only fetch what the user asked for.

On Mon, 2011-09-26 at 16:16 -0700, Junio C Hamano wrote:

Sure.

Do you mean that we should ignore the refspec? Or do you mean that we should look at the refspec if it exists, and only consider deleting those that meet the refspec, so that `--prune --tags` would only delete tags that don't exist in the remote?

Either way, it's a bit more complicated than a two-liner and it's too late in my timezone for that. I'll try to see if I can do it in the next few days.

   cmn

On Tue, Sep 27, 2011 at 01:28:09AM +0200, Carlos Martín Nieto wrote:

The latter. If I say:

  git fetch --prune origin refs/heads/master:refs/remotes/origin/master

and refs/heads/master doesn't exist on the remote, I would expect refs/remotes/origin/master to be deleted locally. And that naturally extends to:

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

We do something similar with "git push --mirror", which does pruning like this[1].

-Peff

[1] Actually, I'm not sure how correct "push --mirror" is. It would be
    nice if the prune operation could be split from the mirror, too. In
    the past, I have wanted to do both:

      # backup to a repository where our objects will be shared
      # with other related backups. So we must only use our slice of the
      # ref namespace.
      git push --mirror backup-repo +refs/*:refs/`hostname`/*

    and:

      # update topic branches we have already published (using the
      # "matching" refspec), but remove any that we have deleted
      # locally.
      git push --mirror publish-point +:

    and I don't think either works.

On Mon, 2011-09-26 at 23:31 -0400, Jeff King wrote:

I have some code locally that solves this second part. If we are given refspecs on the command-line, it will try to match against that instead of blindly trusting what get_stale_heads tells us. I'm looking into putting the logic into get_stale_heads so that we can trust it.

The first part might be more complicated. If the remote head doesn't exist, get_fetch_map dies. It does take a missing_ok flag, so it might be as easy as passing 1 there; but I'm not sure what that would do for a non-prune fetch.

Is --mirror needed there? I would have thought that refs/*:refs/`hostname`/* would do the same by itself.

On Tue, Oct 04, 2011 at 12:33:22PM +0200, Carlos Martín Nieto wrote:

Let's not worry about the first one for now, then. It is a natural extension of the other, but in practice, I don't expect people to really care that much about auto-pruning one specific ref. Instead, they want to prune wildcards. So as long as it works in the wildcard case, that is a good start.

I wanted it to auto-prune the remote branches. So if I delete "refs/heads/foo" locally, then it will be deleted from the backup on the next push. Regular "push" will not do that, but --mirror will.

-Peff

On Tue, 2011-10-04 at 06:36 -0400, Jeff King wrote:

I'm going to add the logic to do specific-ref-prunning because it's just adding a strcmp to an if (I may yet be proven wrong, mind) and if that works, we can use it later. get_stale_heads is going to gain a couple of arguments so it can inspect the user-given refspecs. It should now be a simple matter of adding a check to see if the refspec's dst matches the refname (it's a simple matter once you've spent five hours trying to understand what the fetch code does).

OK, gotcha.

   cmn

[I turns out that my system was slightly misconfigured, so vger didn't want to accept my mail, this a re-send to the list only]

Hello,

I consider this WIP because I haven't addressed the issue when --tags is given. It's likely the most common issue (and what sparked this), but the second patch sets it up so it becomes easier.

The first patch is not that big a deal, but it's better if we're freeing the refspecs, we might as well free all of them.

The second patch teaches get_stale_heads to use the user-provided refspecs instead of the ones in the config. For example, running

    git fetch --prune origin refs/heads/master:refs/heads/master

doesn't remove the other branches anymore. For a more interesting (and believable) example, let's take

    git fetch --prune origin refs/heads/b/*:refs/heads/b/*

because you want to prune the refs inside the b/ namespace only. Currently git will delete all the refs that aren't under that namespace. With the second patch applied, git won't remove any refs outside the b/ namespace.

Now comes the interesting part: when --tags is given, there is no refspec set up, fetch just sets up a global variable. What I'm thinking (and going to implement after dinner, unless people cry out against it) is this: just before calling prune_refs, add a refspec to the user-provided list with the refspec refs/tags/*:refs/tags/* similar to what fetch_one does if you gave it "tag v1.5.6". This would cause us to ignore the configured refspec and keep the branches. The lack of '+' is most certainly on purpose. Is there anything fundamentally wrong with that idea?

Cheers,
   cmn

Carlos Martín Nieto (2):
  fetch: free all the additional refspecs
  fetch: honor the user-provided refspecs when pruning refs

 builtin/fetch.c  |    8 +++---
 builtin/remote.c |    2 +-
 remote.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 74 insertions(+), 13 deletions(-)

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/fetch.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

If the user gave us refspecs on the command line, we should use those when deciding whether to prune a ref instead of relying on the refspecs in the config.

Previously, running

    git fetch --prune origin refs/heads/master:refs/remotes/origin/master

would delete every other tag under the origin namespace because we were using the refspec to filter the available refs but using the configured refspec to figure out if a ref had been deleted on the remote. This is clearly the wrong thing to do.

Teach get_stale_heads about user-provided refspecs and use them if they're available.

Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
 builtin/fetch.c  |    6 ++--
 builtin/remote.c |    2 +-
 remote.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 remote.h         |    3 +-
 4 files changed, 73 insertions(+), 12 deletions(-)

Carlos Martín Nieto <cmn@elego.de> writes:

It sounds like that the approach should work and preserve the current "fetch --tags" semantics, but with one small caveat (which is not a downside).

As was discussed in a few separate threads last month, in the longer term I think we should fix the semantics of "fetch --tags" to mean "in addition to what you usually fetch with the configured refspecs, add that all matching refs/tags/*:refs/tags/* to the refspec" to reduce confusion. "Only fetch tags" may make sense if you have everything else, but by itself it is somewhat a senseless thing to do.

The semantics has been kept this way only from fear of breaking backward compatibility, but because nobody wants to only fetch tags without branches, this forces people to say "git fetch && git fetch --tags".

We should re-evaluate the design and change it at a major version boundary, I would think. And when that happens, we may need to rip out the special case you discussed above.

Thanks.