threads / patch / 32171

patch, 7 partspush: flag updates that require force

Subject: [PATCH 4/7] push: flag updates that require force

## tl;dr

18 messages between Nov 23, 2012 and Nov 28, 2012. Diffs are folded; open one to read it.

replies: 17people: 2as markdown or json

Chris Rorvick· Nov 23, 2012, 04:21 UTC · lore

[PATCH v5 0/7] push: update remote tags only with force

Incorporated Junio's feedback. Also, I broke the last patch of the previous series out into three to make the changes more clear.

This patch set can be divided into two sets:
  1. Provide useful advice for rejected tag references.
     push: return reject reasons via a mask
     push: add advice for rejected tag reference
     Recommending a merge to resolve a rejected tag update seems
     nonsensical since the tag does not come along for the ride.  These
     patches change the advice for rejected tags to suggest using
     "push -f".
  2. Require force when updating tag references, even on a fast-forward.
     push: flag updates
     push: flag updates that require force
     push: require force for refs under refs/tags/
     push: require force for annotated tags
     push: clarify rejection of update to non-commit-ish
     This is in response to the following thread:
       http://thread.gmane.org/gmane.comp.version-control.git/208354
     This series prevents fast-forwards if the reference is under the
     refs/tags/* hierarchy or if the old object is a tag.
Chris Rorvick (7):
  push: return reject reasons via a mask
  push: add advice for rejected tag reference
  push: flag updates
  push: flag updates that require force
  push: require force for refs under refs/tags/
  push: require force for annotated tags
  push: clarify rejection of update to non-commit-ish
 Documentation/git-push.txt |  9 ++++---
 builtin/push.c             | 24 ++++++++++-------
 builtin/send-pack.c        |  9 +++++--
 cache.h                    |  7 ++++-
 remote.c                   | 65 +++++++++++++++++++++++++++++++++++++---------
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 44 ++++++++++++++++++++++++++++++-
 transport-helper.c         |  6 +++++
 transport.c                | 25 +++++++++++-------
 transport.h                | 10 ++++---
 10 files changed, 157 insertions(+), 43 deletions(-)
-- 
1.8.0.209.gf3828dc
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

[PATCH 1/7] push: return reject reasons via a mask

Pass all rejection reasons back from transport_push(). The logic is simpler and more flexible with regard to providing useful feedback.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 builtin/push.c      | 13 ++++---------
 builtin/send-pack.c |  4 ++--
 transport.c         | 17 ++++++++---------
 transport.h         |  9 +++++----
 4 files changed, 19 insertions(+), 24 deletions(-)
Show changes to 4 files +19 −24

builtin/push.c, builtin/send-pack.c, transport.c, transport.h

diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..4a0e7ef 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -244,7 +244,7 @@ static void advise_checkout_pull_push(void)
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
-	int nonfastforward;
+	unsigned int reject_mask;
 
 	transport_set_verbosity(transport, verbosity, progress);
 
@@ -257,7 +257,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (verbosity > 0)
 		fprintf(stderr, _("Pushing to %s\n"), transport->url);
 	err = transport_push(transport, refspec_nr, refspec, flags,
-			     &nonfastforward);
+			     &reject_mask);
 	if (err != 0)
 		error(_("failed to push some refs to '%s'"), transport->url);
 
@@ -265,18 +265,13 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	switch (nonfastforward) {
-	default:
-		break;
-	case NON_FF_HEAD:
+	if (reject_mask & REJECT_NON_FF_HEAD) {
 		advise_pull_before_push();
-		break;
-	case NON_FF_OTHER:
+	} else if (reject_mask & REJECT_NON_FF_OTHER) {
 		if (default_matching_used)
 			advise_use_upstream();
 		else
 			advise_checkout_pull_push();
-		break;
 	}
 
 	return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index d342013..fda28bc 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -85,7 +85,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
 	int flags;
-	int nonfastforward = 0;
+	unsigned int reject_mask;
 	int progress = -1;
 
 	argv++;
@@ -223,7 +223,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	ret |= finish_connect(conn);
 
 	if (!helper_status)
-		transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward);
+		transport_print_push_status(dest, remote_refs, args.verbose, 0, &reject_mask);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
diff --git a/transport.c b/transport.c
index 9932f40..b0c9f1b 100644
--- a/transport.c
+++ b/transport.c
@@ -714,7 +714,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-				  int verbose, int porcelain, int *nonfastforward)
+				  int verbose, int porcelain, unsigned int *reject_mask)
 {
 	struct ref *ref;
 	int n = 0;
@@ -733,18 +733,17 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
-	*nonfastforward = 0;
+	*reject_mask = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
-		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
-		    *nonfastforward != NON_FF_HEAD) {
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
 			if (!strcmp(head, ref->name))
-				*nonfastforward = NON_FF_HEAD;
+				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
-				*nonfastforward = NON_FF_OTHER;
+				*reject_mask |= REJECT_NON_FF_OTHER;
 		}
 	}
 }
@@ -1031,9 +1030,9 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
 
 int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
-		   int *nonfastforward)
+		   unsigned int *reject_mask)
 {
-	*nonfastforward = 0;
+	*reject_mask = 0;
 	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
@@ -1099,7 +1098,7 @@ int transport_push(struct transport *transport,
 		if (!quiet || err)
 			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
-					nonfastforward);
+					reject_mask);
 
 		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
 			set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index 4a61c0c..5f76ca2 100644
--- a/transport.h
+++ b/transport.h
@@ -140,11 +140,12 @@ int transport_set_option(struct transport *transport, const char *name,
 void transport_set_verbosity(struct transport *transport, int verbosity,
 	int force_progress);
 
-#define NON_FF_HEAD 1
-#define NON_FF_OTHER 2
+#define REJECT_NON_FF_HEAD     0x01
+#define REJECT_NON_FF_OTHER    0x02
+
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-		   int * nonfastforward);
+		   unsigned int * reject_mask);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
@@ -170,7 +171,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 int transport_refs_pushed(struct ref *ref);
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-		  int verbose, int porcelain, int *nonfastforward);
+		  int verbose, int porcelain, unsigned int *reject_mask);
 
 typedef void alternate_ref_fn(const struct ref *, void *);
 extern void for_each_alternate_ref(alternate_ref_fn, void *);

-- 
1.8.0.209.gf3828dc
Junio C Hamano· Nov 26, 2012, 18:43 UTC · re: Chris Rorvick · lore

Re: [PATCH 1/7] push: return reject reasons via a mask

Chris Rorvick <chris@rorvick.com> writes:
Show 5 quoted lines
> Pass all rejection reasons back from transport_push().  The logic is
> simpler and more flexible with regard to providing useful feedback.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
> ---
Thanks for a reroll.

I do not think they are "masks", by the way. They are set of flags (i.e. a bitset).

A bitset is often called "a mask" when it is used to "mask" a subset of bits in another bitset that has the real information, either to ignore irrelevant bits or to pick only the relevant bits from the latter. And your "reject_mask" is never used as a mask in this patch---it is the bitset that has the real information and it gets masked by constant masks like REJECT_NON_FF_HEAD.

In any case, naming it as "reject_mask" is like calling a counter as "counter_int". It is more important to name it after its purpose than after its type, and because this is to record the reasons why the push was rejected, "rejection_reason" might be a better name for it.

Chris Rorvick· Nov 27, 2012, 03:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/7] push: return reject reasons via a mask

On Mon, Nov 26, 2012 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> Chris Rorvick <chris@rorvick.com> writes:
>
>> Pass all rejection reasons back from transport_push().  The logic is
>> simpler and more flexible with regard to providing useful feedback.
>>
>> Signed-off-by: Chris Rorvick <chris@rorvick.com>
>> ---
>
> In any case, naming it as "reject_mask" is like calling a counter as
> "counter_int".  It is more important to name it after its purpose
> than after its type
Agreed.
> and because this is to record the reasons why
> the push was rejected, "rejection_reason" might be a better name for
> it.
Yes, that is better for all the reasons you mention.  I will fix this up.
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

[PATCH 2/7] push: add advice for rejected tag reference

Advising the user to fetch and merge only makes sense if the rejected reference is a branch. If none of the rejections are for branches, just tell the user the reference already exists.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 builtin/push.c | 11 +++++++++++
 cache.h        |  1 +
 remote.c       | 10 ++++++++++
 transport.c    |  2 ++
 transport.h    |  1 +
 5 files changed, 25 insertions(+)
Show changes to 5 files +25 −0

builtin/push.c, cache.h, remote.c, transport.c, transport.h

diff --git a/builtin/push.c b/builtin/push.c
index 4a0e7ef..1391983 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] =
 	   "(e.g. 'git pull') before pushing again.\n"
 	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
 
+static const char message_advice_ref_already_exists[] =
+	N_("Updates were rejected because the destination reference already exists\n"
+	   "in the remote and the update is not a fast-forward.");
+
 static void advise_pull_before_push(void)
 {
 	if (!advice_push_non_ff_current || !advice_push_nonfastforward)
@@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void)
 	advise(_(message_advice_checkout_pull_push));
 }
 
+static void advise_ref_already_exists(void)
+{
+	advise(_(message_advice_ref_already_exists));
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -272,6 +281,8 @@ static int push_with_options(struct transport *transport, int flags)
 			advise_use_upstream();
 		else
 			advise_checkout_pull_push();
+	} else if (reject_mask & REJECT_ALREADY_EXISTS) {
+		advise_ref_already_exists();
 	}
 
 	return 1;
diff --git a/cache.h b/cache.h
index dbd8018..d72b64d 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,6 +1002,7 @@ struct ref {
 	unsigned int force:1,
 		merge:1,
 		nonfastforward:1,
+		not_forwardable:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 04fd9ea..5101683 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,6 +1279,14 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	return 0;
 }
 
+static inline int is_forwardable(struct ref* ref)
+{
+	if (!prefixcmp(ref->name, "refs/tags/"))
+		return 0;
+
+	return 1;
+}
+
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	int force_update)
 {
@@ -1316,6 +1324,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     always allowed.
 		 */
 
