[PATCH v6 0/8] push: update remote tags only with force

[PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

This patch series originated in response to the following thread:

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

I made some adjustments based on Junio's last round of feedback
including a new patch reworking the "push rules" comment in remote.c.
Also refined some of the log messages--nothing major.  Finally, took a
stab at putting something together for the release notes, see below.

Chris

Release notes:

"git push" no longer updates tags (lightweight or annotated) by default.
Specifically, if the destination reference already exists and is under
refs/tags/ or it points to a tag object, it is not allowed to fast-
forward (unless forced using +A:B notation or by passing --force.)  This
is consistent with how a tag is normally thought of: a reference that
does not move once defined.  It also ensures a push will not
inadvertently clobber an already existing tag--something that can go
unnoticed if fast-forwarding is allowed.

Chris Rorvick (8):
  push: return reject reasons as a bitset
  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
  push: cleanup push rules comment

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

-- 
1.8.0.158.g0c4328c

[PATCH v6 1/8] push: return reject reasons as a bitset

[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..9d17fc7 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_reasons;
 
 	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_reasons);
 	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_reasons & REJECT_NON_FF_HEAD) {
 		advise_pull_before_push();
-		break;
-	case NON_FF_OTHER:
+	} else if (reject_reasons & 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..9f98607 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_reasons;
 	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_reasons);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
diff --git a/transport.c b/transport.c
index 9932f40..d4568e7 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_reasons)
 {
 	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_reasons = 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_reasons |= REJECT_NON_FF_HEAD;
 			else
-				*nonfastforward = NON_FF_OTHER;
+				*reject_reasons |= 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_reasons)
 {
-	*nonfastforward = 0;
+	*reject_reasons = 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_reasons);
 
 		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
 			set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index 4a61c0c..404b113 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_reasons);
 
 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_reasons);
 
 typedef void alternate_ref_fn(const struct ref *, void *);
 extern void for_each_alternate_ref(alternate_ref_fn, void *);
-- 
1.8.0.158.g0c4328c

[PATCH v6 2/8] 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 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(+)

diff --git a/builtin/push.c b/builtin/push.c
index 9d17fc7..e08485d 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_reasons & 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 d4568e7..bc31e8e 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_reasons |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
 				*reject_reasons |= REJECT_NON_FF_HEAD;
 			else
diff --git a/transport.h b/transport.h
index 404b113..bfd2df5 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.158.g0c4328c

[PATCH v6 3/8] push: flag updates

[Chris Rorvick]

If the reference exists on the remote and it is not being removed, then
mark as an update.  This is in preparation for handling tags (lightweight
and annotated) exceptionally.

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

[PATCH v6 4/8] push: flag updates that require force

[Chris Rorvick]

Add a flag for indicating an update to a reference requires force.
Currently the `nonfastforward` flag is used for this when generating the
status message.  A separate flag insulates dependent 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 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 bc31e8e..f3160b1 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.158.g0c4328c

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

[Chris Rorvick]

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

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 e08485d..83a3cc8 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 9f98607..f849e0a 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 f3160b1..2673d27 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_reasons |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
 				*reject_reasons |= REJECT_NON_FF_HEAD;
 			else
 				*reject_reasons |= REJECT_NON_FF_OTHER;
+		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
+			*reject_reasons |= REJECT_ALREADY_EXISTS;
 		}
 	}
 }
-- 
1.8.0.158.g0c4328c

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

[Chris Rorvick]

Do not allow fast-forwarding of references that point to a tag object.
Updating from a tag is potentially destructive since it would likely
leave the tag dangling.  Disallowing updates to a tag also makes sense
semantically and is consistent with the behavior of lightweight tags.

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

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

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

[Chris Rorvick]

Pushes must already (by default) update to a commit-ish due to 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.

While updating *from* a tag object is potentially destructive, updating
*to* a tag is not.  Additionally, a push to the refs/tags/ hierarchy is
already excluded from fast-forwarding, and refs/heads/ is protected from
anything but commit objects by a check in write_ref_sha1().  Thus
someone fast-forwarding to a tag is probably not doing so by accident.
Since updating to a tag is benign and unlikely to cause confusion, allow
it in case someone finds the behavior useful.

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;
+
 	return 1;
 }
 
-- 
1.8.0.158.g0c4328c

[PATCH v6 8/8] push: cleanup push rules comment

[Chris Rorvick]

Rewrite to remove inter-dependencies amongst the rules.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 remote.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/remote.c b/remote.c
index ee0c1e5..6309a87 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:
+		/*
+		 * The below logic determines whether an individual
+		 * refspec A:B can be pushed.  The push will succeed
+		 * if any of the following are true:
 		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
+		 * (1) the remote reference B does not exist
 		 *
-		 * (1) if the old thing does not exist, it is OK.
+		 * (2) the remote reference B is being removed (i.e.,
+		 *     pushing :B where no source is specified)
 		 *
-		 * (2) if the destination is under refs/tags/ you are
-		 *     not allowed to overwrite it; tags are expected
-		 *     to be static once created
+		 * (3) the update meets all fast-forwarding criteria:
 		 *
-		 * (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.
+		 *     (a) the destination is not under refs/tags/
+		 *     (b) the old is a commit
+		 *     (c) the new is a descendant of the old
 		 *
-		 * (4) if old is a commit and new is a descendant of old
-		 *     (implying new is commit-ish), it is OK.
+		 *     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.
 		 *
-		 * (5) regardless of all of the above, removing :B is
-		 *     always allowed.
+		 * (4) it is forced using the +A:B notation, or by
+		 *     passing the --force argument
 		 */
 
 		ref->not_forwardable = !is_forwardable(ref);
-- 
1.8.0.158.g0c4328c

Re: [PATCH v6 2/8] push: add advice for rejected tag reference

[Junio C Hamano]

Chris Rorvick <chris@rorvick.com> writes:

>  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_reasons & REJECT_ALREADY_EXISTS) {
> +		advise_ref_already_exists();
>  	}

The existing advise_* functions that are called from this function
honor the advice.* configuration, and advise_ref_already_exists()
would want to follow suit here (it is OK to do so as a follow-up
patch without further rerolling the entire series).

Thanks.

[PATCH] remote.c: fix grammatical error in comment

[Chris Rorvick]

The sentence originally began "Note that ..." and was changed to
"NOTE: ..."  This change should have been made at the same time.

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

This applies to the current cr/push-force-tag-update branch.  It can
probably just be folded into the last commit.

Thanks,

Chris

 remote.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 6309a87..aa6b719 100644
--- a/remote.c
+++ b/remote.c
@@ -1337,8 +1337,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *
 		 *     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.
+		 *     and 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
-- 
1.8.0.1.541.g73be2da

[PATCH 0/2] push: honor advice.* configuration

[Chris Rorvick]

Added a new config option to turn off the already-exists advice.  We
also want to observe the 'pushNonFastForward' setting, but the name of
this config is too narrow after this addition.  Renamed to have broader
scope while retaining the old name as an alias for backward-
compatibility.

Chris Rorvick (2):
  push: rename config variable for more general use
  push: allow already-exists advice to be disabled

 Documentation/config.txt | 10 +++++++---
 advice.c                 |  9 +++++++--
 advice.h                 |  3 ++-
 builtin/push.c           |  8 +++++---
 4 files changed, 21 insertions(+), 9 deletions(-)

-- 
1.8.0.1.541.g73be2da

[PATCH 1/2] push: rename config variable for more general use

[Chris Rorvick]

The 'pushNonFastForward' advice config can be used to squelch several
instances of push-related advice.  Rename it to 'pushUpdateRejected' to
cover other reject scenarios that are unrelated to fast-forwarding.
Retain the old name for compatibility.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 Documentation/config.txt | 2 +-
 advice.c                 | 7 +++++--
 advice.h                 | 2 +-
 builtin/push.c           | 6 +++---
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9a0544c..92903f2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -140,7 +140,7 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
-	pushNonFastForward::
+	pushUpdateRejected::
 		Set this variable to 'false' if you want to disable
 		'pushNonFFCurrent', 'pushNonFFDefault', and
 		'pushNonFFMatching' simultaneously.
diff --git a/advice.c b/advice.c
index edfbd4a..329e077 100644
--- a/advice.c
+++ b/advice.c
@@ -1,6 +1,6 @@
 #include "cache.h"
 
-int advice_push_nonfastforward = 1;
+int advice_push_update_rejected = 1;
 int advice_push_non_ff_current = 1;
 int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
@@ -14,7 +14,7 @@ static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
-	{ "pushnonfastforward", &advice_push_nonfastforward },
+	{ "pushupdaterejected", &advice_push_update_rejected },
 	{ "pushnonffcurrent", &advice_push_non_ff_current },
 	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
@@ -23,6 +23,9 @@ static struct {
 	{ "resolveconflict", &advice_resolve_conflict },
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },
+
+	/* make this an alias for backward compatibility */
+	{ "pushnonfastforward", &advice_push_update_rejected }
 };
 
 void advise(const char *advice, ...)
diff --git a/advice.h b/advice.h
index f3cdbbf..c28ef8a 100644
--- a/advice.h
+++ b/advice.h
@@ -3,7 +3,7 @@
 
 #include "git-compat-util.h"
 
-extern int advice_push_nonfastforward;
+extern int advice_push_update_rejected;
 extern int advice_push_non_ff_current;
 extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
diff --git a/builtin/push.c b/builtin/push.c
index 83a3cc8..cf5ecfa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -226,21 +226,21 @@ static const char message_advice_ref_already_exists[] =
 
 static void advise_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current || !advice_push_nonfastforward)
+	if (!advice_push_non_ff_current || !advice_push_update_rejected)
 		return;
 	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_use_upstream(void)
 {
-	if (!advice_push_non_ff_default || !advice_push_nonfastforward)
+	if (!advice_push_non_ff_default || !advice_push_update_rejected)
 		return;
 	advise(_(message_advice_use_upstream));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	if (!advice_push_non_ff_matching || !advice_push_nonfastforward)
+	if (!advice_push_non_ff_matching || !advice_push_update_rejected)
 		return;
 	advise(_(message_advice_checkout_pull_push));
 }
-- 
1.8.0.1.541.g73be2da

[PATCH 2/2] push: allow already-exists advice to be disabled

[Chris Rorvick]

Add 'advice.pushAlreadyExists' option to disable the advice shown when
an update is rejected for a reference that is not allowed to update at
all (verses those that are allowed to fast-forward.)

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 Documentation/config.txt | 8 ++++++--
 advice.c                 | 2 ++
 advice.h                 | 1 +
 builtin/push.c           | 2 ++
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 92903f2..90e7d10 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -142,8 +142,9 @@ advice.*::
 --
 	pushUpdateRejected::
 		Set this variable to 'false' if you want to disable
-		'pushNonFFCurrent', 'pushNonFFDefault', and
-		'pushNonFFMatching' simultaneously.
+		'pushNonFFCurrent', 'pushNonFFDefault',
+		'pushNonFFMatching', and 'pushAlreadyExists'
+		simultaneously.
 	pushNonFFCurrent::
 		Advice shown when linkgit:git-push[1] fails due to a
 		non-fast-forward update to the current branch.
