[RFC/PATCH 0/3] push: add 'prune' option

Hi,

As mentioned in a previous thread[1], git is lacking some functionality to
synchronize completely with remote repositories.

As an example I put my use-case; I want to backup *all* my local branches to a
personal repository, and I want to remove branches that I have removed from my
local repository. git push personal 'refs/heads/*' mostly does the job, but it
doesn't remove anything, and that's where 'prune' comes from.

Do not confuse the remote branches with the upstream ones; you could have two
repositories where you want to synchronize branches to:
% git push --prune --force backup1 'refs/heads/*'
% git push --prune --force backup2 'refs/heads/*'

I still think a 'git remote sync' would be tremendously useful, but it can
actually use this --prune option.

I know, documentation and testing are missing, but perhaps there will be
comments on the code itself.

Cheers.

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

Felipe Contreras (3):
  remote: use a local variable in match_push_refs()
  remote: reorganize check_pattern_match()
  push: add 'prune' option

 builtin/push.c |    2 +
 remote.c       |   91 +++++++++++++++++++++++++++++++++++--------------------
 remote.h       |    3 +-
 transport.c    |    2 +
 transport.h    |    1 +
 5 files changed, 65 insertions(+), 34 deletions(-)

-- 
1.7.9.1

So that we can reuse src later on.

Will be useful in next patches.

Signed-off-by: Felipe Contreras <redacted>
---
 remote.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/remote.c b/remote.c
index 73a3809..55d68d1 100644
--- a/remote.c
+++ b/remote.c

@@ -1157,7 +1157,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	int send_mirror = flags & MATCH_REFS_MIRROR;
 	int errs;
 	static const char *default_refspec[] = { ":", NULL };
-	struct ref **dst_tail = tail_ref(dst);
+	struct ref *ref, **dst_tail = tail_ref(dst);
 
 	if (!nr_refspec) {
 		nr_refspec = 1;

@@ -1167,14 +1167,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	errs = match_explicit_refs(src, *dst, &dst_tail, rs, nr_refspec);
 
 	/* pick the remainder */
-	for ( ; src; src = src->next) {
+	for (ref = src; ref; ref = ref->next) {
 		struct ref *dst_peer;
 		const struct refspec *pat = NULL;
 		char *dst_name;
-		if (src->peer_ref)
+		if (ref->peer_ref)
 			continue;
 
-		pat = check_pattern_match(rs, nr_refspec, src);
+		pat = check_pattern_match(rs, nr_refspec, ref);
 		if (!pat)
 			continue;
 

@@ -1184,13 +1184,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
 			 * including refs outside refs/heads/ hierarchy, but
 			 * that does not make much sense these days.
 			 */
-			if (!send_mirror && prefixcmp(src->name, "refs/heads/"))
+			if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
 				continue;
-			dst_name = xstrdup(src->name);
+			dst_name = xstrdup(ref->name);
+
 
 		} else {
 			const char *dst_side = pat->dst ? pat->dst : pat->src;
-			if (!match_name_with_pattern(pat->src, src->name,
+			if (!match_name_with_pattern(pat->src, ref->name,
 						     dst_side, &dst_name))
 				die("Didn't think it matches any more");
 		}

@@ -1211,9 +1212,9 @@ int match_push_refs(struct ref *src, struct ref **dst,
 
 			/* Create a new one and link it */
 			dst_peer = make_linked_ref(dst_name, &dst_tail);
-			hashcpy(dst_peer->new_sha1, src->new_sha1);
+			hashcpy(dst_peer->new_sha1, ref->new_sha1);
 		}
-		dst_peer->peer_ref = copy_ref(src);
+		dst_peer->peer_ref = copy_ref(ref);
 		dst_peer->force = pat->force;
 	free_name:
 		free(dst_name);

-- 
1.7.9.1

There's a lot of code that can be consolidated there, and will be useful
for next patches.

Signed-off-by: Felipe Contreras <redacted>
---
 remote.c |   59 ++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/remote.c b/remote.c
index 55d68d1..019aafc 100644
--- a/remote.c
+++ b/remote.c

@@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 	return errs;
 }
 
-static const struct refspec *check_pattern_match(const struct refspec *rs,
-						 int rs_nr,
-						 const struct ref *src)
+static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
+		int send_mirror, const struct refspec **ret_pat)
 {
+	const struct refspec *pat;
+	char *name;
 	int i;
 	int matching_refs = -1;
 	for (i = 0; i < rs_nr; i++) {

@@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,
 			continue;
 		}
 
-		if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
-							     NULL, NULL))
-			return rs + i;
+		if (rs[i].pattern) {
+			const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
+			if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+				matching_refs = i;
+				break;
+			}
+		}
 	}