+		ref->not_forwardable = !is_forwardable(ref);
+
 		ref->nonfastforward =
 			!ref->deletion &&
 			!is_null_sha1(ref->old_sha1) &&
diff --git a/transport.c b/transport.c
index b0c9f1b..271965e 100644
--- a/transport.c
+++ b/transport.c
@@ -740,6 +740,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
+			if (ref->not_forwardable)
+				*reject_mask |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
 				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
diff --git a/transport.h b/transport.h
index 5f76ca2..7e86352 100644
--- a/transport.h
+++ b/transport.h
@@ -142,6 +142,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 
 #define REJECT_NON_FF_HEAD     0x01
 #define REJECT_NON_FF_OTHER    0x02
+#define REJECT_ALREADY_EXISTS  0x04
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,

-- 
1.8.0.209.gf3828dc
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

[PATCH 3/7] push: flag updates

If the reference exists on the remote and the update is not a delete, then mark as an update. This is in preparation for handling tags and branches differently when pushing.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 cache.h  |  1 +
 remote.c | 18 +++++++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)
Show changes to 2 files +12 −7

cache.h, remote.c

diff --git a/cache.h b/cache.h
index d72b64d..722321c 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,6 +1003,7 @@ struct ref {
 		merge:1,
 		nonfastforward:1,
 		not_forwardable:1,
+		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 5101683..07040b8 100644
--- a/remote.c
+++ b/remote.c
@@ -1326,15 +1326,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 		ref->not_forwardable = !is_forwardable(ref);
 
-		ref->nonfastforward =
+		ref->update =
 			!ref->deletion &&
-			!is_null_sha1(ref->old_sha1) &&
-			(!has_sha1_file(ref->old_sha1)
-			  || !ref_newer(ref->new_sha1, ref->old_sha1));
+			!is_null_sha1(ref->old_sha1);
 
-		if (ref->nonfastforward && !ref->force && !force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-			continue;
+		if (ref->update) {
+			ref->nonfastforward =
+				!has_sha1_file(ref->old_sha1)
+				  || !ref_newer(ref->new_sha1, ref->old_sha1);
+
+			if (ref->nonfastforward && !ref->force && !force_update) {
+				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+				continue;
+			}
 		}
 	}
 }

