[PATCH 2/5] send-pack.c: add an --atomic-push command line argument

[PATCH 2/5] send-pack.c: add an --atomic-push command line argument

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 Documentation/git-send-pack.txt | 7 ++++++-
 builtin/send-pack.c             | 6 +++++-
 send-pack.c                     | 8 +++++++-
 send-pack.h                     | 1 +
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index dc3a568..4ee2ca1 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]
+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic-push] [<host>:]<directory> [<ref>...]
 
 DESCRIPTION
 -----------
@@ -52,6 +52,11 @@ OPTIONS
 	Send a "thin" pack, which records objects in deltified form based
 	on objects not included in the pack to reduce network traffic.
 
+--atomic-push::
+	With atomic-push all refs are updated in one single atomic transaction.
+	This means that if any of the refs fails then the entire push will
+	fail without changing any refs.
+
 <host>::
 	A remote host to house the repository.  When this
 	part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..78e7d8f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
 #include "sha1-array.h"
 
 static const char send_pack_usage[] =
-"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
+"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic-push] [<host>:]<directory> [<ref>...]\n"
 "  --all and explicit <ref> specification are mutually exclusive.";
 
 static struct send_pack_args args;
@@ -165,6 +165,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				args.use_thin_pack = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--atomic-push")) {
+				args.use_atomic_push = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--stateless-rpc")) {
 				args.stateless_rpc = 1;
 				continue;
