git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [BUG?] git fetch -p -t prunes all non-tag refs
@ 2011-09-26 18:47 Ben Boeckel
  2011-09-26 22:30 ` Junio C Hamano
  0 siblings, 1 reply; 14+ messages in thread
From: Ben Boeckel @ 2011-09-26 18:47 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-26 18:47 [BUG?] git fetch -p -t prunes all non-tag refs Ben Boeckel
@ 2011-09-26 22:30 ` Junio C Hamano
  2011-09-26 22:51   ` Ben Boeckel
  2011-09-26 23:11   ` Carlos Martín Nieto
  0 siblings, 2 replies; 14+ messages in thread
From: Junio C Hamano @ 2011-09-26 22:30 UTC (permalink / raw)
  To: mathstuf; +Cc: git

Ben Boeckel <mathstuf@gmail.com> writes:

> 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.

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".

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-26 22:30 ` Junio C Hamano
@ 2011-09-26 22:51   ` Ben Boeckel
  2011-09-26 23:11   ` Carlos Martín Nieto
  1 sibling, 0 replies; 14+ messages in thread
From: Ben Boeckel @ 2011-09-26 22:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Mon, Sep 26, 2011 at 15:30:33 -0700, Junio C Hamano wrote:
> 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".

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

--Ben

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-26 22:30 ` Junio C Hamano
  2011-09-26 22:51   ` Ben Boeckel
@ 2011-09-26 23:11   ` Carlos Martín Nieto
  2011-09-26 23:16     ` Junio C Hamano
  1 sibling, 1 reply; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-09-26 23:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: mathstuf, git

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

On Mon, 2011-09-26 at 15:30 -0700, Junio C Hamano wrote:
> Ben Boeckel <mathstuf@gmail.com> writes:
> 
> > 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.
> 
> 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".

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(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index e422ced..158b20a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -967,6 +967,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			if (!add_remote_or_group(argv[i], &list))
 				die(_("No such remote or remote group: %s"), argv[i]);
 		result = fetch_multiple(&list);
+	} else if (prune && (argc == 2 || tags != TAGS_DEFAULT)) {
+		/* The if (multiple) is above so we don't report multiple remotes as an error */
+		die(_("Pruning and limiting refs is not compatible"));
 	} else {
 		/* Single remote or group */
 		(void) add_remote_or_group(argv[0], &list);
-- 
1.7.5.2.354.g349bf




[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-26 23:11   ` Carlos Martín Nieto
@ 2011-09-26 23:16     ` Junio C Hamano
  2011-09-26 23:28       ` Carlos Martín Nieto
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2011-09-26 23:16 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: mathstuf, git

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

> On Mon, 2011-09-26 at 15:30 -0700, Junio C Hamano wrote:
>> Ben Boeckel <mathstuf@gmail.com> writes:
>> 
>> > 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.
>> 
>> 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".
>
> This should fix it (in a way). Let's agree that it's a bad idea and
> complain to the user.

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-26 23:16     ` Junio C Hamano
@ 2011-09-26 23:28       ` Carlos Martín Nieto
  2011-09-27  3:31         ` Jeff King
  0 siblings, 1 reply; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-09-26 23:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: mathstuf, git

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]

On Mon, 2011-09-26 at 16:16 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
> 
> > On Mon, 2011-09-26 at 15:30 -0700, Junio C Hamano wrote:
> >> Ben Boeckel <mathstuf@gmail.com> writes:
> >> 
> >> > 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.
> >> 
> >> 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".
> >
> > This should fix it (in a way). Let's agree that it's a bad idea and
> > complain to the user.
> 
> That might be a reasonable short-term safety measure, but in the longer

Sure.

> 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.

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-26 23:28       ` Carlos Martín Nieto
@ 2011-09-27  3:31         ` Jeff King
  2011-10-04 10:33           ` Carlos Martín Nieto
  0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2011-09-27  3:31 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: Junio C Hamano, mathstuf, git

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