@@ -158,6 +159,9 @@ advice.*::
 		'matching refs' explicitly (i.e. you used ':', or
 		specified a refspec that isn't your current branch) and
 		it resulted in a non-fast-forward error.
+	pushAlreadyExists::
+		Shown when linkgit:git-push[1] rejects an update that
+		does not qualify for fast-forwarding (e.g., a tag.)
 	statusHints::
 		Show directions on how to proceed from the current
 		state in the output of linkgit:git-status[1] and in
diff --git a/advice.c b/advice.c
index 329e077..d287927 100644
--- a/advice.c
+++ b/advice.c
@@ -4,6 +4,7 @@ int advice_push_update_rejected = 1;
 int advice_push_non_ff_current = 1;
 int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
+int advice_push_already_exists = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -18,6 +19,7 @@ static struct {
 	{ "pushnonffcurrent", &advice_push_non_ff_current },
 	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
+	{ "pushalreadyexists", &advice_push_already_exists },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index c28ef8a..8bf6356 100644
--- a/advice.h
+++ b/advice.h
@@ -7,6 +7,7 @@ extern int advice_push_update_rejected;
 extern int advice_push_non_ff_current;
 extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
+extern int advice_push_already_exists;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index cf5ecfa..8491e43 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -247,6 +247,8 @@ static void advise_checkout_pull_push(void)
 
 static void advise_ref_already_exists(void)
 {
+	if (!advice_push_already_exists || !advice_push_update_rejected)
+		return;
 	advise(_(message_advice_ref_already_exists));
 }
 
-- 
1.8.0.1.541.g73be2da

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Thanks; will queue.

Re: [PATCH v6 0/8] push: update remote tags only with force

[Max Horn]

Hi there,

I was just working on improving git-remote-helper.txt by documenting how remote helper can signal error conditions to git. This lead me to notice a (to me) surprising change in behavior between master and next that I traced back to this patch series.

Specifically:

On 30.11.2012, at 02:41, Chris Rorvick wrote:

> This patch series originated in response to the following thread:
> 
>  http://thread.gmane.org/gmane.comp.version-control.git/208354
> 
> I made some adjustments based on Junio's last round of feedback
> including a new patch reworking the "push rules" comment in remote.c.
> Also refined some of the log messages--nothing major.  Finally, took a
> stab at putting something together for the release notes, see below.

>From the discussion in that gmane thread and from the commits in this series, I had the impression that it should mostly affect pushing tags. However, this is not the case: It also changes messages upon regular push "conflicts. Consider this test script:


#!/bin/sh -ex
git init repo_orig
cd repo_orig
echo a > a
git add a
git commit -m a
cd ..

git clone repo_orig repo_clone

cd repo_orig
echo b > b
git add b
git commit -m b
cd ..

cd repo_clone
echo B > b
git add b
git commit -m B
git push


With git 1.8.1, I get this message:

 ! [rejected]        master -> master (non-fast-forward)
error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.



But with next, I get this:


 ! [rejected]        master -> master (already exists)
error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
hint: Updates were rejected because the destination reference already exists
hint: in the remote.


This looks like a regression to me. No tags were involve, and the new message is very confusing if not outright wrong -- at least in my mind, but perhaps I am missing a way to interpret it "correctly" ? What am I missing?


Cheers,
Max

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> But with next, I get this:
>
>  ! [rejected]        master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.

It is in master now X-<, and this looks like a bug to me.

Re: [PATCH v6 0/8] push: update remote tags only with force

[Jeff King]

On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote:

> With git 1.8.1, I get this message:
> 
>  ! [rejected]        master -> master (non-fast-forward)
> [...]
> But with next, I get this:
> 
>  ! [rejected]        master -> master (already exists)

Thanks for the detailed report. I was able to reproduce easily here.

The problem is the logic in is_forwardable:

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;

        /* 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;
}

The intent is to allow fast-forward only between objects that both point
to commits eventually. But we are doing this check on the client, which
does not necessarily have the object for ref->old_sha1 at all. So it
cannot know the type, and cannot enforce this condition accurately.

I.e., we trigger the "!o" branch after the parse_object in your example.

-Peff

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

> But with next, I get this:
>
>
>  ! [rejected]        master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.

It is an outright bug.  The new helper function is_forwrdable() is
bogus to assume that both original and updated objects can be
locally inspected, but you do not necessarily have the original
locally.

 remote.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index aa6b719..4a253ef 100644
--- a/remote.c
+++ b/remote.c
@@ -1286,9 +1286,12 @@ static inline int is_forwardable(struct ref* ref)
 	if (!prefixcmp(ref->name, "refs/tags/"))
 		return 0;
 
-	/* old object must be a commit */
+	/*
+	 * old object must be a commit, but we may be forcing
+	 * without having it in the first place!
+	 */
 	o = parse_object(ref->old_sha1);
-	if (!o || o->type != OBJ_COMMIT)
+	if (o && o->type != OBJ_COMMIT)
 		return 0;
 
 	/* new object must be commit-ish */

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Max Horn <max@quendi.de> writes:
>
>> But with next, I get this:
>>
>>  ! [rejected]        master -> master (already exists)
>> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
>> hint: Updates were rejected because the destination reference already exists
>> hint: in the remote.
>>
>> This looks like a regression to me.
>
> It is an outright bug.  The new helper function is_forwrdable() is
> bogus to assume that both original and updated objects can be
> locally inspected, but you do not necessarily have the original
> locally.

The way the caller uses the result of this function is equally
questionable.  If this function says "we do not want to let this
push go through", it translates that unconditionally into "we
blocked it because the destination already exists".

It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
OK if the type check does not satisfy this function.  In that case,
we do not actually see the existence of the destination as a
problem, but it is reported as such.  We are blocking because we do
not like the type of the new object or the type of the old object.
If the destination points at a commit, the push can succeed if the
user changes what object to push, so saying "you cannot push because
the destination already exists" is just wrong in such a case.

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I.e., we trigger the "!o" branch after the parse_object in your example.

Heh, I didn't see this message until now (gmane seems to be lagging
a bit).

I am very tempted to do this.

 * Remove unnecessary not_forwardable from "struct ref"; it is only
   used inside set_ref_status_for_push();

 * "refs/tags/" is the only hierarchy that cannot be replaced
   without --force;

 * Remove the misguided attempt to force that everything that
   updates an existing ref has to be a commit outside "refs/tags/"
   hierarchy.  This code does not know what kind of objects the user
   wants to place in "refs/frotz/" hierarchy it knows nothing about.

I feel moderately strongly about the last point.  Defining special
semantics for one hierarchy (e.g. "refs/tags/") and implementing a
policy for enforcement is one thing, but a random policy that
depends on object type that applies globally is simply insane.  The
user may want to do "refs/tested/" hierarchy that is meant to hold
references to commit, with one annotated tag "refs/tested/latest"
that points at the "latest tested version" with some commentary, and
maintain the latter by keep pushing to it.  If that is the semantics
the user wanted to ahve in the "refs/tested/" hierarchy, it is not
reasonable to require --force for such a workflow.  The user knows
better than Git in such a case.


 cache.h               |  1 -
 remote.c              | 24 +-----------------------
 t/t5516-fetch-push.sh | 21 ---------------------
 3 files changed, 1 insertion(+), 45 deletions(-)

diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
 		requires_force:1,
 		merge:1,
 		nonfastforward:1,
-		not_forwardable:1,
 		update:1,
 		deletion:1;
 	enum {
diff --git a/remote.c b/remote.c
index aa6b719..2c747c4 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	return 0;
 }
 
-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;
-
-	/* 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;
-}
-
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	int force_update)
 {
@@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     passing the --force argument
 		 */
 
-		ref->not_forwardable = !is_forwardable(ref);
-
 		ref->update =
 			!ref->deletion &&
 			!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1333,7 @@ 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->not_forwardable) {
+			if (!prefixcmp(ref->name, "refs/tags/")) {
 				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ 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" &&

Re: [PATCH v6 0/8] push: update remote tags only with force

[Jeff King]

On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I.e., we trigger the "!o" branch after the parse_object in your example.
> 
> Heh, I didn't see this message until now (gmane seems to be lagging
> a bit).

I think it is vger lagging, actually.

> I am very tempted to do this.
> 
>  * Remove unnecessary not_forwardable from "struct ref"; it is only
>    used inside set_ref_status_for_push();
> 
>  * "refs/tags/" is the only hierarchy that cannot be replaced
>    without --force;

Agreed.

>  * Remove the misguided attempt to force that everything that
>    updates an existing ref has to be a commit outside "refs/tags/"
>    hierarchy.  This code does not know what kind of objects the user
>    wants to place in "refs/frotz/" hierarchy it knows nothing about.

I agree with what your patch does, but my thinking is a bit different.

My original suggestion with respect to object types was that the rule
for --force should be "do not ever lose any objects without --force". So
a fast-forward is OK, as the new objects reference the old. A non-fast
forward is not, because objects become unreferenced. Replacing a tag
object is not OK, even if it points to the same commit, as you are
losing the old tag object (replacing an object with a tag that points to
the original object or its descendent is OK in theory, though I doubt it
is common enough to worry about).

I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.

But it is somewhat orthogonal to the "already exists" idea, and checking
refs/tags/. Those ideas are about enforcing sane rules on the tag
hierarchy. My rule is a safety valve that is meant to extend the idea of
"is fast-forwardable" to non-commit object types. If we do it at all, it
should be part of the fast-forward check (e.g., as part of ref_newer).

The current code conflates the two under the "already exists" condition,
which is just wrong.  I think the best thing at this point is to split
the two ideas apart, keep the refs/tags check (and translate it to
"already exists" in the UI, as we do), and table the safety valve. I am
not even sure if it is something that is useful, and it can come later
if we decide it is.

> I feel moderately strongly about the last point.  Defining special
> semantics for one hierarchy (e.g. "refs/tags/") and implementing a
> policy for enforcement is one thing, but a random policy that
> depends on object type that applies globally is simply insane.  The
> user may want to do "refs/tested/" hierarchy that is meant to hold
> references to commit, with one annotated tag "refs/tested/latest"
> that points at the "latest tested version" with some commentary, and
> maintain the latter by keep pushing to it.  If that is the semantics
> the user wanted to ahve in the "refs/tested/" hierarchy, it is not
> reasonable to require --force for such a workflow.  The user knows
> better than Git in such a case.

I see what you are saying, but I think the ship has already sailed to
some degree. We already implement the non-fast-forward check everywhere,
and I cannot have a "refs/tested" hierarchy that pushes arbitrary
commits without regard to their history. If I have such a hierarchy, I
have to use "--force" (or more likely, mark the refspec with "+").

In my mind, the object-type checking is just making that fast-forward
check more thorough (i.e., extending it to non-commit objects).

>  cache.h               |  1 -
>  remote.c              | 24 +-----------------------
>  t/t5516-fetch-push.sh | 21 ---------------------
>  3 files changed, 1 insertion(+), 45 deletions(-)

The patch itself looks fine to me. Whether we agree on the fast-forward
object-type checking or not, it is the correct first step to take in
either case.

-Peff

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I see what you are saying, but I think the ship has already sailed to
> some degree. We already implement the non-fast-forward check everywhere,
> and I cannot have a "refs/tested" hierarchy that pushes arbitrary
> commits without regard to their history. If I have such a hierarchy, I
> have to use "--force" (or more likely, mark the refspec with "+").

Yeah, actually in that example, I meant refs/tested/ would have
pointers to bare tree objects. I often rebuild 'pu' and another
private integration branch for testing, reordering the series that
are still not in 'next' and also after rewriting log messages of
some commits. It is not unusual to end up the updated 'pu' having
the identical tree as 'pu' before update, and I want to skip testing
the result (tree equality matters while commit equality does not in
such a use case).

> In my mind, the object-type checking is just making that fast-forward
> check more thorough (i.e., extending it to non-commit objects).

Yes, I agree with that point of view.

Thanks.

Here is what I am planning to queue (the patch is the same, but the
message is different on the third point). 

-- >8 --
Subject: [PATCH] push: fix "refs/tags/ hierarchy cannot be updated without --force"

When pushing to update a branch with a commit that is not a
descendant of the commit at the tip, a wrong message "already
exists" was given, instead of the correct "non-fast-forward", if we
do not have the object sitting in the destination repository at the
tip of the ref we are updating.

The primary cause of the bug is that the check in a new helper
function is_forwardable() assumed both old and new objects are
available and can be checked, which is not always the case.

The way the caller uses the result of this function is also wrong.
If the helper says "we do not want to let this push go through", the
caller unconditionally translates it into "we blocked it because the
destination already exists", which is not true at all in this case.

Fix this by doing these three things:

 * Remove unnecessary not_forwardable from "struct ref"; it is only
   used inside set_ref_status_for_push();

 * Make "refs/tags/" the only hierarchy that cannot be replaced
   without --force;

 * Remove the misguided attempt to force that everything that
   updates an existing ref has to be a commit outside "refs/tags/"
   hierarchy.

The policy last one tried to implement may later be resurrected and
extended to ensure fast-forwardness (defined as "not losing
objects", extending from the traditional "not losing commits from
the resulting history") when objects that are not commit are
involved (e.g. an annotated tag in hierarchies outside refs/tags),
but such a logic belongs to "is this a fast-forward?" check that is
done by ref_newer(); is_forwardable(), which is now removed, was not
the right place to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h               |  1 -
 remote.c              | 43 +++++++------------------------------------
 t/t5516-fetch-push.sh | 21 ---------------------
 3 files changed, 7 insertions(+), 58 deletions(-)

diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
 		requires_force:1,
 		merge:1,
 		nonfastforward:1,
-		not_forwardable:1,
 		update:1,
 		deletion:1;
 	enum {
diff --git a/remote.c b/remote.c
index aa6b719..d3a1ca2 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
 	return 0;
 }
 
-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;
-
-	/* 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;
-}
-
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	int force_update)
 {
@@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		}
 
 		/*
-		 * The below logic determines whether an individual
-		 * refspec A:B can be pushed.  The push will succeed
-		 * if any of the following are true:
+		 * Decide 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 the new object must be commit-ish.  These are
-		 *     implied by (b) and (c) respectively.
+		 * (3) the destination is not under refs/tags/, and
+		 *     if the old and new value is a commit, the new
+		 *     is a descendant of the old.
 		 *
 		 * (4) it is forced using the +A:B notation, or by
 		 *     passing the --force argument
 		 */
 
-		ref->not_forwardable = !is_forwardable(ref);
-
 		ref->update =
 			!ref->deletion &&
 			!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1326,7 @@ 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->not_forwardable) {
+			if (!prefixcmp(ref->name, "refs/tags/")) {
 				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ 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.1.1.426.g616047d

Re: [PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

On Wed, Jan 16, 2013 at 11:43 AM, Jeff King <peff@peff.net> wrote:
> I think that is a reasonable rule that could be applied across all parts
> of the namespace hierarchy. And it could be applied by the client,
> because all you need to know is whether ref->old_sha1 is reachable from
> ref->new_sha1.

is_forwardable() did solve a UI issue.  Previously all instances where
old is not reachable by new were assumed to be addressable with a
merge.  is_forwardable() attempted to determine if the concept of
forwarding made sense given the inputs.  For example, if old is a blob
it is useless to suggest merging it.

Chris

Re: [PATCH v6 0/8] push: update remote tags only with force

[Jeff King]

On Wed, Jan 16, 2013 at 08:19:28PM -0600, Chris Rorvick wrote:

> On Wed, Jan 16, 2013 at 11:43 AM, Jeff King <peff@peff.net> wrote:
> > I think that is a reasonable rule that could be applied across all parts
> > of the namespace hierarchy. And it could be applied by the client,
> > because all you need to know is whether ref->old_sha1 is reachable from
> > ref->new_sha1.
> 
> is_forwardable() did solve a UI issue.  Previously all instances where
> old is not reachable by new were assumed to be addressable with a
> merge.  is_forwardable() attempted to determine if the concept of
> forwarding made sense given the inputs.  For example, if old is a blob
> it is useless to suggest merging it.

I think it makes sense to mark such a case as different from a regular
non-fast-forward (because "git pull" is not the right advice), but:

  1. is_forwardable should assume a missing object is a commit not to
     regress the common case; otherwise we do not show the pull advice
     when we probably should, and most of the time it is going to be a
     commit

  2. When we know that we are not working with commits, I am not sure
     that "already exists" is the right advice to give for such a case.
     It is neither "this tag already exists, so we do not update it",
     nor is it strictly "cannot fast forward this commit", but rather
     something else.

     The expanded definition of "what is a fast forward" that I
     suggested would let this fall naturally between the two.

-Peff

Re: [PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

On Wed, Jan 16, 2013 at 9:11 PM, Jeff King <peff@peff.net> wrote:
>> is_forwardable() did solve a UI issue.  Previously all instances where
>> old is not reachable by new were assumed to be addressable with a
>> merge.  is_forwardable() attempted to determine if the concept of
>> forwarding made sense given the inputs.  For example, if old is a blob
>> it is useless to suggest merging it.
>
> I think it makes sense to mark such a case as different from a regular
> non-fast-forward (because "git pull" is not the right advice), but:
>
>   1. is_forwardable should assume a missing object is a commit not to
>      regress the common case; otherwise we do not show the pull advice
>      when we probably should, and most of the time it is going to be a
>      commit

Yes, obviously this was a bug, thus the use of "attempted" above.  It
would have been better to assume a missing 'old' was potentially
forwardable to present the user with the most helpful advice.

>   2. When we know that we are not working with commits, I am not sure
>      that "already exists" is the right advice to give for such a case.
>      It is neither "this tag already exists, so we do not update it",
>      nor is it strictly "cannot fast forward this commit", but rather
>      something else.

But the reference already existing in the remote is a substantial
reason for not allowing the push in all of these cases.  You can break
this out further if you like to explain why the specific reference
shouldn't be moved on the remote, but this is even more complicated a
simple "is old reachable from new?" test.

Chris

Re: [PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
> OK if the type check does not satisfy this function.  In that case,
> we do not actually see the existence of the destination as a
> problem, but it is reported as such.  We are blocking because we do
> not like the type of the new object or the type of the old object.
> If the destination points at a commit, the push can succeed if the
> user changes what object to push, so saying "you cannot push because
> the destination already exists" is just wrong in such a case.

So the solution is to revert back to recommending a merge?

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Chris Rorvick <chris@rorvick.com> writes:

> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
>> OK if the type check does not satisfy this function.  In that case,
>> we do not actually see the existence of the destination as a
>> problem, but it is reported as such.  We are blocking because we do
>> not like the type of the new object or the type of the old object.
>> If the destination points at a commit, the push can succeed if the
>> user changes what object to push, so saying "you cannot push because
>> the destination already exists" is just wrong in such a case.
>
> So the solution is to revert back to recommending a merge?

Of course not, because at that point you may not even have what you
were attempting to overwrite.  Nobody says it is even something you
could merge.

The recommended solution certainly will involve a "fetch" (not
"pull" or "pull --rebase").  You fetch from over there to check what
you were about to overwrite, examine the situation to decide what
the appropriate action is.

The point is that Git in general, and the codepath that was touched
by the patch in particular, does not have enough information to
decide what the appropriate action is for the user, especially when
the ref is outside the ones we know what the conventional uses of
them are.  We can make policy decisions like "tags are meant to be
unmoving anchor points, so it is unusual to overwrite any old with
any new", "heads are meant to be branch tips, and because rewinding
them while more than one repositories are working with them will
cause issues to other repositories, it is unusual to push a
non-fast-forward" and enforcement mechanism for such policy
decisions will help users, but that is only because we know what
their uses are.

The immediate action we should take is to get closer to the original
behaviour of not complaining with "ref already exists", which is
nonsensical.  That does not mean that we will forbid improving the
codepath by giving different advices depending on the case.

One of the new advices could tell them to "fetch it and inspect the
situation", if old is not something we do not even have (hence we
cannot check its type, let alone the ancestry relationship of it
with new), for example.

Re: [PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Rorvick <chris@rorvick.com> writes:
>
>> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> It is fine when pushing into "refs/tags/" hierarchy.  It is *NOT*
>>> OK if the type check does not satisfy this function.  In that case,
>>> we do not actually see the existence of the destination as a
>>> problem, but it is reported as such.  We are blocking because we do
>>> not like the type of the new object or the type of the old object.
>>> If the destination points at a commit, the push can succeed if the
>>> user changes what object to push, so saying "you cannot push because
>>> the destination already exists" is just wrong in such a case.
>>
>> So the solution is to revert back to recommending a merge?
>
> Of course not, because at that point you may not even have what you
> were attempting to overwrite.  Nobody says it is even something you
> could merge.

I was referring to your concern about rejecting based on type.  A push
causing a reference to move (for example) from a commit to a blob is
rejected as "already exists" with this patch.  You emphatically state
this is not OK and your solution is to revert back to behavior that
advises a merge.

Clearly the bug regarding an 'old' unknown to the client should be
fixed.  This is a obvious test case I should have covered and it's
unfortunate it made it into master.  But I don't understand why
is_forwardable() was misguided (maybe poorly named) nor why
ref_newer() is a better place to solve the issues it was addressing.

Chris

Re: [PATCH v6 0/8] push: update remote tags only with force

[Jeff King]

On Thu, Jan 17, 2013 at 07:09:16AM -0600, Chris Rorvick wrote:

> I was referring to your concern about rejecting based on type.  A push
> causing a reference to move (for example) from a commit to a blob is
> rejected as "already exists" with this patch.  You emphatically state
> this is not OK and your solution is to revert back to behavior that
> advises a merge.
> 
> Clearly the bug regarding an 'old' unknown to the client should be
> fixed.  This is a obvious test case I should have covered and it's
> unfortunate it made it into master.  But I don't understand why
> is_forwardable() was misguided (maybe poorly named) nor why
> ref_newer() is a better place to solve the issues it was addressing.

I think that a type-based rule that relies on knowing the type of the
other side will always have to guess in some cases, because we do not
necessarily have that information. However, if instead of the rule being
"blobs on the remote side cannot be replaced", if it becomes "the old
value on the remote side must be referenced by what we replace it with",
that _is_ something we can calculate reliably on the sending side. And
that is logically an extension of the fast-forward rule, which is why I
suggested placing it with ref_newer (but the latter should probably be
extended to not suggest merging if we _know_ it is a non-commit object).

-Peff

Re: [PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
> However, if instead of the rule being
> "blobs on the remote side cannot be replaced", if it becomes "the old
> value on the remote side must be referenced by what we replace it with",
> that _is_ something we can calculate reliably on the sending side.

Interesting.  I would have thought knowing reachability implied having
the old object in the sending repository.

> And
> that is logically an extension of the fast-forward rule, which is why I
> suggested placing it with ref_newer (but the latter should probably be
> extended to not suggest merging if we _know_ it is a non-commit object).

Sounds great, especially if it is not dependent on the sender actually
having the old object.  Until this is implemented, though, I don't
understand what was wrong with doing the checks in the
is_forwardable() helper function (of course after fixing the
regression/bug.)

Chris

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> However, if instead of the rule being "blobs on the remote side
> cannot be replaced", if it becomes "the old value on the remote
> side must be referenced by what we replace it with", that _is_
> something we can calculate reliably on the sending side.  And that
> is logically an extension of the fast-forward rule,...

It may be an extension of the fast-forward, but only in the graph
reachability sense.  I can buy that it is mathmatically consistent
with the mode that has proven to be useful for commits at the branch
tips, which we know why "fast-forward" rule is an appropriate
default for.  You haven't shown if that mathmatical consistency is
useful for non-commit case.


The primary reason "fast-forward" is a good default for branches is
not that "we do not want to lose objects to gc" (you have reflog for
that).  The reason is non fast-forward is a sign of unintended
rewind, and later will cause duplicated history with merge
conflicts.

That comes from the way objects pointed by refs/heads aka branches
are used.  It is not just "commit" (as object type), but how these
objects are used.  Think why we decided it was a good idea to do one
thing in the topic that introduced the regression under discussion:
"Even if the new commit is a descendant of the old commit, we do not
want to fast-forward a ref if it is under refs/tags/".  Type of object
may be one factor, but how it is used is more important factor in
deciding what kind of policy is appropriate.

If users have workflows that want to have a ref hierarchy that point
at a blob, there will not be any update to such a ref that will
satisfy your definition of "extended" fast-forward requirement, and
that requirement came solely from mathematical purity (i.e. graph
reachability), not from any workflow consideration.  That is very
disturbing to me.

A workflow that employes such a "blob at a ref" may perfectly be
happy with replacing the blob as last-one-wins basis. I do not think
the client side should enforce a policy to forbid such a push.

I personally think the current client side that insists that updates
to any ref has to have the current object and must fast-forward and
requires --force otherwise was a mistake (this predates the change
by Chris).  The receiving end does not implement such an arbitrary
restriction outside refs/heads/, and does so only for refs/heads/
only when deny-non-fast-forwards is set.

Re: [PATCH v6 0/8] push: update remote tags only with force

[Jeff King]

On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:

> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
> > However, if instead of the rule being
> > "blobs on the remote side cannot be replaced", if it becomes "the old
> > value on the remote side must be referenced by what we replace it with",
> > that _is_ something we can calculate reliably on the sending side.
> 
> Interesting.  I would have thought knowing reachability implied having
> the old object in the sending repository.

No, because if you do not have it, then you know it is not reachable
from your refs (or your repository is corrupted). If you do have it, it
_might_ be reachable. For commits, checking is cheap (merge-base) and we
already do it. For trees and blobs, it is much more expensive, as you
have to walk the whole object graph.  While it might be "more correct"
in some sense to say "it's OK to replace a tree with a commit that
points to it", in practice I doubt anyone cares, so you can probably
just punt on those ones and say "no, it's not a fast forward".

> > And
> > that is logically an extension of the fast-forward rule, which is why I
> > suggested placing it with ref_newer (but the latter should probably be
> > extended to not suggest merging if we _know_ it is a non-commit object).
> 
> Sounds great, especially if it is not dependent on the sender actually
> having the old object.  Until this is implemented, though, I don't
> understand what was wrong with doing the checks in the
> is_forwardable() helper function (of course after fixing the
> regression/bug.)

I don't think it is wrong per se; I just think that the check would go
more naturally where we are checking whether the object does indeed
fast-forward. Because is_forwardable in some cases must say "I don't
know; I don't have the object to check its type, so maybe it is
forwardable, and maybe it is not". Whereas when we do the actual
reachability check, we can say definitely "this is not reachable because
I don't have it, or this is not reachable because it is a commit and I
checked, or this might be reachable but I don't care to check because it
has a funny type".

I think looking at it as the latter makes it more obvious how to handle
the "maybe" situation (e.g., the bug in is_forwardable was hard to see).

Anyway, I do not care that much where it goes. To me, the important
thing is the error message. I do think the error "already exists" is a
reasonable one for refs/tags (we do not allow non-force pushes of
existing tags), but not necessarily for other cases, like trying to push
a blob over a blob. The problem there is not "already exists" but rather
"a blob is not something that can fast-forward". Using the existing
REJECT_NONFASTFORWARD is insufficient (because later code will recommend
pull-then-push, which is wrong). So I'd be in favor of creating a new
error status for it.

-Peff

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... The problem there is not "already exists" but rather
> "a blob is not something that can fast-forward". Using the existing
> REJECT_NONFASTFORWARD is insufficient (because later code will recommend
> pull-then-push, which is wrong). So I'd be in favor of creating a new
> error status for it.

Very well said.

Please make it so ;-) or should I?

Re: [PATCH v6 0/8] push: update remote tags only with force

[Chris Rorvick]

On Mon, Jan 21, 2013 at 5:40 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
>
>> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
>> > However, if instead of the rule being
>> > "blobs on the remote side cannot be replaced", if it becomes "the old
>> > value on the remote side must be referenced by what we replace it with",
>> > that _is_ something we can calculate reliably on the sending side.
>>
>> Interesting.  I would have thought knowing reachability implied having
>> the old object in the sending repository.
>
> No, because if you do not have it, then you know it is not reachable
> from your refs (or your repository is corrupted). If you do have it, it
> _might_ be reachable. For commits, checking is cheap (merge-base) and we
> already do it. For trees and blobs, it is much more expensive, as you
> have to walk the whole object graph.  While it might be "more correct"
> in some sense to say "it's OK to replace a tree with a commit that
> points to it", in practice I doubt anyone cares, so you can probably
> just punt on those ones and say "no, it's not a fast forward".

Thanks for explaining this further.  I'm not exactly sure what I was
thinking when I wrote the above other than I didn't fully grasp you
point and responded in a confused state.  Clear on all fronts now.

>> > And
>> > that is logically an extension of the fast-forward rule, which is why I
>> > suggested placing it with ref_newer (but the latter should probably be
>> > extended to not suggest merging if we _know_ it is a non-commit object).
>>
>> Sounds great, especially if it is not dependent on the sender actually
>> having the old object.  Until this is implemented, though, I don't
>> understand what was wrong with doing the checks in the
>> is_forwardable() helper function (of course after fixing the
>> regression/bug.)
>
> I don't think it is wrong per se; I just think that the check would go
> more naturally where we are checking whether the object does indeed
> fast-forward. Because is_forwardable in some cases must say "I don't
> know; I don't have the object to check its type, so maybe it is
> forwardable, and maybe it is not". Whereas when we do the actual
> reachability check, we can say definitely "this is not reachable because
> I don't have it, or this is not reachable because it is a commit and I
> checked, or this might be reachable but I don't care to check because it
> has a funny type".
>
> I think looking at it as the latter makes it more obvious how to handle
> the "maybe" situation (e.g., the bug in is_forwardable was hard to see).
>
> Anyway, I do not care that much where it goes. To me, the important
> thing is the error message. I do think the error "already exists" is a
> reasonable one for refs/tags (we do not allow non-force pushes of
> existing tags), but not necessarily for other cases, like trying to push
> a blob over a blob. The problem there is not "already exists" but rather
> "a blob is not something that can fast-forward". Using the existing
> REJECT_NONFASTFORWARD is insufficient (because later code will recommend
> pull-then-push, which is wrong). So I'd be in favor of creating a new
> error status for it.

I agree with everything above.  I just don't understand why reverting
the "already exists" behavior for non-commit-ish objects was a
prerequisite to fixing this.  Despite the flaws (I am not referring to
the buggy behavior) you and Junio have pointed out, this still seems
like an improvement over the previous (and soon-to-be current)
behavior.  Saying the remote reference already exists is true, and it
implies that removing it might solve the problem which is also true.
Adding another error status will allow the error message to be made
clearer in both cases (i.e., I avoided the word "tag" specifically so
that it would apply to other cases, or so I thought.)

Chris

[PATCH 0/3] Finishing touches to "push" advises

[Junio C Hamano]

This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).

The main change is in the second patch.  When we

 * do not have the object at the tip of the remote;
 * the object at the tip of the remote is not a commit; or
 * the object we are pushing is not a commit,

there is no point suggesting to fetch, integrate and push again.

If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.

Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push.  Just tell
them that such a push requires a force in such a case.

Junio C Hamano (3):
  push: further clean up fields of "struct ref"
  push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
  push: further reduce "struct ref" and simplify the logic

 advice.c            |  4 ++++
 advice.h            |  2 ++
 builtin/push.c      | 25 +++++++++++++++++++++++++
 builtin/send-pack.c | 10 ++++++++++
 cache.h             |  6 +++---
 remote.c            | 38 ++++++++++++++++----------------------
 send-pack.c         |  2 ++
 transport-helper.c  | 10 ++++++++++
 transport.c         | 14 +++++++++++++-
 transport.h         |  2 ++
 10 files changed, 87 insertions(+), 26 deletions(-)

-- 
1.8.1.1.498.gfdee8be

[PATCH 1/3] push: further clean up fields of "struct ref"

[Junio C Hamano]

The "nonfastforward" field is only used to decide what value to
assign to the "status" locally in a single function.  Remove it from
the "struct ref" and make it into a local variable.

The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere.  It is used by status reporting code that
the particular update was "forced".  Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |  3 +--
 remote.c    | 10 +++++-----
 transport.c |  2 +-
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/cache.h b/cache.h
index a942bbd..baa47b4 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,9 +1001,8 @@ struct ref {
 	char *symref;
 	unsigned int
 		force:1,
-		requires_force:1,
+		forced_update:1,
 		merge:1,
-		nonfastforward:1,
 		update:1,
 		deletion:1;
 	enum {
diff --git a/remote.c b/remote.c
index d3a1ca2..875296c 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,22 +1322,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			!is_null_sha1(ref->old_sha1);
 
 		if (ref->update) {
-			ref->nonfastforward =
+			int nonfastforward =
 				!has_sha1_file(ref->old_sha1)
-				  || !ref_newer(ref->new_sha1, ref->old_sha1);
+				|| !ref_newer(ref->new_sha1, ref->old_sha1);
 
 			if (!prefixcmp(ref->name, "refs/tags/")) {
-				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
 					continue;
 				}
-			} else if (ref->nonfastforward) {
-				ref->requires_force = 1;
+				ref->forced_update = 1;
+			} else if (nonfastforward) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 					continue;
 				}
+				ref->forced_update = 1;
 			}
 		}
 	}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 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->requires_force) {
+		if (ref->forced_update) {
 			strcat(quickref, "...");
 			type = '+';
 			msg = "forced update";
-- 
1.8.1.1.498.gfdee8be

[PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

When pushing update an existing ref, we wouldn't even know if we are
fast-forwarding the ref on the other end if:

 * we do not have the object currently at the tip of remote;
 * the object currently at the tip of remote is not a committish; or
 * the object we are pushing is not a committish.

In such a case, the push has been rejected on the client end, but we
used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again.  This did not make much sense.

Introduce two error classes and suggest fetching from the other side
first and evaluate the situation, if we do not have the current
object, or just tell the user the update needs --force when we do
have the current object and either it or the object we are trying to
push is not a committish, in which case it can never fast-forward
and we know there is no point suggesting to merge.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 advice.c            |  4 ++++
 advice.h            |  2 ++
 builtin/push.c      | 25 +++++++++++++++++++++++++
 builtin/send-pack.c | 10 ++++++++++
 cache.h             |  2 ++
 remote.c            | 22 ++++++++++++++++------
 send-pack.c         |  2 ++
 transport-helper.c  | 10 ++++++++++
 transport.c         | 12 ++++++++++++
 transport.h         |  2 ++
 10 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
 int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
 int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
 	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
 	{ "pushalreadyexists", &advice_push_already_exists },
+	{ "pushfetchfirst", &advice_push_fetch_first },
+	{ "pushneedsforce", &advice_push_needs_force },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
 extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
 extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =
 	N_("Updates were rejected because the destination reference already exists\n"
 	   "in the remote.");
 
+static const char message_advice_ref_fetch_first[] =
+	N_("Updates were rejected; you need to fetch the destination reference\n"
+	   "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+	N_("Updates were rejected; you need to force update.\n");
+
 static void advise_pull_before_push(void)
 {
 	if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
 	advise(_(message_advice_ref_already_exists));
 }
 
+static void advise_ref_fetch_first(void)
+{
+	if (!advice_push_fetch_first || !advice_push_update_rejected)
+		return;
+	advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+	if (!advice_push_needs_force || !advice_push_update_rejected)
+		return;
+	advise(_(message_advice_ref_needs_force));
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)
 			advise_checkout_pull_push();
 	} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
 		advise_ref_already_exists();
+	} else if (reject_reasons & REJECT_FETCH_FIRST) {
+		advise_ref_fetch_first();
+	} else if (reject_reasons & REJECT_NEEDS_FORCE) {
+		advise_ref_needs_force();
 	}
 
 	return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_FETCH_FIRST:
+			res = "error";
+			msg = "fetch first";
+			break;
+
+		case REF_STATUS_REJECT_NEEDS_FORCE:
+			res = "error";
+			msg = "needs force";
+			break;
+
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			res = "error";
 			msg = "already exists";
diff --git a/cache.h b/cache.h
index baa47b4..360bba5 100644
--- a/cache.h
+++ b/cache.h
@@ -1011,6 +1011,8 @@ struct ref {
 		REF_STATUS_REJECT_NONFASTFORWARD,
 		REF_STATUS_REJECT_ALREADY_EXISTS,
 		REF_STATUS_REJECT_NODELETE,
+		REF_STATUS_REJECT_FETCH_FIRST,
+		REF_STATUS_REJECT_NEEDS_FORCE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 875296c..689dcf7 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,17 +1322,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			!is_null_sha1(ref->old_sha1);
 
 		if (ref->update) {
-			int nonfastforward =
-				!has_sha1_file(ref->old_sha1)
-				|| !ref_newer(ref->new_sha1, ref->old_sha1);
-
 			if (!prefixcmp(ref->name, "refs/tags/")) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
 					continue;
 				}
 				ref->forced_update = 1;
-			} else if (nonfastforward) {
+			} else if (!has_sha1_file(ref->old_sha1) ||
+				   !lookup_commit_reference_gently(ref->old_sha1, 1)) {
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+					continue;
+				}
+				ref->forced_update = 1;
+			} else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+					continue;
+				}
+				ref->forced_update = 1;
+			} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 					continue;
@@ -1521,7 +1530,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
 	struct commit_list *list, *used;
 	int found = 0;
 
-	/* Both new and old must be commit-ish and new is descendant of
+	/*
+	 * Both new and old must be commit-ish and new is descendant of
 	 * old.  Otherwise we require --force.
 	 */
 	o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
+		case REF_STATUS_REJECT_FETCH_FIRST:
+		case REF_STATUS_REJECT_NEEDS_FORCE:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "fetch first")) {
+			status = REF_STATUS_REJECT_FETCH_FIRST;
+			free(msg);
+			msg = NULL;
+		}
+		else if (!strcmp(msg, "needs force")) {
+			status = REF_STATUS_REJECT_NEEDS_FORCE;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "already exists", porcelain);
 		break;