-- 
1.8.0.209.gf3828dc
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

Add a flag for indicating an update to a reference requires force. Currently the nonfastforward flag of a ref is used for this when generating status the status message. A separate flag insulates the status logic from the details of set_ref_status_for_push().

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 cache.h     |  4 +++-
 remote.c    | 11 ++++++++---
 transport.c |  2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)
Show changes to 3 files +12 −5

cache.h, remote.c, transport.c

diff --git a/cache.h b/cache.h
index 722321c..b7ab4ac 100644
--- a/cache.h
+++ b/cache.h
@@ -999,7 +999,9 @@ struct ref {
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char *symref;
-	unsigned int force:1,
+	unsigned int
+		force:1,
+		requires_force:1,
 		merge:1,
 		nonfastforward:1,
 		not_forwardable:1,
diff --git a/remote.c b/remote.c
index 07040b8..4a6f822 100644
--- a/remote.c
+++ b/remote.c
@@ -1293,6 +1293,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	struct ref *ref;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
+		int force_ref_update = ref->force || force_update;
+
 		if (ref->peer_ref)
 			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
 		else if (!send_mirror)
@@ -1335,9 +1337,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 				!has_sha1_file(ref->old_sha1)
 				  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-			if (ref->nonfastforward && !ref->force && !force_update) {
-				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-				continue;
+			if (ref->nonfastforward) {
+				ref->requires_force = 1;
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+					continue;
+				}
 			}
 		}
 	}
