[PATCH v4 0/5] push: update remote tags only with force

[PATCH v4 0/5] push: update remote tags only with force

[Chris Rorvick]

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: update remote tags only with force

     This is in response to the following thread:

       http://thread.gmane.org/gmane.comp.version-control.git/208354

     This solution prevents fast-forwards if the reference is of the
     refs/tags/* hierarchy or if the old object is not a commit.

These patches contain the following updates since the v3 set:

  * builtin/push.c: Remove "push --force" suggestion from advice.
  * remote.c: Only require old object to be a commit to be forwardable.
      I added the check for object types based comments from Peff in
      original thread, and I think this implementation is actually what
      he intended.  If the new object is a tag, the operation is not
      destructive so there is no reason to block it (at least within
      the scope of these changes) as was done in the previous iteration.
  * t/t5516-fetch-push.sh: Create separate tests for the lightweight and
      annotated cases.  Do the annotated tests outside of refs/tags/
      so that it actually tests different functionality.

Chris Rorvick (5):
  push: return reject reasons via a mask
  push: add advice for rejected tag reference
  push: flag updates
  push: flag updates that require force
  push: update remote tags only with force

 Documentation/git-push.txt | 10 +++++-----
 builtin/push.c             | 24 +++++++++++++++---------
 builtin/send-pack.c        |  9 +++++++--
 cache.h                    |  7 ++++++-
 remote.c                   | 46 ++++++++++++++++++++++++++++++++++++----------
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                | 25 +++++++++++++++----------
 transport.h                | 10 ++++++----
 10 files changed, 126 insertions(+), 42 deletions(-)

-- 
1.8.0.155.g3a063ad.dirty

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

[Chris Rorvick]

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

diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..eaeaf7e 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 & NON_FF_HEAD) {
 		advise_pull_before_push();
-		break;
-	case NON_FF_OTHER:
+	} else if (reject_mask & 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..ae9fda8 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 |= NON_FF_HEAD;
 			else
-				*nonfastforward = NON_FF_OTHER;
+				*reject_mask |= 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..1f9699c 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 NON_FF_HEAD     0x01
+#define 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.155.g3a063ad.dirty

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

[Chris Rorvick]

Advising the user to fetch and merge only makes sense if the rejected
reference is a branch.  If none of the rejections were for branches,
tell the user they need to force the update(s).

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 builtin/push.c | 15 +++++++++++++--
 cache.h        |  1 +
 remote.c       |  2 ++
 transport.c    |  6 ++++--
 transport.h    |  5 +++--
 5 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index eaeaf7e..0e13e11 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 a matching reference already exists in\n"
+	   "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;
@@ -265,13 +274,15 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	if (reject_mask & NON_FF_HEAD) {
+	if (reject_mask & REJECT_NON_FF_HEAD) {
 		advise_pull_before_push();
-	} else if (reject_mask & NON_FF_OTHER) {
+	} else if (reject_mask & REJECT_NON_FF_OTHER) {
 		if (default_matching_used)
 			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..82dead1 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,6 +1002,7 @@ struct ref {
 	unsigned int force:1,
 		merge:1,
 		nonfastforward:1,
+		is_a_tag:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 04fd9ea..186814d 100644
--- a/remote.c
+++ b/remote.c
@@ -1316,6 +1316,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     always allowed.
 		 */
 
+		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
+
 		ref->nonfastforward =
 			!ref->deletion &&
 			!is_null_sha1(ref->old_sha1) &&
diff --git a/transport.c b/transport.c
index ae9fda8..5fcaea8 100644
--- a/transport.c
+++ b/transport.c
@@ -740,10 +740,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->is_a_tag)
+				*reject_mask |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
-				*reject_mask |= NON_FF_HEAD;
+				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
-				*reject_mask |= NON_FF_OTHER;
+				*reject_mask |= REJECT_NON_FF_OTHER;
 		}
 	}
 }
diff --git a/transport.h b/transport.h
index 1f9699c..7e86352 100644
--- a/transport.h
+++ b/transport.h
@@ -140,8 +140,9 @@ 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     0x01
-#define NON_FF_OTHER    0x02
+#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.155.g3a063ad.dirty

[PATCH v4 3/5] push: flag updates

[Chris Rorvick]

If the reference exists on the remote and the 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(-)