+	case REF_STATUS_REJECT_FETCH_FIRST:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "fetch first", porcelain);
+		break;
+	case REF_STATUS_REJECT_NEEDS_FORCE:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "needs force", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 				*reject_reasons |= REJECT_NON_FF_OTHER;
 		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
 			*reject_reasons |= REJECT_ALREADY_EXISTS;
+		} else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+			*reject_reasons |= REJECT_FETCH_FIRST;
+		} else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+			*reject_reasons |= REJECT_NEEDS_FORCE;
 		}
 	}
 }
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ 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
+#define REJECT_FETCH_FIRST     0x08
+#define REJECT_NEEDS_FORCE     0x10
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-- 
1.8.1.1.498.gfdee8be

[PATCH 3/3] push: further reduce "struct ref" and simplify the logic

[Junio C Hamano]

The "update" field in "struct ref" is only used in a very narrow
scope in a single function.  Remove it.

Also simplify the code that rejects an attempted push by first
checking if the proposed update is forced (in which case we do not
need any check on our end).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h  |  1 -
 remote.c | 42 +++++++++++++-----------------------------
 2 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/cache.h b/cache.h
index 360bba5..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,7 +1003,6 @@ struct ref {
 		force:1,
 		forced_update:1,
 		merge:1,
-		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 689dcf7..248910f 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,37 +1317,21 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     passing the --force argument
 		 */
 
-		ref->update =
-			!ref->deletion &&
-			!is_null_sha1(ref->old_sha1);
-
-		if (ref->update) {
-			if (!prefixcmp(ref->name, "refs/tags/")) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
-					continue;
-				}
-				ref->forced_update = 1;
-			} else if (!has_sha1_file(ref->old_sha1) ||
-				   !lookup_commit_reference_gently(ref->old_sha1, 1)) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_FETCH_FIRST;
-					continue;
-				}
-				ref->forced_update = 1;
-			} else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
-					continue;
-				}
-				ref->forced_update = 1;
-			} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-					continue;
-				}
+		if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+			if (force_ref_update) {
 				ref->forced_update = 1;
+				continue;
 			}
