threads / patch / 32086

v3, 5 partspush: update remote tags only with force

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

## tl;dr

17 messages between Nov 12, 2012 and Nov 15, 2012. Diffs are folded; open one to read it.

replies: 16people: 5as markdown or json

Chris Rorvick· Nov 12, 2012, 04:08 UTC · lore

Minor changes since from v2 set. Reposting primarily because I mucked up the Cc: list (again) and hoping to route feedback to the appropriate audience.

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
     An email thread initiated by Angelo Borsotti did not come to a
     consensus on how push should behave with regard to tag references.
     I think a key point is that you currently cannot be sure your push
     will not clobber a tag (lightweight or not) in the remote.  Also, I
     wonder what workflow would rely on this fast-forward feature of
     pushed tag references that would not be better served a branch?
This patch set contains some minor updates from the previous set:
  * remote.c: remove redundant check of ref->update
  * transport.c: remove extraneous tab in indent
  * builtin/send-pack.c: fix call to transport_print_push_status() (per
      feedback from Peff)

Also, rebased against the latest master in git://github.com/peff/git.git to pickup changes in nd/builtin-to-libgit.

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                   | 38 ++++++++++++++++++++++++++++++--------
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                | 25 +++++++++++++++----------
 transport.h                | 10 ++++++----
 10 files changed, 120 insertions(+), 40 deletions(-)
-- 
1.8.0
Chris Rorvick· Nov 12, 2012, 04:08 UTC · re: Chris Rorvick · lore

[PATCH v3 1/5] 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..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
Chris Rorvick· Nov 12, 2012, 04:08 UTC · re: Chris Rorvick · lore

[PATCH v3 2/5] 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 were for branches, tell the user they need to force the update(s).

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 builtin/push.c | 16 ++++++++++++++--
 cache.h        |  1 +
 remote.c       |  7 +++++++
 transport.c    |  6 ++++--
 transport.h    |  5 +++--
 5 files changed, 29 insertions(+), 6 deletions(-)
Show changes to 5 files +29 −6

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

diff --git a/builtin/push.c b/builtin/push.c
index eaeaf7e..77340c0 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,6 +220,11 @@ 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.  Use git push -f if\n"
+	   "you really want to make this update.");
+
 static void advise_pull_before_push(void)
 {
 	if (!advice_push_non_ff_current || !advice_push_nonfastforward)
@@ -241,6 +246,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 +275,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..4e25840 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,6 +1002,7 @@ struct ref {
 	unsigned int force:1,
 		merge:1,
 		nonfastforward:1,
+		forwardable:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 04fd9ea..5ecd58d 100644
--- a/remote.c
+++ b/remote.c
@@ -1316,6 +1316,13 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     always allowed.
 		 */
 
+		if (prefixcmp(ref->name, "refs/tags/")) {
+			struct object *old = parse_object(ref->old_sha1);
+			struct object *new = parse_object(ref->new_sha1);
+			ref->forwardable = (old && new &&
+			  old->type == OBJ_COMMIT && new->type == OBJ_COMMIT);
+		}
+
 		ref->nonfastforward =
 			!ref->deletion &&
 			!is_null_sha1(ref->old_sha1) &&
diff --git a/transport.c b/transport.c
index ae9fda8..1657798 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->forwardable)
+				*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
Chris Rorvick· Nov 12, 2012, 04:08 UTC · re: Chris Rorvick · lore

[PATCH v3 3/5] push: flag updates

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(-)
Show changes to 2 files +12 −7

cache.h, remote.c

diff --git a/cache.h b/cache.h
index 4e25840..9712d1b 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,6 +1003,7 @@ struct ref {
 		merge:1,
 		nonfastforward:1,
 		forwardable:1,
+		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 5ecd58d..7fa9db6 100644
--- a/remote.c
+++ b/remote.c
@@ -1323,15 +1323,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			  old->type == OBJ_COMMIT && new->type == OBJ_COMMIT);
 		}
 
