threads / patch / 32247

v6, 8 partspush: flag updates that require force

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

## tl;dr

61 messages between Nov 30, 2012 and Jan 25, 2013. Diffs are folded; open one to read it.

replies: 60people: 5as markdown or json

Chris Rorvick· Nov 30, 2012, 01:41 UTC · lore

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

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
Chris Rorvick· Nov 30, 2012, 01:41 UTC · re: Chris Rorvick · lore

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

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

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

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

diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..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
Chris Rorvick· Nov 30, 2012, 01:41 UTC · re: Chris Rorvick · lore

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

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

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

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

diff --git a/builtin/push.c b/builtin/push.c
index 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
Junio C Hamano· Dec 2, 2012, 10:42 UTC · re: Chris Rorvick · lore

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

Chris Rorvick <chris@rorvick.com> writes:
Show 22 quoted lines
>  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.
Chris Rorvick· Dec 3, 2012, 03:27 UTC · re: Junio C Hamano · lore

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

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
Chris Rorvick· Dec 3, 2012, 03:27 UTC · re: Chris Rorvick · lore

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

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

Documentation/config.txt, advice.c, advice.h, builtin/push.c

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
Chris Rorvick· Dec 3, 2012, 03:27 UTC · re: Chris Rorvick · lore

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

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

Documentation/config.txt, advice.c, advice.h, builtin/push.c

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
Chris Rorvick· Nov 30, 2012, 01:41 UTC · re: Chris Rorvick · lore

[PATCH v6 3/8] push: flag updates

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

cache.h, remote.c

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

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

cache.h, remote.c, transport.c

diff --git a/cache.h b/cache.h
index 722321c..b7ab4ac 100644
--- a/cache.h
+++ b/cache.h
@@ -999,7 +999,9 @@ struct ref {
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char *symref;
-	unsigned int force:1,
+	unsigned int
+		force:1,
+		requires_force:1,
 		merge:1,
 		nonfastforward:1,
 		not_forwardable:1,
diff --git a/remote.c b/remote.c
index 07040b8..4a6f822 100644
--- a/remote.c
+++ b/remote.c
@@ -1293,6 +1293,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	struct ref *ref;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
+		int force_ref_update = ref->force || force_update;
+
 		if (ref->peer_ref)
 			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
 		else if (!send_mirror)
@@ -1335,9 +1337,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 				!has_sha1_file(ref->old_sha1)
 				  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-			if (ref->nonfastforward && !ref->force && !force_update) {
-				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-				continue;
+			if (ref->nonfastforward) {
+				ref->requires_force = 1;
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+					continue;
+				}
 			}
 		}
 	}
diff --git a/transport.c b/transport.c
index 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
Chris Rorvick· Nov 30, 2012, 01:41 UTC · re: Chris Rorvick · lore

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

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

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

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

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..09bdec7 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,12 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
 updated.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side, but by default this is only allowed if the
-update can fast-forward <dst>.  By having the optional leading `+`,
-you can tell git to update the <dst> ref even when the update is not a
-fast-forward.  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side.  By default this is only allowed if <dst> is not
+under refs/tags/, and then only if it can fast-forward <dst>.  By having
+the optional leading `+`, you can tell git to update the <dst> ref even
+if it is not allowed by default (e.g., it is not a fast-forward.)  This
+does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for
+details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/builtin/push.c b/builtin/push.c
index 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
Chris Rorvick· Nov 30, 2012, 01:41 UTC · re: Chris Rorvick · lore

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

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

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

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

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

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(+)
Show changes to remote.c +5 −0
diff --git a/remote.c b/remote.c
index f5bc4e7..ee0c1e5 100644
--- a/remote.c
+++ b/remote.c
@@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)
 	if (!o || o->type != OBJ_COMMIT)
 		return 0;
 
+	/* new object must be commit-ish */
+	o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
+	if (!o || o->type != OBJ_COMMIT)
+		return 0;
+
 	return 1;
 }
 
-- 
1.8.0.158.g0c4328c
Chris Rorvick· Nov 30, 2012, 01:41 UTC · re: Chris Rorvick · lore

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

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(-)
Show changes to remote.c +17 −15
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
Chris Rorvick· Dec 2, 2012, 20:43 UTC · re: Chris Rorvick · lore

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

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(-)
Show changes to remote.c +2 −2
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
Max Horn· Jan 16, 2013, 13:32 UTC · re: Chris Rorvick · lore

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

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:
Show 8 quoted lines
> 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

Junio C Hamano· Jan 16, 2013, 16:00 UTC · re: Max Horn · lore

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

Max Horn <max@quendi.de> writes:
Show 8 quoted lines
> 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.
Jeff King· Jan 16, 2013, 16:01 UTC · re: Max Horn · lore

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

On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Jan 16, 2013, 17:10 UTC · re: Jeff King · lore

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

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(-)
Show changes to 3 files +1 −45

cache.h, remote.c, t/t5516-fetch-push.sh

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" &&
Jeff King· Jan 16, 2013, 17:43 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
Show 7 quoted lines
> 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.

Show 11 quoted lines
> 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
Junio C Hamano· Jan 16, 2013, 21:02 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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(-)
Show changes to 3 files +7 −58

cache.h, remote.c, t/t5516-fetch-push.sh

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
Chris Rorvick· Jan 17, 2013, 02:19 UTC · re: Jeff King · lore

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

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
Jeff King· Jan 17, 2013, 03:11 UTC · re: Chris Rorvick · lore

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

On Wed, Jan 16, 2013 at 08:19:28PM -0600, Chris Rorvick wrote:
Show 11 quoted lines
> 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
Chris Rorvick· Jan 17, 2013, 03:42 UTC · re: Jeff King · lore

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