+
+			if (!prefixcmp(ref->name, "refs/tags/"))
+				ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+			else if (!has_sha1_file(ref->old_sha1) ||
+				 !lookup_commit_reference_gently(ref->old_sha1, 1))
+				ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+			else if (!lookup_commit_reference_gently(ref->new_sha1, 1))
+				ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+			else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 		}
 	}
 }
-- 
1.8.1.1.498.gfdee8be

Re: [PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

This one has a logic flaw.  The logic outlined in the cover letter
is correct, and the one described in the log message of this one is
not.

We should say "fetch first" only when we do not have old_sha1.


diff --git a/remote.c b/remote.c
index 248910f..8c39ea2 100644
--- a/remote.c
+++ b/remote.c
@@ -1325,10 +1325,10 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 			if (!prefixcmp(ref->name, "refs/tags/"))
 				ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
-			else if (!has_sha1_file(ref->old_sha1) ||
-				 !lookup_commit_reference_gently(ref->old_sha1, 1))
+			else if (!has_sha1_file(ref->old_sha1))
 				ref->status = REF_STATUS_REJECT_FETCH_FIRST;
-			else if (!lookup_commit_reference_gently(ref->new_sha1, 1))
+			else if (!lookup_commit_reference_gently(ref->new_sha1, 1) ||
+				 !lookup_commit_reference_gently(ref->old_sha1, 1))
 				ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
 			else if (!ref_newer(ref->new_sha1, ref->old_sha1))
 				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;

Re: [PATCH 3/3] push: further reduce "struct ref" and simplify the logic

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The "update" field in "struct ref" is only used in a very narrow
> scope in a single function.  Remove it.
>
> Also simplify the code that rejects an attempted push by first
> checking if the proposed update is forced (in which case we do not
> need any check on our end).

Actually, the latter is a bad idea; it changes the semantics and
mark a push that was done with an unnecessary --force option.

I'm rerolling these three patches (the "update" removal will also be
moved to the preparatory clean-up patch).