diff --git a/transport.c b/transport.c
index 271965e..ea8bbbd 100644
--- a/transport.c
+++ b/transport.c
@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
 		const char *msg;
 
 		strcpy(quickref, status_abbrev(ref->old_sha1));
-		if (ref->nonfastforward) {
+		if (ref->requires_force) {
 			strcat(quickref, "...");
 			type = '+';
 			msg = "forced update";

-- 
1.8.0.209.gf3828dc
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

[PATCH 5/7] push: require force for refs under refs/tags/

References are allowed to update from one commit-ish to another if the former is an ancestor of the latter. This behavior is oriented to branches which are expected to move with commits. Tag references are expected to be static in a repository, though, thus an update to something under refs/tags/ should be rejected unless the update is forced.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 Documentation/git-push.txt | 11 ++++++-----
 builtin/push.c             |  2 +-
 builtin/send-pack.c        |  5 +++++
 cache.h                    |  1 +
 remote.c                   | 18 ++++++++++++++----
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 23 ++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                |  8 ++++++--
 9 files changed, 62 insertions(+), 13 deletions(-)
Show changes to 9 files +62 −13

Documentation/git-push.txt, builtin/push.c, builtin/send-pack.c, cache.h, remote.c, send-pack.c, t/t5516-fetch-push.sh, transport-helper.c, transport.c

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..09bdec7 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,12 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
 updated.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side, but by default this is only allowed if the
-update can fast-forward <dst>.  By having the optional leading `+`,
-you can tell git to update the <dst> ref even when the update is not a
-fast-forward.  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side.  By default this is only allowed if <dst> is not
+under refs/tags/, and then only if it can fast-forward <dst>.  By having
+the optional leading `+`, you can tell git to update the <dst> ref even
+if it is not allowed by default (e.g., it is not a fast-forward.)  This
+does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
+details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/builtin/push.c b/builtin/push.c
index 1391983..2143833 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] =
 
 static const char message_advice_ref_already_exists[] =
 	N_("Updates were rejected because the destination reference already exists\n"
-	   "in the remote and the update is not a fast-forward.");
+	   "in the remote.");
 
 static void advise_pull_before_push(void)
 {
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fda28bc..1eabf42 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
+			res = "error";
+			msg = "already exists";
+			break;
+
 		case REF_STATUS_REJECT_NODELETE:
 		case REF_STATUS_REMOTE_REJECT:
 			res = "error";
diff --git a/cache.h b/cache.h
index b7ab4ac..a32a0ea 100644
--- a/cache.h
+++ b/cache.h
@@ -1011,6 +1011,7 @@ struct ref {
 		REF_STATUS_NONE = 0,
 		REF_STATUS_OK,
 		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_ALREADY_EXISTS,
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/remote.c b/remote.c
index 4a6f822..012b52f 100644
--- a/remote.c
+++ b/remote.c
@@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *
 		 * (1) if the old thing does not exist, it is OK.
 		 *
-		 * (2) if you do not have the old thing, you are not allowed
+		 * (2) if the destination is under refs/tags/ you are
+		 *     not allowed to overwrite it; tags are expected
+		 *     to be static once created
+		 *
+		 * (3) if you do not have the old thing, you are not allowed
 		 *     to overwrite it; you would not know what you are losing
 		 *     otherwise.
 		 *
-		 * (3) if both new and old are commit-ish, and new is a
+		 * (4) if both new and old are commit-ish, and new is a
 		 *     descendant of old, it is OK.
 		 *
-		 * (4) regardless of all of the above, removing :B is
+		 * (5) regardless of all of the above, removing :B is
 		 *     always allowed.
 		 */
 
@@ -1337,7 +1341,13 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 				!has_sha1_file(ref->old_sha1)
 				  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-			if (ref->nonfastforward) {
+			if (ref->not_forwardable) {
+				ref->requires_force = 1;
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+					continue;
+				}
+			} else if (ref->nonfastforward) {
 				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
diff --git a/send-pack.c b/send-pack.c
index f50dfd9..1c375f0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -229,6 +229,7 @@ int send_pack(struct send_pack_args *args,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '
 		git branch -D frotz
 	fi &&
 	git tag -f frotz &&
-	git push testrepo frotz &&
+	git push -f testrepo frotz &&
 	check_push_result $the_commit tags/frotz &&
 	check_push_result $the_first_commit heads/frotz
 
@@ -929,6 +929,27 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
+test_expect_success 'push requires --force to update lightweight tag' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git tag Tag &&
+		git push ../child2 Tag &&
+		git push ../child2 Tag &&
+		>file1 &&
+		git add file1 &&
+		git commit -m "file1" &&
+		git tag -f Tag &&
+		test_must_fail git push ../child2 Tag &&
+		git push --force ../child2 Tag &&
+		git tag -f Tag &&
+		test_must_fail git push ../child2 Tag HEAD~ &&
+		git push --force ../child2 Tag
+	)
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
diff --git a/transport-helper.c b/transport-helper.c
index 4713b69..965b778 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -661,6 +661,11 @@ static void push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "already exists")) {
+			status = REF_STATUS_REJECT_ALREADY_EXISTS;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
@@ -720,6 +725,7 @@ static int push_refs_with_push(struct transport *transport,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport.c b/transport.c
index ea8bbbd..a380ad7 100644
--- a/transport.c
+++ b/transport.c
@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast-forward", porcelain);
 		break;
+	case REF_STATUS_REJECT_ALREADY_EXISTS:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "already exists", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -740,12 +744,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
-			if (ref->not_forwardable)
-				*reject_mask |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
 				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
 				*reject_mask |= REJECT_NON_FF_OTHER;
+		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
+			*reject_mask |= REJECT_ALREADY_EXISTS;
 		}
 	}
 }

