[PATCH resend 0/3] transport: catch non-fast-forwards

[PATCH resend 0/3] transport: catch non-fast-forwards

[Tay Ray Chuan]

This patch series applies on top of 'next', and deals with alerting
the user to non-fast-forward pushes when using helpers (smart).

Previously, git silently exited. This situation involves the curl
helper and the smart protocol. The fast-forward push is only detected
when curl executes the rpc client (git-send-pack). Now, we detect it
before telling the helper to push.

The first patch refactors ref status logic out of builtin-send-pack.c.

The second patch lets git know of non-fast-forwards before actually
telling the helper to push anything.

The third patch changes the return code when ref status indicate an
error (determined by push_had_errors()), so that the caller of
transport_push() is alerted.

PS. There are at least 2 bug fixes related to this series. If you
usually do so from repo.or.cz, please fetch from git.kernel.org; the
former is 2 days old.

 builtin-send-pack.c |   39 +++------------------------------------
 remote.c            |   42 ++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    1 +
 transport-helper.c  |   10 +++-------
 transport.c         |   11 +++++++----
 5 files changed, 56 insertions(+), 47 deletions(-)

--
Cheers,
Ray Chuan

[PATCH resend 1/3] refactor ref status logic for pushing

[Tay Ray Chuan]

Move the logic to a new function in remote.[ch],
 set_ref_status_for_push().

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin-send-pack.c |   39 +++------------------------------------
 remote.c            |   42 ++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    1 +
 3 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..8c98624 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -412,44 +412,11 @@ int send_pack(struct send_pack_args *args,
 		else if (!args->send_mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (ref->deletion && !allow_deleting_refs) {
-			ref->status = REF_STATUS_REJECT_NODELETE;
-			continue;
-		}
-		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		if (set_ref_status_for_push(ref, args->force_update))
 			continue;
-		}

-		/* This part determines what can overwrite what.
-		 * The rules are:
-		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
-		 *
-		 * (1) if the old thing does not exist, it is OK.
-		 *
-		 * (2) if 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
-		 *     descendant of old, it is OK.
-		 *
-		 * (4) regardless of all of the above, removing :B is
-		 *     always allowed.
-		 */
-
-		ref->nonfastforward =
-		    !ref->deletion &&
-		    !is_null_sha1(ref->old_sha1) &&
-		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(ref->new_sha1, ref->old_sha1));
-
-		if (ref->nonfastforward && !ref->force && !args->force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}

diff --git a/remote.c b/remote.c
index e3afecd..188869a 100644
--- a/remote.c
+++ b/remote.c
@@ -1247,6 +1247,48 @@ int match_refs(struct ref *src, struct ref **dst,
 	return 0;
 }

+int set_ref_status_for_push(struct ref *ref, int force_update)
+{
+	ref->deletion = is_null_sha1(ref->new_sha1);
+	if (!ref->deletion &&
+		!hashcmp(ref->old_sha1, ref->new_sha1)) {
+		ref->status = REF_STATUS_UPTODATE;
+		return 1;
+	}
+
+	/* This part determines what can overwrite what.
+	 * The rules are:
+	 *
+	 * (0) you can always use --force or +A:B notation to
+	 *     selectively force individual ref pairs.
+	 *
+	 * (1) if the old thing does not exist, it is OK.
+	 *
+	 * (2) if 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
+	 *     descendant of old, it is OK.
+	 *
+	 * (4) regardless of all of the above, removing :B is
+	 *     always allowed.
+	 */
+
+	ref->nonfastforward =
+		!ref->deletion &&
+		!is_null_sha1(ref->old_sha1) &&
+		(!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;
+		return 1;
+	}
+
+	return 0;
+}
+
 struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
diff --git a/remote.h b/remote.h
index 8b7ecf9..0f45553 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,7 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,

 int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int all);
+int set_ref_status_for_push(struct ref *ref, int force_update);

 /*
  * Given a list of the remote refs and the specification of things to
--
1.6.6.rc1.286.gbc15a

[PATCH resend 2/3] transport-helper.c::push_refs(): know more about refs before pushing

[Tay Ray Chuan]

Know about rejected non-fast-forward refs, in addition to up-to-date
ones, by calling set_ref_status_for_push() in remote.[ch], before
passing push commands to the helper.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 transport-helper.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..6ed7b55 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -334,16 +334,12 @@ static int push_refs(struct transport *transport,
 		else if (!mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (!ref->deletion &&
-			!hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
-			continue;
-		}
-
 		if (force_all)
 			ref->force = 1;

+		if (set_ref_status_for_push(ref, force_all))
+			continue;
+
 		strbuf_addstr(&buf, "push ");
 		if (!ref->deletion) {
 			if (ref->force)
--
1.6.6.rc1.286.gbc15a

[PATCH resend 3/3] transport.c::transport_push(): make ref status affect return value

[Tay Ray Chuan]

When determining whether to print the ref status table,
push_had_errors() is called. Piggy-back this and modify the return
value accordingly.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 transport.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c
index 3eea836..32c122b 100644
--- a/transport.c
+++ b/transport.c
@@ -889,10 +889,13 @@ int transport_push(struct transport *transport,

 		ret = transport->push_refs(transport, remote_refs, flags);

-		if (!quiet || push_had_errors(remote_refs))
-			print_push_status(transport->url, remote_refs,
-					verbose | porcelain, porcelain,
-					nonfastforward);
+		if (!quiet)
+			if (push_had_errors(remote_refs)) {
+				ret = -1;
+				print_push_status(transport->url, remote_refs,
+						verbose | porcelain, porcelain,
+						nonfastforward);
+			}

 		if (!(flags & TRANSPORT_PUSH_DRY_RUN)) {
 			struct ref *ref;
--
1.6.6.rc1.286.gbc15a

Re: [PATCH resend 3/3] transport.c::transport_push(): make ref status affect return value

[Jeff King]

On Fri, Dec 04, 2009 at 02:58:42PM +0800, Tay Ray Chuan wrote:

> -		if (!quiet || push_had_errors(remote_refs))
> -			print_push_status(transport->url, remote_refs,
> -					verbose | porcelain, porcelain,
> -					nonfastforward);
> +		if (!quiet)
> +			if (push_had_errors(remote_refs)) {
> +				ret = -1;
> +				print_push_status(transport->url, remote_refs,
> +						verbose | porcelain, porcelain,
> +						nonfastforward);
> +			}

Er, this doesn't seem right. It will no longer print anything under
non-quiet mode unless there are errors, and it will only set a return
value in non-quiet mode.

I think you want:

  if (push_had_errors(remote_refs)) {
          ret = -1;
          if (!quiet)
                  print_push_status(...)
  }
  else if (!quiet)
          print_push_status(...)

Actually, it may simply be more readable by storing the error result:

  int err = push_had_errors(remote_refs);
  if (err)
    ret = -1;
  if (!quiet || err)
    print_push_status(...);

-Peff

[PATCH 1/3] refactor ref status logic for pushing

[Tay Ray Chuan]

Move the logic to a new function in remote.[ch],
 set_ref_status_for_push().

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin-send-pack.c |   39 +++------------------------------------
 remote.c            |   42 ++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    1 +
 3 files changed, 46 insertions(+), 36 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..8c98624 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -412,44 +412,11 @@ int send_pack(struct send_pack_args *args,
 		else if (!args->send_mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (ref->deletion && !allow_deleting_refs) {
-			ref->status = REF_STATUS_REJECT_NODELETE;
-			continue;
-		}
-		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		if (set_ref_status_for_push(ref, args->force_update))
 			continue;
-		}

-		/* This part determines what can overwrite what.
-		 * The rules are:
-		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
-		 *
-		 * (1) if the old thing does not exist, it is OK.
-		 *
-		 * (2) if 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
-		 *     descendant of old, it is OK.
-		 *
-		 * (4) regardless of all of the above, removing :B is
-		 *     always allowed.
-		 */
-
-		ref->nonfastforward =
-		    !ref->deletion &&
-		    !is_null_sha1(ref->old_sha1) &&
-		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(ref->new_sha1, ref->old_sha1));
-
-		if (ref->nonfastforward && !ref->force && !args->force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}

diff --git a/remote.c b/remote.c
index e3afecd..188869a 100644
--- a/remote.c
+++ b/remote.c
@@ -1247,6 +1247,48 @@ int match_refs(struct ref *src, struct ref **dst,
 	return 0;
 }

+int set_ref_status_for_push(struct ref *ref, int force_update)
+{
+	ref->deletion = is_null_sha1(ref->new_sha1);
+	if (!ref->deletion &&
+		!hashcmp(ref->old_sha1, ref->new_sha1)) {
+		ref->status = REF_STATUS_UPTODATE;
+		return 1;
+	}
+
+	/* This part determines what can overwrite what.
+	 * The rules are:
+	 *
+	 * (0) you can always use --force or +A:B notation to
+	 *     selectively force individual ref pairs.
+	 *
+	 * (1) if the old thing does not exist, it is OK.
+	 *
+	 * (2) if 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
+	 *     descendant of old, it is OK.
+	 *
+	 * (4) regardless of all of the above, removing :B is
+	 *     always allowed.
+	 */
+
+	ref->nonfastforward =
+		!ref->deletion &&
+		!is_null_sha1(ref->old_sha1) &&
+		(!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;
+		return 1;
+	}
+
+	return 0;
+}
+
 struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