-	if (matching_refs != -1)
-		return rs + matching_refs;
-	else
+	if (matching_refs == -1)
 		return NULL;
+
+	pat = rs + matching_refs;
+	if (pat->matching) {
+		/*
+		 * "matching refs"; traditionally we pushed everything
+		 * including refs outside refs/heads/ hierarchy, but
+		 * that does not make much sense these days.
+		 */
+		if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
+			return NULL;
+		name = xstrdup(ref->name);
+	}
+	if (ret_pat)
+		*ret_pat = pat;
+	return name;
 }
 
 static struct ref **tail_ref(struct ref **head)

@@ -1171,36 +1189,19 @@ int match_push_refs(struct ref *src, struct ref **dst,
 		struct ref *dst_peer;
 		const struct refspec *pat = NULL;
 		char *dst_name;
+
 		if (ref->peer_ref)
 			continue;
 
-		pat = check_pattern_match(rs, nr_refspec, ref);
-		if (!pat)
+		dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
+		if (!dst_name)
 			continue;
 
-		if (pat->matching) {
-			/*
-			 * "matching refs"; traditionally we pushed everything
-			 * including refs outside refs/heads/ hierarchy, but
-			 * that does not make much sense these days.
-			 */
-			if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
-				continue;
-			dst_name = xstrdup(ref->name);
-
-
-		} else {
-			const char *dst_side = pat->dst ? pat->dst : pat->src;
-			if (!match_name_with_pattern(pat->src, ref->name,
-						     dst_side, &dst_name))
-				die("Didn't think it matches any more");
-		}
 		dst_peer = find_ref_by_name(*dst, dst_name);
 		if (dst_peer) {
 			if (dst_peer->peer_ref)
 				/* We're already sending something to this ref. */
 				goto free_name;
-
 		} else {
 			if (pat->matching && !(send_all || send_mirror))
 				/*

-- 
1.7.9.1

This will allow us to remove refs from the remote that have been removed
locally.

It's useful to conveniently synchronize all the local branches to
certain remote.

Signed-off-by: Felipe Contreras <redacted>
---
 builtin/push.c |    2 ++
 remote.c       |   29 ++++++++++++++++++++++++++---
 remote.h       |    3 ++-
 transport.c    |    2 ++
 transport.h    |    1 +
 5 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..46b99b1 100644
--- a/builtin/push.c
+++ b/builtin/push.c

@@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
 			TRANSPORT_PUSH_SET_UPSTREAM),
 		OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+		OPT_BIT('p', "prune", &flags, "prune locally removed refs",
+			TRANSPORT_PUSH_PRUNE),
 		OPT_END()
 	};
 

diff --git a/remote.c b/remote.c
index 019aafc..0900bb5 100644
--- a/remote.c
+++ b/remote.c

@@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 }
 
 static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
-		int send_mirror, const struct refspec **ret_pat)
+		int send_mirror, int dir, const struct refspec **ret_pat)
 {
 	const struct refspec *pat;
 	char *name;

@@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref
 
 		if (rs[i].pattern) {
 			const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
-			if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+			int match;
+			if (dir == 0)
+				match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);
+			else
+				match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);
+			if (match) {
 				matching_refs = i;
 				break;
 			}

@@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	struct refspec *rs;
 	int send_all = flags & MATCH_REFS_ALL;
 	int send_mirror = flags & MATCH_REFS_MIRROR;
+	int send_prune = flags & MATCH_REFS_PRUNE;
 	int errs;
 	static const char *default_refspec[] = { ":", NULL };
 	struct ref *ref, **dst_tail = tail_ref(dst);

@@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
 		if (ref->peer_ref)
 			continue;
 
-		dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
+		dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);
 		if (!dst_name)
 			continue;
 

@@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	free_name:
 		free(dst_name);
 	}
+	if (send_prune) {
+		/* check for missing refs on the remote */
+		for (ref = *dst; ref; ref = ref->next) {
+			char *src_name;
+
+			if (ref->peer_ref)
+				/* We're already sending something to this ref. */
+				continue;
+
+			src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);
+			if (src_name) {
+				if (!find_ref_by_name(src, src_name))
+					ref->peer_ref = try_explicit_object_name("");
+				free(src_name);
+			}
+		}
+	}
 	if (errs)
 		return -1;
 	return 0;