> > 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.
> 
> 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?

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-09-27  3:31         ` Jeff King
@ 2011-10-04 10:33           ` Carlos Martín Nieto
  2011-10-04 10:36             ` Jeff King
  0 siblings, 1 reply; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-10-04 10:33 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, mathstuf, git

[-- Attachment #1: Type: text/plain, Size: 2735 bytes --]

On Mon, 2011-09-26 at 23:31 -0400, Jeff King wrote:
> On Tue, Sep 27, 2011 at 01:28:09AM +0200, Carlos Martín Nieto wrote:
> 
> > > 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.
> > 
> > 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?
> 
> 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/*

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.

> 
> 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`/*

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

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



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-10-04 10:33           ` Carlos Martín Nieto
@ 2011-10-04 10:36             ` Jeff King
  2011-10-04 11:06               ` Carlos Martín Nieto
  2011-10-06 16:56               ` [WIP PATCH 0/2] Be more careful when prunning Carlos Martín Nieto
  0 siblings, 2 replies; 14+ messages in thread
From: Jeff King @ 2011-10-04 10:36 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: Junio C Hamano, mathstuf, git

On Tue, Oct 04, 2011 at 12:33:22PM +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/*
> 
> 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.

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.

> >       # 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`/*
> 
> Is --mirror needed there? I would have thought that
> refs/*:refs/`hostname`/* would do the same by itself.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [BUG?] git fetch -p -t prunes all non-tag refs
  2011-10-04 10:36             ` Jeff King
@ 2011-10-04 11:06               ` Carlos Martín Nieto
  2011-10-06 16:56               ` [WIP PATCH 0/2] Be more careful when prunning Carlos Martín Nieto
  1 sibling, 0 replies; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-10-04 11:06 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, mathstuf, git

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Tue, 2011-10-04 at 06:36 -0400, Jeff King wrote:
> On Tue, Oct 04, 2011 at 12:33:22PM +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/*
> > 
> > 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.
> 
> 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'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).

> 
> > >       # 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`/*
> > 
> > Is --mirror needed there? I would have thought that
> > refs/*:refs/`hostname`/* would do the same by itself.
> 
> 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.

OK, gotcha.

   cmn


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [WIP PATCH 0/2] Be more careful when prunning
  2011-10-04 10:36             ` Jeff King
  2011-10-04 11:06               ` Carlos Martín Nieto
@ 2011-10-06 16:56               ` Carlos Martín Nieto
  2011-10-06 16:56                 ` [PATCH 1/2] fetch: free all the additional refspecs Carlos Martín Nieto
                                   ` (2 more replies)
  1 sibling, 3 replies; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 16:56 UTC (permalink / raw)
  To: git

[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(-)

-- 
1.7.5.2.354.g349bf

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] fetch: free all the additional refspecs
  2011-10-06 16:56               ` [WIP PATCH 0/2] Be more careful when prunning Carlos Martín Nieto
@ 2011-10-06 16:56                 ` Carlos Martín Nieto
  2011-10-06 16:56                 ` [PATCH 2/2] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
  2011-10-07 23:00                 ` [WIP PATCH 0/2] Be more careful when prunning Junio C Hamano
  2 siblings, 0 replies; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 16:56 UTC (permalink / raw)
  To: git

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7a4e41c..30b485e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -883,7 +883,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
 	atexit(unlock_pack);
 	refspec = parse_fetch_refspec(ref_nr, refs);
 	exit_code = do_fetch(transport, refspec, ref_nr);
-	free(refspec);
+	free_refspec(ref_nr, refspec);
 	transport_disconnect(transport);
 	transport = NULL;
 	return exit_code;
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] fetch: honor the user-provided refspecs when pruning refs
  2011-10-06 16:56               ` [WIP PATCH 0/2] Be more careful when prunning Carlos Martín Nieto
  2011-10-06 16:56                 ` [PATCH 1/2] fetch: free all the additional refspecs Carlos Martín Nieto
@ 2011-10-06 16:56                 ` Carlos Martín Nieto
  2011-10-07 23:00                 ` [WIP PATCH 0/2] Be more careful when prunning Junio C Hamano
  2 siblings, 0 replies; 14+ messages in thread
From: Carlos Martín Nieto @ 2011-10-06 16:56 UTC (permalink / raw)
  To: git

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(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 30b485e..b937d71 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -505,10 +505,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)
 	return ret;
 }
 
-static int prune_refs(struct transport *transport, struct ref *ref_map)
+static int prune_refs(struct transport *transport, struct refspec *refs, int n, struct ref *ref_map)
 {
 	int result = 0;
-	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map);
+	struct ref *ref, *stale_refs = get_stale_heads(transport->remote, ref_map, refs, n);
 	const char *dangling_msg = dry_run
 		? _("   (%s will become dangling)\n")
 		: _("   (%s has become dangling)\n");
@@ -700,7 +700,7 @@ static int do_fetch(struct transport *transport,
 		return 1;
 	}
 	if (prune)
-		prune_refs(transport, ref_map);
+		prune_refs(transport, refs, ref_count, ref_map);
 	free_refs(ref_map);
 
 	/* if neither --no-tags nor --tags was specified, do automated tag
diff --git a/builtin/remote.c b/builtin/remote.c
index b25dfb4..91a2148 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -349,7 +349,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 		else
 			string_list_append(&states->tracked, abbrev_branch(ref->name));
 	}
-	stale_refs = get_stale_heads(states->remote, fetch_map);
+	stale_refs = get_stale_heads(states->remote, fetch_map, NULL, 0);
 	for (ref = stale_refs; ref; ref = ref->next) {
 		struct string_list_item *item =
 			string_list_append(&states->stale, abbrev_branch(ref->name));
diff --git a/remote.c b/remote.c
index 7840d2f..72a26d3 100644
--- a/remote.c
+++ b/remote.c
@@ -1684,26 +1684,84 @@ struct stale_heads_info {
 	struct remote *remote;
 	struct string_list *ref_names;
 	struct ref **stale_refs_tail;
+	struct refspec *refs;
+	int ref_count;
 };
 
+/*
+ * Find a refspec to a remote's
+ * Returns 0 on success, -1 if it couldn't find a the refspec
+ */
+static int find_in_refs(struct refspec *refs, int ref_count, struct refspec *query)
+{
+	int i;
+	struct refspec *refspec;
+
+	for (i = 0; i < ref_count; ++i) {
+		refspec = &refs[i];
+
+		/*
+		 * No '*' means that it must match exactly. If it does
+		 * have it, try to match it against the pattern. If
+		 * the refspec matches, store the ref name as it would
+		 * appear in the server in query->src.
+		 */
+		if (!strchr(refspec->dst, '*')) {
+			if (!strcmp(query->dst, refspec->dst)) {
+				query->src = xstrdup(refspec->src);
+				return 0;
+			}
+		} else {
+			if (match_name_with_pattern(refspec->dst, query->dst,
+						    refspec->src, &query->src)) {
+				return 0;
+			}
+		}
+	}
+
+	return -1;
+}
+
 static int get_stale_heads_cb(const char *refname,
 	const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct stale_heads_info *info = cb_data;
 	struct refspec refspec;
+	int ret;
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
-	if (!remote_find_tracking(info->remote, &refspec)) {
-		if (!((flags & REF_ISSYMREF) ||
-		    string_list_has_string(info->ref_names, refspec.src))) {
-			struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
-			hashcpy(ref->new_sha1, sha1);
-		}
+
+	/*
+	 * If the user speicified refspecs on the command line, we
+	 * should only use those to check. Otherwise, look in the
+	 * remote's configuration for the branch.
+	 */
+	if (info->ref_count)
+		ret = find_in_refs(info->refs, info->ref_count, &refspec);
+	else
+		ret = remote_find_tracking(info->remote, &refspec);
+
+	/* No matches */
+	if (ret)
+		return 0;
+
+	/*
+	 * If we did find a suitable refspec and it's not a symref and
+	 * it's not in the list of refs that currently exist in that
+	 * remote we consider it to be stale.
+	 */
+	if (!((flags & REF_ISSYMREF) ||
+	      string_list_has_string(info->ref_names, refspec.src))) {
+		struct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);
+		hashcpy(ref->new_sha1, sha1);
 	}
+
+	free(refspec.src);
 	return 0;
 }
 
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+			    struct refspec *refs, int ref_count)
 {
 	struct ref *ref, *stale_refs = NULL;
 	struct string_list ref_names = STRING_LIST_INIT_NODUP;
@@ -1711,6 +1769,8 @@ struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
 	info.remote = remote;
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
+	info.refs = refs;
+	info.ref_count = ref_count;
 	for (ref = fetch_map; ref; ref = ref->next)
 		string_list_append(&ref_names, ref->name);
 	sort_string_list(&ref_names);
diff --git a/remote.h b/remote.h
index 9a30a9d..2f753a0 100644
--- a/remote.h
+++ b/remote.h
@@ -164,6 +164,7 @@ struct ref *guess_remote_head(const struct ref *head,
 			      int all);
 
 /* Return refs which no longer exist on remote */
-struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map);
+struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map,
+			    struct refspec *refs, int ref_count);
 
 #endif