diff --git a/remote.h b/remote.h
index 8b7ecf9..0f45553 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,7 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,

 int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int all);
+int set_ref_status_for_push(struct ref *ref, int force_update);

 /*
  * Given a list of the remote refs and the specification of things to
--
1.6.6.rc1.286.gbc15a

Re: [PATCH resend 0/3] transport: catch non-fast-forwards

[Daniel Barkalow]

On Fri, 4 Dec 2009, Tay Ray Chuan wrote:

> This patch series applies on top of 'next', and deals with alerting
> the user to non-fast-forward pushes when using helpers (smart).
> 
> Previously, git silently exited. This situation involves the curl
> helper and the smart protocol. The fast-forward push is only detected
> when curl executes the rpc client (git-send-pack). Now, we detect it
> before telling the helper to push.
> 
> The first patch refactors ref status logic out of builtin-send-pack.c.
> 
> The second patch lets git know of non-fast-forwards before actually
> telling the helper to push anything.
> 
> The third patch changes the return code when ref status indicate an
> error (determined by push_had_errors()), so that the caller of
> transport_push() is alerted.

Seems to me like transport_push() ought to set this sort of status 
information before calling the push method, rather than requiring all of 
the implementations to remember to call the generic function, since 
there's not really anything else sane they can do, right?

(There's eventually going to be at least a third push implementation: 
export the data to a foreign system a la "git svn dcommit")

	-Daniel
*This .sig left intentionally blank*

[PATCH v2 0/3] transport: catch non-fast-forwards

[Tay Ray Chuan]

Daniel, I've reworked the refactoring of ref status logic, like you
mentioned, such that transport_push() calls the generic function before
calling the transport->push_refs() implementation.

Junio, although this series affects http transports (smart), please treat
this series as separate from 'tr/http-updates' in 'next'.

Summary for those who missed v1:
  This patch series applies on top of 'next', and deals with alerting
  the user to non-fast-forward pushes when using helpers (smart).

  Previously, git silently exited. This situation involves the curl
  helper and the smart protocol. The fast-forward push is only detected
  when curl executes the rpc client (git-send-pack). Now, we detect it
  before telling the helper to push.

Changes from v1:
 - reworked refactoring of ref status logic (patch 1)
 - rewrote the patch for transport_push() (patch 2)
 - minor rewording of commit message (patch 3)

Tay Ray Chuan (3):
  refactor ref status logic for pushing
  transport.c::transport_push(): make ref status affect return value
  transport-helper.c::push_refs(): emit "no refs" error message

 builtin-send-pack.c |   50 +++++++++++---------------------------------------
 remote.c            |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    2 ++
 transport-helper.c  |   18 ++++++++++--------
 transport.c         |   11 +++++++++--
 5 files changed, 82 insertions(+), 49 deletions(-)

--
Cheers,
Ray Chuan

[PATCH v2 1/3] refactor ref status logic for pushing

[Tay Ray Chuan]

Move the logic that detects up-to-date and non-fast-forward refs to a
new function in remote.[ch], set_ref_status_for_push().

Make transport_push() invoke set_ref_status_for_push() before invoking
the push_refs() implementation. (As a side-effect, the push_refs()
implementation in transport-helper.c now knows of non-fast-forward
pushes.)

Removed logic for detecting up-to-date refs from the push_refs()
implementation in transport-helper.c, as transport_push() has already
done so for it.

Make cmd_send_pack() invoke set_ref_status_for_push() before invoking
send_pack(), as transport_push() can't do it for send_pack() here.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

In v1, this was spread over two patches. I squashed them into one
(here).

set_ref_status_for_push() used to be called inside the for loop over
remote_refs; now, it has its own for loop.

 builtin-send-pack.c |   50 +++++++++++---------------------------------------
 remote.c            |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h            |    2 ++
 transport-helper.c  |   13 ++++++-------
 transport.c         |    4 ++++
 5 files changed, 73 insertions(+), 46 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..38580c3 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -406,50 +406,19 @@ int send_pack(struct send_pack_args *args,
 	 */
 	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
-
-		if (ref->peer_ref)
-			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		else if (!args->send_mirror)
+		if (!ref->peer_ref && !args->send_mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (ref->deletion && !allow_deleting_refs) {
-			ref->status = REF_STATUS_REJECT_NODELETE;
-			continue;
-		}
-		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
 			continue;
+		default:
+			; /* do nothing */
 		}

-		/* This part determines what can overwrite what.
-		 * The rules are:
-		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
-		 *
-		 * (1) if the old thing does not exist, it is OK.
-		 *
-		 * (2) if 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
-		 *     descendant of old, it is OK.
-		 *
-		 * (4) regardless of all of the above, removing :B is
-		 *     always allowed.
-		 */
-
-		ref->nonfastforward =
-		    !ref->deletion &&
-		    !is_null_sha1(ref->old_sha1) &&
-		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(ref->new_sha1, ref->old_sha1));
-
-		if (ref->nonfastforward && !ref->force && !args->force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}

@@ -673,6 +642,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	if (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
 		return -1;

+	set_ref_status_for_push(remote_refs, args.send_mirror,
+		args.force_update);
+
 	ret = send_pack(&args, fd, conn, remote_refs, &extra_have);

 	if (helper_status)
diff --git a/remote.c b/remote.c
index e3afecd..c70181c 100644
--- a/remote.c
+++ b/remote.c
@@ -1247,6 +1247,56 @@ int match_refs(struct ref *src, struct ref **dst,
 	return 0;
 }

+void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
+	int force_update)
+{
+	struct ref *ref;
+
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (ref->peer_ref)
+			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
+		else if (!send_mirror)
+			continue;
+
+		ref->deletion = is_null_sha1(ref->new_sha1);
+		if (!ref->deletion &&
+			!hashcmp(ref->old_sha1, ref->new_sha1)) {
+			ref->status = REF_STATUS_UPTODATE;
+			continue;
+		}
+
+		/* This part determines what can overwrite what.
+		 * The rules are:
+		 *
+		 * (0) you can always use --force or +A:B notation to
+		 *     selectively force individual ref pairs.
+		 *
+		 * (1) if the old thing does not exist, it is OK.
+		 *
+		 * (2) if 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
+		 *     descendant of old, it is OK.
+		 *
+		 * (4) regardless of all of the above, removing :B is
+		 *     always allowed.
+		 */
+
+		ref->nonfastforward =
+			!ref->deletion &&
+			!is_null_sha1(ref->old_sha1) &&
+			(!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;
+		}
+	}
+}
+
 struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
diff --git a/remote.h b/remote.h
index 8b7ecf9..6e13643 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,8 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,

 int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int all);
+void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
+	int force_update);

 /*
  * Given a list of the remote refs and the specification of things to
diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..6b1f778 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -329,16 +329,15 @@ static int push_refs(struct transport *transport,
 		return 1;

 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (ref->peer_ref)
-			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		else if (!mirror)
+		if (!ref->peer_ref && !mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (!ref->deletion &&
-			!hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
 			continue;
+		default:
+			; /* do nothing */
 		}

 		if (force_all)
diff --git a/transport.c b/transport.c
index 3eea836..12c4423 100644
--- a/transport.c
+++ b/transport.c
@@ -887,6 +887,10 @@ int transport_push(struct transport *transport,
 			return -1;
 		}

+		set_ref_status_for_push(remote_refs,
+			flags & TRANSPORT_PUSH_MIRROR,
+			flags & TRANSPORT_PUSH_FORCE);
+
 		ret = transport->push_refs(transport, remote_refs, flags);

 		if (!quiet || push_had_errors(remote_refs))
--
1.6.4.4

Re: [PATCH v2 1/3] refactor ref status logic for pushing

[Daniel Barkalow]

On Tue, 8 Dec 2009, Tay Ray Chuan wrote:

> Move the logic that detects up-to-date and non-fast-forward refs to a
> new function in remote.[ch], set_ref_status_for_push().

Is there some reason to not have set_ref_status_for_push() be static in 
transport.c now? (Sorry for not suggesting this before.)

Other than that, it looks good to me.

I think it might be a good idea to have it generate a peer ref for refs 
that don't have one when mirror is used, which would eliminate a couple 
more lines, but there's no reason that has to be done in the same patch.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH v2 1/3] refactor ref status logic for pushing

[Tay Ray Chuan]

Hi,

On Wed, Dec 9, 2009 at 1:17 AM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Tue, 8 Dec 2009, Tay Ray Chuan wrote:
>
>> Move the logic that detects up-to-date and non-fast-forward refs to a
>> new function in remote.[ch], set_ref_status_for_push().
>
> Is there some reason to not have set_ref_status_for_push() be static in
> transport.c now? (Sorry for not suggesting this before.)