-- 
1.8.0.209.gf3828dc
Junio C Hamano· Nov 26, 2012, 18:57 UTC · re: Chris Rorvick · lore

Re: [PATCH 5/7] push: require force for refs under refs/tags/

Chris Rorvick <chris@rorvick.com> writes:
Show 25 quoted lines
> diff --git a/remote.c b/remote.c
> index 4a6f822..012b52f 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>  		 *
>  		 * (1) if the old thing does not exist, it is OK.
>  		 *
> -		 * (2) if you do not have the old thing, you are not allowed
> +		 * (2) if the destination is under refs/tags/ you are
> +		 *     not allowed to overwrite it; tags are expected
> +		 *     to be static once created
> +		 *
> +		 * (3) if you do not have the old thing, you are not allowed
>  		 *     to overwrite it; you would not know what you are losing
>  		 *     otherwise.
>  		 *
> -		 * (3) if both new and old are commit-ish, and new is a
> +		 * (4) if both new and old are commit-ish, and new is a
>  		 *     descendant of old, it is OK.
>  		 *
> -		 * (4) regardless of all of the above, removing :B is
> +		 * (5) regardless of all of the above, removing :B is
>  		 *     always allowed.
>  		 */

We may want to reword (0) to make it clear that --force (and +A:B) can be used to defeat all the other rules.

The updated logic in the patch looks sensible.  Thanks.
Show 22 quoted lines
> ...
> +test_expect_success 'push requires --force to update lightweight tag' '
> +	mk_test heads/master &&
> +	mk_child child1 &&
> +	mk_child child2 &&
> +	(
> +		cd child1 &&
> +		git tag Tag &&
> +		git push ../child2 Tag &&
> +		git push ../child2 Tag &&
> +		>file1 &&
> +		git add file1 &&
> +		git commit -m "file1" &&
> +		git tag -f Tag &&
> +		test_must_fail git push ../child2 Tag &&
> +		git push --force ../child2 Tag &&
> +		git tag -f Tag &&
> +		test_must_fail git push ../child2 Tag HEAD~ &&
> +		git push --force ../child2 Tag
> +	)
> +'
> +
Chris Rorvick· Nov 27, 2012, 04:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 5/7] push: require force for refs under refs/tags/

On Mon, Nov 26, 2012 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> Chris Rorvick <chris@rorvick.com> writes:
>
>> diff --git a/remote.c b/remote.c
>> index 4a6f822..012b52f 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>>                *
>>                * (1) if the old thing does not exist, it is OK.
>>                *
>> -              * (2) if you do not have the old thing, you are not allowed
>> +              * (2) if the destination is under refs/tags/ you are
>> +              *     not allowed to overwrite it; tags are expected
>> +              *     to be static once created
>> +              *
>> +              * (3) if you do not have the old thing, you are not allowed
>>                *     to overwrite it; you would not know what you are losing
>>                *     otherwise.
>>                *
>> -              * (3) if both new and old are commit-ish, and new is a
>> +              * (4) if both new and old are commit-ish, and new is a
>>                *     descendant of old, it is OK.
>>                *
>> -              * (4) regardless of all of the above, removing :B is
>> +              * (5) regardless of all of the above, removing :B is
>>                *     always allowed.
>>                */
>
> We may want to reword (0) to make it clear that --force
> (and +A:B) can be used to defeat all the other rules.