-- 
1.7.5.2.354.g349bf

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [WIP PATCH 0/2] Be more careful when prunning
  2011-10-06 16:56               ` [WIP PATCH 0/2] Be more careful when prunning Carlos Martín Nieto
  2011-10-06 16:56                 ` [PATCH 1/2] fetch: free all the additional refspecs Carlos Martín Nieto
  2011-10-06 16:56                 ` [PATCH 2/2] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
@ 2011-10-07 23:00                 ` Junio C Hamano
  2 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2011-10-07 23:00 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: git

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

> 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?

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2011-10-07 23:00 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-26 18:47 [BUG?] git fetch -p -t prunes all non-tag refs Ben Boeckel
2011-09-26 22:30 ` Junio C Hamano
2011-09-26 22:51   ` Ben Boeckel
2011-09-26 23:11   ` Carlos Martín Nieto
2011-09-26 23:16     ` Junio C Hamano
2011-09-26 23:28       ` Carlos Martín Nieto
2011-09-27  3:31         ` Jeff King
2011-10-04 10:33           ` Carlos Martín Nieto
2011-10-04 10:36             ` Jeff King
2011-10-04 11:06               ` Carlos Martín Nieto
2011-10-06 16:56               ` [WIP PATCH 0/2] Be more careful when prunning Carlos Martín Nieto
2011-10-06 16:56                 ` [PATCH 1/2] fetch: free all the additional refspecs Carlos Martín Nieto
2011-10-06 16:56                 ` [PATCH 2/2] fetch: honor the user-provided refspecs when pruning refs Carlos Martín Nieto
2011-10-07 23:00                 ` [WIP PATCH 0/2] Be more careful when prunning Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).