it can't be static, because builtin-send-pack.c::main() needs it too.

> Other than that, it looks good to me.

Thanks.

-- 
Cheers,
Ray Chuan

Re: [PATCH v2 1/3] refactor ref status logic for pushing

[Daniel Barkalow]

On Wed, 9 Dec 2009, Tay Ray Chuan wrote:

> Hi,
> 
> On Wed, Dec 9, 2009 at 1:17 AM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > On Tue, 8 Dec 2009, Tay Ray Chuan wrote:
> >
> >> Move the logic that detects up-to-date and non-fast-forward refs to a
> >> new function in remote.[ch], set_ref_status_for_push().
> >
> > Is there some reason to not have set_ref_status_for_push() be static in
> > transport.c now? (Sorry for not suggesting this before.)
> 
> it can't be static, because builtin-send-pack.c::main() needs it too.

Oh, right, fair enough.

	-Daniel
*This .sig left intentionally blank*

[PATCH v2 2/3] transport.c::transport_push(): make ref status affect return value

[Tay Ray Chuan]

Use push_had_errors() to check the refs for errors and modify the
return value.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Rewrote this.

 transport.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/transport.c b/transport.c
index 12c4423..9b23989 100644
--- a/transport.c
+++ b/transport.c
@@ -875,7 +875,7 @@ int transport_push(struct transport *transport,
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
 		int quiet = flags & TRANSPORT_PUSH_QUIET;
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
-		int ret;
+		int ret, err;

 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
@@ -892,8 +892,11 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_FORCE);

 		ret = transport->push_refs(transport, remote_refs, flags);
+		err = push_had_errors(remote_refs);

-		if (!quiet || push_had_errors(remote_refs))
+		ret |= err;
+
+		if (!quiet || err)
 			print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
 					nonfastforward);
--
1.6.4.4

[PATCH v2 3/3] transport-helper.c::push_refs(): emit "no refs" error message

[Tay Ray Chuan]

Emit an error message when remote_refs is not set.

This behaviour is consistent with that of builtin-send-pack.c and
http-push.c.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 transport-helper.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 6b1f778..96fc48b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -321,8 +321,11 @@ static int push_refs(struct transport *transport,
 	struct child_process *helper;
 	struct ref *ref;

-	if (!remote_refs)
+	if (!remote_refs) {
+		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
+			"Perhaps you should specify a branch such as 'master'.\n");
 		return 0;
+	}

 	helper = get_helper(transport);
 	if (!data->push)
--
1.6.4.4

[PATCH v3 0/6] transport: catch non-fast forwards

[Tay Ray Chuan]

Junio, this is re-roll of the 'tr/http-push-ref-status' branch in 'pu'.

Summary:
  This patch series applies on top of 'next', and deals with alerting
  the user to rejected non-fast-forward pushes when using helpers
  (smart).

  Previously, git silently exited. This situation involves the curl
  helper and the smart protocol. The non-fast-forward push is only
  detected when curl executes the rpc client (git-send-pack). Now, we
  detect it before telling the helper to push.

Changes from v2:
 - add tests
 - modify commit messages to mention tests
 - report rejected non-fast-forward pushes for unmatched refs
   (without an explicit refspec) by the remote helper (see patches #2
   and #5).

   This remedies the scenario where a user would not be aware of
   rejected non-fast-forward pushes.

   It occurs when
     1) there are one or more pushes that succeed and
     2) there are one or more rejected non-fast-forward pushes that
	    involve refs that cannot be matched without explicit refspecs.

   This is due to the re-marking of ref status in transport-helper.c::
   push_refs() when interacting with the remote helper.

   If only non-matched, non-fast-forward refs are involved (ie.
   condition #2 is present without #1), then the situtation does not
   occur - no 'push' commands are passed to the remote helper, no
   interaction with the helper takes place, and no re-marking of ref
   status takes place. The user will be alerted to the rejected
   non-fast-forward push.

   If both are present, re-marking occurs. Even if a ref was not part
   of a 'push' command to the helper, 'ok'/'error' status reports might
   be made for it, since the remote helper does ref matching on its
   side. This led to refs that cannot be matched without an explicit
   refspec marked as REF_STATUS_REJECT_NONFASTFORWARD to be re-marked
   REF_STATUS_NONE.

   (Note: helpers fail to match refs as explicit refspecs are always
   available to the top-level transport mechanism, but only on a need-
   to-know basis to the remote helper via a 'push' command when a ref
   is to be pushed.)

   Interestingly, it is possible for a user to be forever unaware of
   the rejected push if the user relies on information from git push
   alone. As long as the user ensures that one or more other pushes are
   successful (eg. change a tracked ref fast-forwardedly) between the
   execution of git push, the rejected non-fast-forward push won't be
   reported.

Tay Ray Chuan (6):
  t5541-http-push.sh: add tests for non-fast-forward pushes
  t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs
  refactor ref status logic for pushing
  transport.c::transport_push(): make ref status affect return value
  transport-helper.c::push_refs(): ignore helper-reported status if ref
    is not to be pushed
  transport-helper.c::push_refs(): emit "no refs" error message

 builtin-send-pack.c  |   50 +++++++++++---------------------------------------
 remote.c             |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h             |    2 ++
 t/t5541-http-push.sh |   43 +++++++++++++++++++++++++++++++++++++++++++
 transport-helper.c   |   32 ++++++++++++++++++++++----------
 transport.c          |   11 +++++++++--
 6 files changed, 137 insertions(+), 51 deletions(-)

--
Cheers,
Ray Chuan

[PATCH v3 1/6] t5541-http-push.sh: add tests for non-fast-forward pushes

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5541-http-push.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 2a58d0c..f49c7c4 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -88,5 +88,28 @@ test_expect_success 'used receive-pack service' '
 	test_cmp exp act
 '

+test_expect_success 'non-fast-forward push fails' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	git checkout master &&
+	echo "changed" > path2 &&
+	git commit -a -m path2 --amend &&
+
+	HEAD=$(git rev-parse --verify HEAD) &&
+	!(git push -v origin >output 2>&1) &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+	 test $HEAD != $(git rev-parse --verify HEAD))
+'
+
+test_expect_failure 'non-fast-forward push show ref status' '
+	grep "^ ! \[rejected\][ ]*master -> master (non-fast-forward)$" output
+'
+
+test_expect_failure 'non-fast-forward push shows help message' '
+	grep \
+"To prevent you from losing history, non-fast-forward updates were rejected
+Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
+section of '"'git push --help'"' for details." output
+'
+
 stop_httpd
 test_done
--
1.6.6.rc1.249.g048b3

[PATCH v3 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Tay Ray Chuan]

Test that when non-fast-forwarded refs cannot be matched without an
explicit refspect, the push fails with a non-fast-forward ref status and
help message.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5541-http-push.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index f49c7c4..5ebe04a 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -111,5 +111,25 @@ Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
 section of '"'git push --help'"' for details." output
 '

+test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
+	# create a dissimilarly-named ref so that git is unable to match the refs
+	git push origin master:retsam
+
+	echo "change changed" > path2 &&
+	git commit -a -m path2 --amend &&
+
+	# push master too. This ensures there is at least one '"'push'"' command to
+	# the remote helper and triggers interaction with the helper.
+	!(git push -v origin +master master:retsam >output 2>&1) &&
+
+	grep "^ + [a-z0-9]\+\.\.\.[a-z0-9]\+[ ]*master -> master (forced update)$" output &&
+	grep "^ ! \[rejected\][ ]*master -> retsam (non-fast-forward)$" output &&
+
+	grep \
+"To prevent you from losing history, non-fast-forward updates were rejected
+Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
+section of '"'git push --help'"' for details." output
+'
+
 stop_httpd
 test_done
--
1.6.6.rc1.249.g048b3

Re: [PATCH v3 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Jeff King]

On Thu, Dec 24, 2009 at 03:41:58PM +0800, Tay Ray Chuan wrote:

> Test that when non-fast-forwarded refs cannot be matched without an
> explicit refspect, the push fails with a non-fast-forward ref status and
> help message.

I don't understand what you're testing here. If it's not matched, then
how is it a non-fast-forward? Isn't it simply unmatched?

Your test:

> +test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
> +	# create a dissimilarly-named ref so that git is unable to match the refs
> +	git push origin master:retsam
> +
> +	echo "change changed" > path2 &&
> +	git commit -a -m path2 --amend &&
> +
> +	# push master too. This ensures there is at least one '"'push'"' command to
> +	# the remote helper and triggers interaction with the helper.
> +	!(git push -v origin +master master:retsam >output 2>&1) &&
> +
> +	grep "^ + [a-z0-9]\+\.\.\.[a-z0-9]\+[ ]*master -> master (forced update)$" output &&
> +	grep "^ ! \[rejected\][ ]*master -> retsam (non-fast-forward)$" output &&

Looks like you're just testing the usual "master -> retsam is not a
fast-forward" case. I don't understand how this is different from the
previous tests. Can you elaborate?

-Peff

