[PATCH 0/4] push: add 'prune' option

[PATCH 0/4] 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.

Cheers.

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

Since RFC:

 - Most of comments addressed
 - Documentation and tests added

Felipe Contreras (4):
  remote: use a local variable in match_push_refs()
  remote: reorganize check_pattern_match()
  remote: refactor code into alloc_delete_ref()
  push: add 'prune' option

 Documentation/git-push.txt |    9 +++-
 builtin/push.c             |    2 +
 remote.c                   |  107 ++++++++++++++++++++++++++++----------------
 remote.h                   |    3 +-
 t/t5516-fetch-push.sh      |   16 +++++++
 transport.c                |    2 +
 transport.h                |    1 +
 7 files changed, 100 insertions(+), 40 deletions(-)

-- 
1.7.9.1

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

[Felipe Contreras]

So that we can reuse src later on. No functional changes.

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

[PATCH 2/4] 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.

Right now match_name_with_pattern() is called twice, the first one
without value and result, and the second one with them. Since
check_pattern_match() is only used in one place, we can just reorganize
it to make a single call and fetch the values at the same time.

Since this is a semantic change, also rename the function to
get_ref_match() which actually describes more closely what it's actually
doing now.

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..9d29acc 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 *get_ref_match(const struct refspec *rs, int rs_nr, const 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 = get_ref_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

[PATCH 3/4] remote: refactor code into alloc_delete_ref()

[Felipe Contreras]

Will be useful in next patches. No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 remote.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 9d29acc..4709e20 100644
--- a/remote.c
+++ b/remote.c
@@ -978,16 +978,20 @@ static void tail_link_ref(struct ref *ref, struct ref ***tail)
 	*tail = &ref->next;
 }
 
