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

Carlos Martín Nieto [off-list ref] writes:

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

quoted

Ben Boeckel [off-list ref] writes:

quoted

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.

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

Carlos Martín Nieto [off-list ref] writes:

quoted

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

quoted

Ben Boeckel [off-list ref] writes:

quoted

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

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

quoted

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.

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:

quoted

quoted

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

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

quoted

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.

quoted

      # 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

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:

quoted

quoted

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

quoted

quoted

      # 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

[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

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

Signed-off-by: Carlos Martín Nieto <redacted>
---
 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