Re: [PATCH v3 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Tay Ray Chuan]

Hi,

On Tue, Jan 5, 2010 at 2:35 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 24, 2009 at 03:41:58PM +0800, Tay Ray Chuan wrote:
>
>> Test that when non-fast-forwarded refs cannot be matched without an
>> explicit refspect, the push fails with a non-fast-forward ref status and
>> help message.
>
> I don't understand what you're testing here. If it's not matched, then
> how is it a non-fast-forward? Isn't it simply unmatched?

Let me rephrase this as:

  Some refs can only be matched to a remote ref with an explicit
  refspec. When such a ref is a non-fast-forward of its remote ref,
  test that pushing them (with the explicit refspec specified) fails
  with a non-fast-foward-type error.

> Your test:
>
>> +test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
>> +     # create a dissimilarly-named ref so that git is unable to match the refs
>> +     git push origin master:retsam
>> +
>> +     echo "change changed" > path2 &&
>> +     git commit -a -m path2 --amend &&
>> +
>> +     # push master too. This ensures there is at least one '"'push'"' command to
>> +     # the remote helper and triggers interaction with the helper.
>> +     !(git push -v origin +master master:retsam >output 2>&1) &&
>> +
>> +     grep "^ + [a-z0-9]\+\.\.\.[a-z0-9]\+[ ]*master -> master (forced update)$" output &&
>> +     grep "^ ! \[rejected\][ ]*master -> retsam (non-fast-forward)$" output &&
>
> Looks like you're just testing the usual "master -> retsam is not a
> fast-forward" case. I don't understand how this is different from the
> previous tests. Can you elaborate?

The problem in question is that a non-fast-forward error is not being
reported, and this test sets up a situation to trigger this - it's not
meant to be just another non-fast-forward test.

-- 
Cheers,
Ray Chuan

Re: [PATCH v3 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Jeff King]

On Tue, Jan 05, 2010 at 06:01:32PM +0800, Tay Ray Chuan wrote:

> > I don't understand what you're testing here. If it's not matched, then
> > how is it a non-fast-forward? Isn't it simply unmatched?
> 
> Let me rephrase this as:
> 
>   Some refs can only be matched to a remote ref with an explicit
>   refspec. When such a ref is a non-fast-forward of its remote ref,
>   test that pushing them (with the explicit refspec specified) fails
>   with a non-fast-foward-type error.

Thanks, that makes sense to me now.

-Peff

Re: [PATCH v3 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

> Test that when non-fast-forwarded refs cannot be matched without an
> explicit refspect, the push fails with a non-fast-forward ref status and
> help message.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>  t/t5541-http-push.sh |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
> index f49c7c4..5ebe04a 100755
> --- a/t/t5541-http-push.sh
> +++ b/t/t5541-http-push.sh
> @@ -111,5 +111,25 @@ Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
>  section of '"'git push --help'"' for details." output
>  '
>
> +test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
> +	# create a dissimilarly-named ref so that git is unable to match the refs
> +	git push origin master:retsam
> +
> +	echo "change changed" > path2 &&
> +	git commit -a -m path2 --amend &&
> +
> +	# push master too. This ensures there is at least one '"'push'"' command to
> +	# the remote helper and triggers interaction with the helper.
> +	!(git push -v origin +master master:retsam >output 2>&1) &&

A dumb question.  Why is this done in a sub-shell?

> +
> +	grep "^ + [a-z0-9]\+\.\.\.[a-z0-9]\+[ ]*master -> master (forced update)$" output &&
> +	grep "^ ! \[rejected\][ ]*master -> retsam (non-fast-forward)$" output &&

[a-z0-9] seems too broad to catch hexadecimal.  "\+" to introduce ERE
elements to grep that expects BRE is a GNU extension, IIRC.  You could use
egrep if you really want to say one-or-more, but I think in this case it
is better to simply replace it with a zero-or-more '*'.  Why is a single
SP made into character class with "[ ]" pair?

> +
> +	grep \
> +"To prevent you from losing history, non-fast-forward updates were rejected
> +Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
> +section of '"'git push --help'"' for details." output
> +'
> +
>  stop_httpd
>  test_done
> --
> 1.6.6.rc1.249.g048b3

Re: [PATCH v3 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Tay Ray Chuan]

Hi,

On Wed, Jan 6, 2010 at 9:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>> Test that when non-fast-forwarded refs cannot be matched without an
>> explicit refspect, the push fails with a non-fast-forward ref status and
>> help message.
>>
>> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
>> ---
>>  t/t5541-http-push.sh |   20 ++++++++++++++++++++
>>  1 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
>> index f49c7c4..5ebe04a 100755
>> --- a/t/t5541-http-push.sh
>> +++ b/t/t5541-http-push.sh
>> @@ -111,5 +111,25 @@ Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
>>  section of '"'git push --help'"' for details." output
>>  '
>>
>> +test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
>> +     # create a dissimilarly-named ref so that git is unable to match the refs
>> +     git push origin master:retsam
>> +
>> +     echo "change changed" > path2 &&
>> +     git commit -a -m path2 --amend &&
>> +
>> +     # push master too. This ensures there is at least one '"'push'"' command to
>> +     # the remote helper and triggers interaction with the helper.
>> +     !(git push -v origin +master master:retsam >output 2>&1) &&
>
> A dumb question.  Why is this done in a sub-shell?
>
>> +
>> +     grep "^ + [a-z0-9]\+\.\.\.[a-z0-9]\+[ ]*master -> master (forced update)$" output &&
>> +     grep "^ ! \[rejected\][ ]*master -> retsam (non-fast-forward)$" output &&
>
> [a-z0-9] seems too broad to catch hexadecimal.

Oops, my bad.

>  "\+" to introduce ERE
> elements to grep that expects BRE is a GNU extension, IIRC.  You could use
> egrep if you really want to say one-or-more, but I think in this case it
> is better to simply replace it with a zero-or-more '*'.

Ok.

>  Why is a single
> SP made into character class with "[ ]" pair?

To make it clearer that I'm trying to match a SP.

-- 
Cheers,
Ray Chuan

[PATCH v4 2/6] t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs

[Tay Ray Chuan]

Some refs can only be matched to a remote ref with an explicit refspec.
When such a ref is a non-fast-forward of its remote ref,  test that
pushing them (with the explicit refspec specified) fails with a non-
fast-foward-type error (viz. printing of ref status and help message).

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed from v3:
 - Reworded commit message
 - Reword the comments
 - Used '*' instead of '\+' for grep expressions
 - Used [a-f0-9] instead of [a-z0-9] for matching hexadecimals
 - Used ' ' instead of '[ ]' for matching SP

 t/t5541-http-push.sh |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index f49c7c4..cc740fe 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -111,5 +111,26 @@ Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
 section of '"'git push --help'"' for details." output
 '

+test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
+	# create a dissimilarly-named remote ref so that git is unable to match the
+	# two refs (viz. local, remote) unless an explicit refspec is provided.
+	git push origin master:retsam
+
+	echo "change changed" > path2 &&
+	git commit -a -m path2 --amend &&
+
+	# push master too; this ensures there is at least one '"'push'"' command to
+	# the remote helper and triggers interaction with the helper.
+	!(git push -v origin +master master:retsam >output 2>&1) &&
+
+	grep "^ + [a-f0-9]*\.\.\.[a-f0-9]* *master -> master (forced update)$" output &&
+	grep "^ ! \[rejected\] *master -> retsam (non-fast-forward)$" output &&
+
+	grep \
+"To prevent you from losing history, non-fast-forward updates were rejected
+Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
+section of '"'git push --help'"' for details." output
+'
+
 stop_httpd
 test_done
--
1.6.6.341.ga7aec

[PATCH v4 3/6] refactor ref status logic for pushing

[Tay Ray Chuan]

Move the logic that detects up-to-date and non-fast-forward refs to a
new function in remote.[ch], set_ref_status_for_push().

Make transport_push() invoke set_ref_status_for_push() before invoking
the push_refs() implementation. (As a side-effect, the push_refs()
implementation in transport-helper.c now knows of non-fast-forward
pushes.)

Removed logic for detecting up-to-date refs from the push_refs()
implementation in transport-helper.c, as transport_push() has already
done so for it.

Make cmd_send_pack() invoke set_ref_status_for_push() before invoking
send_pack(), as transport_push() can't do it for send_pack() here.

Mark the test on the return status of non-fast-forward push to fail.
Git now exits with success, as transport.c::transport_push() does not
check for refs with status REF_STATUS_REJECT_NONFASTFORWARD nor does it
indicate rejected pushes with its return value.

Mark the test for ref status to succeed. As mentioned earlier, refs
might be marked as non-fast-forwards, triggering the push status
printing mechanism in transport.c.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 builtin-send-pack.c  |   51 +++++++++++--------------------------------------
 remote.c             |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h             |    2 +
 t/t5541-http-push.sh |    4 +-
 transport-helper.c   |   14 ++++++------
 transport.c          |    4 +++
 6 files changed, 77 insertions(+), 48 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..76c7206 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -406,50 +406,20 @@ int send_pack(struct send_pack_args *args,
 	 */
 	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
-
-		if (ref->peer_ref)
-			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		else if (!args->send_mirror)
+		if (!ref->peer_ref && !args->send_mirror)
 			continue;
 
-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (ref->deletion && !allow_deleting_refs) {
-			ref->status = REF_STATUS_REJECT_NODELETE;
-			continue;
-		}
-		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		/* Check for statuses set by set_ref_status_for_push() */
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
 			continue;
+		default:
+			; /* do nothing */
 		}
 
-		/* This part determines what can overwrite what.
-		 * The rules are:
-		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
-		 *
-		 * (1) if the old thing does not exist, it is OK.
-		 *
-		 * (2) if 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
-		 *     descendant of old, it is OK.
-		 *
-		 * (4) regardless of all of the above, removing :B is
-		 *     always allowed.
-		 */
-
-		ref->nonfastforward =
-		    !ref->deletion &&
-		    !is_null_sha1(ref->old_sha1) &&
-		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(ref->new_sha1, ref->old_sha1));
-
-		if (ref->nonfastforward && !ref->force && !args->force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}
 