[PATCH 0/3] Finishing touches to "push" advises

[Junio C Hamano]

This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).

The series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).

The main change is in the second patch.  When we

 * do not have the object at the tip of the remote;
 * the object at the tip of the remote is not a commit; or
 * the object we are pushing is not a commit,

there is no point suggesting to fetch, integrate and push again.

If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.

Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push.  Just tell
them that such a push requires a force in such a case.

Junio C Hamano (3):
  push: further clean up fields of "struct ref"
  push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE
  push: further simplify the logic to assign rejection status

 advice.c            |  4 ++++
 advice.h            |  2 ++
 builtin/push.c      | 25 +++++++++++++++++++++++++
 builtin/send-pack.c | 10 ++++++++++
 cache.h             |  6 +++---
 remote.c            | 42 +++++++++++++++++++-----------------------
 send-pack.c         |  2 ++
 transport-helper.c  | 10 ++++++++++
 transport.c         | 14 +++++++++++++-
 transport.h         |  2 ++
 10 files changed, 90 insertions(+), 27 deletions(-)

-- 
1.8.1.1.498.gfdee8be

[PATCH v2 1/3] push: further clean up fields of "struct ref"

[Junio C Hamano]

The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function.  Remove them from the "struct ref".

The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere.  It is used by status reporting code that
the particular update was "forced".  Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The "update" removal in v1 has been moved to this.

 cache.h     |  4 +---
 remote.c    | 16 ++++++----------
 transport.c |  2 +-
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/cache.h b/cache.h
index a942bbd..12631a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,10 +1001,8 @@ struct ref {
 	char *symref;
 	unsigned int
 		force:1,
-		requires_force:1,
+		forced_update:1,
 		merge:1,
-		nonfastforward:1,
-		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index d3a1ca2..3375914 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,27 +1317,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     passing the --force argument
 		 */
 
-		ref->update =
-			!ref->deletion &&
-			!is_null_sha1(ref->old_sha1);
-
-		if (ref->update) {
-			ref->nonfastforward =
+		if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+			int nonfastforward =
 				!has_sha1_file(ref->old_sha1)
-				  || !ref_newer(ref->new_sha1, ref->old_sha1);
+				|| !ref_newer(ref->new_sha1, ref->old_sha1);
 
 			if (!prefixcmp(ref->name, "refs/tags/")) {
-				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
 					continue;
 				}
-			} else if (ref->nonfastforward) {
-				ref->requires_force = 1;
+				ref->forced_update = 1;
+			} else if (nonfastforward) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 					continue;
 				}
+				ref->forced_update = 1;
 			}
 		}
 	}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 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->requires_force) {
+		if (ref->forced_update) {
 			strcat(quickref, "...");
 			type = '+';
 			msg = "forced update";
-- 
1.8.1.1.498.gfdee8be

[PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

When we push to update an existing ref, if:

 * we do not have the object at the tip of the remote; or
 * the object at the tip of the remote is not a commit; or
 * the object we are pushing is not a commit,

there is no point suggesting to fetch, integrate and push again.

If we do not have the current object at the tip of the remote, we
should tell the user to fetch first and evaluate the situation
before deciding what to do next.

Otherwise, if the current object is not a commit, or if we are
trying to push an object that is not a commit, then the user does
not have to fetch first (we already have the object), but it still
does not make sense to suggest to integrate and re-push.  Just tell
them that such a push requires a force in such a case.

In these cases, the push was locally rejected on the client end, but
we used the same error and advice messages as the ones used when
rejecting a non-fast-forward push, i.e. pull from there and
integrate before pushing again.  This did not make much sense.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Updated log message and fixed the logic to decide "fetch first";
   we should say "fetch first" only when we do not have the current
   tip of the remote end.

   send-pack.c has style violation that "else" is not on the same
   line as closing brace of its corresponding "if", but I followed
   the existing style of surrounding code.  Cleaning them up is for
   a separate topic.

 advice.c            |  4 ++++
 advice.h            |  2 ++
 builtin/push.c      | 25 +++++++++++++++++++++++++
 builtin/send-pack.c | 10 ++++++++++
 cache.h             |  2 ++
 remote.c            | 22 ++++++++++++++++------
 send-pack.c         |  2 ++
 transport-helper.c  | 10 ++++++++++
 transport.c         | 12 ++++++++++++
 transport.h         |  2 ++
 10 files changed, 85 insertions(+), 6 deletions(-)

diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
 int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
 int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
 	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
 	{ "pushalreadyexists", &advice_push_already_exists },
+	{ "pushfetchfirst", &advice_push_fetch_first },
+	{ "pushneedsforce", &advice_push_needs_force },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
 extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
 extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..da928fa 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -224,6 +224,13 @@ static const char message_advice_ref_already_exists[] =
 	N_("Updates were rejected because the destination reference already exists\n"
 	   "in the remote.");
 
+static const char message_advice_ref_fetch_first[] =
+	N_("Updates were rejected; you need to fetch the destination reference\n"
+	   "to decide what to do.\n");
+
+static const char message_advice_ref_needs_force[] =
+	N_("Updates were rejected; you need to force update.\n");
+
 static void advise_pull_before_push(void)
 {
 	if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)
 	advise(_(message_advice_ref_already_exists));
 }
 
+static void advise_ref_fetch_first(void)
+{
+	if (!advice_push_fetch_first || !advice_push_update_rejected)
+		return;
+	advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+	if (!advice_push_needs_force || !advice_push_update_rejected)
+		return;
+	advise(_(message_advice_ref_needs_force));
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)
 			advise_checkout_pull_push();
 	} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
 		advise_ref_already_exists();
+	} else if (reject_reasons & REJECT_FETCH_FIRST) {
+		advise_ref_fetch_first();
+	} else if (reject_reasons & REJECT_NEEDS_FORCE) {
+		advise_ref_needs_force();
 	}
 
 	return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_FETCH_FIRST:
+			res = "error";
+			msg = "fetch first";
+			break;
+
+		case REF_STATUS_REJECT_NEEDS_FORCE:
+			res = "error";
+			msg = "needs force";
+			break;
+
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			res = "error";
 			msg = "already exists";