On that note, having (5) be "regardless of all of the above ..." seems a little awkward. That would seem to fit better towards the top.

Junio C Hamano· Nov 27, 2012, 17:12 UTC · re: Chris Rorvick · lore

Re: [PATCH 5/7] push: require force for refs under refs/tags/

Chris Rorvick <chris@rorvick.com> writes:
Show 34 quoted lines
> On Mon, Nov 26, 2012 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Chris Rorvick <chris@rorvick.com> writes:
>>
>>> diff --git a/remote.c b/remote.c
>>> index 4a6f822..012b52f 100644
>>> --- a/remote.c
>>> +++ b/remote.c
>>> @@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>>>                *
>>>                * (1) if the old thing does not exist, it is OK.
>>>                *
>>> -              * (2) if you do not have the old thing, you are not allowed
>>> +              * (2) if the destination is under refs/tags/ you are
>>> +              *     not allowed to overwrite it; tags are expected
>>> +              *     to be static once created
>>> +              *
>>> +              * (3) if you do not have the old thing, you are not allowed
>>>                *     to overwrite it; you would not know what you are losing
>>>                *     otherwise.
>>>                *
>>> -              * (3) if both new and old are commit-ish, and new is a
>>> +              * (4) if both new and old are commit-ish, and new is a
>>>                *     descendant of old, it is OK.
>>>                *
>>> -              * (4) regardless of all of the above, removing :B is
>>> +              * (5) regardless of all of the above, removing :B is
>>>                *     always allowed.
>>>                */
>>
>> We may want to reword (0) to make it clear that --force
>> (and +A:B) can be used to defeat all the other rules.
>
> On that note, having (5) be "regardless of all of the above ..." seems
> a little awkward.  That would seem to fit better towards the top.

Sure. (0) you can always force; (1) you can always delete; and then other requirements. That may indeed read better.

Thanks.
Chris Rorvick· Nov 28, 2012, 05:18 UTC · re: Junio C Hamano · lore

[PATCH] push: cleanup push rules comment

---

I ended up rewriting most of the comment. The new version removes inter-rule dependencies (e.g., rule 5 overrides rule 3) which I think makes it more readable.

This patch applies on top of the latest patch series regarding pushing tags. If will include this in a re-roll of that series if these changes are deemed a good idea.

Also, I hand-edited the patch so that the changes were not interleaved to make it much easier to read. Can this be done automatically? Something like a minimum # of matching lines required between differences?

Chris
 remote.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)
Show changes to remote.c +23 −21
diff --git a/remote.c b/remote.c
index ee0c1e5..3fb1068 100644
--- a/remote.c
+++ b/remote.c
@@ -1319,27 +1319,29 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			continue;
 		}
 
-		/* This part determines what can overwrite what.
-		 * The rules are:
-		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
-		 *
-		 * (1) if the old thing does not exist, it is OK.
-		 *
-		 * (2) if the destination is under refs/tags/ you are
-		 *     not allowed to overwrite it; tags are expected
-		 *     to be static once created
-		 *
-		 * (3) if you do not have the old thing, you are not allowed
-		 *     to overwrite it; you would not know what you are losing
-		 *     otherwise.
-		 *
-		 * (4) if old is a commit and new is a descendant of old
-		 *     (implying new is commit-ish), it is OK.
-		 *
-		 * (5) regardless of all of the above, removing :B is
-		 *     always allowed.
+		/*
+		 * The below logic determines whether an individual
+		 * refspec A:B can be pushed.  The push will succeed
+		 * if any of the following are true:
+		 *
+		 * (1) the remote reference B does not exist
+		 *
+		 * (2) the remote reference B is being removed (i.e.
+		 *     pushing :B where no source is specified)
+		 *
+		 * (3) the update meets all fast-forwarding criteria:
+		 *
+		 *     (a) the destination is not under refs/tags/
+		 *     (b) the old is a commit
+		 *     (c) the new is a descendant of the old
+		 *
+		 *     NOTE: We must actually have the old object in
+		 *     order to overwrite it in the remote reference,
+		 *     and that the new object must be commit-ish.
+		 *     These are implied by (b) and (c) respectively.
+		 *
+		 * (4) it is forced using the +A:B notation, or by
+		 *     passing the --force argument
 		 */
 
 		ref->not_forwardable = !is_forwardable(ref);