@@ -673,6 +643,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	if (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
 		return -1;
 
+	set_ref_status_for_push(remote_refs, args.send_mirror,
+		args.force_update);
+
 	ret = send_pack(&args, fd, conn, remote_refs, &extra_have);
 
 	if (helper_status)
diff --git a/remote.c b/remote.c
index e3afecd..c70181c 100644
--- a/remote.c
+++ b/remote.c
@@ -1247,6 +1247,56 @@ int match_refs(struct ref *src, struct ref **dst,
 	return 0;
 }
 
+void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
+	int force_update)
+{
+	struct ref *ref;
+
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (ref->peer_ref)
+			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
+		else if (!send_mirror)
+			continue;
+
+		ref->deletion = is_null_sha1(ref->new_sha1);
+		if (!ref->deletion &&
+			!hashcmp(ref->old_sha1, ref->new_sha1)) {
+			ref->status = REF_STATUS_UPTODATE;
+			continue;
+		}
+
+		/* This part determines what can overwrite what.
+		 * The rules are:
+		 *
+		 * (0) you can always use --force or +A:B notation to
+		 *     selectively force individual ref pairs.
+		 *
+		 * (1) if the old thing does not exist, it is OK.
+		 *
+		 * (2) if 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
+		 *     descendant of old, it is OK.
+		 *
+		 * (4) regardless of all of the above, removing :B is
+		 *     always allowed.
+		 */
+
+		ref->nonfastforward =
+			!ref->deletion &&
+			!is_null_sha1(ref->old_sha1) &&
+			(!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;
+		}
+	}
+}
+
 struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
diff --git a/remote.h b/remote.h
index 8b7ecf9..6e13643 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,8 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
 
 int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int all);
+void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
+	int force_update);
 
 /*
  * Given a list of the remote refs and the specification of things to
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index cc740fe..6d92196 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -88,7 +88,7 @@ test_expect_success 'used receive-pack service' '
 	test_cmp exp act
 '
 
-test_expect_success 'non-fast-forward push fails' '
+test_expect_failure 'non-fast-forward push fails' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout master &&
 	echo "changed" > path2 &&
@@ -100,7 +100,7 @@ test_expect_success 'non-fast-forward push fails' '
 	 test $HEAD != $(git rev-parse --verify HEAD))
 '
 
-test_expect_failure 'non-fast-forward push show ref status' '
+test_expect_success 'non-fast-forward push show ref status' '
 	grep "^ ! \[rejected\][ ]*master -> master (non-fast-forward)$" output
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..7c9b569 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -329,16 +329,16 @@ static int push_refs(struct transport *transport,
 		return 1;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (ref->peer_ref)
-			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		else if (!mirror)
+		if (!ref->peer_ref && !mirror)
 			continue;
 
-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (!ref->deletion &&
-			!hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		/* Check for statuses set by set_ref_status_for_push() */
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
 			continue;
+		default:
+			; /* do nothing */
 		}
 
 		if (force_all)
diff --git a/transport.c b/transport.c
index 3eea836..12c4423 100644
--- a/transport.c
+++ b/transport.c
@@ -887,6 +887,10 @@ int transport_push(struct transport *transport,
 			return -1;
 		}
 
+		set_ref_status_for_push(remote_refs,
+			flags & TRANSPORT_PUSH_MIRROR,
+			flags & TRANSPORT_PUSH_FORCE);
+
 		ret = transport->push_refs(transport, remote_refs, flags);
 
 		if (!quiet || push_had_errors(remote_refs))
-- 
1.6.6.341.ga7aec

[PATCH v4 4/6] transport.c::transport_push(): make ref status affect return value

[Tay Ray Chuan]

Use push_had_errors() to check the refs for errors and modify the
return value.

Mark the non-fast-forward push tests to succeed.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5541-http-push.sh |    4 ++--
 transport.c          |    7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 6d92196..979624d 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -88,7 +88,7 @@ test_expect_success 'used receive-pack service' '
 	test_cmp exp act
 '
 
-test_expect_failure 'non-fast-forward push fails' '
+test_expect_success 'non-fast-forward push fails' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout master &&
 	echo "changed" > path2 &&
@@ -104,7 +104,7 @@ test_expect_success 'non-fast-forward push show ref status' '
 	grep "^ ! \[rejected\][ ]*master -> master (non-fast-forward)$" output
 '
 
-test_expect_failure 'non-fast-forward push shows help message' '
+test_expect_success 'non-fast-forward push shows help message' '
 	grep \
 "To prevent you from losing history, non-fast-forward updates were rejected
 Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
diff --git a/transport.c b/transport.c
index 12c4423..9b23989 100644
--- a/transport.c
+++ b/transport.c
@@ -875,7 +875,7 @@ int transport_push(struct transport *transport,
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
 		int quiet = flags & TRANSPORT_PUSH_QUIET;
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
-		int ret;
+		int ret, err;
 
 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
@@ -892,8 +892,11 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_FORCE);
 
 		ret = transport->push_refs(transport, remote_refs, flags);
+		err = push_had_errors(remote_refs);
 
-		if (!quiet || push_had_errors(remote_refs))
+		ret |= err;
+
+		if (!quiet || err)
 			print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
 					nonfastforward);
-- 
1.6.6.341.ga7aec

[PATCH v3 3/6] refactor ref status logic for pushing

[Tay Ray Chuan]

Move the logic that detects up-to-date and non-fast-forward refs to a
new function in remote.[ch], set_ref_status_for_push().

Make transport_push() invoke set_ref_status_for_push() before invoking
the push_refs() implementation. (As a side-effect, the push_refs()
implementation in transport-helper.c now knows of non-fast-forward
pushes.)

Removed logic for detecting up-to-date refs from the push_refs()
implementation in transport-helper.c, as transport_push() has already
done so for it.

Make cmd_send_pack() invoke set_ref_status_for_push() before invoking
send_pack(), as transport_push() can't do it for send_pack() here.

Mark the test on the return status of non-fast-forward push to fail.
Git now exits with success, as transport.c::transport_push() does not
check for refs with status REF_STATUS_REJECT_NONFASTFORWARD nor does it
indicate rejected pushes with its return value.