diff --git a/remote.h b/remote.h
index b395598..341142c 100644
--- a/remote.h
+++ b/remote.h

@@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);
 enum match_refs_flags {
 	MATCH_REFS_NONE		= 0,
 	MATCH_REFS_ALL 		= (1 << 0),
-	MATCH_REFS_MIRROR	= (1 << 1)
+	MATCH_REFS_MIRROR	= (1 << 1),
+	MATCH_REFS_PRUNE	= (1 << 2),
 };
 
 /* Reporting of tracking info */

diff --git a/transport.c b/transport.c
index cac0c06..c20267c 100644
--- a/transport.c
+++ b/transport.c

@@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,
 			match_flags |= MATCH_REFS_ALL;
 		if (flags & TRANSPORT_PUSH_MIRROR)
 			match_flags |= MATCH_REFS_MIRROR;
+		if (flags & TRANSPORT_PUSH_PRUNE)
+			match_flags |= MATCH_REFS_PRUNE;
 
 		if (match_push_refs(local_refs, &remote_refs,
 				    refspec_nr, refspec, match_flags)) {

diff --git a/transport.h b/transport.h
index 059b330..5d30328 100644
--- a/transport.h
+++ b/transport.h

@@ -101,6 +101,7 @@ struct transport {
 #define TRANSPORT_PUSH_MIRROR 8
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_PUSH_PRUNE 64
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)

-- 
1.7.9.1

On Fri, Feb 17, 2012 at 09:12:37PM +0200, Felipe Contreras wrote:

This will allow us to remove refs from the remote that have been removed
locally.

It's useful to conveniently synchronize all the local branches to
certain remote.

Thanks for picking up this topic again.

There's one subtlety in the motivation above that you may find helpful
when you end up writing the documentation: "push --mirror" already does
this sort of pruning, but it _also_ implies that we are pushing
"refs/*:refs/*".  So this is really about giving access to the pruning
half, but still being able to use custom refspecs. So the features
together might end up being explained something like:

  --prune::
    ... prune things that no longer exist locally ...

  --mirror::
    ... turn on --prune, and also match all refs ...

At least that is my understanding of how the code is meant to work.

 builtin/push.c |    2 ++
 remote.c       |   29 ++++++++++++++++++++++++++---
 remote.h       |    3 ++-
 transport.c    |    2 ++
 transport.h    |    1 +
 5 files changed, 33 insertions(+), 4 deletions(-)

I've just given a quick read to the patches so far, but I did notice
this:

 static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
-		int send_mirror, const struct refspec **ret_pat)
+		int send_mirror, int dir, const struct refspec **ret_pat)