diff --git a/cache.h b/cache.h
index 12631a1..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1010,6 +1010,8 @@ struct ref {
 		REF_STATUS_REJECT_NONFASTFORWARD,
 		REF_STATUS_REJECT_ALREADY_EXISTS,
 		REF_STATUS_REJECT_NODELETE,
+		REF_STATUS_REJECT_FETCH_FIRST,
+		REF_STATUS_REJECT_NEEDS_FORCE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 3375914..c991915 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,17 +1318,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 */
 
 		if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
-			int nonfastforward =
-				!has_sha1_file(ref->old_sha1)
-				|| !ref_newer(ref->new_sha1, ref->old_sha1);
-
 			if (!prefixcmp(ref->name, "refs/tags/")) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
 					continue;
 				}
 				ref->forced_update = 1;
-			} else if (nonfastforward) {
+			} else if (!has_sha1_file(ref->old_sha1)) {
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_FETCH_FIRST;
+					continue;
+				}
+				ref->forced_update = 1;
+			} else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+				   !lookup_commit_reference_gently(ref->new_sha1, 1)) {
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
+					continue;
+				}
+				ref->forced_update = 1;
+			} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 					continue;
@@ -1517,7 +1526,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
 	struct commit_list *list, *used;
 	int found = 0;
 
-	/* Both new and old must be commit-ish and new is descendant of
+	/*
+	 * Both new and old must be commit-ish and new is descendant of
 	 * old.  Otherwise we require --force.
 	 */
 	o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
+		case REF_STATUS_REJECT_FETCH_FIRST:
+		case REF_STATUS_REJECT_NEEDS_FORCE:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "fetch first")) {
+			status = REF_STATUS_REJECT_FETCH_FIRST;
+			free(msg);
+			msg = NULL;
+		}
+		else if (!strcmp(msg, "needs force")) {
+			status = REF_STATUS_REJECT_NEEDS_FORCE;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "already exists", porcelain);
 		break;
+	case REF_STATUS_REJECT_FETCH_FIRST:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "fetch first", porcelain);
+		break;
+	case REF_STATUS_REJECT_NEEDS_FORCE:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "needs force", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 				*reject_reasons |= REJECT_NON_FF_OTHER;
 		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
 			*reject_reasons |= REJECT_ALREADY_EXISTS;
+		} else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+			*reject_reasons |= REJECT_FETCH_FIRST;
+		} else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+			*reject_reasons |= REJECT_NEEDS_FORCE;
 		}
 	}
 }
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ 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
+#define REJECT_FETCH_FIRST     0x08
+#define REJECT_NEEDS_FORCE     0x10
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-- 
1.8.1.1.498.gfdee8be

[PATCH v2 3/3] push: further simplify the logic to assign rejection status

[Junio C Hamano]

Instead of using deeply nested if/else statements, first decide what
rejection status we would get if this push weren't forced, and then
assign the rejection reason to the ref->status field and flip the
ref->forced_update field when we forced a push for a ref that indeed
required forcing.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The first one mistakenly changed the semantics and reported a
   forced push even when the push was done with useless and
   unnecessary --force option (e.g. the update was properly
   fast-forwarding but --force was given from the command line).
   This fixes it.

 remote.c | 40 +++++++++++++++-------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/remote.c b/remote.c
index c991915..af2136d 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,32 +1318,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 */
 
 		if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
-			if (!prefixcmp(ref->name, "refs/tags/")) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
-					continue;
-				}
+			int status = 0;
+
+			if (!prefixcmp(ref->name, "refs/tags/"))
+				status = REF_STATUS_REJECT_ALREADY_EXISTS;
+			else if (!has_sha1_file(ref->old_sha1))
+				status = REF_STATUS_REJECT_FETCH_FIRST;
+			else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+				 !lookup_commit_reference_gently(ref->new_sha1, 1))
+				status = REF_STATUS_REJECT_NEEDS_FORCE;
+			else if (!ref_newer(ref->new_sha1, ref->old_sha1))
+				status = REF_STATUS_REJECT_NONFASTFORWARD;
+
+			if (!force_ref_update)
+				ref->status = status;
+			else if (status)
 				ref->forced_update = 1;
-			} else if (!has_sha1_file(ref->old_sha1)) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_FETCH_FIRST;
-					continue;
-				}
-				ref->forced_update = 1;
-			} else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
-				   !lookup_commit_reference_gently(ref->new_sha1, 1)) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_NEEDS_FORCE;
-					continue;
-				}
-				ref->forced_update = 1;
-			} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-					continue;
-				}
-				ref->forced_update = 1;
-			}
 		}
 	}
 }
-- 
1.8.1.1.498.gfdee8be

Re: [PATCH v6 0/8] push: update remote tags only with force

[Junio C Hamano]

Chris Rorvick <chris@rorvick.com> writes:

> I agree with everything above.  I just don't understand why reverting
> the "already exists" behavior for non-commit-ish objects was a
> prerequisite to fixing this.

Because it is a regression.  People who did not force such a push
did not get "already exists", but with your patch they do.

By reverting the wrong message so that we get the old wrong message
instead, people will only have to deal with an already known
breakage; a known devil is better than an unknown new devil (or an
unknown angel).

When a change that brings in a regression and an improvement at the
same time, it does not matter what the improvement is; we fix the
regression first as soon as safely possible and we then attempt to
resurrect and polish the improvement.

Re: [PATCH 0/3] Finishing touches to "push" advises

[Junio C Hamano]

As far as I am concerned, I am pretty much done with this topic, at
least for now.  Of course if there are bugreports I'll try to help
resolving them, but I do not expect myself adding new object-type
based policy decision to this codepath.

The call the updated call makes to ref_newer() no longer feeds
certain combinations to the function, because the NULL-ness of the
old and commit-ness of both are checked before making a call.

I notice that builtin/remote.c has another callsite for ref_newer().
Although I didn't look at the code, I think it is trying to see if
the branch can be pushed as a fast-forward to the remote (or the
remote tip moved since you started building on top of it).

It probably makes sense to refactor the logic that is run per-ref in
the loop in the set_ref_status_for_push() function into a new helper
function, inline ref_newer() there, and have the remaining callers
of ref_newer() to use that new helper function, which knows the new
rules such as "refs/tags/ cannot be replaced with anything without
force".

Re: [PATCH v2 1/3] push: further clean up fields of "struct ref"

[Jeff King]

On Mon, Jan 21, 2013 at 10:30:28PM -0800, Junio C Hamano wrote:

> The "nonfastforward" and "update" fields are only used while
> deciding what value to assign to the "status" locally in a single
> function.  Remove them from the "struct ref".
> 
> The "requires_force" field is not used to decide if the proposed
> update requires a --force option to succeed, or to record such a
> decision made elsewhere.  It is used by status reporting code that
> the particular update was "forced".  Rename it to "forced_udpate",

Typo.

> and move the code to assign to it around to further clarify how it
> is used and what it is used for.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * The "update" removal in v1 has been moved to this.
> 
>  cache.h     |  4 +---
>  remote.c    | 16 ++++++----------
>  transport.c |  2 +-
>  3 files changed, 8 insertions(+), 14 deletions(-)

Looks much better.

I wondered briefly why nonfastforward was even there, as I recall that I
was the one who added it many years ago. It turns out that it used to
serve the purpose of the new forced_update, but Chris's series from a
few months ago split it out to "nonfastforward" and "not_forwardable",
and then added "requires_force" to give a single flag that is set in
either case.

So I think your simplification is correct; the first two can be local
variables, and the only thing that matters to carry forward is
requires_force (and I agree that forced_update is a better name).

-Peff

Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Jeff King]

On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:

> When we push to update an existing ref, if:
> 
>  * we do not have the object at the tip of the remote; or
>  * the object at the tip of the remote is not a commit; or
>  * the object we are pushing is not a commit,
> 
> there is no point suggesting to fetch, integrate and push again.
> 
> If we do not have the current object at the tip of the remote, we
> should tell the user to fetch first and evaluate the situation
> before deciding what to do next.

Should we? I know that it is more correct to do so, because we do not
even know for sure that the remote object is a commit, and fetching
_might_ lead to us saying "hey, this is not something that can be
fast-forwarded".

But by far the common case will be that it _is_ a commit, and the right
thing is going to be to pull. Adding in the extra steps makes the
workflow longer and more complicated, and most of the time doesn't
matter. For example, imagine that Alice is working on "master", and when
she goes to push, she finds that Bob has already pushed his work. With
the current code, she sees:

  $ git push
  To ...
   ! [rejected]        HEAD -> master (non-fast-forward)
  error: failed to push some refs to '...'
  hint: Updates were rejected because the tip of your current branch is behind
  hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
  hint: before pushing again.

and she presumably pulls, and all is well with the follow-up push.

With your patch, she sees:

  $ git push
  To ...
   ! [rejected]        HEAD -> master (fetch first)
  error: failed to push some refs to '...'
  hint: Updates were rejected; you need to fetch the destination reference
  hint: to decide what to do.

  $ git fetch
  ...

  $ git push
  To ...
   ! [rejected]        HEAD -> master (non-fast-forward)
  error: failed to push some refs to '...'
  hint: Updates were rejected because the tip of your current branch is behind
  hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
  hint: before pushing again.
  hint: See the 'Note about fast-forwards' in 'git push --help' for details.

which is technically more correct (it's possible that in the second
step, she would find that Bob pushed a tree or something). But in the
common case that it is a commit, we've needlessly added two extra steps
(a fetch and another failed push), both of which involve network access
(so they are slow, and may involve Alice having to type her credentials).

Is the extra hassle in the common case worth it for the off chance that
we might give a more accurate message? Should the "fetch first" message
be some hybrid that covers both cases accurately, but still points the
user towards "git pull" (which will fail anyway if the remote ref is not
a commit)?

-Peff

Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:
>
>> When we push to update an existing ref, if:
>> 
>>  * we do not have the object at the tip of the remote; or
>>  * the object at the tip of the remote is not a commit; or
>>  * the object we are pushing is not a commit,
>> 
>> there is no point suggesting to fetch, integrate and push again.
>> 
>> If we do not have the current object at the tip of the remote, we
>> should tell the user to fetch first and evaluate the situation
>> before deciding what to do next.
>
> Should we? I know that it is more correct to do so, because we do not
> even know for sure that the remote object is a commit, and fetching
> _might_ lead to us saying "hey, this is not something that can be
> fast-forwarded".
>
> But by far the common case will be that it _is_ a commit, and the right
> thing is going to be to pull....
> Is the extra hassle in the common case worth it for the off chance that
> we might give a more accurate message? Should the "fetch first" message
> be some hybrid that covers both cases accurately, but still points the
> user towards "git pull" (which will fail anyway if the remote ref is not
> a commit)?

I was actually much less happy with "needs force" than this one, as
you have to assume too many things for the message to be a useful
and a safe advise: the user has actually examined the situation and
forcing the push is the right thing to do.  Both old and new objects
exist, so the user _could_ have done so, but did he really check
them, thought about the situation and made the right decision?
Perhaps the attempted push had a typo in the object name it wanted
to update the other end with, and the right thing to do is not to
force but to fix the refspec instead?  "You need --force to perform
this push" was a very counter-productive advice in this case, but I
didn't think of a better wording.

The "fetch first and inspect" was an attempt to reduce the risk of
that "needs force" message that could encourage brainless forced
pushes.  Perhaps if we reword "needs force" to something less risky,
we do not have to be so explicit in "You have to fetch first and
examine".

How about doing this?

For "needs force" cases, we say this instead:

 hint: you cannot update a ref that points at a non-commit object, or
 hint: update a ref to point at a non-commit object, without --force.

Being explicit about "non-commit" twice will catch user's eyes and
cause him to double check that it is not a mistyped LHS of the push
refspec (if he is sending a non-commit) or mistyped RHS (if the ref
is pointing at a non-commit).  If he _is_ trying to push a blob out,
the advice makes it clear what to do next: he does want to force it.

If we did that, then we could loosen the "You should fetch first"
case to say something like this:

 hint: you do not have the object at the tip of the remote ref;
 hint: perhaps you want to pull from there first?