Mark the test for ref status to succeed. As mentioned earlier, refs
might be marked as non-fast-forwards, triggering the push status
printing mechanism in transport.c.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed nothing from v2 except tests and mentioning it in the commit
message.

 builtin-send-pack.c  |   50 +++++++++++---------------------------------------
 remote.c             |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h             |    2 ++
 t/t5541-http-push.sh |    4 ++--
 transport-helper.c   |   13 ++++++-------
 transport.c          |    4 ++++
 6 files changed, 75 insertions(+), 48 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8fffdbf..38580c3 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -406,50 +406,19 @@ int send_pack(struct send_pack_args *args,
 	 */
 	new_refs = 0;
 	for (ref = remote_refs; ref; ref = ref->next) {
-
-		if (ref->peer_ref)
-			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		else if (!args->send_mirror)
+		if (!ref->peer_ref && !args->send_mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (ref->deletion && !allow_deleting_refs) {
-			ref->status = REF_STATUS_REJECT_NODELETE;
-			continue;
-		}
-		if (!ref->deletion &&
-		    !hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
 			continue;
+		default:
+			; /* do nothing */
 		}

-		/* This part determines what can overwrite what.
-		 * The rules are:
-		 *
-		 * (0) you can always use --force or +A:B notation to
-		 *     selectively force individual ref pairs.
-		 *
-		 * (1) if the old thing does not exist, it is OK.
-		 *
-		 * (2) if 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
-		 *     descendant of old, it is OK.
-		 *
-		 * (4) regardless of all of the above, removing :B is
-		 *     always allowed.
-		 */
-
-		ref->nonfastforward =
-		    !ref->deletion &&
-		    !is_null_sha1(ref->old_sha1) &&
-		    (!has_sha1_file(ref->old_sha1)
-		      || !ref_newer(ref->new_sha1, ref->old_sha1));
-
-		if (ref->nonfastforward && !ref->force && !args->force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+		if (ref->deletion && !allow_deleting_refs) {
+			ref->status = REF_STATUS_REJECT_NODELETE;
 			continue;
 		}

@@ -673,6 +642,9 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	if (match_refs(local_refs, &remote_refs, nr_refspecs, refspecs, flags))
 		return -1;

+	set_ref_status_for_push(remote_refs, args.send_mirror,
+		args.force_update);
+
 	ret = send_pack(&args, fd, conn, remote_refs, &extra_have);

 	if (helper_status)
diff --git a/remote.c b/remote.c
index e3afecd..c70181c 100644
--- a/remote.c
+++ b/remote.c
@@ -1247,6 +1247,56 @@ int match_refs(struct ref *src, struct ref **dst,
 	return 0;
 }

+void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
+	int force_update)
+{
+	struct ref *ref;
+
+	for (ref = remote_refs; ref; ref = ref->next) {
+		if (ref->peer_ref)
+			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
+		else if (!send_mirror)
+			continue;
+
+		ref->deletion = is_null_sha1(ref->new_sha1);
+		if (!ref->deletion &&
+			!hashcmp(ref->old_sha1, ref->new_sha1)) {
+			ref->status = REF_STATUS_UPTODATE;
+			continue;
+		}
+
+		/* This part determines what can overwrite what.
+		 * The rules are:
+		 *
+		 * (0) you can always use --force or +A:B notation to
+		 *     selectively force individual ref pairs.
+		 *
+		 * (1) if the old thing does not exist, it is OK.
+		 *
+		 * (2) if 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
+		 *     descendant of old, it is OK.
+		 *
+		 * (4) regardless of all of the above, removing :B is
+		 *     always allowed.
+		 */
+
+		ref->nonfastforward =
+			!ref->deletion &&
+			!is_null_sha1(ref->old_sha1) &&
+			(!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;
+		}
+	}
+}
+
 struct branch *branch_get(const char *name)
 {
 	struct branch *ret;
diff --git a/remote.h b/remote.h
index 8b7ecf9..6e13643 100644
--- a/remote.h
+++ b/remote.h
@@ -98,6 +98,8 @@ char *apply_refspecs(struct refspec *refspecs, int nr_refspec,

 int match_refs(struct ref *src, struct ref **dst,
 	       int nr_refspec, const char **refspec, int all);
+void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
+	int force_update);

 /*
  * Given a list of the remote refs and the specification of things to
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 5ebe04a..86dbcb2 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -88,7 +88,7 @@ test_expect_success 'used receive-pack service' '
 	test_cmp exp act
 '

-test_expect_success 'non-fast-forward push fails' '
+test_expect_failure 'non-fast-forward push fails' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout master &&
 	echo "changed" > path2 &&
@@ -100,7 +100,7 @@ test_expect_success 'non-fast-forward push fails' '
 	 test $HEAD != $(git rev-parse --verify HEAD))
 '

-test_expect_failure 'non-fast-forward push show ref status' '
+test_expect_success 'non-fast-forward push show ref status' '
 	grep "^ ! \[rejected\][ ]*master -> master (non-fast-forward)$" output
 '

diff --git a/transport-helper.c b/transport-helper.c
index 11f3d7e..6b1f778 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -329,16 +329,15 @@ static int push_refs(struct transport *transport,
 		return 1;

 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (ref->peer_ref)
-			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
-		else if (!mirror)
+		if (!ref->peer_ref && !mirror)
 			continue;

-		ref->deletion = is_null_sha1(ref->new_sha1);
-		if (!ref->deletion &&
-			!hashcmp(ref->old_sha1, ref->new_sha1)) {
-			ref->status = REF_STATUS_UPTODATE;
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
 			continue;
+		default:
+			; /* do nothing */
 		}

 		if (force_all)
diff --git a/transport.c b/transport.c
index 3eea836..12c4423 100644
--- a/transport.c
+++ b/transport.c
@@ -887,6 +887,10 @@ int transport_push(struct transport *transport,
 			return -1;
 		}

+		set_ref_status_for_push(remote_refs,
+			flags & TRANSPORT_PUSH_MIRROR,
+			flags & TRANSPORT_PUSH_FORCE);
+
 		ret = transport->push_refs(transport, remote_refs, flags);

 		if (!quiet || push_had_errors(remote_refs))
--
1.6.6.rc1.249.g048b3

[PATCH v3 4/6] transport.c::transport_push(): make ref status affect return value

[Tay Ray Chuan]

Use push_had_errors() to check the refs for errors and modify the
return value.

Mark the non-fast-forward push tests to succeed.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed nothing from v2 except tests and mentioning it in the commit
message.

 t/t5541-http-push.sh |    4 ++--
 transport.c          |    7 +++++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 86dbcb2..fee9494 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -88,7 +88,7 @@ test_expect_success 'used receive-pack service' '
 	test_cmp exp act
 '

-test_expect_failure 'non-fast-forward push fails' '
+test_expect_success 'non-fast-forward push fails' '
 	cd "$ROOT_PATH"/test_repo_clone &&
 	git checkout master &&
 	echo "changed" > path2 &&
@@ -104,7 +104,7 @@ test_expect_success 'non-fast-forward push show ref status' '
 	grep "^ ! \[rejected\][ ]*master -> master (non-fast-forward)$" output
 '

-test_expect_failure 'non-fast-forward push shows help message' '
+test_expect_success 'non-fast-forward push shows help message' '
 	grep \
 "To prevent you from losing history, non-fast-forward updates were rejected
 Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
diff --git a/transport.c b/transport.c
index 12c4423..9b23989 100644
--- a/transport.c
+++ b/transport.c
@@ -875,7 +875,7 @@ int transport_push(struct transport *transport,
 		int verbose = flags & TRANSPORT_PUSH_VERBOSE;
 		int quiet = flags & TRANSPORT_PUSH_QUIET;
 		int porcelain = flags & TRANSPORT_PUSH_PORCELAIN;
-		int ret;
+		int ret, err;

 		if (flags & TRANSPORT_PUSH_ALL)
 			match_flags |= MATCH_REFS_ALL;
@@ -892,8 +892,11 @@ int transport_push(struct transport *transport,
 			flags & TRANSPORT_PUSH_FORCE);

 		ret = transport->push_refs(transport, remote_refs, flags);
+		err = push_had_errors(remote_refs);

-		if (!quiet || push_had_errors(remote_refs))
+		ret |= err;
+
+		if (!quiet || err)
 			print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
 					nonfastforward);
--
1.6.6.rc1.249.g048b3

[PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Tay Ray Chuan]

If the status of a ref is REF_STATUS_REJECT_NONFASTFORWARD or
REF_STATUS_UPTODATE, the remote helper will not be told to push the ref
(via a 'push' command).

Therefore, if a ref is not to be pushed, ignore the status report by the
remote helper for that ref - don't overwrite the status of the ref with
the status reported by the helper, nor the message in the remote_status
member.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5541-http-push.sh |    2 +-
 transport-helper.c   |   14 ++++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index fee9494..79867bc 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -111,7 +111,7 @@ Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
 section of '"'git push --help'"' for details." output
 '

-test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
+test_expect_success 'push fails for non-fast-forward refs unmatched by remote helper' '
 	# create a dissimilarly-named ref so that git is unable to match the refs
 	git push origin master:retsam

diff --git a/transport-helper.c b/transport-helper.c
index 6b1f778..bdfa07e 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -429,8 +429,18 @@ static int push_refs(struct transport *transport,
 			continue;
 		}

-		ref->status = status;
-		ref->remote_status = msg;
+		switch (ref->status) {
+		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_UPTODATE:
+			/*
+			 * Earlier, the ref was marked not to be pushed, so ignore what
+			 * the remote helper said about the ref.
+			 */
+			continue;
+		default:
+			ref->status = status;
+			ref->remote_status = msg;
+		}
 	}
 	strbuf_release(&buf);
 	return 0;
--
1.6.6.rc1.249.g048b3

Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Jeff King]

On Thu, Dec 24, 2009 at 03:44:45PM +0800, Tay Ray Chuan wrote:

> -		ref->status = status;
> -		ref->remote_status = msg;
> +		switch (ref->status) {
> +		case REF_STATUS_REJECT_NONFASTFORWARD:
> +		case REF_STATUS_UPTODATE:
> +			/*
> +			 * Earlier, the ref was marked not to be pushed, so ignore what
> +			 * the remote helper said about the ref.
> +			 */
> +			continue;
> +		default:
> +			ref->status = status;
> +			ref->remote_status = msg;
> +		}

It seems like this should be checking for REF_STATUS_NONE explicitly
instead of trying to enumerate the reasons we might not have tried to
push. Shouldn't helpers _only_ be pushing REF_STATUS_NONE refs?

I think right now the two cases are equivalent, since non-ff and
uptodate are the only two states set before the helper is invoked. But
we have discussed in the past (and I still have a patch floating around
for) a REF_STATUS_REWIND which would treat strict rewinds differently
(silently ignoring them instead of making an error). Explicitly checking
REF_STATUS_NONE future-proofs against new states being added.

-Peff

Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Tay Ray Chuan]