diff --git a/cache.h b/cache.h
index 82dead1..0f53d11 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,6 +1003,7 @@ struct ref {
 		merge:1,
 		nonfastforward:1,
 		is_a_tag:1,
+		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 186814d..fe89869 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,15 +1318,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
 
-		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.155.g3a063ad.dirty

[PATCH v4 4/5] push: flag updates that require force

[Chris Rorvick]

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

diff --git a/cache.h b/cache.h
index 0f53d11..f410d94 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,
 		is_a_tag:1,
diff --git a/remote.c b/remote.c
index fe89869..1e263b0 100644
--- a/remote.c
+++ b/remote.c
@@ -1285,6 +1285,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)
@@ -1327,9 +1329,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 5fcaea8..60a7421 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.155.g3a063ad.dirty

[PATCH v4 5/5] push: update remote tags only with force

[Chris Rorvick]

References are allowed to update from one commit-ish to another if the
former is a 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 a
tag (lightweight and annotated) should be rejected unless the update is
forced.

To enable this functionality, the following checks have been added to
set_ref_status_for_push() for updating refs (i.e, not new or deletion)
to restrict fast-forwarding in pushes:

  1) The old and new references must be commits.  If this fails,
     it is not a valid update for a branch.

  2) The reference name cannot start with "refs/tags/".  This
     catches lightweight tags which (usually) point to commits
     and therefore would not be caught by (1).

If either of these checks fails, then it is flagged (by default) with a
status indicating the update is being rejected due to the reference
already existing in the remote.  This can be overridden by passing
--force to git push.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 Documentation/git-push.txt | 10 +++++-----
 builtin/push.c             |  2 +-
 builtin/send-pack.c        |  5 +++++
 cache.h                    |  3 ++-
 remote.c                   | 23 +++++++++++++++++++----
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                |  8 ++++++--
 9 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..479e25f 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,11 @@ 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 the update is
+a branch, and then only if it 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 branch or 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 0e13e11..cd7aa3f 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 a matching reference already exists in\n"
-	   "the remote and the update is not a fast-forward.");
+	   "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 f410d94..127e504 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,13 +1004,14 @@ struct ref {
 		requires_force:1,
 		merge:1,
 		nonfastforward:1,
-		is_a_tag:1,
+		forwardable:1,
 		update:1,
 		deletion:1;
 	enum {
 		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 1e263b0..4fcd22c 100644
--- a/remote.c
+++ b/remote.c
@@ -1311,14 +1311,23 @@ 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.
 		 *
-		 * (3) if both new and old are commit-ish, and new is a
-		 *     descendant of old, it is OK.
+		 * (3) if new is commit-ish and old is a commit, new is a
+		 *     descendant of old, and the reference is not of the
+		 *     refs/tags/ hierarchy it is OK.
 		 *
 		 * (4) regardless of all of the above, removing :B is
 		 *     always allowed.
 		 */
 
-		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
+		if (prefixcmp(ref->name, "refs/tags/")) {
+			// Additionally, disallow fast-forwarding if
+			// the old object is not a commit.  This kicks
+			// out annotated tags that might pass the
+			// is_newer() test but dangle if the reference
+			// is updated.
+			struct object *obj = parse_object(ref->old_sha1);
+			ref->forwardable = !obj || obj->type == OBJ_COMMIT;
+		}
 
 		ref->update =
 			!ref->deletion &&
@@ -1329,7 +1338,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->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..afb9b1b 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,34 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
+test_expect_success 'push tag requires --force to update remote tag' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git tag lw_tag &&
+		git tag -a -m "message 1" ann_tag &&
+		git push ../child2 lw_tag &&
+		git push ../child2 ann_tag &&
+		>file1 &&
+		git add file1 &&
+		git commit -m "file1" &&
+		git tag -f lw_tag &&
+		git tag -f -a -m "message 2" ann_tag &&
+		test_must_fail git push ../child2 lw_tag &&
+		test_must_fail git push ../child2 ann_tag &&
+		git push --force ../child2 lw_tag &&
+		git push --force ../child2 ann_tag &&
+		git tag -f lw_tag HEAD~ &&
+		git tag -f -a -m "message 3" ann_tag &&
+		test_must_fail git push ../child2 lw_tag &&
+		test_must_fail git push ../child2 ann_tag &&
+		git push --force ../child2 lw_tag &&
+		git push --force ../child2 ann_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 60a7421..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->is_a_tag)
-				*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.155.g3a063ad.dirty