On Wed, Jan 16, 2013 at 9:11 PM, Jeff King <peff@peff.net> wrote:
Show 13 quoted lines
>> 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.

Show 5 quoted lines
>   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
Junio C Hamano· Jan 16, 2013, 16:36 UTC · re: Max Horn · lore

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

Max Horn <max@quendi.de> writes:
Show 9 quoted lines
> 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(-)
Show changes to remote.c +5 −2
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 */
Junio C Hamano· Jan 16, 2013, 16:48 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> 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.

Chris Rorvick· Jan 17, 2013, 06:20 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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?
Junio C Hamano· Jan 17, 2013, 06:59 UTC · re: Chris Rorvick · lore

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

Chris Rorvick <chris@rorvick.com> writes:
Show 11 quoted lines
> 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.

Chris Rorvick· Jan 17, 2013, 13:09 UTC · re: Junio C Hamano · lore

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

On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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
Jeff King· Jan 18, 2013, 01:06 UTC · re: Chris Rorvick · lore

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

On Thu, Jan 17, 2013 at 07:09:16AM -0600, Chris Rorvick wrote:
Show 11 quoted lines
> 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
Chris Rorvick· Jan 18, 2013, 03:18 UTC · re: Jeff King · lore

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

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
Jeff King· Jan 21, 2013, 23:40 UTC · re: Chris Rorvick · lore

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

On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:
Show 8 quoted lines
> 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".

Show 10 quoted lines
> > 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
Junio C Hamano· Jan 21, 2013, 23:53 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> ... 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?
Chris Rorvick· Jan 22, 2013, 04:59 UTC · re: Jeff King · lore

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

On Mon, Jan 21, 2013 at 5:40 PM, Jeff King <peff@peff.net> wrote:
Show 19 quoted lines
> 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.

Show 33 quoted lines
>> > 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
Junio C Hamano· Jan 22, 2013, 06:44 UTC · re: Chris Rorvick · lore

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

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.

Junio C Hamano· Jan 22, 2013, 05:53 UTC · re: Jeff King · lore

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

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
Junio C Hamano· Jan 22, 2013, 05:53 UTC · re: Junio C Hamano · lore

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

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

cache.h, remote.c, transport.c

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
Junio C Hamano· Jan 22, 2013, 05:53 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 10 files +85 −6

advice.c, advice.h, builtin/push.c, builtin/send-pack.c, cache.h, remote.c, send-pack.c, transport-helper.c, transport.c, transport.h

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
Junio C Hamano· Jan 22, 2013, 06:04 UTC · re: Junio C Hamano · lore

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

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.
Show changes to remote.c +3 −3
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;
Junio C Hamano· Jan 22, 2013, 05:53 UTC · re: Junio C Hamano · lore

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

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

cache.h, remote.c

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
Junio C Hamano· Jan 22, 2013, 06:21 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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).

Junio C Hamano· Jan 22, 2013, 06:30 UTC · re: Jeff King · lore

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

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
Junio C Hamano· Jan 22, 2013, 06:30 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 3 files +8 −14

cache.h, remote.c, transport.c

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
Jeff King· Jan 23, 2013, 06:43 UTC · re: Junio C Hamano · lore

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

On Mon, Jan 21, 2013 at 10:30:28PM -0800, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 12 quoted lines
> 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
Junio C Hamano· Jan 22, 2013, 06:30 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 10 files +85 −6

advice.c, advice.h, builtin/push.c, builtin/send-pack.c, cache.h, remote.c, send-pack.c, transport-helper.c, transport.c, transport.h

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
Jeff King· Jan 23, 2013, 06:56 UTC · re: Junio C Hamano · lore

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

On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Jan 23, 2013, 16:28 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 26 quoted lines
> 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.

Jeff King· Jan 24, 2013, 06:43 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 23, 2013 at 08:28:49AM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> 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.

Show 5 quoted lines
> 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.

Show 6 quoted lines
> 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
Junio C Hamano· Jan 22, 2013, 06:30 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to remote.c +15 −25
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
Junio C Hamano· Jan 22, 2013, 07:26 UTC · re: Junio C Hamano · lore

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

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".

Junio C Hamano· Jan 23, 2013, 21:55 UTC · re: Jeff King · lore

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

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
Junio C Hamano· Jan 23, 2013, 21:55 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 3 files +8 −14

cache.h, remote.c, transport.c

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
Eric Sunshine· Jan 24, 2013, 22:22 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 23, 2013 at 4:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> 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/
Junio C Hamano· Jan 23, 2013, 21:55 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to remote.c +11 −16
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
Junio C Hamano· Jan 23, 2013, 21:55 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 11 files +92 −4

Documentation/config.txt, advice.c, advice.h, builtin/push.c, builtin/send-pack.c, cache.h, remote.c, send-pack.c, transport-helper.c, transport.c, transport.h

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
Jeff King· Jan 24, 2013, 06:58 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 23, 2013 at 01:55:30PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> 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:

Show 6 quoted lines
> +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
Junio C Hamano· Jan 24, 2013, 17:19 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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.

Chris Rorvick· Jan 25, 2013, 04:31 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
> 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
Junio C Hamano· Jan 25, 2013, 05:04 UTC · re: Chris Rorvick · lore

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

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.

Show changes to builtin/push.c +1 −1
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[] =
Chris Rorvick· Jan 25, 2013, 05:14 UTC · re: Junio C Hamano · lore

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

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.
Show 13 quoted lines
> 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
Junio C Hamano· Jan 18, 2013, 04:36 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.

← back to recent threads