Hi,

On Tue, Jan 5, 2010 at 2:32 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Dec 24, 2009 at 03:44:45PM +0800, Tay Ray Chuan wrote:
>
>> -             ref->status = status;
>> -             ref->remote_status = msg;
>> +             switch (ref->status) {
>> +             case REF_STATUS_REJECT_NONFASTFORWARD:
>> +             case REF_STATUS_UPTODATE:
>> +                     /*
>> +                      * Earlier, the ref was marked not to be pushed, so ignore what
>> +                      * the remote helper said about the ref.
>> +                      */
>> +                     continue;
>> +             default:
>> +                     ref->status = status;
>> +                     ref->remote_status = msg;
>> +             }
>
> It seems like this should be checking for REF_STATUS_NONE explicitly
> instead of trying to enumerate the reasons we might not have tried to
> push. Shouldn't helpers _only_ be pushing REF_STATUS_NONE refs?
>
> I think right now the two cases are equivalent, since non-ff and
> uptodate are the only two states set before the helper is invoked. But
> we have discussed in the past (and I still have a patch floating around
> for) a REF_STATUS_REWIND which would treat strict rewinds differently
> (silently ignoring them instead of making an error). Explicitly checking
> REF_STATUS_NONE future-proofs against new states being added.

I'm not really sure if this is true (ie. that if status is not non-ff
or uptodate, then it is REF_STATUS_NONE), but we could step around this
by introducing a property, say, ref->should_push, that is set to 1,
after all the vetting has been carried out and just before we talk to
the server.

That way, we just check that property, without having to know the ref
statuses that would mark a ref not-for-pushing. Sounds future-proof (in
your sense) to me.

--
Cheers,
Ray Chuan

Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Jeff King]

On Tue, Jan 05, 2010 at 06:01:13PM +0800, Tay Ray Chuan wrote:

> > It seems like this should be checking for REF_STATUS_NONE explicitly
> > instead of trying to enumerate the reasons we might not have tried to
> > push. Shouldn't helpers _only_ be pushing REF_STATUS_NONE refs?
> >
> > I think right now the two cases are equivalent, since non-ff and
> > uptodate are the only two states set before the helper is invoked. But
> > we have discussed in the past (and I still have a patch floating around
> > for) a REF_STATUS_REWIND which would treat strict rewinds differently
> > (silently ignoring them instead of making an error). Explicitly checking
> > REF_STATUS_NONE future-proofs against new states being added.
> 
> I'm not really sure if this is true (ie. that if status is not non-ff
> or uptodate, then it is REF_STATUS_NONE), but we could step around this

Well, consider it this way. If it's _not_ REF_STATUS_NONE, then what is
it, and what does it mean to be overwriting it?

Maybe I am misunderstanding the problem the patch is addressing, but the
point of these REF_STATUS feels was to act as a small state machine.
Everything starts as NONE, and then:

  - we compare locally against remote refs. We may transition:
      NONE -> UPTODATE
      NONE -> REJECT_NONFASTFORWARD
      NONE -> REJECT_NODELETE

  - we send the push list
      NONE -> EXPECTING_REPORT (if the remote supports individual status)
      NONE -> OK (otherwise)

  - we get back status responses
      EXPECTING_REPORT -> OK
      EXPECTING_REPORT -> REMOTE_REJECT

I haven't looked closely at the new transport helper code, but I would
think it should stick more or less to those transitions. The exception
would be that some transports don't necessarily handle EXPECTING_REPORT
in the same way, and may transition directly from NONE to
OK/REMOTE_REJECT.

So offhand, I would say that your list should also probably include
REJECT_NODELETE. However, I think that status is just for old servers
which didn't support the delete-refs protocol extension. So presumably
that is none of the new helpers, as they all post-date the addition of
that feature by quite a few years.

> by introducing a property, say, ref->should_push, that is set to 1,
> after all the vetting has been carried out and just before we talk to
> the server.

I'd rather not introduce new state. The point of the status flag was to
encapsulate all of that information, and a new state variable just seems
like introducing extra complexity. If we are not in the NONE state, I
don't see why we would tell the helper about a ref at all.

-Peff

Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Tay Ray Chuan]

Hi,

On Wed, Jan 6, 2010 at 8:04 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 05, 2010 at 06:01:13PM +0800, Tay Ray Chuan wrote:
> 
> > > It seems like this should be checking for REF_STATUS_NONE explicitly
> > > instead of trying to enumerate the reasons we might not have tried to
> > > push. Shouldn't helpers _only_ be pushing REF_STATUS_NONE refs?
> > >
> > > I think right now the two cases are equivalent, since non-ff and
> > > uptodate are the only two states set before the helper is invoked. But
> > > we have discussed in the past (and I still have a patch floating around
> > > for) a REF_STATUS_REWIND which would treat strict rewinds differently
> > > (silently ignoring them instead of making an error). Explicitly checking
> > > REF_STATUS_NONE future-proofs against new states being added.
> > 
> > I'm not really sure if this is true (ie. that if status is not non-ff
> > or uptodate, then it is REF_STATUS_NONE), but we could step around this
> 
> Well, consider it this way. If it's _not_ REF_STATUS_NONE, then what is
> it, and what does it mean to be overwriting it?

Ok, I'll take your suggestion from your previous email and do this:

@@ -429,8 +429,16 @@ static int push_refs(struct transport *transport,
 			continue;
 		}

-		ref->status = status;
-		ref->remote_status = msg;
+		if (ref->status == REF_STATUS_NONE) {
+			ref->status = status;
+			ref->remote_status = msg;
+		} else {
+			/*
+			 * Earlier, the ref was marked not to be pushed, so ignore what
+			 * the remote helper said about the ref.
+			 */
+			continue;
+		}
 	}
 	strbuf_release(&buf);
 	return 0;

Going by this principle (only refs with status of none will be pushed),
I think I should also squash the below into patch 3 (refactor ref status
logic for pushing):