-- 
1.8.0.209.gf3828dc
Junio C Hamano· Nov 28, 2012, 16:58 UTC · re: Chris Rorvick · lore

Re: [PATCH] push: cleanup push rules comment

Chris Rorvick <chris@rorvick.com> writes:
Show 5 quoted lines
> ---
>
> I ended up rewriting most of the comment.  The new version removes
> inter-rule dependencies (e.g., rule 5 overrides rule 3) which I think
> makes it more readable.
Nice; thanks.
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

[PATCH 6/7] push: require force for annotated tags

Do not allow fast-forwarding of references that point to a tag object. This keeps the behavior consistent with lightweight tags. Additionally, allowing the reference to update could leave the old object dangling.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 Documentation/git-push.txt | 10 +++++-----
 remote.c                   | 11 +++++++++--
 t/t5516-fetch-push.sh      | 21 +++++++++++++++++++++
 3 files changed, 35 insertions(+), 7 deletions(-)
Show changes to 3 files +35 −7

Documentation/git-push.txt, remote.c, t/t5516-fetch-push.sh

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 09bdec7..7a04ce5 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -52,11 +52,11 @@ updated.
 +
 The object referenced by <src> is used to update the <dst> reference
 on the remote side.  By default this is only allowed if <dst> is not
-under refs/tags/, and then only if it can fast-forward <dst>.  By having
-the optional leading `+`, you can tell git to update the <dst> ref even
-if it is not allowed by default (e.g., it is not a fast-forward.)  This
-does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
-details.
+a tag (annotated or lightweight), and then only if it can fast-forward
+<dst>.  By having the optional leading `+`, you can tell git to update
+the <dst> ref even if it is not allowed by default (e.g., it is not a
+fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See
+EXAMPLES below for details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/remote.c b/remote.c
index 012b52f..f5bc4e7 100644
--- a/remote.c
+++ b/remote.c
@@ -1281,9 +1281,16 @@ int match_push_refs(struct ref *src, struct ref **dst,
 
 static inline int is_forwardable(struct ref* ref)
 {
+	struct object *o;
+
 	if (!prefixcmp(ref->name, "refs/tags/"))
 		return 0;
 
+	/* old object must be a commit */
+	o = parse_object(ref->old_sha1);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+
 	return 1;
 }
 
@@ -1323,8 +1330,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     to overwrite it; you would not know what you are losing
 		 *     otherwise.
 		 *
-		 * (4) if both new and old are commit-ish, and new is a
-		 *     descendant of old, it is OK.
+		 * (4) if old is a commit and new is a descendant of old
+		 *     (implying new is commit-ish), it is OK.
 		 *
 		 * (5) regardless of all of the above, removing :B is
 		 *     always allowed.
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8f024a0..6009372 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,6 +950,27 @@ test_expect_success 'push requires --force to update lightweight tag' '
 	)
 '
 
+test_expect_success 'push requires --force to update annotated tag' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git tag -a -m "message 1" Tag &&
+		git push ../child2 Tag:refs/tmp/Tag &&
+		git push ../child2 Tag:refs/tmp/Tag &&
+		>file1 &&
+		git add file1 &&
+		git commit -m "file1" &&
+		git tag -f -a -m "message 2" Tag &&
+		test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
+		git push --force ../child2 Tag:refs/tmp/Tag &&
+		git tag -f -a -m "message 3" Tag HEAD~ &&
+		test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
+		git push --force ../child2 Tag:refs/tmp/Tag
+	)
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&

-- 
1.8.0.209.gf3828dc
Chris Rorvick· Nov 23, 2012, 04:21 UTC · re: Chris Rorvick · lore

[PATCH 7/7] push: clarify rejection of update to non-commit-ish

Pushes must already (by default) update to a commit-ish due the fast- forward check in set_ref_status_for_push(). But rejecting for not being a fast-forward suggests the situation can be resolved with a merge. Flag these updates (i.e., to a blob or a tree) as not forwardable so the user is presented with more appropriate advice.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 remote.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to remote.c +5 −0
diff --git a/remote.c b/remote.c
index f5bc4e7..ee0c1e5 100644
--- a/remote.c
+++ b/remote.c
@@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)
 	if (!o || o->type != OBJ_COMMIT)
 		return 0;
 