[PATCH v4.1 5/5] push: update remote tags only with force

[Chris Rorvick]

References are allowed to update from one commit-ish to another if the
former is a 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 a
tag (lightweight and annotated) should be rejected unless the update is
forced.

To enable this functionality, the following checks have been added to
set_ref_status_for_push() for updating refs (i.e, not new or deletion)
to restrict fast-forwarding in pushes:

  1) The old and new references must be commits.  If this fails,
     it is not a valid update for a branch.

  2) The reference name cannot start with "refs/tags/".  This
     catches lightweight tags which (usually) point to commits
     and therefore would not be caught by (1).

If either of these checks fails, then it is flagged (by default) with a
status indicating the update is being rejected due to the reference
already existing in the remote.  This can be overridden by passing
--force to git push.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---

Fix C99 comment.

 Documentation/git-push.txt | 10 +++++-----
 builtin/push.c             |  2 +-
 builtin/send-pack.c        |  5 +++++
 cache.h                    |  3 ++-
 remote.c                   | 24 ++++++++++++++++++++----
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 42 +++++++++++++++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                |  8 ++++++--
 9 files changed, 87 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..479e25f 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,11 @@ 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 the update is
+a branch, and then only if it 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 branch or 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 0e13e11..cd7aa3f 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 a matching reference already exists in\n"
-	   "the remote and the update is not a fast-forward.");
+	   "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 f410d94..127e504 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,13 +1004,14 @@ struct ref {
 		requires_force:1,
 		merge:1,
 		nonfastforward:1,
-		is_a_tag:1,
+		forwardable:1,
 		update:1,
 		deletion:1;
 	enum {
 		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 44e72d6..a723f7a 100644
--- a/remote.c
+++ b/remote.c
@@ -1311,14 +1311,24 @@ 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.
 		 *
-		 * (3) if both new and old are commit-ish, and new is a
-		 *     descendant of old, it is OK.
+		 * (3) if new is commit-ish and old is a commit, new is a
+		 *     descendant of old, and the reference is not of the
+		 *     refs/tags/ hierarchy it is OK.
 		 *
 		 * (4) regardless of all of the above, removing :B is
 		 *     always allowed.
 		 */
 
-		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
+		if (prefixcmp(ref->name, "refs/tags/")) {
+			/* Additionally, disallow fast-forwarding if
+			 * the old object is not a commit.  This kicks
+			 * out annotated tags that might pass the
+			 * is_newer() test but dangle if the reference
+			 * is updated.
+			 */
+			struct object *obj = parse_object(ref->old_sha1);
+			ref->forwardable = !obj || obj->type == OBJ_COMMIT;
+		}
 
 		ref->update =
 			!ref->deletion &&
@@ -1329,7 +1339,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->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..ca800b2 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,46 @@ 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 &&
+		>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 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 &&
+		>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" &&
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 60a7421..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->is_a_tag)
-				*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.7.11.7

Re: [PATCH v4.1 5/5] push: update remote tags only with force

[Junio C Hamano]

Chris Rorvick <chris@rorvick.com> writes:

> References are allowed to update from one commit-ish to another if the
> former is a 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 a
> tag (lightweight and annotated) should be rejected unless the update is
> forced.
>
> To enable this functionality, the following checks have been added to
> set_ref_status_for_push() for updating refs (i.e, not new or deletion)
> to restrict fast-forwarding in pushes:
>
>   1) The old and new references must be commits.  If this fails,
>      it is not a valid update for a branch.
>
>   2) The reference name cannot start with "refs/tags/".  This
>      catches lightweight tags which (usually) point to commits
>      and therefore would not be caught by (1).
>
> If either of these checks fails, then it is flagged (by default) with a
> status indicating the update is being rejected due to the reference
> already existing in the remote.  This can be overridden by passing
> --force to git push.

This, if it were implemented back when we first added "git push",
would have been a nice safety, but added after 1.8.0 it would be a
regression, so we would need backward compatibility notes in the
release notes.

Not an objection; just making a mental note.

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index fe46c42..479e25f 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -51,11 +51,11 @@ 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 the update is
> +a branch, and then only if it can fast-forward <dst>.  By having the