@@ -336,11 +336,10 @@ static int push_refs(struct transport *transport,
 			continue;
 
 		switch (ref->status) {
-		case REF_STATUS_REJECT_NONFASTFORWARD:
-		case REF_STATUS_UPTODATE:
-			continue;
+		case REF_STATUS_NONE:
+			; /* carry on with pushing */
 		default:
-			; /* do nothing */
+			continue;
 		}
 
 		if (force_all)

> Maybe I am misunderstanding the problem the patch is addressing, but the
> point of these REF_STATUS feels was to act as a small state machine.
> Everything starts as NONE, and then:
> 
>   - we compare locally against remote refs. We may transition:
>       NONE -> UPTODATE
>       NONE -> REJECT_NONFASTFORWARD
>       NONE -> REJECT_NODELETE
> 
>   - we send the push list
>       NONE -> EXPECTING_REPORT (if the remote supports individual status)
>       NONE -> OK (otherwise)
>
>   - we get back status responses
>       EXPECTING_REPORT -> OK
>       EXPECTING_REPORT -> REMOTE_REJECT
> 
> I haven't looked closely at the new transport helper code, but I would
> think it should stick more or less to those transitions. The exception
> would be that some transports don't necessarily handle EXPECTING_REPORT
> in the same way, and may transition directly from NONE to
> OK/REMOTE_REJECT.

minor nit: yes, this may differ from transport-to-transport, but
EXPECTING_REPORT is not used at all in the top-level transport (the
level above the helper).

There's also something I'd like to point out for accuracy: it's that
this sequence of transitions occur at two levels, separately: one at
the top-level transport/transport-helper, and another at the helper.

So, for certain non-ff refs (the type this patch series is looking at),
the sequence of state transitions stops and doesn't continue to step 2
in the top-level transport (sending the push list); but separately, in
the helper, the ref goes through another sequence of state transitions.

What this patch touches is the part in the top-level transport that
syncs the ref status between the helper and the top-level transport: do
we take and present to the user what the helper has done, or not?

Regarding this point, I now think that we should ignore the
helper-reported status only if that status is none, and continue
updating the ref status in the top-level transport if the helper did
push successfully/failed, even if we didn't tell it to push:

@@ -429,7 +429,7 @@ static int push_refs(struct transport *transport,

		ref->status = status;
		ref->remote_status = msg;
-		if (ref->status == REF_STATUS_NONE) {
+		if (ref->status == REF_STATUS_NONE && status == REF_STATUS_NONE) {
			ref->status = status;
			ref->remote_status = msg;
		} else {

> So offhand, I would say that your list should also probably include
> REJECT_NODELETE. However, I think that status is just for old servers
> which didn't support the delete-refs protocol extension. So presumably
> that is none of the new helpers, as they all post-date the addition of
> that feature by quite a few years.

You're right, AFAIK, for the smart http protocol; I don't think it
supports NODELETE.

> > by introducing a property, say, ref->should_push, that is set to 1,
> > after all the vetting has been carried out and just before we talk to
> > the server.
> 
> I'd rather not introduce new state. The point of the status flag was to
> encapsulate all of that information, and a new state variable just seems
> like introducing extra complexity. If we are not in the NONE state, I
> don't see why we would tell the helper about a ref at all.

Noted.

-- 
Cheers,
Ray Chuan

Re: [PATCH v3 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Tay Ray Chuan]

Hi,

On Thu, Jan 7, 2010 at 5:41 AM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Regarding this point, I now think that we should ignore the
> helper-reported status only if that status is none, and continue
> updating the ref status in the top-level transport if the helper did
> push successfully/failed, even if we didn't tell it to push:
>
> @@ -429,7 +429,7 @@ static int push_refs(struct transport *transport,
>
>                ref->status = status;
>                ref->remote_status = msg;
> -               if (ref->status == REF_STATUS_NONE) {
> +               if (ref->status == REF_STATUS_NONE && status == REF_STATUS_NONE) {
>                        ref->status = status;
>                        ref->remote_status = msg;
>                } else {

sorry for this broken hunk. I'll be re-sending shortly to make things clearer.

-- 
Cheers,
Ray Chuan

[PATCH v4 5/6] transport-helper.c::push_refs(): ignore helper-reported status if ref is not to be pushed

[Tay Ray Chuan]

If the status of a ref is REF_STATUS_NONE, the remote helper will not
be told to push the ref (via a 'push' command).

However, the remote helper may still act on these refs.

If the helper does act on the ref, and prints a status for it, ignore
the report (ie. don't overwrite the status of the ref with it, nor the
message in the remote_status member) if the reported status is 'no
match'.

This allows the user to be alerted to more "interesting" ref statuses,
like REF_STATUS_NONFASTFORWARD.

Cc: Jeff King <peff@peff.net>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed from v3:
 - consider a ref as marked not for pushing if its status is _not_
   none, rather than checking if the status is nonff or uptodate.
 - ignore ref status reported by remote helper only if it reported 'no
   match'

 t/t5541-http-push.sh |    2 +-
 transport-helper.c   |    9 +++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 979624d..83a8e14 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -111,7 +111,7 @@ Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
 section of '"'git push --help'"' for details." output
 '

-test_expect_failure 'push fails for non-fast-forward refs unmatched by remote helper' '
+test_expect_success 'push fails for non-fast-forward refs unmatched by remote helper' '
 	# create a dissimilarly-named remote ref so that git is unable to match the
 	# two refs (viz. local, remote) unless an explicit refspec is provided.
 	git push origin master:retsam
diff --git a/transport-helper.c b/transport-helper.c
index 7c9b569..71a1e50 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -430,6 +430,15 @@ static int push_refs(struct transport *transport,
 			continue;
 		}

+		if (ref->status != REF_STATUS_NONE) {
+			/*
+			 * Earlier, the ref was marked not to be pushed, so ignore the ref
+			 * status reported by the remote helper if the latter is 'no match'.
+			 */
+			if (status == REF_STATUS_NONE)
+				continue;
+		}
+
 		ref->status = status;
 		ref->remote_status = msg;
 	}
--
1.6.6.341.ga7aec

[PATCH v4 6/6] transport-helper.c::push_refs(): emit "no refs" error message

[Tay Ray Chuan]

Emit an error message when remote_refs is not set.

This behaviour is consistent with that of builtin-send-pack.c and
http-push.c.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 transport-helper.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 71a1e50..8c0b575 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -321,8 +321,11 @@ static int push_refs(struct transport *transport,
 	struct child_process *helper;
 	struct ref *ref;
 
-	if (!remote_refs)
+	if (!remote_refs) {
+		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
+			"Perhaps you should specify a branch such as 'master'.\n");
 		return 0;
+	}
 
 	helper = get_helper(transport);
 	if (!data->push)
-- 
1.6.6.341.ga7aec

[PATCH v3 6/6] transport-helper.c::push_refs(): emit "no refs" error message

[Tay Ray Chuan]

Emit an error message when remote_refs is not set.

This behaviour is consistent with that of builtin-send-pack.c and
http-push.c.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changed nothing from v2.

 transport-helper.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index bdfa07e..5910384 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -321,8 +321,11 @@ static int push_refs(struct transport *transport,
 	struct child_process *helper;
 	struct ref *ref;

-	if (!remote_refs)
+	if (!remote_refs) {
+		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
+			"Perhaps you should specify a branch such as 'master'.\n");
 		return 0;
+	}

 	helper = get_helper(transport);
 	if (!data->push)
--
1.6.6.rc1.249.g048b3

[PATCH v4 0/6] transport: catch non-fast forwards

[Tay Ray Chuan]

Junio, this is re-roll of the 'tr/http-push-ref-status' branch in 'pu'.

Jeff, please don't see this re-roll as me assuming you gave your go-
ahead for the latest discussion; I didn't, I thought squashing in the
hunks I mentioned would make things easier for you, rather than having
them floating around.

Summary:
  This patch series applies on top of 'next', and deals with alerting
  the user to rejected non-fast-forward pushes when using helpers
  (smart).

  Previously, git silently exited. This situation involves the curl
  helper and the smart protocol. The non-fast-forward push is only
  detected when curl executes the rpc client (git-send-pack). Now, we
  detect it before telling the helper to push.

  The series also remedies the scenario where a user would not be aware
  of rejected non-fast-forward pushes.

  It occurs when
    1) there are one or more pushes that succeed and
    2) there are one or more rejected non-fast-forward pushes that
	   involve refs that cannot be matched without explicit refspecs.

  This is due to the re-marking of ref status in transport-helper.c::
  push_refs() when interacting with the remote helper.

  If only non-matched, non-fast-forward refs are involved (ie.
  condition #2 is present without #1), then the situtation does not
  occur - no 'push' commands are passed to the remote helper, no
  interaction with the helper takes place, and no re-marking of ref
  status takes place. The user will be alerted to the rejected
  non-fast-forward push.

  If both are present, re-marking occurs. Even if a ref was not part
  of a 'push' command to the helper, 'ok'/'error' status reports might
  be made for it, since the remote helper does ref matching on its
  side. This led to refs that cannot be matched without an explicit
  refspec marked as REF_STATUS_REJECT_NONFASTFORWARD to be re-marked
  REF_STATUS_NONE.

  (Note: helpers fail to match refs as explicit refspecs are always
  available to the top-level transport mechanism, but only on a need-
  to-know basis to the remote helper via a 'push' command when a ref
  is to be pushed.)

  Interestingly, it is possible for a user to be forever unaware of
  the rejected push if the user relies on information from git push
  alone. As long as the user ensures that one or more other pushes are
  successful (eg. change a tracked ref fast-forwardedly) between the
  execution of git push, the rejected non-fast-forward push won't be
  reported.

Changes from v3:
 - reworded commit message for the test in patch 2
 - added comment on what the switch block is checking for in patch 3
 - changed condition under which ref status reported by remote helper
   is ignored in patch 5

Tay Ray Chuan (6):
  t5541-http-push.sh: add tests for non-fast-forward pushes
  t5541-http-push.sh: add test for unmatched, non-fast-forwarded refs
  refactor ref status logic for pushing
  transport.c::transport_push(): make ref status affect return value
  transport-helper.c::push_refs(): ignore helper-reported status if ref
    is not to be pushed
  transport-helper.c::push_refs(): emit "no refs" error message

 builtin-send-pack.c  |   51 +++++++++++--------------------------------------
 remote.c             |   50 +++++++++++++++++++++++++++++++++++++++++++++++++
 remote.h             |    2 +
 t/t5541-http-push.sh |   44 +++++++++++++++++++++++++++++++++++++++++++
 transport-helper.c   |   28 +++++++++++++++++++-------
 transport.c          |   11 ++++++++-
 6 files changed, 137 insertions(+), 49 deletions(-)

[PATCH v4 1/6] t5541-http-push.sh: add tests for non-fast-forward pushes

[Tay Ray Chuan]

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 t/t5541-http-push.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 2a58d0c..f49c7c4 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -88,5 +88,28 @@ test_expect_success 'used receive-pack service' '
 	test_cmp exp act
 '
 
+test_expect_success 'non-fast-forward push fails' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	git checkout master &&
+	echo "changed" > path2 &&
+	git commit -a -m path2 --amend &&
+
+	HEAD=$(git rev-parse --verify HEAD) &&
+	!(git push -v origin >output 2>&1) &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+	 test $HEAD != $(git rev-parse --verify HEAD))
+'
+
+test_expect_failure 'non-fast-forward push show ref status' '
+	grep "^ ! \[rejected\][ ]*master -> master (non-fast-forward)$" output
+'
+
+test_expect_failure 'non-fast-forward push shows help message' '
+	grep \
+"To prevent you from losing history, non-fast-forward updates were rejected
+Merge the remote changes before pushing again.  See the '"'non-fast-forward'"'
+section of '"'git push --help'"' for details." output
+'
+
 stop_httpd
 test_done
-- 
1.6.6.341.ga7aec