-		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
Chris Rorvick· Nov 12, 2012, 04:08 UTC · re: Chris Rorvick · lore

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

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 9712d1b..e61dca3 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,
 		forwardable:1,
diff --git a/remote.c b/remote.c
index 7fa9db6..82ba5ed 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)
@@ -1332,9 +1334,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 1657798..632f8b0 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
Chris Rorvick· Nov 12, 2012, 04:08 UTC · re: Chris Rorvick · lore

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

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             |  3 +--
 builtin/send-pack.c        |  5 +++++
 cache.h                    |  1 +
 remote.c                   |  8 +++++++-
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                |  8 ++++++--
 9 files changed, 61 insertions(+), 11 deletions(-)
Show changes to 9 files +61 −11

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..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 77340c0..d097348 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -222,8 +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.  Use git push -f if\n"
-	   "you really want to make this update.");
+	   "the remote.  Use git push -f if you really want to make this update.");
 
 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 e61dca3..127e504 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 82ba5ed..e5f8a56 100644
--- a/remote.c
+++ b/remote.c
@@ -1334,7 +1334,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 632f8b0..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->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
Junio C Hamano· Nov 13, 2012, 21:20 UTC · re: Chris Rorvick · lore

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

Chris Rorvick <chris@rorvick.com> writes:
Show 15 quoted lines
> Minor changes since from v2 set.  Reposting primarily because I mucked
> up the Cc: list (again) and hoping to route feedback to the appropriate
> audience.
>
> 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".

Below, I take that you mean by "tag reference" everything under refs/tags/ (not limited to "annotated tag objects", but also lightweight tags).

Given that the second point below is to strongly discourage updating of existing any tag, it might be even better to advise *not* to push tags in the first place, instead of destructive "push -f", no?

Show 8 quoted lines
>   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
>
>      An email thread initiated by Angelo Borsotti did not come to a
>      consensus on how push should behave with regard to tag references.

I think the original motivation of allowing fast-forward updates to tags was for people who wanted to have "today's recommended version" tag that can float from day to day. I tend to think that was a misguided notion and it is better implemented with a tip of a branch (iow, I personally am OK with the change to forbid tag updates altogether, without --force).

>      I think a key point is that you currently cannot be sure your push
>      will not clobber a tag (lightweight or not) in the remote.