I can already see the next person asking "I can update refs/notes
the same way.  The doc says it applies only to the branches.  Is
this a bug?".

> diff --git a/cache.h b/cache.h
> index f410d94..127e504 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1004,13 +1004,14 @@ struct ref {
>  		requires_force:1,
>  		merge:1,
>  		nonfastforward:1,
> -		is_a_tag:1,
> +		forwardable:1,

This is somewhat an unfortunate churn.  Perhaps is_a_tag could have
started its life under its final name in the series?

I am assuming that the struct members will be initialized to 0 (false),
so everything by default is now not forwardable if somebody forgets
to set this flag?

>  	enum {
>  		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 44e72d6..a723f7a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1311,14 +1311,24 @@ 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.
>  		 *
> -		 * (3) if both new and old are commit-ish, and new is a
> -		 *     descendant of old, it is OK.
> +		 * (3) if new is commit-ish and old is a commit, new is a
> +		 *     descendant of old, and the reference is not of the
> +		 *     refs/tags/ hierarchy it is OK.
>  		 *
>  		 * (4) regardless of all of the above, removing :B is
>  		 *     always allowed.
>  		 */

I think this is unreadable.  Isn't this more like

    (1.5) if the destination is inside refs/tags/ and already
          exists, you are not allowed to overwrite it without
          --force or +A:B notation.

early in the rule set?

> -		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
> +		if (prefixcmp(ref->name, "refs/tags/")) {
> +			/* Additionally, disallow fast-forwarding if
> +			 * the old object is not a commit.  This kicks
> +			 * out annotated tags that might pass the
> +			 * is_newer() test but dangle if the reference
> +			 * is updated.
> +			 */

Huh?  prefixcmp() excludes refs/tags/ hierarchy. What is an
annotated tag doing there?  Is this comment outdated???

	/*
         * Also please end the first line of a multi-line
         * comment with '/', '*', and nothing else.
         */

Regarding the other arm of this "if (not in refs/tags/ hierarchy)",
what happens when refs/tags/foo is a reference to a blob or a tree?
This series declares that the point of tag is not to let people to
move it willy-nilly, and I think it is in line with its spirit if
you just rejected non-creation events.

Also, I suspect that you do not even need to have old object locally
when looking at refs/tags/ hierarchy.  "Do not overwrite tags" can
be enforced by only noticing that they already have something; you
do not know what that something actually is to prevent yourself from
overwriting it.  You may have to update the rule (2) in remote.c
around l.1311 for this.

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

Don't you want to make sure that another "git push ../child2 Tag" at
this point, i.e. attempt to update Tag with the same, should succeed
without --force?

Re: [PATCH v4.1 5/5] push: update remote tags only with force

[Chris Rorvick]

On Mon, Nov 19, 2012 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Rorvick <chris@rorvick.com> writes:
>>  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 the update is
>> +a branch, and then only if it can fast-forward <dst>.  By having the
>
> I can already see the next person asking "I can update refs/notes
> the same way.  The doc says it applies only to the branches.  Is
> this a bug?".

Sure, will fix.

>> diff --git a/cache.h b/cache.h
>> index f410d94..127e504 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1004,13 +1004,14 @@ struct ref {
>>               requires_force:1,
>>               merge:1,
>>               nonfastforward:1,
>> -             is_a_tag:1,
>> +             forwardable:1,
>
> This is somewhat an unfortunate churn.  Perhaps is_a_tag could have
> started its life under its final name in the series?
>
> I am assuming that the struct members will be initialized to 0 (false),
> so everything by default is now not forwardable if somebody forgets
> to set this flag?

Yeah, this was one of a few stupid mistakes.  Previously I used
'forwardable' throughout, but that is awkward except in the last
commit since until then everything is allowed to fast-forward and the
flag is only used to output tag-specific advice.  But inverting the
meaning of the flag is dumb, and I didn't even do it right.

But, as I think you're suggesting, it probably makes more sense to use
a flag that prevents fast-forwarding when set.  So maybe
"not_forwardable", or "is_a_tag" => "not_forwardable" if you think the
renaming is a good idea.  Or maybe just "is_a_tag"?  I think the
rename is good because its meaning is more general in the last commit.

>> diff --git a/remote.c b/remote.c
>> index 44e72d6..a723f7a 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1311,14 +1311,24 @@ 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.
>>                *
>> -              * (3) if both new and old are commit-ish, and new is a
>> -              *     descendant of old, it is OK.
>> +              * (3) if new is commit-ish and old is a commit, new is a
>> +              *     descendant of old, and the reference is not of the
>> +              *     refs/tags/ hierarchy it is OK.
>>                *
>>                * (4) regardless of all of the above, removing :B is
>>                *     always allowed.
>>                */
>
> I think this is unreadable.  Isn't this more like
>
>     (1.5) if the destination is inside refs/tags/ and already
>           exists, you are not allowed to overwrite it without
>           --force or +A:B notation.
>
> early in the rule set?

Yes, something like that is much better.

>> -             ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
>> +             if (prefixcmp(ref->name, "refs/tags/")) {
>> +                     /* Additionally, disallow fast-forwarding if
>> +                      * the old object is not a commit.  This kicks
>> +                      * out annotated tags that might pass the
>> +                      * is_newer() test but dangle if the reference
>> +                      * is updated.
>> +                      */
>
> Huh?  prefixcmp() excludes refs/tags/ hierarchy. What is an
> annotated tag doing there?  Is this comment outdated???

I think the comment is accurate, although inverting the meaning of the
flag probably doesn't help the clarity of this change.

What do you mean by "there"?  Annotated tags normally are referenced
via something under refs/tags/, but they could be elsewhere, right?
So what I meant to say was: if the reference is not under refs/tags/
then the starting point has to be a commit for it to be forwardable.

I interpreted parse_object(ref->old_sha1) == NULL as the old thing not
existing (rule 1) but I'm pretty sure that was a mistake.  I should
first test it with is_null_sha1, if true then it doesn't exist.
Otherwise parse_object() returning NULL means we just don't have the
object and therefore should follow rule 2.

> Regarding the other arm of this "if (not in refs/tags/ hierarchy)",
> what happens when refs/tags/foo is a reference to a blob or a tree?
> This series declares that the point of tag is not to let people to
> move it willy-nilly, and I think it is in line with its spirit if
> you just rejected non-creation events.

Nothing under refs/tags/ is updateable and only commits are updateable
outside of that due to the new logic.  The ref_newer() call will
reject updating to blobs and trees, although that probably isn't the
right thing to do.  Previously I was ensuring both old and new objects
were commits if the ref was not under refs/tags/, but it occurred to
me that fast-forwarding from a commit to a tag might be a reasonable
thing to want to do.  But a rejected update to a blob should get the
already-exists advice, not a message suggesting a merge (which is what
you get by relying on ref_newer() to fail.)

> Also, I suspect that you do not even need to have old object locally
> when looking at refs/tags/ hierarchy.  "Do not overwrite tags" can
> be enforced by only noticing that they already have something; you
> do not know what that something actually is to prevent yourself from
> overwriting it.  You may have to update the rule (2) in remote.c
> around l.1311 for this.

Isn't it already doing this?  I think I've either confounded things
with my mistakes or I'm missing this point.

>> +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 &&
>
> Don't you want to make sure that another "git push ../child2 Tag" at
> this point, i.e. attempt to update Tag with the same, should succeed
> without --force?

Yes, that is clearly a good addition.

Thanks for all the feedback.  Sorry I mucked up this series a bit,
tried to get an update out too quickly I guess.

Chris

Re: [PATCH v4.1 5/5] push: update remote tags only with force

[Junio C Hamano]

Chris Rorvick <chris@rorvick.com> writes:

> On Mon, Nov 19, 2012 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Yeah, this was one of a few stupid mistakes.  Previously I used
> 'forwardable' throughout, but that is awkward except in the last
> commit since until then everything is allowed to fast-forward and the
> flag is only used to output tag-specific advice.  But inverting the
> meaning of the flag is dumb, and I didn't even do it right.
>
> But, as I think you're suggesting, it probably makes more sense to use
> a flag that prevents fast-forwarding when set.  So maybe
> "not_forwardable", or "is_a_tag" => "not_forwardable" if you think the
> renaming is a good idea.

Yeah, calling it not-forwardable from the beginning would be a
sensible approach, I would think.

Thanks.