+	/* new object must be commit-ish */
+	o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+
 	return 1;
 }
 

-- 
1.8.0.209.gf3828dc
Junio C Hamano· Nov 26, 2012, 18:53 UTC · re: Chris Rorvick · lore

Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish

Chris Rorvick <chris@rorvick.com> writes:
Show 24 quoted lines
> Pushes must already (by default) update to a commit-ish due the fast-
> forward check in set_ref_status_for_push().  But rejecting for not being
> a fast-forward suggests the situation can be resolved with a merge.
> Flag these updates (i.e., to a blob or a tree) as not forwardable so the
> user is presented with more appropriate advice.
>
> Signed-off-by: Chris Rorvick <chris@rorvick.com>
> ---
>  remote.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/remote.c b/remote.c
> index f5bc4e7..ee0c1e5 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)
>  	if (!o || o->type != OBJ_COMMIT)
>  		return 0;
>  
> +	/* new object must be commit-ish */
> +	o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
> +	if (!o || o->type != OBJ_COMMIT)
> +		return 0;
> +

I think the original code used ref_newer() which took commit-ish on both sides.

With this code, the old must be a commit but new can be a tag that points at a commit? Why?

Chris Rorvick· Nov 27, 2012, 03:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish

On Mon, Nov 26, 2012 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 29 quoted lines
> Chris Rorvick <chris@rorvick.com> writes:
>
>> Pushes must already (by default) update to a commit-ish due the fast-
>> forward check in set_ref_status_for_push().  But rejecting for not being
>> a fast-forward suggests the situation can be resolved with a merge.
>> Flag these updates (i.e., to a blob or a tree) as not forwardable so the
>> user is presented with more appropriate advice.
>>
>> Signed-off-by: Chris Rorvick <chris@rorvick.com>
>> ---
>>  remote.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/remote.c b/remote.c
>> index f5bc4e7..ee0c1e5 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)
>>       if (!o || o->type != OBJ_COMMIT)
>>               return 0;
>>
>> +     /* new object must be commit-ish */
>> +     o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
>> +     if (!o || o->type != OBJ_COMMIT)
>> +             return 0;
>> +
>
> I think the original code used ref_newer() which took commit-ish on
> both sides.

That is still called later in set_ref_status_for_push() to calculate the nonfastforward flag. The only reason for even checking the new here is to exclude trees and blobs now so they are flagged as already-existing and thus avoid nonsensical fetch-and-merge advice. Otherwise the behavior is unchanged by this last patch.

ref_newer() does end up redoing computation now done in the new is_forwardable() function. I could probably factor this out of ref_newer() into a commit_newer() function that could be reused in set_ref_status_for_push() to avoid this overhead, but it didn't seem like a big deal. Thoughts?

> With this code, the old must be a commit but new can be a tag that
> points at a commit?  Why?

The old must not be a tag because fast-forwarding from it is potentially destructive; a tag would likely be left dangling in this case. This is not true for the new, though. I'm not sure fast-forwarding from a commit to a tag is useful, but I didn't see a reason to prevent it either. The refs/tags/ hierarchy is excluded from fast-forwarding before this check, and refs/heads/ is already protected against anything but commits. So it seems very unlikely that someone would accidentally make use of this behavior.

So, fast-forwarding to a tag seemed fairly benign and unlikely to cause confusion, so I leaned towards allowing it in case someone found a use case for it.

Junio C Hamano· Nov 27, 2012, 17:11 UTC · re: Chris Rorvick · lore

Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish

Chris Rorvick <chris@rorvick.com> writes:
Show 15 quoted lines
>> With this code, the old must be a commit but new can be a tag that
>> points at a commit?  Why?
>
> The old must not be a tag because fast-forwarding from it is
> potentially destructive; a tag would likely be left dangling in this
> case.  This is not true for the new, though.   I'm not sure
> fast-forwarding from a commit to a tag is useful, but I didn't see a
> reason to prevent it either.   The refs/tags/ hierarchy is excluded
> from fast-forwarding before this check, and refs/heads/ is already
> protected against anything but commits.  So it seems very unlikely
> that someone would accidentally make use of this behavior.
>
> So, fast-forwarding to a tag seemed fairly benign and unlikely to
> cause confusion, so I leaned towards allowing it in case someone found
> a use case for it.
Sounds sensible.

Perhaps some of that thinking behind this change (i.e. earlier we checked the forwardee was any commit-ish, but the new code only allows a commit object if it were to be fast-forwarded) belongs to the log message?

Thanks.

← back to recent threads