This explicitly denies one of Chris's wish "we shouldn't suggest to
merge something that we may not be able to", but in the "You should
fetch first" case, we cannot fundamentally know if we can merge
until we fetch.  I agree with you that the most common case is that
the unknown object is a commit, and that suggesting to pull is a
good compromise.

Note that you _could_ split the "needs force" case into two, namely,
"cannot replace a non-commit" and "cannot push a non-commit".  You
could even further split them into combinations (e.g. an attempt to
replace an annotated tag with a commit and an attempt to replace a
tree with a commit may be different situations), but I think the
advices we can give to these cases would end up being the same, so I
tend to think it is not worth it.  That is what I meant by "I do not
expect me doing the type-based policy myself" in the concluding
message of the series.

[PATCH v4 0/3] Finishing touches to "push" advises

[Junio C Hamano]

This builds on Chris Rorvick's earlier effort to forbid unforced
updates to refs/tags/ hierarchy and giving sensible error and advise
messages for that case (we are not rejecting such a push due to fast
forwardness, and suggesting to fetch and integrate before pushing
again does not make sense).

The series applies on top of 256b9d7 (push: fix "refs/tags/
hierarchy cannot be updated without --force", 2013-01-16).

This fourth round swaps the order of clean-up patches and now the
bottom two are clean-up patches.  The main change is in the third
one.

When the object at the tip of the remote is not a committish, or the
object we are pushing is not a committish, the existing code already
rejects such a push on the client end, but we used the same error
and advice messages as the ones used when rejecting a push that does
not fast-forward, i.e. pull from there and integrate before pushing
again.  Introduce a new rejection reason NEEDS_FORCE and explain why
the push was rejected, stressing the fact that --force is required
when non committish objects are involved, so that the user can (1)
notice a possibly mistyped source object name or destination ref
name, when the user is trying to push an ordinary commit, or (2)
learn that "--force" is an appropriate thing to use when the user is
sure that s/he wants to push a non-committish (which is unusual).

Unlike the third round, we do not say "fetch first, inspect the
situation to decide what to do", when we do not have the object
sitting at the tip of the remote.  Most likely, it is a commit
somebody who has been working on the same branch pushed that we
haven't fetched yet, so suggesting to pull is often sufficient and
appropriate, and in a more uncommon case in which the unknown object
is not a committish, the suggested pull will fail without making
permanent damage anywhere.  Next atttempt to push without changing
anything (e.g. "reset --hard") will then trigger the NEEDS_FORCE
"Your push involves non-commit objects" case.


Junio C Hamano (3):
  push: further clean up fields of "struct ref"
  push: further simplify the logic to assign rejection reason
  push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

 Documentation/config.txt | 12 +++++++++++-
 advice.c                 |  4 ++++
 advice.h                 |  2 ++
 builtin/push.c           | 29 +++++++++++++++++++++++++++++
 builtin/send-pack.c      | 10 ++++++++++
 cache.h                  |  6 +++---
 remote.c                 | 42 +++++++++++++++++++-----------------------
 send-pack.c              |  2 ++
 transport-helper.c       | 10 ++++++++++
 transport.c              | 14 +++++++++++++-
 transport.h              |  2 ++
 11 files changed, 105 insertions(+), 28 deletions(-)

-- 
1.8.1.1.517.g0318d2b

[PATCH v4 1/3] push: further clean up fields of "struct ref"

[Junio C Hamano]

The "nonfastforward" and "update" fields are only used while
deciding what value to assign to the "status" locally in a single
function.  Remove them from the "struct ref".

The "requires_force" field is not used to decide if the proposed
update requires a --force option to succeed, or to record such a
decision made elsewhere.  It is used by status reporting code that
the particular update was "forced".  Rename it to "forced_udpate",
and move the code to assign to it around to further clarify how it
is used and what it is used for.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h     |  4 +---
 remote.c    | 16 ++++++----------
 transport.c |  2 +-
 3 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/cache.h b/cache.h
index a942bbd..12631a1 100644
--- a/cache.h
+++ b/cache.h
@@ -1001,10 +1001,8 @@ struct ref {
 	char *symref;
 	unsigned int
 		force:1,
-		requires_force:1,
+		forced_update:1,
 		merge:1,
-		nonfastforward:1,
-		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index d3a1ca2..3375914 100644
--- a/remote.c
+++ b/remote.c
@@ -1317,27 +1317,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     passing the --force argument
 		 */
 
-		ref->update =
-			!ref->deletion &&
-			!is_null_sha1(ref->old_sha1);
-
-		if (ref->update) {
-			ref->nonfastforward =
+		if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
+			int nonfastforward =
 				!has_sha1_file(ref->old_sha1)
-				  || !ref_newer(ref->new_sha1, ref->old_sha1);
+				|| !ref_newer(ref->new_sha1, ref->old_sha1);
 
 			if (!prefixcmp(ref->name, "refs/tags/")) {
-				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
 					continue;
 				}
-			} else if (ref->nonfastforward) {
-				ref->requires_force = 1;
+				ref->forced_update = 1;
+			} else if (nonfastforward) {
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 					continue;
 				}
+				ref->forced_update = 1;
 			}
 		}
 	}
diff --git a/transport.c b/transport.c
index 2673d27..585ebcd 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->requires_force) {
+		if (ref->forced_update) {
 			strcat(quickref, "...");
 			type = '+';
 			msg = "forced update";
-- 
1.8.1.1.517.g0318d2b

[PATCH v4 2/3] push: further simplify the logic to assign rejection reason

[Junio C Hamano]

First compute the reason why this push would fail if done without
"--force", and then fail it by assigning that reason when the push
was not forced (or if there is no reason to require force, allow it
to succeed).

Record the fact that the push was forced in the forced_update field
only when the push would have failed without the option.

The code becomes shorter, less repetitive and easier to read this
way, especially given that the set of rejection reasons will be
extended in a later patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/remote.c b/remote.c
index 3375914..969aa11 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,23 +1318,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 */
 
 		if (!ref->deletion && !is_null_sha1(ref->old_sha1)) {
-			int nonfastforward =
-				!has_sha1_file(ref->old_sha1)
-				|| !ref_newer(ref->new_sha1, ref->old_sha1);
-
-			if (!prefixcmp(ref->name, "refs/tags/")) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
-					continue;
-				}
-				ref->forced_update = 1;
-			} else if (nonfastforward) {
-				if (!force_ref_update) {
-					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-					continue;
-				}
+			int why = 0; /* why would this push require --force? */
+
+			if (!prefixcmp(ref->name, "refs/tags/"))
+				why = REF_STATUS_REJECT_ALREADY_EXISTS;
+			else if (!has_sha1_file(ref->old_sha1)
+				 || !ref_newer(ref->new_sha1, ref->old_sha1))
+				why = REF_STATUS_REJECT_NONFASTFORWARD;
+
+			if (!force_ref_update)
+				ref->status = why;
+			else if (why)
 				ref->forced_update = 1;
-			}
 		}
 	}
 }
-- 
1.8.1.1.517.g0318d2b

[PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

When we push to update an existing ref, if:

 * the object at the tip of the remote is not a commit; or
 * the object we are pushing is not a commit,

it won't be correct to suggest to fetch, integrate and push again,
as the old and new objects will not "merge".

If we do not have the current object at the tip of the remote, we do
not even know that object, when fetched, is something that can be
merged.  In such a case, suggesting to pull first just like
non-fast-forward case may not be technically correct, but in
practice, most such failures are seen when you try to push your work
to a branch without knowing that somebody else already pushed to
update the same branch since you forked, so "pull first" would work
as a suggestion most of the time.

In these cases, the current code already rejects such a push on the
client end, but we used the same error and advice messages as the
ones used when rejecting a non-fast-forward push, i.e. pull from
there and integrate before pushing again.  Introduce new
rejection reasons and reword the messages appropriately.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 12 +++++++++++-
 advice.c                 |  4 ++++
 advice.h                 |  2 ++
 builtin/push.c           | 29 +++++++++++++++++++++++++++++
 builtin/send-pack.c      | 10 ++++++++++
 cache.h                  |  2 ++
 remote.c                 | 11 ++++++++---
 send-pack.c              |  2 ++
 transport-helper.c       | 10 ++++++++++
 transport.c              | 12 ++++++++++++
 transport.h              |  2 ++
 11 files changed, 92 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 90e7d10..1f47761 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -143,7 +143,8 @@ advice.*::
 	pushUpdateRejected::
 		Set this variable to 'false' if you want to disable
 		'pushNonFFCurrent', 'pushNonFFDefault',
-		'pushNonFFMatching', and 'pushAlreadyExists'
+		'pushNonFFMatching', 'pushAlreadyExists',
+		'pushFetchFirst', and 'pushNeedsForce'
 		simultaneously.
 	pushNonFFCurrent::
 		Advice shown when linkgit:git-push[1] fails due to a
@@ -162,6 +163,15 @@ advice.*::
 	pushAlreadyExists::
 		Shown when linkgit:git-push[1] rejects an update that
 		does not qualify for fast-forwarding (e.g., a tag.)
+	pushFetchFirst::
+		Shown when linkgit:git-push[1] rejects an update that
+		tries to overwrite a remote ref that points at an
+		object we do not have.
+	pushNeedsForce::
+		Shown when linkgit:git-push[1] rejects an update that
+		tries to overwrite a remote ref that points at an
+		object that is not a committish, or make the remote
+		ref point at an object that is not a committish.
 	statusHints::
 		Show directions on how to proceed from the current
 		state in the output of linkgit:git-status[1] and in
diff --git a/advice.c b/advice.c
index d287927..780f58d 100644
--- a/advice.c
+++ b/advice.c
@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;
 int advice_push_non_ff_default = 1;
 int advice_push_non_ff_matching = 1;
 int advice_push_already_exists = 1;
+int advice_push_fetch_first = 1;
+int advice_push_needs_force = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -20,6 +22,8 @@ static struct {
 	{ "pushnonffdefault", &advice_push_non_ff_default },
 	{ "pushnonffmatching", &advice_push_non_ff_matching },
 	{ "pushalreadyexists", &advice_push_already_exists },
+	{ "pushfetchfirst", &advice_push_fetch_first },
+	{ "pushneedsforce", &advice_push_needs_force },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 8bf6356..fad36df 100644
--- a/advice.h
+++ b/advice.h
@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;
 extern int advice_push_non_ff_default;
 extern int advice_push_non_ff_matching;
 extern int advice_push_already_exists;
+extern int advice_push_fetch_first;
+extern int advice_push_needs_force;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 8491e43..92ca3d7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,10 +220,21 @@ 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_fetch_first[] =
+	N_("Updates were rejected because you do not have the object at the tip\n"
+	   "of the remote. You may want to first merge the remote changes (e.g.\n"
+	   " '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.");
 
+static const char message_advice_ref_needs_force[] =
+	N_("You cannot update a remote ref that points at a non-commit object,\n"
+	   "or update a remote ref to make it point at a non-commit object,\n"
+	   "without using the '--force' option.\n");
+
 static void advise_pull_before_push(void)
 {
 	if (!advice_push_non_ff_current || !advice_push_update_rejected)
@@ -252,6 +263,20 @@ static void advise_ref_already_exists(void)
 	advise(_(message_advice_ref_already_exists));
 }
 
+static void advise_ref_fetch_first(void)
+{
+	if (!advice_push_fetch_first || !advice_push_update_rejected)
+		return;
+	advise(_(message_advice_ref_fetch_first));
+}
+
+static void advise_ref_needs_force(void)
+{
+	if (!advice_push_needs_force || !advice_push_update_rejected)
+		return;
+	advise(_(message_advice_ref_needs_force));
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -285,6 +310,10 @@ static int push_with_options(struct transport *transport, int flags)
 			advise_checkout_pull_push();
 	} else if (reject_reasons & REJECT_ALREADY_EXISTS) {
 		advise_ref_already_exists();
+	} else if (reject_reasons & REJECT_FETCH_FIRST) {
+		advise_ref_fetch_first();
+	} else if (reject_reasons & REJECT_NEEDS_FORCE) {
+		advise_ref_needs_force();
 	}
 
 	return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f849e0a..57a46b2 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,16 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_FETCH_FIRST:
+			res = "error";
+			msg = "fetch first";
+			break;
+
+		case REF_STATUS_REJECT_NEEDS_FORCE:
+			res = "error";
+			msg = "needs force";
+			break;
+
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
 			res = "error";
 			msg = "already exists";
diff --git a/cache.h b/cache.h
index 12631a1..377a3df 100644
--- a/cache.h
+++ b/cache.h
@@ -1010,6 +1010,8 @@ struct ref {
 		REF_STATUS_REJECT_NONFASTFORWARD,
 		REF_STATUS_REJECT_ALREADY_EXISTS,
 		REF_STATUS_REJECT_NODELETE,
+		REF_STATUS_REJECT_FETCH_FIRST,
+		REF_STATUS_REJECT_NEEDS_FORCE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT
diff --git a/remote.c b/remote.c
index 969aa11..a772e74 100644
--- a/remote.c
+++ b/remote.c
@@ -1322,8 +1322,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 			if (!prefixcmp(ref->name, "refs/tags/"))
 				why = REF_STATUS_REJECT_ALREADY_EXISTS;
-			else if (!has_sha1_file(ref->old_sha1)
-				 || !ref_newer(ref->new_sha1, ref->old_sha1))
+			else if (!has_sha1_file(ref->old_sha1))
+				why = REF_STATUS_REJECT_FETCH_FIRST;
+			else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||
+				 !lookup_commit_reference_gently(ref->new_sha1, 1))
+				why = REF_STATUS_REJECT_NEEDS_FORCE;
+			else if (!ref_newer(ref->new_sha1, ref->old_sha1))
 				why = REF_STATUS_REJECT_NONFASTFORWARD;
 
 			if (!force_ref_update)
@@ -1512,7 +1516,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)
 	struct commit_list *list, *used;
 	int found = 0;
 
-	/* Both new and old must be commit-ish and new is descendant of
+	/*
+	 * Both new and old must be commit-ish and new is descendant of
 	 * old.  Otherwise we require --force.
 	 */
 	o = deref_tag(parse_object(old_sha1), NULL, 0);
diff --git a/send-pack.c b/send-pack.c
index 1c375f0..97ab336 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
 		case REF_STATUS_REJECT_ALREADY_EXISTS:
+		case REF_STATUS_REJECT_FETCH_FIRST:
+		case REF_STATUS_REJECT_NEEDS_FORCE:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport-helper.c b/transport-helper.c
index 965b778..cb3ef7d 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -666,6 +666,16 @@ static void push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "fetch first")) {
+			status = REF_STATUS_REJECT_FETCH_FIRST;
+			free(msg);
+			msg = NULL;
+		}
+		else if (!strcmp(msg, "needs force")) {
+			status = REF_STATUS_REJECT_NEEDS_FORCE;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
diff --git a/transport.c b/transport.c
index 585ebcd..5105562 100644
--- a/transport.c
+++ b/transport.c
@@ -699,6 +699,14 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "already exists", porcelain);
 		break;