"Do not update, only add new" may be a good feature, but at the same time I have this suspicion that its usefulness may not necessarily be limited to refs/tags/* hierarchy.

I dunno.
Drew Northup· Nov 13, 2012, 23:58 UTC · re: Chris Rorvick · lore

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

On Sun, Nov 11, 2012 at 11:08 PM, Chris Rorvick <chris@rorvick.com> wrote:
> Minor changes since from v2 set.
.....
>      An email thread initiated by Angelo Borsotti did not come to a
>      consensus on how push should behave with regard to tag references.

Minor Nit: Without the link to gmane it is an exercise left to the reviewer to find that you're talking about this thread: http://thread.gmane.org/gmane.comp.version-control.git/208354

Cheers.
-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
Chris Rorvick· Nov 14, 2012, 06:29 UTC · lore

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

resending to list ...
On Tue, Nov 13, 2012 at 3:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> Chris Rorvick <chris@rorvick.com> writes:
>
>> Minor changes since from v2 set.  Reposting primarily because I mucked
>> up the Cc: list (again) and hoping to route feedback to the appropriate
>> audience.
>>
>> 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".
>
> Below, I take that you mean by "tag reference" everything under
> refs/tags/ (not limited to "annotated tag objects", but also
> lightweight tags).
Yes.
> Given that the second point below is to strongly discourage updating
> of existing any tag, it might be even better to advise *not* to push
> tags in the first place, instead of destructive "push -f", no?

That does seem like a better idea. Read the full manual page to figure out how to force the update if that's what you want to do--the advice should not suggest something exceptional.

Show 24 quoted lines
>>   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
>>
>>      An email thread initiated by Angelo Borsotti did not come to a
>>      consensus on how push should behave with regard to tag references.
>
> I think the original motivation of allowing fast-forward updates to
> tags was for people who wanted to have "today's recommended version"
> tag that can float from day to day. I tend to think that was a
> misguided notion and it is better implemented with a tip of a
> branch (iow, I personally am OK with the change to forbid tag
> updates altogether, without --force).
>
>>      I think a key point is that you currently cannot be sure your push
>>      will not clobber a tag (lightweight or not) in the remote.
>
> "Do not update, only add new" may be a good feature, but at the same
> time I have this suspicion that its usefulness may not necessarily
> be limited to refs/tags/* hierarchy.
>
> I dunno.

Are you suggesting allowing forwards for just refs/heads/*? I initially went this route based on some feedback in the original thread, but being that specific broke a couple tests in t5516 (i.e., pushing to refs/remotes/origin/master and another into refs/tmp/*.) My initial thought was that I'd broken something and I need to modify the patch, but now I think I should just modify those tests. Branches are restricted to refs/heads/* (if I understand correctly), so allowing fast-forwards when pushing should be limited to this hierarchy, too.

Thanks,
Chris
Kacper Kornet· Nov 14, 2012, 08:19 UTC · re: Chris Rorvick · lore

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

On Wed, Nov 14, 2012 at 12:29:14AM -0600, Chris Rorvick wrote:
> >>   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
> >>      An email thread initiated by Angelo Borsotti did not come to a
> >>      consensus on how push should behave with regard to tag references.
Show 6 quoted lines
> > I think the original motivation of allowing fast-forward updates to
> > tags was for people who wanted to have "today's recommended version"
> > tag that can float from day to day. I tend to think that was a
> > misguided notion and it is better implemented with a tip of a
> > branch (iow, I personally am OK with the change to forbid tag
> > updates altogether, without --force).
> >>      I think a key point is that you currently cannot be sure your push
> >>      will not clobber a tag (lightweight or not) in the remote.
> > "Do not update, only add new" may be a good feature, but at the same
> > time I have this suspicion that its usefulness may not necessarily
> > be limited to refs/tags/* hierarchy.
> > I dunno.
Show 9 quoted lines
> Are you suggesting allowing forwards for just refs/heads/*?  I
> initially went this route based on some feedback in the original
> thread, but being that specific broke a couple tests in t5516 (i.e.,
> pushing to refs/remotes/origin/master and another into refs/tmp/*.)
> My initial thought was that I'd broken something and I need to modify
> the patch, but now I think I should just modify those tests.  Branches
> are restricted to refs/heads/* (if I understand correctly), so
> allowing fast-forwards when pushing should be limited to this
> hierarchy, too.

What about notes? I think they should be treated in the same way as branches. My impression is that tags are exceptional in this respect.

-- 
  Kacper
Junio C Hamano· Nov 14, 2012, 13:22 UTC · re: Chris Rorvick · lore

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

Chris Rorvick <chris@rorvick.com> writes:
Show 7 quoted lines
>> "Do not update, only add new" may be a good feature, but at the same
>> time I have this suspicion that its usefulness may not necessarily
>> be limited to refs/tags/* hierarchy.
>>
>> I dunno.
>
> Are you suggesting allowing forwards for just refs/heads/*?

No, it is a nonsense to unconditionally forbid fast-forwards to refs outside refs/heads/ hierarchy.

I was imagining a more general feature to allow the *user* to ask Git not to fast-forward some refs (not limited to refs/tags/) during a push.

If such a general feature were in place, you can think of your patch as automatically making the user to ask Git not to fast-forward refs in refs/tags/, which would be a mere special case of it.

And I was wondering if such a general feature makes sense.
Angelo Borsotti· Nov 14, 2012, 14:58 UTC · re: Junio C Hamano · lore

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

Hi Junio,

actually, I proposed to add a key in config files, e.g. pushTagsNoChange to be set in the remote repo do disallow changes to tags, similar to pushNonFastForward that disallows non-fastforward changes to branches. I still have the impression that this is simple and clear, and allows the owner of the remote repository to enforce the policy s/he wants on her/his repository.

-Angelo
On 14 November 2012 14:22, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> Chris Rorvick <chris@rorvick.com> writes:
>
>>> "Do not update, only add new" may be a good feature, but at the same
>>> time I have this suspicion that its usefulness may not necessarily
>>> be limited to refs/tags/* hierarchy.
>>>
>>> I dunno.
>>
>> Are you suggesting allowing forwards for just refs/heads/*?
>
> No, it is a nonsense to unconditionally forbid fast-forwards to refs
> outside refs/heads/ hierarchy.
>
> I was imagining a more general feature to allow the *user* to ask
> Git not to fast-forward some refs (not limited to refs/tags/) during
> a push.
>
> If such a general feature were in place, you can think of your patch
> as automatically making the user to ask Git not to fast-forward refs
> in refs/tags/, which would be a mere special case of it.
>
> And I was wondering if such a general feature makes sense.
>
>
>
>
Junio C Hamano· Nov 14, 2012, 17:32 UTC · re: Angelo Borsotti · lore

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

Angelo Borsotti <angelo.borsotti@gmail.com> writes:
Show 6 quoted lines
> actually, I proposed to add a key in config files, e.g.
> pushTagsNoChange to be set in the remote repo do disallow changes to
> tags, similar to pushNonFastForward that disallows non-fastforward
> changes to branches. I still have the impression that this is simple
> and clear, and allows the owner of the remote repository to enforce
> the policy s/he wants on her/his repository.

That is an independent issue of deciding to accept or reject receiving a push from outside, no? You can implement any such policy in the pre-receive hook on the receiving end with a simple and clear manner, instead of adding specific logic to enforce a single hardcoded policy to the code that is flipped on with a configuration variable.

In any case, I thought this series was about users who run "push" voluntarily stopping themselves from pushing updates to tags that may happen to fast-forward, so if we were to go with the configuration route, the suggestion would be more like

    [push]
	updateNeedsForce = refs/tags/:refs/frotz/
or perhaps
    [remote "origin"]
	updateNeedsForce = refs/tags/:refs/frotz/

if we want to configure it per-remote, to specify that you would need to say "--force" to update the refs in the listed hierarchies.

Then your patch series could become just the matter of declaring that the value of push.updateNeedsForce, when unspecified, defaults to "refs/tags/".

Angelo Borsotti· Nov 14, 2012, 23:43 UTC · re: Junio C Hamano · lore

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

Hi Junio,
> That is an independent issue of deciding to accept or reject
> receiving a push from outside, no?

Yes, it is. Actually I thought some means to let the owner do decide what to accept were already present (the pushNonFastForward config key), and going along this avenue I thought it could be appropriate to extent this a bit.

-Angelo
On 14 November 2012 18:32, Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
> Angelo Borsotti <angelo.borsotti@gmail.com> writes:
>
>> actually, I proposed to add a key in config files, e.g.
>> pushTagsNoChange to be set in the remote repo do disallow changes to
>> tags, similar to pushNonFastForward that disallows non-fastforward
>> changes to branches. I still have the impression that this is simple
>> and clear, and allows the owner of the remote repository to enforce
>> the policy s/he wants on her/his repository.
>
> That is an independent issue of deciding to accept or reject
> receiving a push from outside, no?  You can implement any such
> policy in the pre-receive hook on the receiving end with a simple
> and clear manner, instead of adding specific logic to enforce a
> single hardcoded policy to the code that is flipped on with a
> configuration variable.
>
> In any case, I thought this series was about users who run "push"
> voluntarily stopping themselves from pushing updates to tags that
> may happen to fast-forward, so if we were to go with the
> configuration route, the suggestion would be more like
>
>     [push]
>         updateNeedsForce = refs/tags/:refs/frotz/
>
> or perhaps
>
>     [remote "origin"]
>         updateNeedsForce = refs/tags/:refs/frotz/
>
> if we want to configure it per-remote, to specify that you would
> need to say "--force" to update the refs in the listed hierarchies.
>
> Then your patch series could become just the matter of declaring
> that the value of push.updateNeedsForce, when unspecified, defaults
> to "refs/tags/".
>
Junio C Hamano· Nov 15, 2012, 00:09 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Addendum.
Show 19 quoted lines
> In any case, I thought this series was about users who run "push"
> voluntarily stopping themselves from pushing updates to tags that
> may happen to fast-forward, so if we were to go with the
> configuration route, the suggestion would be more like
>
>     [push]
> 	updateNeedsForce = refs/tags/:refs/frotz/
>
> or perhaps
>
>     [remote "origin"]
> 	updateNeedsForce = refs/tags/:refs/frotz/
>
> if we want to configure it per-remote, to specify that you would
> need to say "--force" to update the refs in the listed hierarchies.
>
> Then your patch series could become just the matter of declaring
> that the value of push.updateNeedsForce, when unspecified, defaults
> to "refs/tags/".

The above is not a "you should do it this way" suggestion, by the way.

I was just explaining what I meant by "it may be a good feature, but may not necessarily be limited to refs/tags" in my earlier message in a different way "... and a possible design that lifts the limitation may go like this".

I am *not* convinced that the "refs/tags/ is the only special hierarchy whose contents should not move" is a bad limitation we should avoid, but if it indeed is a bad limitation, the above is one possible way to think about avoiding it.

Thanks.
Angelo Borsotti· Nov 15, 2012, 07:48 UTC · re: Junio C Hamano · lore

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

Hi Junio,
> I am *not* convinced that the "refs/tags/ is the only special
> hierarchy whose contents should not move" is a bad limitation we
> should avoid, but if it indeed is a bad limitation, the above is one
> possible way to think about avoiding it.

What other hierarchy besides branches and tags is there? Do you have in mind some other that should not move?

-Angelo
On 15 November 2012 01:09, Junio C Hamano <gitster@pobox.com> wrote:
Show 38 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> Addendum.
>
>> In any case, I thought this series was about users who run "push"
>> voluntarily stopping themselves from pushing updates to tags that
>> may happen to fast-forward, so if we were to go with the
>> configuration route, the suggestion would be more like
>>
>>     [push]
>>       updateNeedsForce = refs/tags/:refs/frotz/
>>
>> or perhaps
>>
>>     [remote "origin"]
>>       updateNeedsForce = refs/tags/:refs/frotz/
>>
>> if we want to configure it per-remote, to specify that you would
>> need to say "--force" to update the refs in the listed hierarchies.
>>
>> Then your patch series could become just the matter of declaring
>> that the value of push.updateNeedsForce, when unspecified, defaults
>> to "refs/tags/".
>
> The above is not a "you should do it this way" suggestion, by the
> way.
>
> I was just explaining what I meant by "it may be a good feature, but
> may not necessarily be limited to refs/tags" in my earlier message
> in a different way "... and a possible design that lifts the
> limitation may go like this".
>
> I am *not* convinced that the "refs/tags/ is the only special
> hierarchy whose contents should not move" is a bad limitation we
> should avoid, but if it indeed is a bad limitation, the above is one
> possible way to think about avoiding it.
>
> Thanks.
Junio C Hamano· Nov 15, 2012, 16:50 UTC · re: Angelo Borsotti · lore

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

Angelo Borsotti <angelo.borsotti@gmail.com> writes:
Show 7 quoted lines
>> I am *not* convinced that the "refs/tags/ is the only special
>> hierarchy whose contents should not move" is a bad limitation we
>> should avoid, but if it indeed is a bad limitation, the above is one
>> possible way to think about avoiding it.
>
> What other hierarchy besides branches and tags is there? Do you have
> in mind some other that should not move?

People use their own hierarchies for various purposes that are not pre-defined by git-core, e.g. refs/changes/, refs/pull/, etc. Depending on the semantics the projects want out of these hierarchies, some of them may well be considered "create-only".

← back to recent threads