The "dir" flag looks like it is meant to be short for "direction". But
the callers only pass 0 or 1.  I'm not clear which direction is which.
Either symbolic constants for directions, or perhaps giving it a more
boolean name like "match_to_dst" might make it more clear.

I'll try to take a closer look later tonight.

-Peff

Felipe Contreras [off-list ref] writes:

So that we can reuse src later on.

Will be useful in next patches.

Signed-off-by: Felipe Contreras <redacted>
---

If the 2/3 were much less complex, it might have made sense to squash this
into that, but I think it makes it easier to see what is happening if an
obvious and safe no-op change like this is done as a separate patch like
this series did.

Looking good so far.

Felipe Contreras [off-list ref] writes:

For a change that not just adds parameters but removes an existing one, 
this is way under-described with neither in-code comment nor log message.

So I'll have to think aloud in this review.  Take it as a chance to learn
how the thought process of other people with lessor intelligence than you
have might work, and how to help those slower than you are ;-)

We used to discard what match_name_with_pattern() finds out by matching a
wildcard refspec against the ref by passing two NULLs.  This updates the
code to capture what destination ref ref->name is mapped to, by using the
same logic as the original and only caller, i.e. 'foo' without destination
maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.

This function is not used by fetching side of the codepath, so we do not
have to worry about its need to use different dst_side selection logic
(i.e. 'foo' without destination maps to "do not store anywhere other than
FETCH_HEAD").  Good.

+			}
+		}
 	}
-...
+	if (matching_refs == -1)
 		return NULL;
+
+	pat = rs + matching_refs;
+	if (pat->matching) {
+		/*
+		 * "matching refs"; traditionally we pushed everything
+		 * including refs outside refs/heads/ hierarchy, but
+		 * that does not make much sense these days.
+		 */
+		if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
+			return NULL;
+		name = xstrdup(ref->name);
+	}

So you are moving some code from what the sole caller of this function
does after calling us, and that is where the new parameters come from.
And by doing so, you do not have to run the same match_name_with_pattern()
again.  OK.

+	if (ret_pat)
+		*ret_pat = pat;
+	return name;
 }

You did not initialize name to anything at the beginning, but if the
earlier match_name_with_pattern() didn't find anything, we could only come
here after matching_refs is set by the if (rs[i].matching) codepath; by
the time we come here, we would have xstrdup(ref->name) in name, so we
would never return a garbage pointer to the caller.  OK.

OK, it is easy to tell that the patch is trivially correct, once a reader
figures out that the patch is really about:

	Move code to check_pattern_match() from its sole caller to make it
	unnecessary to call match_name_with_pattern() twice.

Saying so in the log message would have prepared the reader, instead of
the "There's a lot of code that can be consolidated there." which does not
give hints on what to look for in the patch.

Also this changes the semantics (because it changed the meaning of its
return value) of check_pattern_match() so much that it would deserve a
rename, which I will address in my review of 3/3.

Otherwise this step looks good.

Felipe Contreras [off-list ref] writes:

This will allow us to remove refs from the remote that have been removed
locally.

