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

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

[Felipe Contreras]

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

[RFC/PATCH 1/3] remote: use a local variable in match_push_refs()

[Felipe Contreras]

So that we can reuse src later on.

Will be useful in next patches.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

Re: [RFC/PATCH 1/3] remote: use a local variable in match_push_refs()

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> So that we can reuse src later on.
>
> Will be useful in next patches.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---

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.

[RFC/PATCH 2/3] remote: reorganize check_pattern_match()

[Felipe Contreras]

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's a lot of code that can be consolidated there, and will be useful
> for next patches.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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.

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

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

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

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

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.

Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()

[Felipe Contreras]

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> There's a lot of code that can be consolidated there, and will be useful
>> for next patches.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  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.

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

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

Re: [RFC/PATCH 2/3] remote: reorganize check_pattern_match()

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> 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.
>
> But it doesn't. src is renamed to ref.

That is exactly why I mentioned that this is a "chance" in the part of my
message you did not quote ;-).

It was a chance for you to learn how the thought process of others may
work or not work well, depending on how well the log message at the top
prepares their mind before they start to read the patch, by looking at how
one reader (me) tried to figure your patch out.

Anything that I said in the message you are responding to that tempts you
to say "No you are reading wrong" is an indication that the patch did not
do a good job to help the reader understand what you wanted to do.

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

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

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

[Jeff King]

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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> 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.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  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),

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

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

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

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

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

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

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?

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

>  #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)

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

[Felipe Contreras]

On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> 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.

OK.

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

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

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

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

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

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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

Yeah, as you can tell from my "or something", as long as the name makes
the direction clear, I don't care too much about the exact spelling.

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

Again, as long as the name makes it clear this is meant only for "push"
and never be used for "fetch", I am fine with it.  One way to make it sure
is to have a substring "_push" somewhere in that name.

> Probably some rebase mistake =/

Thanks; I was wondering if there is something subtle unexplained going on.

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

[Felipe Contreras]

On Wed, Feb 22, 2012 at 10:43 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Feb 18, 2012 at 12:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> ... Of course, using --prune
> --mirror is the same as --mirror.

Sounds good.  Thanks.

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

[Nguyen Thai Ngoc Duy]

On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras
<felipe.contreras@gmail.com> 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

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

[Jeff King]

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
> <felipe.contreras@gmail.com> 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.

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

-Peff

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

[Nguyen Thai Ngoc Duy]

On Wed, Feb 22, 2012 at 12:35 AM, Jeff King <peff@peff.net> 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