diff --git a/send-pack.c b/send-pack.c
index f91b8d9..66f3724 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -228,6 +228,11 @@ int send_pack(struct send_pack_args *args,
 	if (server_supports("atomic-push"))
 		atomic_push_supported = 1;
 
+	if (args->use_atomic_push && !atomic_push_supported) {
+		fprintf(stderr, "Server does not support atomic-push.");
+		return -1;
+	}
+
 	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");
@@ -272,7 +277,8 @@ int send_pack(struct send_pack_args *args,
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 			int quiet = quiet_supported && (args->quiet || !args->progress);
-			int atomic_push = atomic_push_supported;
+			int atomic_push = atomic_push_supported &&
+				args->use_atomic_push;
 
 			if (!cmds_sent && (status_report || use_sideband ||
 					   quiet || agent_supported ||
diff --git a/send-pack.h b/send-pack.h
index 8e84392..0374ed8 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -10,6 +10,7 @@ struct send_pack_args {
 		force_update:1,
 		use_thin_pack:1,
 		use_ofs_delta:1,
+		use_atomic_push:1,
 		dry_run:1,
 		stateless_rpc:1;
 };
-- 
2.0.1.528.gd0e7a84

[PATCH 0/5] ref-transactions-send-pack

[Ronnie Sahlberg]

List,

This small patch series adds atomic-push support for pushes.
By default git will use the old style non-atomic updates for pushes,
as not to cause disruption in client scripts that may depend on that
behaviour.

Command line arguments are introduced to allow the client side to request/
negotiate atomic pushes if the remote repo supports it.
There is also a new configuration variable where a repo can set that it
wants all pushes to become atomic whether the client requests it or not.

This patch series is called ref-transactions-send-pack and depends on/is built
ontop of the series called ref-transactions-req-strbuf-err

Ronnie Sahlberg (5):
  receive-pack.c: add protocol support to negotiate atomic-push
  send-pack.c: add an --atomic-push command line argument
  receive-pack.c: use a single transaction when atomic-push is
    negotiated
  receive-pack.c: add receive.atomicpush configuration option
  push.c: add an --atomic-push argument

 Documentation/config.txt                          |  5 ++
 Documentation/git-push.txt                        |  7 ++-
 Documentation/git-send-pack.txt                   |  7 ++-
 Documentation/technical/protocol-capabilities.txt |  7 +++
 builtin/push.c                                    |  2 +
 builtin/receive-pack.c                            | 66 ++++++++++++++++++-----
 builtin/send-pack.c                               |  6 ++-
 send-pack.c                                       | 18 +++++--
 send-pack.h                                       |  1 +
 transport.c                                       |  1 +
 transport.h                                       |  1 +
 11 files changed, 103 insertions(+), 18 deletions(-)

-- 
2.0.1.556.ge8f7cba.dirty

[PATCH 1/5] receive-pack.c: add protocol support to negotiate atomic-push

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/receive-pack.c |  6 +++++-
 send-pack.c            | 12 +++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0565b94..f6b20cb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -36,6 +36,7 @@ static int transfer_unpack_limit = -1;
 static int unpack_limit = 100;
 static int report_status;
 static int use_sideband;
+static int use_atomic_push;
 static int quiet;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
@@ -142,7 +143,8 @@ static void show_ref(const char *path, const unsigned char *sha1)
 	else
 		packet_write(1, "%s %s%c%s%s agent=%s\n",
 			     sha1_to_hex(sha1), path, 0,
-			     " report-status delete-refs side-band-64k quiet",
+			     " report-status delete-refs side-band-64k quiet"
+			     " atomic-push",
 			     prefer_ofs_delta ? " ofs-delta" : "",
 			     git_user_agent_sanitized());
 	sent_capabilities = 1;
@@ -892,6 +894,8 @@ static struct command *read_head_info(struct sha1_array *shallow)
 				use_sideband = LARGE_PACKET_MAX;
 			if (parse_feature_request(feature_list, "quiet"))
 				quiet = 1;
+			if (parse_feature_request(feature_list, "atomic-push"))
+				use_atomic_push = 1;
 		}
 		cmd = xcalloc(1, sizeof(struct command) + len - 80);
 		hashcpy(cmd->old_sha1, old_sha1);
diff --git a/send-pack.c b/send-pack.c
index 6129b0f..f91b8d9 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -205,6 +205,7 @@ int send_pack(struct send_pack_args *args,
 	int use_sideband = 0;
 	int quiet_supported = 0;
 	int agent_supported = 0;
+	int atomic_push_supported = 0;
 	unsigned cmds_sent = 0;
 	int ret;
 	struct async demux;
@@ -224,6 +225,8 @@ int send_pack(struct send_pack_args *args,
 		agent_supported = 1;
 	if (server_supports("no-thin"))
 		args->use_thin_pack = 0;
+	if (server_supports("atomic-push"))
+		atomic_push_supported = 1;
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -269,17 +272,20 @@ int send_pack(struct send_pack_args *args,
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 			int quiet = quiet_supported && (args->quiet || !args->progress);
+			int atomic_push = atomic_push_supported;
 
 			if (!cmds_sent && (status_report || use_sideband ||
-					   quiet || agent_supported)) {
+					   quiet || agent_supported ||
+					   atomic_push)) {
 				packet_buf_write(&req_buf,
-						 "%s %s %s%c%s%s%s%s%s",
+						 "%s %s %s%c%s%s%s%s%s%s",
 						 old_hex, new_hex, ref->name, 0,
 						 status_report ? " report-status" : "",
 						 use_sideband ? " side-band-64k" : "",
 						 quiet ? " quiet" : "",
 						 agent_supported ? " agent=" : "",
-						 agent_supported ? git_user_agent_sanitized() : ""
+						 agent_supported ? git_user_agent_sanitized() : "",
+						 atomic_push ? " atomic-push" : ""
 						);
 			}
 			else
-- 
2.0.1.556.ge8f7cba.dirty

[PATCH 2/5] send-pack.c: add an --atomic-push command line argument

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 Documentation/git-send-pack.txt | 7 ++++++-
 builtin/send-pack.c             | 6 +++++-
 send-pack.c                     | 8 +++++++-
 send-pack.h                     | 1 +
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index dc3a568..4ee2ca1 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]
+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic-push] [<host>:]<directory> [<ref>...]
 
 DESCRIPTION
 -----------
@@ -52,6 +52,11 @@ OPTIONS
 	Send a "thin" pack, which records objects in deltified form based
 	on objects not included in the pack to reduce network traffic.
 
+--atomic-push::
+	With atomic-push all refs are updated in one single atomic transaction.
+	This means that if any of the refs fails then the entire push will
+	fail without changing any refs.
+
 <host>::
 	A remote host to house the repository.  When this
 	part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f420b74..78e7d8f 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
 #include "sha1-array.h"
 
 static const char send_pack_usage[] =
-"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
+"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic-push] [<host>:]<directory> [<ref>...]\n"
 "  --all and explicit <ref> specification are mutually exclusive.";
 
 static struct send_pack_args args;
@@ -165,6 +165,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				args.use_thin_pack = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--atomic-push")) {
+				args.use_atomic_push = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--stateless-rpc")) {
 				args.stateless_rpc = 1;
 				continue;
diff --git a/send-pack.c b/send-pack.c
index f91b8d9..66f3724 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -228,6 +228,11 @@ int send_pack(struct send_pack_args *args,
 	if (server_supports("atomic-push"))
 		atomic_push_supported = 1;
 
+	if (args->use_atomic_push && !atomic_push_supported) {
+		fprintf(stderr, "Server does not support atomic-push.");
+		return -1;
+	}
+
 	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");
@@ -272,7 +277,8 @@ int send_pack(struct send_pack_args *args,
 			char *old_hex = sha1_to_hex(ref->old_sha1);
 			char *new_hex = sha1_to_hex(ref->new_sha1);
 			int quiet = quiet_supported && (args->quiet || !args->progress);
-			int atomic_push = atomic_push_supported;
+			int atomic_push = atomic_push_supported &&
+				args->use_atomic_push;
 
 			if (!cmds_sent && (status_report || use_sideband ||
 					   quiet || agent_supported ||
diff --git a/send-pack.h b/send-pack.h
index 8e84392..0374ed8 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -10,6 +10,7 @@ struct send_pack_args {
 		force_update:1,
 		use_thin_pack:1,
 		use_ofs_delta:1,
+		use_atomic_push:1,
 		dry_run:1,
 		stateless_rpc:1;
 };
-- 
2.0.1.556.ge8f7cba.dirty

[PATCH 3/5] receive-pack.c: use a single transaction when atomic-push is negotiated

[Ronnie Sahlberg]

Update receive-pack to use an atomic transaction IFF the client negotiated
that it wanted atomic-push.
This leaves the default behaviour to be the old non-atomic one ref at a
time update. This is to cause as little disruption as possible to existing
clients. It is unknown if there are client scripts that depend on the old
non-atomic behaviour so we make it opt-in for now.

Later patch in this series also adds a configuration variable where you can
override the atomic push behaviour on the receiving repo and force it
to use atomic updates always.

If it turns out over time that there are no client scripts that depend on the
old behaviour we can change git to default to use atomic pushes and instead
offer an opt-out argument for people that do not want atomic pushes.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 Documentation/technical/protocol-capabilities.txt |  7 +++
 builtin/receive-pack.c                            | 55 ++++++++++++++++++-----
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index e174343..93d99c5 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -250,3 +250,10 @@ allow-tip-sha1-in-want
 If the upload-pack server advertises this capability, fetch-pack may
 send "want" lines with SHA-1s that exist at the server but are not
 advertised by upload-pack.
+
+atomic-push
+-----------
+
+If the receive-pack server advertises the 'atomic-push' capability, it means
+that it is capable of accepting atomic pushes. An atomic push is a push
+where the ref updates are done as a single atomic transaction.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f6b20cb..47f778d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -47,6 +47,8 @@ static void *head_name_to_free;
 static int sent_capabilities;
 static int shallow_update;
 static const char *alt_shallow_file;
+struct strbuf err = STRBUF_INIT;
+struct ref_transaction *transaction;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
 {
@@ -577,26 +579,38 @@ static char *update(struct command *cmd, struct shallow_info *si)
 		return NULL; /* good */
 	}
 	else {
-		struct strbuf err = STRBUF_INIT;
-		struct ref_transaction *transaction;
-
 		if (shallow_update && si->shallow_ref[cmd->index] &&
 		    update_shallow_ref(cmd, si))
 			return xstrdup("shallow error");
-
-		transaction = transaction_begin(&err);
-		if (!transaction ||
-		    transaction_update_sha1(transaction, namespaced_name,
+		if (!use_atomic_push) {
+			transaction = transaction_begin(&err);
+			if (!transaction) {
+				char *str = xstrdup(err.buf);
+
+				strbuf_release(&err);
+				transaction_free(transaction);
+				rp_error("%s", str);
+				return str;
+			}
+		}
+		if (transaction_update_sha1(transaction, namespaced_name,
 					    new_sha1, old_sha1, 0, 1, "push",
-					    &err) ||
-		    transaction_commit(transaction, &err)) {
-			char *str = strbuf_detach(&err, NULL);
-			transaction_free(transaction);
+					    &err)) {
+			char *str = xstrdup(err.buf);
 
+			strbuf_release(&err);
+			transaction_free(transaction);
 			rp_error("%s", str);
 			return str;
 		}
+		if (!use_atomic_push && transaction_commit(transaction, &err)) {
+			char *str = xstrdup(err.buf);
 
+			strbuf_release(&err);
+			transaction_free(transaction);
+			rp_error("%s", str);
+			return str;
+		}
 		transaction_free(transaction);
 		strbuf_release(&err);
 		return NULL; /* good */
@@ -810,6 +824,16 @@ static void execute_commands(struct command *commands,
 		return;
 	}
 
+	if (use_atomic_push) {
+		transaction = transaction_begin(&err);
+		if (!transaction) {
+			error("%s", err.buf);
+			strbuf_release(&err);
+			for (cmd = commands; cmd; cmd = cmd->next)
+				cmd->error_string = "transaction error";
+			return;
+		}
+	}
 	data.cmds = commands;
 	data.si = si;
 	if (check_everything_connected(iterate_receive_command_list, 0, &data))
@@ -848,6 +872,14 @@ static void execute_commands(struct command *commands,
 		}
 	}
 
+	if (use_atomic_push) {
+		if (transaction_commit(transaction, &err)) {
+			rp_error("%s", err.buf);
+			for (cmd = commands; cmd; cmd = cmd->next)
+				cmd->error_string = err.buf;
+		}
+		transaction_free(transaction);
+	}
 	if (shallow_update && !checked_connectivity)
 		error("BUG: run 'git fsck' for safety.\n"
 		      "If there are errors, try to remove "
@@ -1250,5 +1282,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 	sha1_array_clear(&shallow);
 	sha1_array_clear(&ref);
 	free_commands(commands);
+	strbuf_release(&err);
 	return 0;
 }
-- 
2.0.1.556.ge8f7cba.dirty

[PATCH 4/5] receive-pack.c: add receive.atomicpush configuration option

[Ronnie Sahlberg]

Add a configuration argument to the receive side to force atomic pushes
for all pushes to the repo.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 Documentation/config.txt | 5 +++++
 builtin/receive-pack.c   | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1d718bd..75ce157 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2038,6 +2038,11 @@ rebase.autostash::
 	successful rebase might result in non-trivial conflicts.
 	Defaults to false.
 
+receive.atomicpush::
+	By default, git-receive-pack will only use atomic ref transactions
+	if the client negotiates it. When set to true, git-receive-pack
+	will force atomic ref updates for all client pushes.
+
 receive.autogc::
 	By default, git-receive-pack will run "git-gc --auto" after
 	receiving data from git-push and updating refs.  You can stop
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 47f778d..9fa637a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -132,6 +132,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.atomicpush") == 0) {
+		use_atomic_push = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_default_config(var, value, cb);
 }
 
-- 
2.0.1.556.ge8f7cba.dirty

[PATCH 5/5] push.c: add an --atomic-push argument

[Ronnie Sahlberg]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 Documentation/git-push.txt | 7 ++++++-
 builtin/push.c             | 2 ++
 transport.c                | 1 +
 transport.h                | 1 +
 4 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21cd455..b80b0ac 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -9,7 +9,7 @@ git-push - Update remote refs along with associated objects
 SYNOPSIS
 --------
 [verse]
-'git push' [--all | --mirror | --tags] [--follow-tags] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
+'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic-push] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
 	   [--repo=<repository>] [-f | --force] [--prune] [-v | --verbose] [-u | --set-upstream]
 	   [--force-with-lease[=<refname>[:<expect>]]]
 	   [--no-verify] [<repository> [<refspec>...]]
@@ -129,6 +129,11 @@ already exists on the remote side.
 	from the remote but are pointing at commit-ish that are
 	reachable from the refs being pushed.
 
+--atomic-push::
+	Try using atomic push. If atomic push is negotiated with the server
+	then any push covering multiple refs will be atomic. Either all
+	refs are updated, or on error, no refs are updated.
+
 --receive-pack=<git-receive-pack>::
 --exec=<git-receive-pack>::
 	Path to the 'git-receive-pack' program on the remote
diff --git a/builtin/push.c b/builtin/push.c
index f8dfea4..f37390c 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -507,6 +507,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), TRANSPORT_PUSH_NO_HOOK),
 		OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
 			TRANSPORT_PUSH_FOLLOW_TAGS),
+		OPT_BIT(0, "atomic-push", &flags, N_("use atomic push, if available"),
+			TRANSPORT_ATOMIC_PUSH),
 		OPT_END()
 	};
 
diff --git a/transport.c b/transport.c
index a3b7f48..ab5f553 100644
--- a/transport.c
+++ b/transport.c
@@ -837,6 +837,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
 	args.progress = transport->progress;
 	args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
 	args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
+	args.use_atomic_push = !!(flags & TRANSPORT_ATOMIC_PUSH);
 
 	ret = send_pack(&args, data->fd, data->conn, remote_refs,
 			&data->extra_have);
diff --git a/transport.h b/transport.h
index 02ea248..407d641 100644
--- a/transport.h
+++ b/transport.h
@@ -123,6 +123,7 @@ struct transport {
 #define TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND 256
 #define TRANSPORT_PUSH_NO_HOOK 512
 #define TRANSPORT_PUSH_FOLLOW_TAGS 1024
+#define TRANSPORT_ATOMIC_PUSH 2048
 
 #define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
 #define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
-- 
2.0.1.556.ge8f7cba.dirty

[PATCH 2/5] send-pack.c: add an --atomic-push command line argument

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

This adds support to send-pack to negotiate and use atomic pushes
iff the server supports it. Atomic pushes are activated by a new command
line flag --atomic-push.

In order to do this we also need to change the semantics for send_pack()
slightly. The existing send_pack() function actually doesn't send all the
refs back to the server when multiple refs are involved, for example
when using --all. Several of the failure modes for pushes can already be
detected locally in the send_pack client based on the information from the
initial server side list of all the refs as generated by receive-pack.
Any such refs that we thus know would fail to push are thus pruned from
the list of refs we send to the server to update.

For atomic pushes, we have to deal thus with both failures that are detected
locally as well as failures that are reported back from the server. In order
to do so we treat all local failures as push failures too.

We introduce a new status code REF_STATUS_ATOMIC_PUSH_FAILED so we can
flag all refs that we would normally have tried to push to the server
but we did not due to local failures. This is to improve the error message
back to the end user to flag that "these refs failed to update since the
atomic push operation failed."

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-send-pack.txt |  7 ++++++-
 builtin/send-pack.c             |  6 +++++-
 remote.h                        |  3 ++-
 send-pack.c                     | 39 ++++++++++++++++++++++++++++++++++-----
 send-pack.h                     |  1 +
 transport.c                     |  4 ++++
 6 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2a0de42..9296587 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,7 +9,7 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]
+'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic-push] [<host>:]<directory> [<ref>...]
 
 DESCRIPTION
 -----------
@@ -62,6 +62,11 @@ be in a separate packet, and the list must end with a flush packet.
 	Send a "thin" pack, which records objects in deltified form based
 	on objects not included in the pack to reduce network traffic.
 
+--atomic-push::
+	Use an atomic transaction for updating the refs. If any of the refs
+	fails to update then the entire push will fail without changing any
+	refs.
+
 <host>::
 	A remote host to house the repository.  When this
 	part is specified, 'git-receive-pack' is invoked via
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index b564a77..93cb17c 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -13,7 +13,7 @@
 #include "sha1-array.h"
 
 static const char send_pack_usage[] =
-"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [<host>:]<directory> [<ref>...]\n"
+"git send-pack [--all | --mirror] [--dry-run] [--force] [--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic-push] [<host>:]<directory> [<ref>...]\n"
 "  --all and explicit <ref> specification are mutually exclusive.";
 
 static struct send_pack_args args;
@@ -170,6 +170,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 				args.use_thin_pack = 1;
 				continue;
 			}
+			if (!strcmp(arg, "--atomic-push")) {
+				args.use_atomic_push = 1;
+				continue;
+			}
 			if (!strcmp(arg, "--stateless-rpc")) {
 				args.stateless_rpc = 1;
 				continue;
diff --git a/remote.h b/remote.h
index 8b62efd..f346524 100644
--- a/remote.h
+++ b/remote.h
@@ -115,7 +115,8 @@ struct ref {
 		REF_STATUS_REJECT_SHALLOW,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
-		REF_STATUS_EXPECTING_REPORT
+		REF_STATUS_EXPECTING_REPORT,
+		REF_STATUS_ATOMIC_PUSH_FAILED
 	} status;
 	char *remote_status;
 	struct ref *peer_ref; /* when renaming */
diff --git a/send-pack.c b/send-pack.c
index 1ccc84c..08602a8 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,7 +190,7 @@ static void advertise_shallow_grafts_buf(struct strbuf *sb)
 	for_each_commit_graft(advertise_shallow_grafts_cb, sb);
 }
 
-static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)
+static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args, int *atomic_push_failed)
 {
 	if (!ref->peer_ref && !args->send_mirror)
 		return 0;
@@ -203,6 +203,12 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a
 	case REF_STATUS_REJECT_NEEDS_FORCE:
 	case REF_STATUS_REJECT_STALE:
 	case REF_STATUS_REJECT_NODELETE:
+		if (atomic_push_failed) {
+			fprintf(stderr, "Atomic push failed for ref %s. "
+				"Status:%d\n", ref->name, ref->status);
+			*atomic_push_failed = 1;
+		}
+		/* fallthrough */
 	case REF_STATUS_UPTODATE:
 		return 0;
 	default:
@@ -250,7 +256,7 @@ static int generate_push_cert(struct strbuf *req_buf,
 	strbuf_addstr(&cert, "\n");
 
 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (!ref_update_to_be_sent(ref, args))
+		if (!ref_update_to_be_sent(ref, args, NULL))
 			continue;
 		update_seen = 1;
 		strbuf_addf(&cert, "%s %s %s\n",
@@ -297,7 +303,7 @@ int send_pack(struct send_pack_args *args,
 	int atomic_push_supported = 0;
 	int atomic_push = 0;
 	unsigned cmds_sent = 0;
-	int ret;
+	int ret, atomic_push_failed = 0;
 	struct async demux;
 	const char *push_cert_nonce = NULL;
 
@@ -332,6 +338,11 @@ int send_pack(struct send_pack_args *args,
 			"Perhaps you should specify a branch such as 'master'.\n");
 		return 0;
 	}
+	if (args->use_atomic_push && !atomic_push_supported) {
+		fprintf(stderr, "Server does not support atomic-push.");
+		return -1;
+	}
+	atomic_push = atomic_push_supported && args->use_atomic_push;
 
 	if (status_report)
 		strbuf_addstr(&cap_buf, " report-status");
@@ -365,7 +376,8 @@ int send_pack(struct send_pack_args *args,
 	 * the pack data.
 	 */
 	for (ref = remote_refs; ref; ref = ref->next) {
-		if (!ref_update_to_be_sent(ref, args))
+		if (!ref_update_to_be_sent(ref, args,
+			args->use_atomic_push ? &atomic_push_failed : NULL))
 			continue;
 
 		if (!ref->deletion)
@@ -377,6 +389,23 @@ int send_pack(struct send_pack_args *args,
 			ref->status = REF_STATUS_EXPECTING_REPORT;
 	}
 
+	if (atomic_push_failed) {
+		for (ref = remote_refs; ref; ref = ref->next) {
+			if (!ref->peer_ref && !args->send_mirror)
+				continue;
+
+			switch (ref->status) {
+			case REF_STATUS_EXPECTING_REPORT:
+				ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;
+				continue;
+			default:
+				; /* do nothing */
+			}
+		}
+		fprintf(stderr, "Atomic push failed.");
+		return -1;
+	}
+
 	/*
 	 * Finally, tell the other end!
 	 */
@@ -386,7 +415,7 @@ int send_pack(struct send_pack_args *args,
 		if (args->dry_run || args->push_cert)
 			continue;
 
-		if (!ref_update_to_be_sent(ref, args))
+		if (!ref_update_to_be_sent(ref, args, NULL))
 			continue;
 
 		old_hex = sha1_to_hex(ref->old_sha1);
diff --git a/send-pack.h b/send-pack.h
index 5635457..7486e65 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -11,6 +11,7 @@ struct send_pack_args {
 		force_update:1,
 		use_thin_pack:1,
 		use_ofs_delta:1,
+		use_atomic_push:1,
 		dry_run:1,
 		push_cert:1,
 		stateless_rpc:1;
diff --git a/transport.c b/transport.c
index 70d38e4..e87481d 100644
--- a/transport.c
+++ b/transport.c
@@ -728,6 +728,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 						 ref->deletion ? NULL : ref->peer_ref,
 						 "remote failed to report status", porcelain);
 		break;
+	case REF_STATUS_ATOMIC_PUSH_FAILED:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "atomic-push-failed", porcelain);
+		break;
 	case REF_STATUS_OK:
 		print_ok_ref_status(ref, porcelain);
 		break;
-- 
2.2.0.33.gc2219e3.dirty

Re: [PATCH 2/5] send-pack.c: add an --atomic-push command line argument

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> -static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args)
> +static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_args *args, int *atomic_push_failed)

Hmph.  Is "atomic push" so special that it deserves a separate
parameter?  When we come up with yet another mode of failure, would
we add another parameter to the callers to this function?

> @@ -203,6 +203,12 @@ static int ref_update_to_be_sent(const struct ref *ref, const struct send_pack_a
>  	case REF_STATUS_REJECT_NEEDS_FORCE:
>  	case REF_STATUS_REJECT_STALE:
>  	case REF_STATUS_REJECT_NODELETE:
> +		if (atomic_push_failed) {
> +			fprintf(stderr, "Atomic push failed for ref %s. "
> +				"Status:%d\n", ref->name, ref->status);
> +			*atomic_push_failed = 1;

All other error message that come from the codepaths around here
seem to avoid uppercase.  Also maybe you want to use error() here?

> +	if (args->use_atomic_push && !atomic_push_supported) {
> +		fprintf(stderr, "Server does not support atomic-push.");
> +		return -1;
> +	}

This check logically belongs to the previous step, no?

> +	atomic_push = atomic_push_supported && args->use_atomic_push;

>  
>  	if (status_report)
>  		strbuf_addstr(&cap_buf, " report-status");
> @@ -365,7 +376,8 @@ int send_pack(struct send_pack_args *args,
>  	 * the pack data.
>  	 */
>  	for (ref = remote_refs; ref; ref = ref->next) {
> -		if (!ref_update_to_be_sent(ref, args))
> +		if (!ref_update_to_be_sent(ref, args,
> +			args->use_atomic_push ? &atomic_push_failed : NULL))
>  			continue;
>  
>  		if (!ref->deletion)
> @@ -377,6 +389,23 @@ int send_pack(struct send_pack_args *args,
>  			ref->status = REF_STATUS_EXPECTING_REPORT;
>  	}
>  
> +	if (atomic_push_failed) {
> +		for (ref = remote_refs; ref; ref = ref->next) {
> +			if (!ref->peer_ref && !args->send_mirror)
> +				continue;
> +
> +			switch (ref->status) {
> +			case REF_STATUS_EXPECTING_REPORT:
> +				ref->status = REF_STATUS_ATOMIC_PUSH_FAILED;
> +				continue;
> +			default:
> +				; /* do nothing */
> +			}
> +		}
> +		fprintf(stderr, "Atomic push failed.");
> +		return -1;

The same comment as the other fprintf() applies here.

> @@ -728,6 +728,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
>  						 ref->deletion ? NULL : ref->peer_ref,
>  						 "remote failed to report status", porcelain);
>  		break;
> +	case REF_STATUS_ATOMIC_PUSH_FAILED:
> +		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
> +						 "atomic-push-failed", porcelain);

Why dashed-words-here?

> +		break;
>  	case REF_STATUS_OK:
>  		print_ok_ref_status(ref, porcelain);
>  		break;