Can you enhance this a bit more to summarize the gist of what the semantic
of this new feature is, perhaps like this:

	After pushing refs, "git push --prune" will remove refs from the
	remote that existed before the push and would have been pushed
	from us if we had some local refs that would have matched the
	refspecs used.  For example,

           $ git push --prune remote refs/heads/*:refs/remotes/repo1/*

	will push all local branches in our repository to refs with
	corresponding names under refs/remotes/repo1/ at the remote, and
	removes remote's refs in refs/remotes/repo1/ that no longer have
        corresponding local branches in our repository.  The refs at the
        remote outside refs/remotes/repo1/ are not affected.

In order to alley the worries raised in the previous discussion, something
to the effect of the last sentence above is crucial to have, I would think. 

It's useful to conveniently synchronize all the local branches to
certain remote.

When an update to this patch comes with a documentation update to
illustrate how to exercise this useful feature with an example, it will
start to make sense to write "this is useful" in the log message.  I know
you haven't gotten around the documentation part while the patch is marked
as RFC, and that is OK.

Please refrain from squatting on a short-and-sweet one letter option
before this new feature proves to be popular and useful in a few cycles,
especially when there already is a long option that begins with 'p'.

Can we name this a bit better?  I first thought "Huh? directory?", and had
to scratch my head, wondering if it is an offset into refs/heads/* string
or something....

....until the code told us that it is some sort of direction of the
matching.  A symbolic constant or two would be even better.

Originally this funcion was fed a list of refs in the source (i.e. on our
end, as this is only used in 'push') and matched them against the source
side of the refspec, rs[i].src, to see under what name the destination
side will store it (i.e. give dst_side as value to find out the result in
&name).  This patch adds a new caller, who feeds a list of refs in the
destination (i.e. on the remote end) to find out how they map to the names
on our end (i.e. source).  So "direction" is not necessarily incorrect; it
is the direction this function maps the names (either src-to-dst for the
original caller, or dst-to-src for the new caller).

Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something?

+			if (match) {
 				matching_refs = i;
 				break;
 			}

So what is the updated semantics of this function?  Is it still
appropriate to name it "check_pattern_match()"?

It seems that by now this does a lot more than just "check if a pattern
matches".  Since your patch 2/3, it is a function that finds out the
refname in the remote that the given one refspec would try to update, and
with this patch, it can also map in the reverse direction, given the list
of remote refs, finding out which local ref a refspec would use to update
them.

At the same time, to reduce risk of future breakage, we probably should
rename this function to make it clear that this function is to be only
used by the push side.

Perhaps rename this to "map_push_refs()" or something in the patch 2/3?

Yuck.  You do not want it to "try" as its name says.  You just want to
trigger its "delete" codepath.

Please extract the body of "if (!*name) { ... }" block out of that
function into a separate helper function, i.e.

	static struct ref *deleted_ref(void)
        {
		struct ref *ref = alloc_ref("(delete)");
                hashclr(ref->new_sha1);
                return ref;
        }

then update try_explicit_...() to call it, and call the same helper here.

This is not for runtime efficiency; feeding a constant to a function that
says try_foo() or check_bar() that makes decision on the parameter only to
trigger a partial codepath hurts readability.

Does it make sense to specify --prune when --mirror is in effect?  If so,
how would it behave differently from a vanilla --mirror?  If not, should
it be detected as an error?

I couldn't infer from the context shown in the patch, but how in general
does this new feature interact with the codepath for --mirror?

Hrm...?

 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)

On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras
[off-list ref] wrote:

As an example I put my use-case; I want to backup *all* my local branches to a
personal repository, and I want to remove branches that I have removed from my
local repository. git push personal 'refs/heads/*' mostly does the job, but it
doesn't remove anything, and that's where 'prune' comes from.

Yeah, may I have "fetch --prune" too, please? Looking at diffstat
gives me a feeling that you only need to add maybe four lines to
builtin/fetch.c and my dream would come true.
-- 
Duy

On Tue, Feb 21, 2012 at 10:30:31PM +0700, Nguyen Thai Ngoc Duy wrote:

On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras
[off-list ref] wrote:

quoted

As an example I put my use-case; I want to backup *all* my local branches to a
personal repository, and I want to remove branches that I have removed from my
local repository. git push personal 'refs/heads/*' mostly does the job, but it
doesn't remove anything, and that's where 'prune' comes from.

Yeah, may I have "fetch --prune" too, please? Looking at diffstat
gives me a feeling that you only need to add maybe four lines to
builtin/fetch.c and my dream would come true.

Huh? Don't we already have "fetch --prune"?

-Peff

On Wed, Feb 22, 2012 at 12:35 AM, Jeff King [off-list ref] wrote:

Huh? Don't we already have "fetch --prune"?

We do indeed. I went to git-fetch.txt, searched for prune but missed
many "include" directives in there.
-- 
Duy

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

There's a lot of code that can be consolidated there, and will be useful
for next patches.

Signed-off-by: Felipe Contreras <redacted>
---
 remote.c |   59 ++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 30 insertions(+), 29 deletions(-)

diff --git a/remote.c b/remote.c
index 55d68d1..019aafc 100644
--- a/remote.c
+++ b/remote.c

@@ -1110,10 +1110,11 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,

      return errs;
 }

-static const struct refspec *check_pattern_match(const struct refspec *rs,
-                                              int rs_nr,
-                                              const struct ref *src)
+static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
+             int send_mirror, const struct refspec **ret_pat)
 {

For a change that not just adds parameters but removes an existing one,
this is way under-described with neither in-code comment nor log message.

But it doesn't. src is renamed to ref.

quoted

+     const struct refspec *pat;
+     char *name;
      int i;
      int matching_refs = -1;
      for (i = 0; i < rs_nr; i++) {

@@ -1123,14 +1124,31 @@ static const struct refspec *check_pattern_match(const struct refspec *rs,

                      continue;
              }

-             if (rs[i].pattern && match_name_with_pattern(rs[i].src, src->name,
-                                                          NULL, NULL))
-                     return rs + i;
+             if (rs[i].pattern) {
+                     const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
+                     if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+                             matching_refs = i;
+                             break;

We used to discard what match_name_with_pattern() finds out by matching a
wildcard refspec against the ref by passing two NULLs.  This updates the
code to capture what destination ref ref->name is mapped to, by using the
same logic as the original and only caller, i.e. 'foo' without destination
maps to the same 'foo' destination, 'foo:bar' maps to the named 'bar'.

This function is not used by fetching side of the codepath, so we do not
have to worry about its need to use different dst_side selection logic
(i.e. 'foo' without destination maps to "do not store anywhere other than
FETCH_HEAD").  Good.

I actually didn't parse a lot of that.

quoted

+                     }
+             }
      }
-...
+     if (matching_refs == -1)
              return NULL;
+
+     pat = rs + matching_refs;
+     if (pat->matching) {
+             /*
+              * "matching refs"; traditionally we pushed everything
+              * including refs outside refs/heads/ hierarchy, but
+              * that does not make much sense these days.
+              */
+             if (!send_mirror && prefixcmp(ref->name, "refs/heads/"))
+                     return NULL;
+             name = xstrdup(ref->name);
+     }