+static struct ref *alloc_delete_ref(void)
+{
+	struct ref *ref = alloc_ref("(delete)");
+	hashclr(ref->new_sha1);
+	return ref;
+}
+
 static struct ref *try_explicit_object_name(const char *name)
 {
 	unsigned char sha1[20];
 	struct ref *ref;
 
-	if (!*name) {
-		ref = alloc_ref("(delete)");
-		hashclr(ref->new_sha1);
-		return ref;
-	}
+	if (!*name)
+		return alloc_delete_ref();
 	if (get_sha1(name, sha1))
 		return NULL;
 	ref = alloc_ref(name);
-- 
1.7.9.1

[PATCH 4/4] 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.

Unfortunately, if we want to handle src:dst refspecs properly, some
extra complexity is needed, so check_pattern_match() needs to be
reorganized (previous patch).

This ensures that the following works:

 $ git push --prune remote refs/heads/*:refs/tmp/*

So that local branches are not only pushed as custom refs in the remote,
but also refs/tmp/foo would be removed if there's no corresponding local
refs/heads/foo.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-push.txt |    9 ++++++++-
 builtin/push.c             |    2 ++
 remote.c                   |   31 ++++++++++++++++++++++++++++---
 remote.h                   |    3 ++-
 t/t5516-fetch-push.sh      |   16 ++++++++++++++++
 transport.c                |    2 ++
 transport.h                |    1 +
 7 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index aede488..204f36d 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git push' [--all | --mirror | --tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
-	   [--repo=<repository>] [-f | --force] [-v | --verbose] [-u | --set-upstream]
+	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
 	   [<repository> [<refspec>...]]
 
 DESCRIPTION
@@ -71,6 +71,13 @@ nor in any Push line of the corresponding remotes file---see below).
 	Instead of naming each ref to push, specifies that all
 	refs under `refs/heads/` be pushed.
 
+--prune::
+	Remove remote branches that don't have a local counterpart. For example
+	a remote branch `tmp` will be removed if a local branch with the same
+	name doesn't exist any more. This also respects refspecs, e.g.
+	`refs/heads/*:refs/tmp/*` would make sure that remote `refs/tmp/foo`
+	will be removed if `refs/heads/foo` doesn't exist.
+
 --mirror::
 	Instead of naming each ref to push, specifies that all
 	refs under `refs/` (which includes but is not
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..fdfb2c4 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(0, "prune", &flags, "prune locally removed refs",
+			TRANSPORT_PUSH_PRUNE),
 		OPT_END()
 	};
 
diff --git a/remote.c b/remote.c
index 4709e20..9c590ae 100644
--- a/remote.c
+++ b/remote.c
@@ -8,6 +8,8 @@
 #include "tag.h"
 #include "string-list.h"
 
+enum map_direction { FROM_SRC, FROM_DST };
+
 static struct refspec s_tag_refspec = {
 	0,
 	1,
@@ -1115,7 +1117,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 }
 
 static char *get_ref_match(const struct refspec *rs, int rs_nr, const struct ref *ref,
-		int send_mirror, const struct refspec **ret_pat)
+		int send_mirror, int direction, const struct refspec **ret_pat)
 {
 	const struct refspec *pat;
 	char *name;
@@ -1130,7 +1132,12 @@ static char *get_ref_match(const struct refspec *rs, int rs_nr, const 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 (direction == FROM_SRC)
+				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;
 			}
@@ -1177,6 +1184,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);
@@ -1197,7 +1205,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
 		if (ref->peer_ref)
 			continue;
 
-		dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, &pat);
+		dst_name = get_ref_match(rs, nr_refspec, ref, send_mirror, FROM_SRC, &pat);
 		if (!dst_name)
 			continue;
 
@@ -1224,6 +1232,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 = get_ref_match(rs, nr_refspec, ref, send_mirror, FROM_DST, NULL);
+			if (src_name) {
+				if (!find_ref_by_name(src, src_name))
+					ref->peer_ref = alloc_delete_ref();
+				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/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b69cf57..b5417cc 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -979,4 +979,20 @@ test_expect_success 'push --porcelain --dry-run rejected' '
 	test_cmp .git/foo .git/bar
 '
 
+test_expect_success 'push --prune' '
+	mk_test heads/master heads/second heads/foo heads/bar &&
+	git push --prune testrepo &&
+	check_push_result $the_commit heads/master &&
+	check_push_result $the_first_commit heads/second &&
+	! check_push_result $the_first_commit heads/foo heads/bar
+'
+
+test_expect_success 'push --prune refspec' '
+	mk_test tmp/master tmp/second tmp/foo tmp/bar &&
+	git push --prune testrepo "refs/heads/*:refs/tmp/*" &&
+	check_push_result $the_commit tmp/master &&
+	check_push_result $the_first_commit tmp/second &&
+	! check_push_result $the_first_commit tmp/foo tmp/bar
+'
+
 test_done
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..ce99ef8 100644
--- a/transport.h
+++ b/transport.h
@@ -102,6 +102,7 @@ struct transport {
 #define TRANSPORT_PUSH_PORCELAIN 16
 #define TRANSPORT_PUSH_SET_UPSTREAM 32
 #define TRANSPORT_RECURSE_SUBMODULES_CHECK 64
+#define TRANSPORT_PUSH_PRUNE 128
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 
-- 
1.7.9.1

Re: [PATCH 4/4] push: add 'prune' option

[Junio C Hamano]

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

> +--prune::
> +	Remove remote branches that don't have a local counterpart. For example
> +	a remote branch `tmp` will be removed if a local branch with the same
> +	name doesn't exist any more. This also respects refspecs, e.g.
> +	`refs/heads/*:refs/tmp/*` would make sure that remote `refs/tmp/foo`
> +	will be removed if `refs/heads/foo` doesn't exist.

I do not think it adds much useful information to mention `tmp` only once
over what is already said by the first sentence.  Also, the first sentence
of the example does not make it clear that it is assuming a same-for-same
mapping.

Coming up with a precise technical description is easy, but it is hard to
explain to the end user in easy terms, and I commend you for attempting to
add an example in a short single sentence, though.

Perhaps spelling out the underlying assumption the example makes is the
best we could do here without going too technical.

        ... For example, if you are pushing all your local branches to
        update the local branches of the remote, `tmp` branch will be
        removed from the remote if you removed your `tmp` branch locally.

        If you are pushing all your local branches on your laptop to a
        repository on your desktop machine under `refs/remotes/laptop/`
        hierarchy to back them up, `refs/remotes/laptop/tmp` is removed
        from the remote if you no longer have the branch `tmp` on your
        laptop.


Will queue with a slight fix-ups, including this bit:

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

Lose the ',' at the end, for the same reason why deleted line did not have
one.

Thanks.

Re: [PATCH 4/4] push: add 'prune' option

[Felipe Contreras]

On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +--prune::
>> +     Remove remote branches that don't have a local counterpart. For example
>> +     a remote branch `tmp` will be removed if a local branch with the same
>> +     name doesn't exist any more. This also respects refspecs, e.g.
>> +     `refs/heads/*:refs/tmp/*` would make sure that remote `refs/tmp/foo`
>> +     will be removed if `refs/heads/foo` doesn't exist.
>
> I do not think it adds much useful information to mention `tmp` only once
> over what is already said by the first sentence.  Also, the first sentence
> of the example does not make it clear that it is assuming a same-for-same
> mapping.

Sure, the first sentence doesn't make it clear, but it would be a
valid and obvious assumption. The second sentence makes it clear, and
the name `tmp` immediately evokes a branch that will probably be
removed.

> Coming up with a precise technical description is easy, but it is hard to
> explain to the end user in easy terms, and I commend you for attempting to
> add an example in a short single sentence, though.
>
> Perhaps spelling out the underlying assumption the example makes is the
> best we could do here without going too technical.
>
>        ... For example, if you are pushing all your local branches to
>        update the local branches of the remote,

Yeah, but that's 'git push --all', and that's not the common
operation--'git push' is. So that's what I presumed the reader would
assume, and it really doesn't make a difference as to what will
follow:

>        `tmp` branch will be
>        removed from the remote if you removed your `tmp` branch locally.

This reuses the name `tmp`, which seems to be your objective, but it
doesn't explain _why_ it would remove `tmp`; is it because `tmp` is
the upstream branch, or is it because it has the same name?

>        If you are pushing all your local branches on your laptop to a
>        repository on your desktop machine under `refs/remotes/laptop/`
>        hierarchy to back them up, `refs/remotes/laptop/tmp` is removed
>        from the remote if you no longer have the branch `tmp` on your
>        laptop.

Unfortunately, I as a reader have trouble understanding this. More
specifically I have trouble understanding where `refs/remotes/laptop/`
is coming from, and what it is meaning. I have always pictured
`refs/remotes` as something that 'git fetch' updates, and always from
the relevant repository. While eventually I could understand what this
thing is doing, it took me more than one read, and I had to read
slowly, and even then it seems completely non-standard to me.

I think a synthetic example, like `refs/heads/*:refs/tmp/*`, is much
easier to understand because it doesn't mess up with any established
refs, and also has the advantage that it shows the relevant refspec,
which is useful for people that are not familiar with refspecs, and in
fact, people could try it out without messing with their current refs.

>> 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),
>>  };
>
> Lose the ',' at the end, for the same reason why deleted line did not have
> one.

And why is that? Isn't git c99? That comma would only ensure that the
next patch that touches this would be two lines instead of one.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 4/4] push: add 'prune' option

[Junio C Hamano]

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

> On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Yeah, but that's 'git push --all', and that's not the common
> operation--'git push' is.

"git push" is common but that does not give you a solid base to guess what
the reader's assumption would be.  Are you assuming "matching" semantics?

> So that's what I presumed the reader would
> assume,...

I do not want let us guess what the reader assumes, as many people seem to
suggest setting push.default to different values and that would change
what the reader would assume.  That was the whole reason that I suggested
to spell the assumption out, so that the reader's assumption does not have
to get into the picture.

> This reuses the name `tmp`, which seems to be your objective, but it
> doesn't explain _why_ it would remove `tmp`; is it because `tmp` is
> the upstream branch, or is it because it has the same name?

The example is to clarify "local counterpart" in the main text.  I
actually would prefer to get rid of `tmp` but I left it as-is as you
wrote.  The exact name used in the example does not matter, whether it is
`tmp` or `xyzzy`.

> Unfortunately, I as a reader have trouble understanding this. More
> specifically I have trouble understanding where `refs/remotes/laptop/`
> is coming from, and what it is meaning. I have always pictured
> `refs/remotes` as something that 'git fetch' updates, and always from
> the relevant repository.

The layout is the recommended set-up to emulate a fetch with a push in the
reverse direction, which I thought anybody should notice.  It is a failure
in our documentation that even an expert didn't.

>>> 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),
>>>  };
>>
>> Lose the ',' at the end, for the same reason why deleted line did not have
>> one.
>
> And why is that?

Because I told you so ;-).

More seriously, we have had patches to accomodate other people's compilers
by dropping the last comma in enum {}.  See c9b6782 (enums: omit trailing
comma for portability, 2011-03-16), 4b05548 (enums: omit trailing comma
for portability, 2010-05-14) for examples.

Re: [PATCH 4/4] push: add 'prune' option

[Felipe Contreras]

On Thu, Feb 23, 2012 at 3:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Thu, Feb 23, 2012 at 2:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Yeah, but that's 'git push --all', and that's not the common
>> operation--'git push' is.
>
> "git push" is common but that does not give you a solid base to guess what
> the reader's assumption would be.  Are you assuming "matching" semantics?

But if you want to spell the assumption, why not spell the most common one?

>> So that's what I presumed the reader would
>> assume,...
>
> I do not want let us guess what the reader assumes, as many people seem to
> suggest setting push.default to different values and that would change
> what the reader would assume.

No, it wouldn't. Almost all readers are familiar about what 'git push'
does by default--I would even adventure to say all--the ones that set
a custom push.default would do it because they _know_ they don't want
the default.

And in fact, as I said, it doesn't matter what the reader has set in
push.default, or if the reader is assuming 'git push --all', or 'git
push :', or 'git push --tags', or 'git push refs/heads/*', the end
result is the same.

> That was the whole reason that I suggested
> to spell the assumption out, so that the reader's assumption does not have
> to get into the picture.

I don't think it matters what the reader assumes, because the end
result is the same, so there's no reason to spell any assumption.

>> This reuses the name `tmp`, which seems to be your objective, but it
>> doesn't explain _why_ it would remove `tmp`; is it because `tmp` is
>> the upstream branch, or is it because it has the same name?
>
> The example is to clarify "local counterpart" in the main text.  I
> actually would prefer to get rid of `tmp` but I left it as-is as you
> wrote.  The exact name used in the example does not matter, whether it is
> `tmp` or `xyzzy`.

I believe `tmp` is more useful than `xyzzy`, or even more than
`master`. I already explained why.

>> Unfortunately, I as a reader have trouble understanding this. More
>> specifically I have trouble understanding where `refs/remotes/laptop/`
>> is coming from, and what it is meaning. I have always pictured
>> `refs/remotes` as something that 'git fetch' updates, and always from
>> the relevant repository.
>
> The layout is the recommended set-up to emulate a fetch with a push in the
> reverse direction, which I thought anybody should notice.  It is a failure
> in our documentation that even an expert didn't.

The problem is not figuring out the `refs/remotes/foo`, but picturing
the fact that there are two repositories, the user ran on one of them
'git remote add laptop', and then 'git remote add pc', and then 'git
push --prune refs/heads*:refs/remotes/laptop/*'. Lots of things to
keep in mind for this paragraph. And, as you say, it's the reverse
direction of a fetch, so it's immediately weird.

Moreover, I find it strange that you want to rely on the assumption
that the reader is familiar with the pattern `refs/remotes/foo`, and
the refspec syntax, which is pretty deep plumbing, and not on the
default action of 'git push', which is high-level porcelain, and as I
said, not even needed to understand my example.

>>>> 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),
>>>>  };
>>>
>>> Lose the ',' at the end, for the same reason why deleted line did not have
>>> one.
>>
>> And why is that?
>
> Because I told you so ;-).

You didn't tell me anything about the previous line :)

> More seriously, we have had patches to accomodate other people's compilers
> by dropping the last comma in enum {}.  See c9b6782 (enums: omit trailing
> comma for portability, 2011-03-16), 4b05548 (enums: omit trailing comma
> for portability, 2010-05-14) for examples.

Yeah, I remembered those patches after sending the mail. Shame. Maybe
next decade =/

Cheers.
-- 
Felipe Contreras