+	case REF_STATUS_REJECT_FETCH_FIRST:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "fetch first", porcelain);
+		break;
+	case REF_STATUS_REJECT_NEEDS_FORCE:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "needs force", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 				*reject_reasons |= REJECT_NON_FF_OTHER;
 		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
 			*reject_reasons |= REJECT_ALREADY_EXISTS;
+		} else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {
+			*reject_reasons |= REJECT_FETCH_FIRST;
+		} else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {
+			*reject_reasons |= REJECT_NEEDS_FORCE;
 		}
 	}
 }
diff --git a/transport.h b/transport.h
index bfd2df5..c818763 100644
--- a/transport.h
+++ b/transport.h
@@ -143,6 +143,8 @@ 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
+#define REJECT_FETCH_FIRST     0x08
+#define REJECT_NEEDS_FORCE     0x10
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-- 
1.8.1.1.517.g0318d2b

Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Jeff King]

On Wed, Jan 23, 2013 at 08:28:49AM -0800, Junio C Hamano wrote:

> How about doing this?
> 
> For "needs force" cases, we say this instead:
> 
>  hint: you cannot update a ref that points at a non-commit object, or
>  hint: update a ref to point at a non-commit object, without --force.
> 
> Being explicit about "non-commit" twice will catch user's eyes and
> cause him to double check that it is not a mistyped LHS of the push
> refspec (if he is sending a non-commit) or mistyped RHS (if the ref
> is pointing at a non-commit).  If he _is_ trying to push a blob out,
> the advice makes it clear what to do next: he does want to force it.

Yeah, I think that is sensible.

> Note that you _could_ split the "needs force" case into two, namely,
> "cannot replace a non-commit" and "cannot push a non-commit".  You
> could even further split them [...etc...]

I do not think it is worth worrying too much about. This should really
not happen very often, and the user should be able to investigate and
figure out what is going on. I think making the error message extremely
specific is just going to end up making it harder to understand.

> If we did that, then we could loosen the "You should fetch first"
> case to say something like this:
> 
>  hint: you do not have the object at the tip of the remote ref;
>  hint: perhaps you want to pull from there first?

Yeah, better. I'll comment on the specific message you used in response
to the patch itself.

> This explicitly denies one of Chris's wish "we shouldn't suggest to
> merge something that we may not be able to", but in the "You should
> fetch first" case, we cannot fundamentally know if we can merge
> until we fetch.  I agree with you that the most common case is that
> the unknown object is a commit, and that suggesting to pull is a
> good compromise.

I thought the wish was more about "we shouldn't suggest to merge
something we _know_ we will not be able to", and you are still handling
that (i.e., the "needs force" case).

-Peff

Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Jeff King]

On Wed, Jan 23, 2013 at 01:55:30PM -0800, Junio C Hamano wrote:

> If we do not have the current object at the tip of the remote, we do
> not even know that object, when fetched, is something that can be
> merged.  In such a case, suggesting to pull first just like
> non-fast-forward case may not be technically correct, but in
> practice, most such failures are seen when you try to push your work
> to a branch without knowing that somebody else already pushed to
> update the same branch since you forked, so "pull first" would work
> as a suggestion most of the time.
> 
> In these cases, the current code already rejects such a push on the
> client end, but we used the same error and advice messages as the
> ones used when rejecting a non-fast-forward push, i.e. pull from
> there and integrate before pushing again.  Introduce new
> rejection reasons and reword the messages appropriately.

So obviously from our previous discussion, I agree with the general
behavior of this patch. Let me get nit-picky on the message itself,
though:

> +static const char message_advice_ref_fetch_first[] =
> +	N_("Updates were rejected because you do not have the object at the tip\n"
> +	   "of the remote. You may want to first merge the remote changes (e.g.\n"
> +	   " 'git pull') before pushing again.\n"
> +	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
> +

The condition that triggers this message is going to come up fairly
often for new git users (e.g., anyone using a central repo model), which
I think is why the original message_advice_pull_before_push has gotten
so much attention.  And in most cases, users will be seeing this message
now instead of "pull before push", because the common triggering cause
is somebody else pushing unrelated work.

The existing message says:

  Updates were rejected because a pushed branch tip is behind its remote
  counterpart. Check out this branch and merge the remote changes
  (e.g. 'git pull') before pushing again.

I wonder: will the new message be as comprehensible to a new user as the
old?

They are quite similar, but something about the presence of the word
"behind" in the latter makes me think it helps explain what is going on
a bit more. When I read the new one, my first question is "why don't I
have that object?". Of course, saying "behind" in this case would not be
strictly accurate, because we do not even know the remote has a commit.

I wonder if we can reword it to explain more about why we do not have
the object, without getting too inaccurate. Something like:

  Updates were rejected because the remote contains objects that you do
  not have locally. This is usually caused by another repository pushing
  to the same ref. You may want to first merge the remote changes (e.g.,
  'git pull') before pushing again.

I was also tempted to s/objects/work/, which is more vague, but is less
jargon-y for new users who do not know how git works.

Also, how should this interact with the checkout-then-pull-then-push
advice? We make a distinction for the non-fastforward case between HEAD
and other refs. Should we be making the same distinction here?

-Peff

Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wonder if we can reword it to explain more about why we do not have
> the object, without getting too inaccurate. Something like:
>
>   Updates were rejected because the remote contains objects that you do
>   not have locally. This is usually caused by another repository pushing
>   to the same ref. You may want to first merge the remote changes (e.g.,
>   'git pull') before pushing again.
>
> I was also tempted to s/objects/work/, which is more vague, but is less
> jargon-y for new users who do not know how git works.

After all this is "hint", and there is a value in being more
approachable at the cost of being less accurate, over being
impenetrable to achieve perfect correctness.

> Also, how should this interact with the checkout-then-pull-then-push
> advice? We make a distinction for the non-fastforward case between HEAD
> and other refs. Should we be making the same distinction here?

Possibly, but I am not among the people who cared most about the
distinction there; with the default behaviour switching to 'simple',
that distinction will start mattering even less, I suspect.

Re: [PATCH v4 1/3] push: further clean up fields of "struct ref"

[Eric Sunshine]

On Wed, Jan 23, 2013 at 4:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The "nonfastforward" and "update" fields are only used while
> deciding what value to assign to the "status" locally in a single
> function.  Remove them from the "struct ref".
>
> The "requires_force" field is not used to decide if the proposed
> update requires a --force option to succeed, or to record such a
> decision made elsewhere.  It is used by status reporting code that
> the particular update was "forced".  Rename it to "forced_udpate",

s/udpate/update/

Re: [PATCH v4 0/3] Finishing touches to "push" advises

[Chris Rorvick]

On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
> This builds on Chris Rorvick's earlier effort to forbid unforced
> updates to refs/tags/ hierarchy and giving sensible error and advise
> messages for that case (we are not rejecting such a push due to fast
> forwardness, and suggesting to fetch and integrate before pushing
> again does not make sense).

FWIW, these changes look good to me.  The logic in
set_ref_status_for_push() is easier to follow and the additional error
statuses (and associated advice) make things much clearer.

Had I written the the "already exists" advice in the context of these
additional statuses I would have said "the destination *tag* reference
already exists", or maybe even just "the destination *tag* already
exists".  It's probably fine the way it is, but I only avoided using
"tag" in the advice because I was abusing it.

Thanks,

Chris

Re: [PATCH v4 0/3] Finishing touches to "push" advises

[Junio C Hamano]

Chris Rorvick <chris@rorvick.com> writes:

> Had I written the the "already exists" advice in the context of these
> additional statuses I would have said "the destination *tag* reference
> already exists", or maybe even just "the destination *tag* already
> exists".

Yeah, now we do not use "already exists" for anything other than
refs/tags/, right?  Your rewording sounds like the right thing to
make it even clearer.

Thanks for bringing it up.  

Would it be sufficient to do this?  I think "the tag already exists
in the remote" is already clear that we are talking about the
destination.

diff --git a/builtin/push.c b/builtin/push.c
index a2b3fbe..78789be 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
 	   "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"
+	N_("Updates were rejected because the tag already exists\n"
 	   "in the remote.");
 
 static const char message_advice_ref_needs_force[] =

Re: [PATCH v4 0/3] Finishing touches to "push" advises

[Chris Rorvick]

On Thu, Jan 24, 2013 at 11:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Would it be sufficient to do this?  I think "the tag already exists
> in the remote" is already clear that we are talking about the
> destination.

Good point.

> diff --git a/builtin/push.c b/builtin/push.c
> index a2b3fbe..78789be 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
>            "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"
> +       N_("Updates were rejected because the tag already exists\n"
>            "in the remote.");
>
>  static const char message_advice_ref_needs_force[] =

Looks like the new-line is now unnecessary, but that looks good to me.

Thanks,

Chris