So you are moving some code from what the sole caller of this function
does after calling us, and that is where the new parameters come from.
And by doing so, you do not have to run the same match_name_with_pattern()
again.  OK.

Indeed.

-- 
Felipe Contreras

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

This will allow us to remove refs from the remote that have been removed
locally.

Can you enhance this a bit more to summarize the gist of what the semantic
of this new feature is, perhaps like this:

       After pushing refs, "git push --prune" will remove refs from the
       remote that existed before the push and would have been pushed
       from us if we had some local refs that would have matched the
       refspecs used.  For example,

          $ git push --prune remote refs/heads/*:refs/remotes/repo1/*

       will push all local branches in our repository to refs with
       corresponding names under refs/remotes/repo1/ at the remote, and
       removes remote's refs in refs/remotes/repo1/ that no longer have
       corresponding local branches in our repository.  The refs at the
       remote outside refs/remotes/repo1/ are not affected.

In order to alley the worries raised in the previous discussion, something
to the effect of the last sentence above is crucial to have, I would think.

OK.

quoted

--- a/builtin/push.c
+++ b/builtin/push.c

@@ -261,6 +261,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)

              OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
                      TRANSPORT_PUSH_SET_UPSTREAM),
              OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+             OPT_BIT('p', "prune", &flags, "prune locally removed refs",
+                     TRANSPORT_PUSH_PRUNE),

Please refrain from squatting on a short-and-sweet one letter option
before this new feature proves to be popular and useful in a few cycles,
especially when there already is a long option that begins with 'p'.

OK.

quoted

              OPT_END()
      };

diff --git a/remote.c b/remote.c
index 019aafc..0900bb5 100644
--- a/remote.c
+++ b/remote.c

@@ -1111,7 +1111,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,

 }

 static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref *ref,
-             int send_mirror, const struct refspec **ret_pat)
+             int send_mirror, int dir, const struct refspec **ret_pat)

Can we name this a bit better?  I first thought "Huh? directory?", and had
to scratch my head, wondering if it is an offset into refs/heads/* string
or something....

OK.

quoted

 {
      const struct refspec *pat;
      char *name;

@@ -1126,7 +1126,12 @@ static char *check_pattern_match(const struct refspec *rs, int rs_nr, struct ref

              if (rs[i].pattern) {
                      const char *dst_side = rs[i].dst ? rs[i].dst : rs[i].src;
-                     if (match_name_with_pattern(rs[i].src, ref->name, dst_side, &name)) {
+                     int match;
+                     if (dir == 0)
+                             match = match_name_with_pattern(rs[i].src, ref->name, dst_side, &name);
+                     else
+                             match = match_name_with_pattern(dst_side, ref->name, rs[i].src, &name);

....until the code told us that it is some sort of direction of the
matching.  A symbolic constant or two would be even better.

Originally this funcion was fed a list of refs in the source (i.e. on our
end, as this is only used in 'push') and matched them against the source
side of the refspec, rs[i].src, to see under what name the destination
side will store it (i.e. give dst_side as value to find out the result in
&name).  This patch adds a new caller, who feeds a list of refs in the
destination (i.e. on the remote end) to find out how they map to the names
on our end (i.e. source).  So "direction" is not necessarily incorrect; it
is the direction this function maps the names (either src-to-dst for the
original caller, or dst-to-src for the new caller).

Perhaps "enum map_direction { SRC_TO_DST, DST_TO_SRC }" or something?

I think only FROM_SRC, FROM_DST is more than enough to figure it out.

quoted

+                     if (match) {
                              matching_refs = i;
                              break;
                      }

So what is the updated semantics of this function?  Is it still
appropriate to name it "check_pattern_match()"?

It seems that by now this does a lot more than just "check if a pattern
matches".  Since your patch 2/3, it is a function that finds out the
refname in the remote that the given one refspec would try to update, and
with this patch, it can also map in the reverse direction, given the list
of remote refs, finding out which local ref a refspec would use to update
them.

At the same time, to reduce risk of future breakage, we probably should
rename this function to make it clear that this function is to be only
used by the push side.

Perhaps rename this to "map_push_refs()" or something in the patch 2/3?

I think get_ref_match() would be more appropriate because we are
acting on a specific (singular) ref, and the primary thing we care
about is getting the peer name, based on the refspec match, which we
might want as a return value.

quoted

@@ -1173,6 +1178,7 @@ int match_push_refs(struct ref *src, struct ref **dst,

      struct refspec *rs;
      int send_all = flags & MATCH_REFS_ALL;
      int send_mirror = flags & MATCH_REFS_MIRROR;
+     int send_prune = flags & MATCH_REFS_PRUNE;
      int errs;
      static const char *default_refspec[] = { ":", NULL };
      struct ref *ref, **dst_tail = tail_ref(dst);

@@ -1193,7 +1199,7 @@ int match_push_refs(struct ref *src, struct ref **dst,

              if (ref->peer_ref)
                      continue;

-             dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, &pat);
+             dst_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 0, &pat);
              if (!dst_name)
                      continue;

@@ -1220,6 +1226,23 @@ int match_push_refs(struct ref *src, struct ref **dst,

      free_name:
              free(dst_name);
      }
+     if (send_prune) {
+             /* check for missing refs on the remote */
+             for (ref = *dst; ref; ref = ref->next) {
+                     char *src_name;
+
+                     if (ref->peer_ref)
+                             /* We're already sending something to this ref. */
+                             continue;
+
+                     src_name = check_pattern_match(rs, nr_refspec, ref, send_mirror, 1, NULL);
+                     if (src_name) {
+                             if (!find_ref_by_name(src, src_name))
+                                     ref->peer_ref = try_explicit_object_name("");

Yuck.  You do not want it to "try" as its name says.  You just want to
trigger its "delete" codepath.

Please extract the body of "if (!*name) { ... }" block out of that
function into a separate helper function, i.e.

       static struct ref *deleted_ref(void)
       {
               struct ref *ref = alloc_ref("(delete)");
               hashclr(ref->new_sha1);
               return ref;
       }

then update try_explicit_...() to call it, and call the same helper here.

This is not for runtime efficiency; feeding a constant to a function that
says try_foo() or check_bar() that makes decision on the parameter only to
trigger a partial codepath hurts readability.

All right.

quoted

+                             free(src_name);
+                     }
+             }
+     }
      if (errs)
              return -1;
      return 0;

diff --git a/remote.h b/remote.h
index b395598..341142c 100644
--- a/remote.h
+++ b/remote.h

@@ -145,7 +145,8 @@ int branch_merge_matches(struct branch *, int n, const char *);

 enum match_refs_flags {
      MATCH_REFS_NONE         = 0,
      MATCH_REFS_ALL          = (1 << 0),
-     MATCH_REFS_MIRROR       = (1 << 1)
+     MATCH_REFS_MIRROR       = (1 << 1),
+     MATCH_REFS_PRUNE        = (1 << 2),
 };

 /* Reporting of tracking info */

diff --git a/transport.c b/transport.c
index cac0c06..c20267c 100644
--- a/transport.c
+++ b/transport.c

@@ -1028,6 +1028,8 @@ int transport_push(struct transport *transport,

                      match_flags |= MATCH_REFS_ALL;
              if (flags & TRANSPORT_PUSH_MIRROR)
                      match_flags |= MATCH_REFS_MIRROR;
+             if (flags & TRANSPORT_PUSH_PRUNE)
+                     match_flags |= MATCH_REFS_PRUNE;

Does it make sense to specify --prune when --mirror is in effect?  If so,
how would it behave differently from a vanilla --mirror?  If not, should
it be detected as an error?

Probably doesn't make sense, should be an error.

I couldn't infer from the context shown in the patch, but how in general
does this new feature interact with the codepath for --mirror?

quoted

              if (match_push_refs(local_refs, &remote_refs,
                                  refspec_nr, refspec, match_flags)) {

diff --git a/transport.h b/transport.h
index 059b330..5d30328 100644
--- a/transport.h
+++ b/transport.h

@@ -101,6 +101,7 @@ struct transport {

 #define TRANSPORT_PUSH_MIRROR 8
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
+#define TRANSPORT_PUSH_PRUNE 64
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64

Hrm...?

Probably some rebase mistake =/

Cheers.

-- 
Felipe Contreras

On Wed, Feb 22, 2012 at 10:43 PM, Felipe Contreras
[off-list ref] wrote:

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano [off-list ref] wrote:

quoted

Does it make sense to specify --prune when --mirror is in effect?  If so,
how would it behave differently from a vanilla --mirror?  If not, should
it be detected as an error?

Probably doesn't make sense, should be an error.

Actually, after writing the patch for the documentation I realized
this would be difficult to describe: you can use --all --prune, but
not --mirror --prune, and the documentation currently has '[--all |
--mirror | --tags]'. So I decided to make it orthogonal, you can use
--prune with --all, --tags, *and* --mirror. Of course, using --prune
--mirror is the same as --mirror.

Cheers.

-- 
Felipe Contreras