* [PATCHv8 0/9] atomic pushes
@ 2014-12-30 2:36 Stefan Beller
2014-12-30 2:36 ` [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support Stefan Beller
` (8 more replies)
0 siblings, 9 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
The patch
[PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes
was dropped and redone as 3 separate patches. This wasn't just done for doing it,
but the end result has also changed. We have more smaller functions doing
one thing instead of these larger functions. Thanks for the ideas, Eric!
Also the advertisement of the atomic capabilites was moved to a later new patch
in this series. This helps when you want to bisect this series later.
Thanks Michael for pointing this out!
Thanks,
Stefan
Ronnie Sahlberg (3):
receive-pack.c: add documentation for atomic push support
send-pack.c: add --atomic command line argument
push.c: add an --atomic argument
Stefan Beller (6):
send-pack: rename ref_update_to_be_sent to check_to_send_update
receive-pack.c: simplify execute_commands
receive-pack.c: move transaction handling in a central place
receive-pack.c: add execute_commands_atomic function
receive-pack.c: enable atomic push protocol support
t5543-atomic-push.sh: add basic tests for atomic pushes
Documentation/git-push.txt | 7 +-
Documentation/git-send-pack.txt | 7 +-
Documentation/technical/protocol-capabilities.txt | 13 +-
builtin/push.c | 5 +
builtin/receive-pack.c | 168 +++++++++++++++-----
builtin/send-pack.c | 6 +-
remote.h | 3 +-
send-pack.c | 65 +++++++-
send-pack.h | 3 +-
t/t5543-atomic-push.sh | 178 ++++++++++++++++++++++
transport.c | 5 +
transport.h | 1 +
12 files changed, 410 insertions(+), 51 deletions(-)
create mode 100755 t/t5543-atomic-push.sh
--
2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 7:08 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 2/9] send-pack: rename ref_update_to_be_sent to check_to_send_update Stefan Beller
` (7 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine
Cc: git, Ronnie Sahlberg, Stefan Beller
From: Ronnie Sahlberg <sahlberg@google.com>
This documents the protocol option between send-pack and receive-pack to
* allow receive-pack to inform the client that it has atomic push capability
* allow send-pack to request atomic push back.
There is currently no setting in send-pack to actually request that atomic
pushes are to be used yet. This only adds protocol capability not ability
for the user to activate it. The capability is also not yet advertised
by receive-pack as git doesn't know how to handle it yet.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
Changes v1 -> v2:
* Name it atomic instead of atomic-push. The name changes affects
the flags send over the wire as well as the flags in
struct send_pack_args
* Add check, which was part of the later patch here:
if (args->atomic && !atomic_supported) {
fprintf(stderr, "Server does not support atomic push.");
return -1;
}
v2 -> v3:
More concise error reporting as suggested by Junio
- fprintf(stderr, "Server does not support atomic push.");
- return -1;
+ return error("server does not support atomic push.");
skipped v4 v5
v6:
* s/one single atomic transaction./one atomic transaction./
* die(_( instead of return error(
v7:
* Don't advertise the atomic push capability yet. We cannot handle it at
this point.
* reword commit message
v8:
no changes
Documentation/technical/protocol-capabilities.txt | 13 +++++++++++--
builtin/receive-pack.c | 3 +++
send-pack.c | 10 ++++++++++
send-pack.h | 3 ++-
4 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 6d5424c..4f8a7bf 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client requested
and server advertised. As a consequence of these rules, server MUST
NOT advertise capabilities it does not understand.
-The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
-are sent and recognized by the receive-pack (push to server) process.
+The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
+capabilities are sent and recognized by the receive-pack (push to server)
+process.
The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
by both upload-pack and receive-pack protocols. The 'agent' capability
@@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress
reporting if the local progress reporting is also being suppressed
(e.g., via `push -q`, or if stderr does not go to a tty).
+atomic
+------
+
+If the server sends the 'atomic' capability it is capable of accepting
+atomic pushes. If the pushing client requests this capability, the server
+will update the refs in one atomic transaction. Either all refs are
+updated or none.
+
allow-tip-sha1-in-want
----------------------
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..4e8eaf7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
static int unpack_limit = 100;
static int report_status;
static int use_sideband;
+static int use_atomic;
static int quiet;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
@@ -1179,6 +1180,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"))
+ use_atomic = 1;
}
if (!strcmp(line, "push-cert")) {
diff --git a/send-pack.c b/send-pack.c
index 949cb61..6d0c159 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
int use_sideband = 0;
int quiet_supported = 0;
int agent_supported = 0;
+ int use_atomic = 0;
+ int atomic_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -314,6 +316,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"))
+ atomic_supported = 1;
if (args->push_cert) {
int len;
@@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,
"Perhaps you should specify a branch such as 'master'.\n");
return 0;
}
+ if (args->atomic && !atomic_supported)
+ die(_("server does not support --atomic push"));
+
+ use_atomic = atomic_supported && args->atomic;
if (status_report)
strbuf_addstr(&cap_buf, " report-status");
@@ -335,6 +343,8 @@ int send_pack(struct send_pack_args *args,
strbuf_addstr(&cap_buf, " side-band-64k");
if (quiet_supported && (args->quiet || !args->progress))
strbuf_addstr(&cap_buf, " quiet");
+ if (use_atomic)
+ strbuf_addstr(&cap_buf, " atomic");
if (agent_supported)
strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
diff --git a/send-pack.h b/send-pack.h
index 5635457..b664648 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -13,7 +13,8 @@ struct send_pack_args {
use_ofs_delta:1,
dry_run:1,
push_cert:1,
- stateless_rpc:1;
+ stateless_rpc:1,
+ atomic:1;
};
int send_pack(struct send_pack_args *args,
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 2/9] send-pack: rename ref_update_to_be_sent to check_to_send_update
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
2014-12-30 2:36 ` [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 2:36 ` [PATCHv8 3/9] send-pack.c: add --atomic command line argument Stefan Beller
` (6 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
This renames ref_update_to_be_sent to check_to_send_update and inverts
the meaning of the return value. Having the return value inverted we
can have different values for the error codes. This is useful in a
later patch when we want to know if we hit the CHECK_REF_STATUS_REJECTED
case.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
This was introduced with the [PATCHv2] series.
Changes v2 -> v3:
* Rename to check_to_send_update
* Negative error values.
* errors values are #define'd and not raw numbers.
skipped v4 v5
v6:
* negative #define'd values
v7, v8:
* no changes
send-pack.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/send-pack.c b/send-pack.c
index 6d0c159..b7d8e01 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -190,10 +190,13 @@ 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)
+#define CHECK_REF_NO_PUSH -1
+#define CHECK_REF_STATUS_REJECTED -2
+#define CHECK_REF_UPTODATE -3
+static int check_to_send_update(const struct ref *ref, const struct send_pack_args *args)
{
if (!ref->peer_ref && !args->send_mirror)
- return 0;
+ return CHECK_REF_NO_PUSH;
/* Check for statuses set by set_ref_status_for_push() */
switch (ref->status) {
@@ -203,10 +206,11 @@ 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:
+ return CHECK_REF_STATUS_REJECTED;
case REF_STATUS_UPTODATE:
- return 0;
+ return CHECK_REF_UPTODATE;
default:
- return 1;
+ return 0;
}
}
@@ -250,7 +254,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 (check_to_send_update(ref, args) < 0)
continue;
update_seen = 1;
strbuf_addf(&cert, "%s %s %s\n",
@@ -369,7 +373,7 @@ 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 (check_to_send_update(ref, args) < 0)
continue;
if (!ref->deletion)
@@ -390,7 +394,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 (check_to_send_update(ref, args) < 0)
continue;
old_hex = sha1_to_hex(ref->old_sha1);
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 3/9] send-pack.c: add --atomic command line argument
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
2014-12-30 2:36 ` [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support Stefan Beller
2014-12-30 2:36 ` [PATCHv8 2/9] send-pack: rename ref_update_to_be_sent to check_to_send_update Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 2:36 ` [PATCHv8 4/9] receive-pack.c: simplify execute_commands Stefan Beller
` (5 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine
Cc: git, Ronnie Sahlberg, 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.
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>
---
Notes:
Notes:
Changes v1 -> v2:
* Now we only need a very small change in the existing code and have
a new static function, which cares about error reporting.
Junio wrote:
> 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?
* error messages are worded differently (lower case!),
* use of error function instead of fprintf
* undashed the printed error message ("atomic push failed");
Changes v2 -> v3:
> We avoid assignment inside a conditional.
Ok I switched to using a switch statement.
skipped v4 v5
v6:
* realign to one error string:
+ error("atomic push failed for ref %s. status: %d\n",
+ failing_ref->name, failing_ref->status);
* Use correct value now (negative defined from previous patch)
v7:
* return error(...); instead of error(...); return -1;
v8:
no changes
Documentation/git-send-pack.txt | 7 ++++++-
builtin/send-pack.c | 6 +++++-
remote.h | 3 ++-
send-pack.c | 39 +++++++++++++++++++++++++++++++++++++--
transport.c | 4 ++++
5 files changed, 54 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 2a0de42..45c7725 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] [<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::
+ 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..b961e5a 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] [<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")) {
+ args.atomic = 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 b7d8e01..e8f60df 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -282,6 +282,29 @@ free_return:
return update_seen;
}
+
+static int atomic_push_failure(struct send_pack_args *args,
+ struct ref *remote_refs,
+ struct ref *failing_ref)
+{
+ struct ref *ref;
+ /* Mark other refs as 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 */
+ }
+ }
+ return error("atomic push failed for ref %s. status: %d\n",
+ failing_ref->name, failing_ref->status);
+}
+
int send_pack(struct send_pack_args *args,
int fd[], struct child_process *conn,
struct ref *remote_refs,
@@ -373,9 +396,21 @@ int send_pack(struct send_pack_args *args,
* the pack data.
*/
for (ref = remote_refs; ref; ref = ref->next) {
- if (check_to_send_update(ref, args) < 0)
+ switch (check_to_send_update(ref, args)) {
+ case 0: /* no error */
+ break;
+ case CHECK_REF_STATUS_REJECTED:
+ /*
+ * When we know the server would reject a ref update if
+ * we were to send it and we're trying to send the refs
+ * atomically, abort the whole operation.
+ */
+ if (use_atomic)
+ return atomic_push_failure(args, remote_refs, ref);
+ /* Fallthrough for non atomic case. */
+ default:
continue;
-
+ }
if (!ref->deletion)
need_pack_data = 1;
diff --git a/transport.c b/transport.c
index 70d38e4..c67feee 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.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
` (2 preceding siblings ...)
2014-12-30 2:36 ` [PATCHv8 3/9] send-pack.c: add --atomic command line argument Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 6:11 ` Eric Sunshine
2014-12-30 7:46 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place Stefan Beller
` (4 subsequent siblings)
8 siblings, 2 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
No functional changes intended.
This commit shortens execute_commands by moving some parts of the code
to extra functions.
Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
v8: no change
v7:
new in v7 as in v7 I'd split up the previous
[PATCH 4/7] receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes
as suggested by Eric.
This is pretty much
> patch 1: Factor out code into helper functions which will be needed by
> the upcoming atomic and non-atomic worker functions. Example helpers:
> 'cmd->error_string' and cmd->skip_update' check; and the
> 'si->shallow_ref[cmd->index]' check and handling.
builtin/receive-pack.c | 49 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4e8eaf7..06eb287 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
}
}
+static int should_process_cmd(struct command *cmd)
+{
+ if (cmd->error_string)
+ return 0;
+ if (cmd->skip_update)
+ return 0;
+ return 1;
+}
+
+static void check_shallow_bugs(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ int checked_connectivity = 1;
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ if (shallow_update && si->shallow_ref[cmd->index]) {
+ error("BUG: connectivity check has not been run on ref %s",
+ cmd->ref_name);
+ checked_connectivity = 0;
+ }
+ }
+ if (shallow_update && !checked_connectivity)
+ error("BUG: run 'git fsck' for safety.\n"
+ "If there are errors, try to remove "
+ "the reported refs above");
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
{
- int checked_connectivity;
struct command *cmd;
unsigned char sha1[20];
struct iterate_data data;
@@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
- checked_connectivity = 1;
for (cmd = commands; cmd; cmd = cmd->next) {
- if (cmd->error_string)
- continue;
-
- if (cmd->skip_update)
+ if (!should_process_cmd(cmd))
continue;
cmd->error_string = update(cmd, si);
- if (shallow_update && !cmd->error_string &&
- si->shallow_ref[cmd->index]) {
- error("BUG: connectivity check has not been run on ref %s",
- cmd->ref_name);
- checked_connectivity = 0;
- }
}
-
- if (shallow_update && !checked_connectivity)
- error("BUG: run 'git fsck' for safety.\n"
- "If there are errors, try to remove "
- "the reported refs above");
+ check_shallow_bugs(commands, si);
}
static struct command **queue_command(struct command **tail,
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
` (3 preceding siblings ...)
2014-12-30 2:36 ` [PATCHv8 4/9] receive-pack.c: simplify execute_commands Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 8:36 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function Stefan Beller
` (3 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
No functional changes intended.
This moves all code related to transactions into the execute_commands_loop
function which was factored out of execute_commands. This includes
beginning and committing the transaction as well as dealing with the
errors which may occur during the begin and commit phase of a transaction.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
v8:
move execute_commands_loop before execute_commands, so it compiles/links
without warnings.
v7:
new in v7, this is part of the previous "[PATCH 4/7]
receive-pack.c: receive-pack.c: use a single ref_transaction for atomic pushes"
This covers the suggestion of patch 2 and 3 by Eric
> patch 2: Factor out the main 'for' loop of execute_commands() into a
> new function. This new function will eventually become
> execute_commands_non_atomic(). At this point, execute_commands() is
> pretty much in its final form with the exception of the upcoming 'if
> (use_atomic)' conditional.
> patch 3: Morph the function extracted in patch 2 into
> execute_commands_non_atomic() by adding transaction handling inside
> the 'for' loop (and applying the changes from the early part of the
> patch which go along with that).
builtin/receive-pack.c | 69 ++++++++++++++++++++++++++++++++++++--------------
1 file changed, 50 insertions(+), 19 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 06eb287..5f44466 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -67,6 +67,7 @@ static const char *NONCE_SLOP = "SLOP";
static const char *nonce_status;
static long nonce_stamp_slop;
static unsigned long nonce_stamp_slop_limit;
+static struct ref_transaction *transaction;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -822,6 +823,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
}
if (is_null_sha1(new_sha1)) {
+ struct strbuf err = STRBUF_INIT;
if (!parse_object(old_sha1)) {
old_sha1 = NULL;
if (ref_exists(name)) {
@@ -831,35 +833,36 @@ static const char *update(struct command *cmd, struct shallow_info *si)
cmd->did_not_exist = 1;
}
}
- if (delete_ref(namespaced_name, old_sha1, 0)) {
- rp_error("failed to delete %s", name);
+ if (ref_transaction_delete(transaction,
+ namespaced_name,
+ old_sha1,
+ 0, old_sha1 != NULL,
+ "push", &err)) {
+ rp_error("%s", err.buf);
+ strbuf_release(&err);
return "failed to delete";
}
+ strbuf_release(&err);
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 "shallow error";
- transaction = ref_transaction_begin(&err);
- if (!transaction ||
- ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, "push",
- &err) ||
- ref_transaction_commit(transaction, &err)) {
- ref_transaction_free(transaction);
-
+ if (ref_transaction_update(transaction,
+ namespaced_name,
+ new_sha1, old_sha1,
+ 0, 1, "push",
+ &err)) {
rp_error("%s", err.buf);
strbuf_release(&err);
+
return "failed to update ref";
}
-
- ref_transaction_free(transaction);
strbuf_release(&err);
+
return NULL; /* good */
}
}
@@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,
"the reported refs above");
}
+static void execute_commands_loop(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ struct strbuf err = STRBUF_INIT;
+
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!should_process_cmd(cmd))
+ continue;
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ rp_error("%s", err.buf);
+ strbuf_reset(&err);
+ cmd->error_string = "transaction failed to start";
+ continue;
+ }
+
+ cmd->error_string = update(cmd, si);
+
+ if (!cmd->error_string
+ && ref_transaction_commit(transaction, &err)) {
+ rp_error("%s", err.buf);
+ strbuf_reset(&err);
+ cmd->error_string = "failed to update ref";
+ }
+ ref_transaction_free(transaction);
+ }
+
+ strbuf_release(&err);
+}
+
static void execute_commands(struct command *commands,
const char *unpacker_error,
struct shallow_info *si)
@@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
- for (cmd = commands; cmd; cmd = cmd->next) {
- if (!should_process_cmd(cmd))
- continue;
+ execute_commands_loop(commands, si);
- cmd->error_string = update(cmd, si);
- }
check_shallow_bugs(commands, si);
}
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
` (4 preceding siblings ...)
2014-12-30 2:36 ` [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 8:57 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 7/9] receive-pack.c: enable atomic push protocol support Stefan Beller
` (2 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
Update receive-pack to use an atomic transaction iff the client negotiated
that it wanted atomic push. This leaves the default behavior 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 behavior so we make it opt-in for now.
If it turns out over time that there are no client scripts that depend on the
old behavior 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.
Inspired-by: Ronnie Sahlberg <sahlberg@google.com>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
Changes in v8:
removed superflous "}" to make it compile again
Changes in v7:
Eric suggested to replace "[PATCH 4/7] receive-pack.c:
receive-pack.c: use a single ref_transaction for atomic pushes"
by smaller patches
This is the last patch replacing said large commit.
Changes in v6:
This is a complete rewrite of the patch essentially.
Eric suggested to split up the flow into functions, so
it is easier to read. It's more lines of code, but indeed
it's easier to follow. Thanks Eric!
Note there is another patch following this one
moving the helper functions above execute_commands.
I just choose the order of the functions in this patch
to have a better diff (just inserting the call to the function
execute_commands_non_atomic and that function directly follows.)
The next patch of the series will move that up.
Because of the rewrite and the fixes of the previous five
versions there is not much left of Ronnies original patch,
so I'll claim authorship of this one.
Changes v1 -> v2:
* update(...) assumes to be always in a transaction
* Caring about when to begin/commit transactions is put
into execute_commands
v2->v3:
* meditated about the error flow. Now we always construct a local
strbuf err if required. Then the flow is easier to follow and
destruction of it is performed nearby.
* early return in execute_commands if transaction_begin fails.
v3->v4:
* revamp logic again. This should keep the non atomic behavior
as is (in case of error say so, in non error case just free the
transaction). In the atomic case we either do nothing (when no error),
or abort with the goto.
if (!cmd->error_string) {
if (!use_atomic
&& ref_transaction_commit(transaction, &err)) {
ref_transaction_free(transaction);
rp_error("%s", err.buf);
strbuf_release(&err);
cmd->error_string = "failed to update ref";
}
} else if (use_atomic) {
goto atomic_failure;
} else {
ref_transaction_free(transaction);
}
* Having the goto directly there when checking for cmd->error_string,
we don't need to do it again, so the paragraph explaining the error
checking is gone as well. (Previous patch had the following, this is
put at the end of the function, where the goto jumps to and the comment
has been dropped.
+ /*
+ * update(...) may abort early (i.e. because the hook refused to
+ * update that ref) which then doesn't even record a transaction
+ * regarding that ref. Make sure all commands are without error
+ * and then commit atomically.
+ */
+ for (cmd = commands; cmd; cmd = cmd->next)
+ if (cmd->error_string)
+ break;
v4->v5:
Eric wrote:
> Repeating from my earlier review[1]: If the 'pre-receive' hook
> "declines", then this transaction is left dangling (and its resources
> leaked).
You're right. The initialization of the transaction is now
near the actual loop after the pre receive hook.
> The !use_atomic case (below), calls this error "failed to start
> transaction", not merely "transaction error".
ok, now both are "transaction failed to start".
In all cases where these generic errors are reported,
we do have a rp_error(...) with details.
> Furthermore, in the use_atomic case (also below), when a commit fails,
> you assign err.buf to cmd->error_string rather than a generic
> "transaction error" message. What differs between these cases which
> makes the generic message preferable here over the more specific
> err.buf message?
They are the same now.
> Repeating from my earlier review[1]: This is leaking 'transaction' for
> each successful commit (and only freeing it upon commit error).
Right. I thought I had it covered with the else clause. Of course not.
> At the end of this function, strbuf_release(&err) is invoked, which
> leaves all these cmd->error_strings dangling.
I removed all assignments of err.buf now.
> goto's can help simplify error-handling when multiple conditional
> branches need to perform common cleanup, however, this label
> corresponds to only a single goto statement.
moved up again.
builtin/receive-pack.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 49 insertions(+), 3 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5f44466..35a2264 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1076,8 +1076,8 @@ static void check_shallow_bugs(struct command *commands,
"the reported refs above");
}
-static void execute_commands_loop(struct command *commands,
- struct shallow_info *si)
+static void execute_commands_non_atomic(struct command *commands,
+ struct shallow_info *si)
{
struct command *cmd;
struct strbuf err = STRBUF_INIT;
@@ -1104,7 +1104,50 @@ static void execute_commands_loop(struct command *commands,
}
ref_transaction_free(transaction);
}
+ strbuf_release(&err);
+}
+
+static void execute_commands_atomic(struct command *commands,
+ struct shallow_info *si)
+{
+ struct command *cmd;
+ struct strbuf err = STRBUF_INIT;
+ const char *reported_error = "atomic push failure";
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ rp_error("%s", err.buf);
+ strbuf_reset(&err);
+ reported_error = "transaction failed to start";
+ goto failure;
+ }
+
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!should_process_cmd(cmd))
+ continue;
+ cmd->error_string = update(cmd, si);
+
+ if (cmd->error_string)
+ goto failure;
+ }
+
+ if (ref_transaction_commit(transaction, &err)) {
+ rp_error("%s", err.buf);
+ reported_error = "atomic transaction failed";
+ goto failure;
+ }
+
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
+ return;
+
+failure:
+ for (cmd = commands; cmd; cmd = cmd->next)
+ if (!cmd->error_string)
+ cmd->error_string = reported_error;
+
+ ref_transaction_free(transaction);
strbuf_release(&err);
}
@@ -1142,7 +1185,10 @@ static void execute_commands(struct command *commands,
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
- execute_commands_loop(commands, si);
+ if (use_atomic)
+ execute_commands_atomic(commands, si);
+ else
+ execute_commands_non_atomic(commands, si);
check_shallow_bugs(commands, si);
}
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 7/9] receive-pack.c: enable atomic push protocol support
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
` (5 preceding siblings ...)
2014-12-30 2:36 ` [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 2:36 ` [PATCHv8 8/9] push.c: add an --atomic argument Stefan Beller
2014-12-30 2:36 ` [PATCHv8 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes Stefan Beller
8 siblings, 0 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
This enables the atomic protocol option as implemented in the previous
patches.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
v8:
no changes
v7:
* new with v7 of the patch series.
* this was part of the first patch in the series, moved back here
for bisectability
builtin/receive-pack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 35a2264..61eba4e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -173,7 +173,8 @@ static void show_ref(const char *path, const unsigned char *sha1)
struct strbuf cap = STRBUF_INIT;
strbuf_addstr(&cap,
- "report-status delete-refs side-band-64k quiet");
+ "report-status delete-refs side-band-64k quiet "
+ "atomic");
if (prefer_ofs_delta)
strbuf_addstr(&cap, " ofs-delta");
if (push_cert_nonce)
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 8/9] push.c: add an --atomic argument
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
` (6 preceding siblings ...)
2014-12-30 2:36 ` [PATCHv8 7/9] receive-pack.c: enable atomic push protocol support Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
2014-12-30 2:36 ` [PATCHv8 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes Stefan Beller
8 siblings, 0 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine
Cc: git, Ronnie Sahlberg, Stefan Beller
From: Ronnie Sahlberg <sahlberg@google.com>
Add a command line argument to the git push command to request atomic
pushes.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
v8:
no changes
v7:
Use OPT_BOOL instead of OPT_BIT.
This allows for --no-atomic option on the command line.
v6:
- OPT_BIT(0, "atomic", &flags, N_("use an atomic transaction remote"),
+ OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on remote side"),
skipped v4 v5
v2->v3:
* s/atomic-push/atomic/
* s/the an/an/
* no serverside, but just remote instead
* TRANSPORT_PUSH_ATOMIC instead of TRANSPORT_ATOMIC_PUSH
Changes v1 -> v2
It's --atomic now! (dropping the -push)
Documentation/git-push.txt | 7 ++++++-
builtin/push.c | 5 +++++
transport.c | 1 +
transport.h | 1 +
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21b3f29..4764fcf 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] [-n | --dry-run] [--receive-pack=<git-receive-pack>]
[--repo=<repository>] [-f | --force] [--prune] [-v | --verbose]
[-u | --set-upstream] [--signed]
[--force-with-lease[=<refname>[:<expect>]]]
@@ -136,6 +136,11 @@ already exists on the remote side.
logged. See linkgit:git-receive-pack[1] for the details
on the receiving end.
+--[no-]atomic::
+ Use an atomic transaction on the remote side if available.
+ Either all refs are updated, or on error, no refs are updated.
+ If the server does not support atomic pushes the push will fail.
+
--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 a076b19..8f1d945 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -487,6 +487,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
int flags = 0;
int tags = 0;
int rc;
+ int atomic = 0;
const char *repo = NULL; /* default repository */
struct option options[] = {
OPT__VERBOSITY(&verbosity),
@@ -518,6 +519,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant tags"),
TRANSPORT_PUSH_FOLLOW_TAGS),
OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), TRANSPORT_PUSH_CERT),
+ OPT_BOOL(0, "atomic", &atomic, N_("request atomic transaction on remote side")),
OPT_END()
};
@@ -533,6 +535,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
if (tags)
add_refspec("refs/tags/*");
+ if (atomic)
+ flags |= TRANSPORT_PUSH_ATOMIC;
+
if (argc > 0) {
repo = argv[0];
set_refspecs(argv + 1, argc - 1, repo);
diff --git a/transport.c b/transport.c
index c67feee..1373152 100644
--- a/transport.c
+++ b/transport.c
@@ -830,6 +830,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re
args.dry_run = !!(flags & TRANSPORT_PUSH_DRY_RUN);
args.porcelain = !!(flags & TRANSPORT_PUSH_PORCELAIN);
args.push_cert = !!(flags & TRANSPORT_PUSH_CERT);
+ args.atomic = !!(flags & TRANSPORT_PUSH_ATOMIC);
args.url = transport->url;
ret = send_pack(&args, data->fd, data->conn, remote_refs,
diff --git a/transport.h b/transport.h
index 3e0091e..18d2cf8 100644
--- a/transport.h
+++ b/transport.h
@@ -125,6 +125,7 @@ struct transport {
#define TRANSPORT_PUSH_NO_HOOK 512
#define TRANSPORT_PUSH_FOLLOW_TAGS 1024
#define TRANSPORT_PUSH_CERT 2048
+#define TRANSPORT_PUSH_ATOMIC 4096
#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCHv8 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
` (7 preceding siblings ...)
2014-12-30 2:36 ` [PATCHv8 8/9] push.c: add an --atomic argument Stefan Beller
@ 2014-12-30 2:36 ` Stefan Beller
8 siblings, 0 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 2:36 UTC (permalink / raw)
To: ronniesahlberg, mhagger, jrnieder, gitster, sunshine; +Cc: git, Stefan Beller
This adds tests for the atomic push option.
The first four tests check if the atomic option works in
good conditions and the last three patches check if the atomic
option prevents any change to be pushed if just one ref cannot
be updated.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Notes:
v7, v8: no changes
v6:
the same as v2, so I can resend the whole series as v6
v3 v4 v5 were skipped
Changes v1 -> v2:
> Please drop unused comments; they are distracting.
ok
> It is not wrong per-se, but haven't you already tested in
> combination with --mirror in the previous test?
I fixed the previous tests, so that there is no --mirror
and --atomic together. There is still a first --mirror push
for setup and a second with --atomic <branchnames> though
> check_branches upstream master HEAD@{2} second HEAD~
A similar function test_ref_upstream is introduced.
> What's the value of this test? Isn't it a non-fast-forward check
> you already tested in the previous one?
I messed up there. Originally I wanted to test the 2 different
stages of rejection. A non-fast-forward check is done locally and
we don't even try pushing. But I also want to test if we locally
thing all is good, but the server refuses a ref to update.
This is now done with the last test named 'atomic push obeys
update hook preventing a branch to be pushed'. And that still fails.
I'll investigate that, while still sending out the series for another
review though.
* Redone the test helper, there is test_ref_upstream now.
This tests explicitely for SHA1 values of the ref.
(It's needed in the last test for example. The git push fails,
but still modifies the ref :/ )
* checked all && chains and repaired them
* sometimes make use of git -C <workdir>
Notes v1:
Originally Ronnie had a similar patch prepared. But as I added
more tests and cleaned up the existing tests (e.g. use test_commit
instead of "echo one >file && gitadd file && git commit -a -m 'one'",
removal of dead code), the file has changed so much that I'd rather
take ownership.
t/t5543-atomic-push.sh | 178 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 178 insertions(+)
create mode 100755 t/t5543-atomic-push.sh
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
new file mode 100755
index 0000000..b81a542
--- /dev/null
+++ b/t/t5543-atomic-push.sh
@@ -0,0 +1,178 @@
+#!/bin/sh
+
+test_description='pushing to a repository using the atomic push option'
+
+. ./test-lib.sh
+
+mk_repo_pair () {
+ rm -rf workbench upstream &&
+ test_create_repo upstream &&
+ test_create_repo workbench &&
+ (
+ cd upstream &&
+ git config receive.denyCurrentBranch warn
+ ) &&
+ (
+ cd workbench &&
+ git remote add up ../upstream
+ )
+}
+
+# Compare the ref ($1) in upstream with a ref value from workbench ($2)
+# i.e. test_refs second HEAD@{2}
+test_refs () {
+ test $# = 2 &&
+ git -C upstream rev-parse --verify "$1" >expect &&
+ git -C workbench rev-parse --verify "$2" >actual &&
+ test_cmp expect actual
+}
+
+test_expect_success 'atomic push works for a single branch' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git push --mirror up &&
+ test_commit two &&
+ git push --atomic up master
+ ) &&
+ test_refs master master
+'
+
+test_expect_success 'atomic push works for two branches' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git branch second &&
+ git push --mirror up &&
+ test_commit two &&
+ git checkout second &&
+ test_commit three &&
+ git push --atomic up master second
+ ) &&
+ test_refs master master &&
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --mirror' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git checkout -b second &&
+ test_commit two &&
+ git push --atomic --mirror up
+ ) &&
+ test_refs master master &&
+ test_refs second second
+'
+
+test_expect_success 'atomic push works in combination with --force' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git branch second master &&
+ test_commit two_a &&
+ git checkout second &&
+ test_commit two_b &&
+ test_commit three_b &&
+ test_commit four &&
+ git push --mirror up &&
+ # The actual test is below
+ git checkout master &&
+ test_commit three_a &&
+ git checkout second &&
+ git reset --hard HEAD^ &&
+ git push --force --atomic up master second
+ ) &&
+ test_refs master master &&
+ test_refs second second
+'
+
+# set up two branches where master can be pushed but second can not
+# (non-fast-forward). Since second can not be pushed the whole operation
+# will fail and leave master untouched.
+test_expect_success 'atomic push fails if one branch fails' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git checkout -b second master &&
+ test_commit two &&
+ test_commit three &&
+ test_commit four &&
+ git push --mirror up &&
+ git reset --hard HEAD~2 &&
+ test_commit five &&
+ git checkout master &&
+ test_commit six &&
+ test_must_fail git push --atomic --all up
+ ) &&
+ test_refs master HEAD@{7} &&
+ test_refs second HEAD@{4}
+'
+
+test_expect_success 'atomic push fails if one tag fails remotely' '
+ # prepare the repo
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git checkout -b second master &&
+ test_commit two &&
+ git push --mirror up
+ ) &&
+ # a third party modifies the server side:
+ (
+ cd upstream &&
+ git checkout second &&
+ git tag test_tag second
+ ) &&
+ # see if we can now push both branches.
+ (
+ cd workbench &&
+ git checkout master &&
+ test_commit three &&
+ git checkout second &&
+ test_commit four &&
+ git tag test_tag &&
+ test_must_fail git push --tags --atomic up master second
+ ) &&
+ test_refs master HEAD@{3} &&
+ test_refs second HEAD@{1}
+'
+
+test_expect_success 'atomic push obeys update hook preventing a branch to be pushed' '
+ mk_repo_pair &&
+ (
+ cd workbench &&
+ test_commit one &&
+ git checkout -b second master &&
+ test_commit two &&
+ git push --mirror up
+ ) &&
+ (
+ cd upstream &&
+ HOOKDIR="$(git rev-parse --git-dir)/hooks" &&
+ HOOK="$HOOKDIR/update" &&
+ mkdir -p "$HOOKDIR" &&
+ write_script "$HOOK" <<-\EOF
+ # only allow update to master from now on
+ test "$1" = "refs/heads/master"
+ EOF
+ ) &&
+ (
+ cd workbench &&
+ git checkout master &&
+ test_commit three &&
+ git checkout second &&
+ test_commit four &&
+ test_must_fail git push --atomic up master second
+ ) &&
+ test_refs master HEAD@{3} &&
+ test_refs second HEAD@{1}
+'
+
+test_done
--
2.2.1.62.g3f15098
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 2:36 ` [PATCHv8 4/9] receive-pack.c: simplify execute_commands Stefan Beller
@ 2014-12-30 6:11 ` Eric Sunshine
2014-12-30 8:41 ` Stefan Beller
2014-12-30 20:33 ` Stefan Beller
2014-12-30 7:46 ` Eric Sunshine
1 sibling, 2 replies; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 6:11 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
> No functional changes intended.
This is useful to know but is of secondary importance, thus should be
placed after the explanation and justification of the change.
> Subject: receive-pack.c: simplify execute_commands
> This commit shortens execute_commands by moving some parts of the code
> to extra functions.
The _real_ reason for moving these bits of code into their own
functions is that you intend to introduce additional callers in
subsequent patches. That is what the commit message (including
subject) should be conveying to the reader.
The claimed simplification is questionable and not of particular
importance; and it's misleading to paint it as a goal of the patch.
Consequently, you could drop mention of it altogether.
More below.
> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 4e8eaf7..06eb287 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
> }
> }
>
> +static int should_process_cmd(struct command *cmd)
> +{
> + if (cmd->error_string)
> + return 0;
> + if (cmd->skip_update)
> + return 0;
> + return 1;
Alternately, depending upon the polarity of your brain, you could
collapse the entire function body to:
return !cmd->error_string && !cmd->skip_update;
or:
return !(cmd->error_string || cmd->skip_update);
> +}
> +
> +static void check_shallow_bugs(struct command *commands,
> + struct shallow_info *si)
> +{
> + struct command *cmd;
> + int checked_connectivity = 1;
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!should_process_cmd(cmd))
> + continue;
> +
> + if (shallow_update && si->shallow_ref[cmd->index]) {
Here, inside the loop, you check 'shallow_update'...
> + error("BUG: connectivity check has not been run on ref %s",
> + cmd->ref_name);
> + checked_connectivity = 0;
> + }
> + }
> + if (shallow_update && !checked_connectivity)
And here, after the loop, you check 'shallow_update'.
But, if you examine the overall logic, you will find that this
function does _nothing_ at all when 'shallow_update' is false (other
than uselessly looping through 'commands'). Therefore, either check
'shallow_update' just once at the beginning of the function and exit
early if false, or have the caller check 'shallow_update' and only
invoke check_shallow_bugs() if true.
Also, since nothing happens between them, the two conditionals inside
the loop can be coalesced:
if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
> + error("BUG: run 'git fsck' for safety.\n"
> + "If there are errors, try to remove "
> + "the reported refs above");
In v6, you considered this a fatal error in the atomic case, which
caused the entire transaction to be rolled back. However, in this
version, this error has no effect whatsoever on the atomic
transaction, which is a rather significant behavioral departure. Which
is correct? (This is a genuine question; not at all rhetorical.) If
failing the entire transaction is the correct thing to do, then this
is going to need some more work.
> +}
> +
> static void execute_commands(struct command *commands,
> const char *unpacker_error,
> struct shallow_info *si)
> {
> - int checked_connectivity;
> struct command *cmd;
> unsigned char sha1[20];
> struct iterate_data data;
> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>
> - checked_connectivity = 1;
> for (cmd = commands; cmd; cmd = cmd->next) {
> - if (cmd->error_string)
> - continue;
> -
> - if (cmd->skip_update)
> + if (!should_process_cmd(cmd))
> continue;
>
> cmd->error_string = update(cmd, si);
> - if (shallow_update && !cmd->error_string &&
> - si->shallow_ref[cmd->index]) {
> - error("BUG: connectivity check has not been run on ref %s",
> - cmd->ref_name);
> - checked_connectivity = 0;
> - }
> }
> -
> - if (shallow_update && !checked_connectivity)
> - error("BUG: run 'git fsck' for safety.\n"
> - "If there are errors, try to remove "
> - "the reported refs above");
> + check_shallow_bugs(commands, si);
> }
>
> static struct command **queue_command(struct command **tail,
> --
> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support
2014-12-30 2:36 ` [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support Stefan Beller
@ 2014-12-30 7:08 ` Eric Sunshine
2014-12-30 8:33 ` Stefan Beller
0 siblings, 1 reply; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 7:08 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List, Ronnie Sahlberg
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
> Subject: receive-pack.c: add documentation for atomic push support
This patch is doing a lot more than merely adding documentation. It's
also updating send-pack and receive-pack to be able to negotiate the
new protocol capability "atomic". The fact that you removed the actual
advertisement of "atomic" from this patch doesn't turn it into a
documentation-only patch.
When Michael raised the issue of the server being "broken" due to
advertising a capability which it does not yet implement, his
recommendation[1] was to reorder the patches, not to split out the one
tiny bit (capability advertisement) from the larger change. Was there
an insurmountable conflict which prevented you from reordering the
patches? This is a genuine question since splitting off advertisement
-- and only advertisement -- to a patch later in the series smells
like a least-resistance approach, rather than a proper solution to the
issue Michael raised.
[1]: http://article.gmane.org/gmane.comp.version-control.git/261793
> This documents the protocol option between send-pack and receive-pack to
> * allow receive-pack to inform the client that it has atomic push capability
> * allow send-pack to request atomic push back.
>
> There is currently no setting in send-pack to actually request that atomic
> pushes are to be used yet. This only adds protocol capability not ability
> for the user to activate it. The capability is also not yet advertised
> by receive-pack as git doesn't know how to handle it yet.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index 6d5424c..4f8a7bf 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client requested
> and server advertised. As a consequence of these rules, server MUST
> NOT advertise capabilities it does not understand.
>
> -The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
> -are sent and recognized by the receive-pack (push to server) process.
> +The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
> +capabilities are sent and recognized by the receive-pack (push to server)
> +process.
>
> The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
> by both upload-pack and receive-pack protocols. The 'agent' capability
> @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress
> reporting if the local progress reporting is also being suppressed
> (e.g., via `push -q`, or if stderr does not go to a tty).
>
> +atomic
> +------
> +
> +If the server sends the 'atomic' capability it is capable of accepting
> +atomic pushes. If the pushing client requests this capability, the server
> +will update the refs in one atomic transaction. Either all refs are
> +updated or none.
> +
> allow-tip-sha1-in-want
> ----------------------
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 32fc540..4e8eaf7 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
> static int unpack_limit = 100;
> static int report_status;
> static int use_sideband;
> +static int use_atomic;
> static int quiet;
> static int prefer_ofs_delta = 1;
> static int auto_update_server_info;
> @@ -1179,6 +1180,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"))
> + use_atomic = 1;
> }
>
> if (!strcmp(line, "push-cert")) {
> diff --git a/send-pack.c b/send-pack.c
> index 949cb61..6d0c159 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
> int use_sideband = 0;
> int quiet_supported = 0;
> int agent_supported = 0;
> + int use_atomic = 0;
> + int atomic_supported = 0;
> unsigned cmds_sent = 0;
> int ret;
> struct async demux;
> @@ -314,6 +316,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"))
> + atomic_supported = 1;
> if (args->push_cert) {
> int len;
>
> @@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,
> "Perhaps you should specify a branch such as 'master'.\n");
> return 0;
> }
> + if (args->atomic && !atomic_supported)
> + die(_("server does not support --atomic push"));
> +
> + use_atomic = atomic_supported && args->atomic;
>
> if (status_report)
> strbuf_addstr(&cap_buf, " report-status");
> @@ -335,6 +343,8 @@ int send_pack(struct send_pack_args *args,
> strbuf_addstr(&cap_buf, " side-band-64k");
> if (quiet_supported && (args->quiet || !args->progress))
> strbuf_addstr(&cap_buf, " quiet");
> + if (use_atomic)
> + strbuf_addstr(&cap_buf, " atomic");
> if (agent_supported)
> strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
>
> diff --git a/send-pack.h b/send-pack.h
> index 5635457..b664648 100644
> --- a/send-pack.h
> +++ b/send-pack.h
> @@ -13,7 +13,8 @@ struct send_pack_args {
> use_ofs_delta:1,
> dry_run:1,
> push_cert:1,
> - stateless_rpc:1;
> + stateless_rpc:1,
> + atomic:1;
> };
>
> int send_pack(struct send_pack_args *args,
> --
> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 2:36 ` [PATCHv8 4/9] receive-pack.c: simplify execute_commands Stefan Beller
2014-12-30 6:11 ` Eric Sunshine
@ 2014-12-30 7:46 ` Eric Sunshine
2014-12-30 8:42 ` Stefan Beller
1 sibling, 1 reply; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 7:46 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
> No functional changes intended.
>
> This commit shortens execute_commands by moving some parts of the code
> to extra functions.
>
> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 4e8eaf7..06eb287 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
> }
> }
>
> +static void check_shallow_bugs(struct command *commands,
> + struct shallow_info *si)
> +{
> + struct command *cmd;
> + int checked_connectivity = 1;
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!should_process_cmd(cmd))
> + continue;
> +
> + if (shallow_update && si->shallow_ref[cmd->index]) {
Another issue: In the original code, 'si->shallow_ref[cmd->index]' is
only checked if cmd->error_string is NULL, but here you check it
unconditionally, despite the commit message claiming no functional
changes. Did you verify that such a behavioral change is benign? (This
is a genuine question.)
> + error("BUG: connectivity check has not been run on ref %s",
> + cmd->ref_name);
> + checked_connectivity = 0;
> + }
> + }
> + if (shallow_update && !checked_connectivity)
> + error("BUG: run 'git fsck' for safety.\n"
> + "If there are errors, try to remove "
> + "the reported refs above");
> +}
> +
> static void execute_commands(struct command *commands,
> const char *unpacker_error,
> struct shallow_info *si)
> {
> - int checked_connectivity;
> struct command *cmd;
> unsigned char sha1[20];
> struct iterate_data data;
> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>
> - checked_connectivity = 1;
> for (cmd = commands; cmd; cmd = cmd->next) {
> - if (cmd->error_string)
> - continue;
> -
> - if (cmd->skip_update)
> + if (!should_process_cmd(cmd))
> continue;
>
> cmd->error_string = update(cmd, si);
> - if (shallow_update && !cmd->error_string &&
> - si->shallow_ref[cmd->index]) {
> - error("BUG: connectivity check has not been run on ref %s",
> - cmd->ref_name);
> - checked_connectivity = 0;
> - }
> }
> -
> - if (shallow_update && !checked_connectivity)
> - error("BUG: run 'git fsck' for safety.\n"
> - "If there are errors, try to remove "
> - "the reported refs above");
> + check_shallow_bugs(commands, si);
> }
>
> static struct command **queue_command(struct command **tail,
> --
> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support
2014-12-30 7:08 ` Eric Sunshine
@ 2014-12-30 8:33 ` Stefan Beller
2014-12-30 9:09 ` Eric Sunshine
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 8:33 UTC (permalink / raw)
To: Eric Sunshine
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List, Ronnie Sahlberg
On Mon, Dec 29, 2014 at 11:08 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>> Subject: receive-pack.c: add documentation for atomic push support
>
> This patch is doing a lot more than merely adding documentation. It's
> also updating send-pack and receive-pack to be able to negotiate the
> new protocol capability "atomic". The fact that you removed the actual
> advertisement of "atomic" from this patch doesn't turn it into a
> documentation-only patch.
That's true, though the code itself is dead code and doing nothing in
that patch.
>
> When Michael raised the issue of the server being "broken" due to
> advertising a capability which it does not yet implement, his
> recommendation[1] was to reorder the patches, not to split out the one
> tiny bit (capability advertisement) from the larger change. Was there
> an insurmountable conflict which prevented you from reordering the
> patches? This is a genuine question since splitting off advertisement
> -- and only advertisement -- to a patch later in the series smells
> like a least-resistance approach, rather than a proper solution to the
> issue Michael raised.
Well there was no syntactical problem (i.e. the interactive rebase
went flawless),
but rather a semantic dependency. The reordered patches would not compile
as we'd heavily depend on the use_atomic variable.
Of course that could have been introduced where required, but at the
time it did
not look appealing to me.
I'll reword the commit message header to mention the negotiation part as well.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261793
>
>> This documents the protocol option between send-pack and receive-pack to
>> * allow receive-pack to inform the client that it has atomic push capability
>> * allow send-pack to request atomic push back.
>>
>> There is currently no setting in send-pack to actually request that atomic
>> pushes are to be used yet. This only adds protocol capability not ability
>> for the user to activate it. The capability is also not yet advertised
>> by receive-pack as git doesn't know how to handle it yet.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
>> index 6d5424c..4f8a7bf 100644
>> --- a/Documentation/technical/protocol-capabilities.txt
>> +++ b/Documentation/technical/protocol-capabilities.txt
>> @@ -18,8 +18,9 @@ was sent. Server MUST NOT ignore capabilities that client requested
>> and server advertised. As a consequence of these rules, server MUST
>> NOT advertise capabilities it does not understand.
>>
>> -The 'report-status', 'delete-refs', 'quiet', and 'push-cert' capabilities
>> -are sent and recognized by the receive-pack (push to server) process.
>> +The 'atomic', 'report-status', 'delete-refs', 'quiet', and 'push-cert'
>> +capabilities are sent and recognized by the receive-pack (push to server)
>> +process.
>>
>> The 'ofs-delta' and 'side-band-64k' capabilities are sent and recognized
>> by both upload-pack and receive-pack protocols. The 'agent' capability
>> @@ -244,6 +245,14 @@ respond with the 'quiet' capability to suppress server-side progress
>> reporting if the local progress reporting is also being suppressed
>> (e.g., via `push -q`, or if stderr does not go to a tty).
>>
>> +atomic
>> +------
>> +
>> +If the server sends the 'atomic' capability it is capable of accepting
>> +atomic pushes. If the pushing client requests this capability, the server
>> +will update the refs in one atomic transaction. Either all refs are
>> +updated or none.
>> +
>> allow-tip-sha1-in-want
>> ----------------------
>>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 32fc540..4e8eaf7 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -40,6 +40,7 @@ static int transfer_unpack_limit = -1;
>> static int unpack_limit = 100;
>> static int report_status;
>> static int use_sideband;
>> +static int use_atomic;
>> static int quiet;
>> static int prefer_ofs_delta = 1;
>> static int auto_update_server_info;
>> @@ -1179,6 +1180,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"))
>> + use_atomic = 1;
>> }
>>
>> if (!strcmp(line, "push-cert")) {
>> diff --git a/send-pack.c b/send-pack.c
>> index 949cb61..6d0c159 100644
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -294,6 +294,8 @@ int send_pack(struct send_pack_args *args,
>> int use_sideband = 0;
>> int quiet_supported = 0;
>> int agent_supported = 0;
>> + int use_atomic = 0;
>> + int atomic_supported = 0;
>> unsigned cmds_sent = 0;
>> int ret;
>> struct async demux;
>> @@ -314,6 +316,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"))
>> + atomic_supported = 1;
>> if (args->push_cert) {
>> int len;
>>
>> @@ -328,6 +332,10 @@ int send_pack(struct send_pack_args *args,
>> "Perhaps you should specify a branch such as 'master'.\n");
>> return 0;
>> }
>> + if (args->atomic && !atomic_supported)
>> + die(_("server does not support --atomic push"));
>> +
>> + use_atomic = atomic_supported && args->atomic;
>>
>> if (status_report)
>> strbuf_addstr(&cap_buf, " report-status");
>> @@ -335,6 +343,8 @@ int send_pack(struct send_pack_args *args,
>> strbuf_addstr(&cap_buf, " side-band-64k");
>> if (quiet_supported && (args->quiet || !args->progress))
>> strbuf_addstr(&cap_buf, " quiet");
>> + if (use_atomic)
>> + strbuf_addstr(&cap_buf, " atomic");
>> if (agent_supported)
>> strbuf_addf(&cap_buf, " agent=%s", git_user_agent_sanitized());
>>
>> diff --git a/send-pack.h b/send-pack.h
>> index 5635457..b664648 100644
>> --- a/send-pack.h
>> +++ b/send-pack.h
>> @@ -13,7 +13,8 @@ struct send_pack_args {
>> use_ofs_delta:1,
>> dry_run:1,
>> push_cert:1,
>> - stateless_rpc:1;
>> + stateless_rpc:1,
>> + atomic:1;
>> };
>>
>> int send_pack(struct send_pack_args *args,
>> --
>> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place
2014-12-30 2:36 ` [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place Stefan Beller
@ 2014-12-30 8:36 ` Eric Sunshine
2014-12-30 18:45 ` Stefan Beller
0 siblings, 1 reply; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 8:36 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
> Subject: receive-pack.c: move transaction handling in a central place
This is very generic, and doesn't really explain what this patch is
about. (See below.)
> No functional changes intended.
Secondary information can be demoted to the end of the commit message.
> This moves all code related to transactions into the execute_commands_loop
> function which was factored out of execute_commands. This includes
> beginning and committing the transaction as well as dealing with the
> errors which may occur during the begin and commit phase of a transaction.
This explains what you're doing, but not why. The purpose of this
change is that a subsequent patch will be adding another mode of
operation ("atomic") to execute_commands() which differs from the
existing mode ("non-atomic") implemented by its main loop. In its
high-level role, execute_commands() does not need to know or care
about the low-level details of each mode of operation. Therefore, as
preparation for introducing a new mode, you're factoring out the
existing mode into its own stand-alone function.
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Notes:
> This covers the suggestion of patch 2 and 3 by Eric
> > patch 2: Factor out the main 'for' loop of execute_commands() into a
> > new function. This new function will eventually become
> > execute_commands_non_atomic(). At this point, execute_commands() is
> > pretty much in its final form with the exception of the upcoming 'if
> > (use_atomic)' conditional.
> > patch 3: Morph the function extracted in patch 2 into
> > execute_commands_non_atomic() by adding transaction handling inside
> > the 'for' loop (and applying the changes from the early part of the
> > patch which go along with that).
This patch is still rather heavyweight. My suggestion[1] for making
these particular changes across two patches was quite deliberate. The
problem with combining them into a single patch is that you're
performing both code movement and functional changes at the same time.
On its own, pure code movement is easy to review.
On its own, code changes are as easy or difficult to review as the
changes themselves.
When combined, however, the review effort is greater than the sum of
the efforts of reviewing them separately. Partly this is because the
combined changes have a noisier diff. If you move the code in one
patch, and then change it in a second one, the changes pop out --
they're quite obvious. On the other hand, when they are combined, the
reviewer has to deliberately and painstakingly search out the changes,
which is difficult and error-prone. Combining movement and code
changes into a single patch also places greater cognitive load on the
reviewer due to the necessity of keeping a more complex mental
scoreboard relating to the different types of changes.
More below.
[1]: http://article.gmane.org/gmane.comp.version-control.git/261706
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 06eb287..5f44466 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,
> "the reported refs above");
> }
>
> +static void execute_commands_loop(struct command *commands,
> + struct shallow_info *si)
Style: Indent the wrapped line to align with the text following the
'(' in the first line.
It's safe to say that the code which you extracted from
execute_commands() handled the non-atomic case, and it's safe to say
that this new function implements the non-atomic case. Therefore, it
would be truthful to call this function execute_commands_nonatomic().
No need to invent the name execute_commands_loop().
> +{
> + struct command *cmd;
> + struct strbuf err = STRBUF_INIT;
> +
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!should_process_cmd(cmd))
> + continue;
> +
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + rp_error("%s", err.buf);
> + strbuf_reset(&err);
> + cmd->error_string = "transaction failed to start";
> + continue;
> + }
> +
> + cmd->error_string = update(cmd, si);
> +
> + if (!cmd->error_string
> + && ref_transaction_commit(transaction, &err)) {
> + rp_error("%s", err.buf);
> + strbuf_reset(&err);
> + cmd->error_string = "failed to update ref";
> + }
> + ref_transaction_free(transaction);
> + }
> +
> + strbuf_release(&err);
> +}
> +
> static void execute_commands(struct command *commands,
> const char *unpacker_error,
> struct shallow_info *si)
> @@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>
> - for (cmd = commands; cmd; cmd = cmd->next) {
> - if (!should_process_cmd(cmd))
> - continue;
> + execute_commands_loop(commands, si);
>
> - cmd->error_string = update(cmd, si);
> - }
> check_shallow_bugs(commands, si);
> }
>
> --
> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 6:11 ` Eric Sunshine
@ 2014-12-30 8:41 ` Stefan Beller
2014-12-30 20:33 ` Stefan Beller
1 sibling, 0 replies; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 8:41 UTC (permalink / raw)
To: Eric Sunshine
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
reply to all instead of reply to Eric only.
On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> In v6, you considered this a fatal error in the atomic case, which
> caused the entire transaction to be rolled back. However, in this
> version, this error has no effect whatsoever on the atomic
> transaction, which is a rather significant behavioral departure. Which
> is correct? (This is a genuine question; not at all rhetorical.) If
> failing the entire transaction is the correct thing to do, then this
> is going to need some more work.
I don't know. in v6 I thought *any* error would stop and abort the atomic
commit. An atomic commit is either completely free of failures or it
doesn't work out.
However that warning doesn't seem to have any effect as of now
(apart from warning the user obviously), and is about the (highly
unlikely?) case of
a bug in git (or a provided helper script/hook?), I thought we can go
with it as well.
The transaction wen't through so all is fine. The thing the warning is
about is about
reachability checks for the shallow case, so I don't see why that
should fail the transaction.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 7:46 ` Eric Sunshine
@ 2014-12-30 8:42 ` Stefan Beller
2014-12-30 9:10 ` Eric Sunshine
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 8:42 UTC (permalink / raw)
To: Eric Sunshine
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Mon, Dec 29, 2014 at 11:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>> No functional changes intended.
>>
>> This commit shortens execute_commands by moving some parts of the code
>> to extra functions.
>>
>> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 4e8eaf7..06eb287 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
>> }
>> }
>>
>> +static void check_shallow_bugs(struct command *commands,
>> + struct shallow_info *si)
>> +{
>> + struct command *cmd;
>> + int checked_connectivity = 1;
>> + for (cmd = commands; cmd; cmd = cmd->next) {
>> + if (!should_process_cmd(cmd))
>> + continue;
>> +
>> + if (shallow_update && si->shallow_ref[cmd->index]) {
>
> Another issue: In the original code, 'si->shallow_ref[cmd->index]' is
> only checked if cmd->error_string is NULL, but here you check it
> unconditionally, despite the commit message claiming no functional
> changes. Did you verify that such a behavioral change is benign? (This
> is a genuine question.)
The error != NULL check is done in if (!should_process_cmd(cmd))
>
>> + error("BUG: connectivity check has not been run on ref %s",
>> + cmd->ref_name);
>> + checked_connectivity = 0;
>> + }
>> + }
>> + if (shallow_update && !checked_connectivity)
>> + error("BUG: run 'git fsck' for safety.\n"
>> + "If there are errors, try to remove "
>> + "the reported refs above");
>> +}
>> +
>> static void execute_commands(struct command *commands,
>> const char *unpacker_error,
>> struct shallow_info *si)
>> {
>> - int checked_connectivity;
>> struct command *cmd;
>> unsigned char sha1[20];
>> struct iterate_data data;
>> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
>> free(head_name_to_free);
>> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>>
>> - checked_connectivity = 1;
>> for (cmd = commands; cmd; cmd = cmd->next) {
>> - if (cmd->error_string)
>> - continue;
>> -
>> - if (cmd->skip_update)
>> + if (!should_process_cmd(cmd))
>> continue;
>>
>> cmd->error_string = update(cmd, si);
>> - if (shallow_update && !cmd->error_string &&
>> - si->shallow_ref[cmd->index]) {
>> - error("BUG: connectivity check has not been run on ref %s",
>> - cmd->ref_name);
>> - checked_connectivity = 0;
>> - }
>> }
>> -
>> - if (shallow_update && !checked_connectivity)
>> - error("BUG: run 'git fsck' for safety.\n"
>> - "If there are errors, try to remove "
>> - "the reported refs above");
>> + check_shallow_bugs(commands, si);
>> }
>>
>> static struct command **queue_command(struct command **tail,
>> --
>> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function
2014-12-30 2:36 ` [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function Stefan Beller
@ 2014-12-30 8:57 ` Eric Sunshine
0 siblings, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 8:57 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
> Update receive-pack to use an atomic transaction iff the client negotiated
> that it wanted atomic push.
This first line seems germane to this patch...
> This leaves the default behavior 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 behavior so we make it opt-in for now.
>
> If it turns out over time that there are no client scripts that depend on the
> old behavior 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.
However, the remainder feels like it belongs with some other patch,
such as a patch which introduces an --atomic option.
> Inspired-by: Ronnie Sahlberg <sahlberg@google.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 5f44466..35a2264 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1076,8 +1076,8 @@ static void check_shallow_bugs(struct command *commands,
> "the reported refs above");
> }
>
> -static void execute_commands_loop(struct command *commands,
> - struct shallow_info *si)
> +static void execute_commands_non_atomic(struct command *commands,
> + struct shallow_info *si)
Style: Indent the wrapped line to align with the text following the
'(' in the first line.
> {
> struct command *cmd;
> struct strbuf err = STRBUF_INIT;
> @@ -1104,7 +1104,50 @@ static void execute_commands_loop(struct command *commands,
> }
> ref_transaction_free(transaction);
> }
> + strbuf_release(&err);
> +}
> +
> +static void execute_commands_atomic(struct command *commands,
> + struct shallow_info *si)
Style: Indentation.
> +{
> + struct command *cmd;
> + struct strbuf err = STRBUF_INIT;
> + const char *reported_error = "atomic push failure";
> +
> + transaction = ref_transaction_begin(&err);
> + if (!transaction) {
> + rp_error("%s", err.buf);
> + strbuf_reset(&err);
> + reported_error = "transaction failed to start";
> + goto failure;
> + }
> +
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!should_process_cmd(cmd))
> + continue;
>
> + cmd->error_string = update(cmd, si);
> +
> + if (cmd->error_string)
> + goto failure;
> + }
> +
> + if (ref_transaction_commit(transaction, &err)) {
> + rp_error("%s", err.buf);
> + reported_error = "atomic transaction failed";
> + goto failure;
> + }
> +
> + ref_transaction_free(transaction);
> + strbuf_release(&err);
> + return;
Minor comment: This cleanup code is repeated in both the success and
fail branches. It _might_ (or not) be a bit cleaner and more
maintainable to replace the above three lines with:
goto done;
> +
> +failure:
> + for (cmd = commands; cmd; cmd = cmd->next)
> + if (!cmd->error_string)
> + cmd->error_string = reported_error;
And add a 'done' label here:
done:
> + ref_transaction_free(transaction);
> strbuf_release(&err);
> }
>
> @@ -1142,7 +1185,10 @@ static void execute_commands(struct command *commands,
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>
> - execute_commands_loop(commands, si);
> + if (use_atomic)
> + execute_commands_atomic(commands, si);
> + else
> + execute_commands_non_atomic(commands, si);
>
> check_shallow_bugs(commands, si);
> }
> --
> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support
2014-12-30 8:33 ` Stefan Beller
@ 2014-12-30 9:09 ` Eric Sunshine
0 siblings, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 9:09 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List, Ronnie Sahlberg
On Tue, Dec 30, 2014 at 3:33 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 29, 2014 at 11:08 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>>> Subject: receive-pack.c: add documentation for atomic push support
>> When Michael raised the issue of the server being "broken" due to
>> advertising a capability which it does not yet implement, his
>> recommendation[1] was to reorder the patches, not to split out the one
>> tiny bit (capability advertisement) from the larger change. Was there
>> an insurmountable conflict which prevented you from reordering the
>> patches? This is a genuine question since splitting off advertisement
>> -- and only advertisement -- to a patch later in the series smells
>> like a least-resistance approach, rather than a proper solution to the
>> issue Michael raised.
>
> Well there was no syntactical problem (i.e. the interactive rebase
> went flawless),
> but rather a semantic dependency. The reordered patches would not compile
> as we'd heavily depend on the use_atomic variable.
>
> Of course that could have been introduced where required, but at the
> time it did
> not look appealing to me.
>
> I'll reword the commit message header to mention the negotiation part as well.
This still smells like a least-resistance (lazy) approach. You
shouldn't need the stand-alone patch enabling "atomic" advertisement
just to address Michael's concern. His suggestion[1] to reorder the
patches sounds sensible and correct. Toward that end, moving the
declaration of 'use_atomic' to the patch where it is first needed
makes sense, even if the machinery for manipulating that variable
arrives with a later patch.
[1]: http://article.gmane.org/gmane.comp.version-control.git/261793
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 8:42 ` Stefan Beller
@ 2014-12-30 9:10 ` Eric Sunshine
0 siblings, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 9:10 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Tue, Dec 30, 2014 at 3:42 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 29, 2014 at 11:46 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>>> No functional changes intended.
>>> +static void check_shallow_bugs(struct command *commands,
>>> + struct shallow_info *si)
>>> +{
>>> + struct command *cmd;
>>> + int checked_connectivity = 1;
>>> + for (cmd = commands; cmd; cmd = cmd->next) {
>>> + if (!should_process_cmd(cmd))
>>> + continue;
>>> +
>>> + if (shallow_update && si->shallow_ref[cmd->index]) {
>>
>> Another issue: In the original code, 'si->shallow_ref[cmd->index]' is
>> only checked if cmd->error_string is NULL, but here you check it
>> unconditionally, despite the commit message claiming no functional
>> changes. Did you verify that such a behavioral change is benign? (This
>> is a genuine question.)
>
> The error != NULL check is done in if (!should_process_cmd(cmd))
Right. Thanks for setting me straight on that.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place
2014-12-30 8:36 ` Eric Sunshine
@ 2014-12-30 18:45 ` Stefan Beller
2014-12-30 20:33 ` Eric Sunshine
0 siblings, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 18:45 UTC (permalink / raw)
To: Eric Sunshine
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Tue, Dec 30, 2014 at 12:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>> Subject: receive-pack.c: move transaction handling in a central place
>
> This is very generic, and doesn't really explain what this patch is
> about. (See below.)
>
>> No functional changes intended.
>
> Secondary information can be demoted to the end of the commit message.
I think this would help in case there is a subtle bug introduced with
such a commit.
If you bisect it 2 years later and then ask yourself if that subtle
behavior was
intentional (can easily happen if the commit message is
short/unclear). This would then
tell you it's definitely a bug introduced. I believe to have seen such
a comment
somewhere in the history, but I cannot find it again. I'll drop it
into the notes for now.
>
>> This moves all code related to transactions into the execute_commands_loop
>> function which was factored out of execute_commands. This includes
>> beginning and committing the transaction as well as dealing with the
>> errors which may occur during the begin and commit phase of a transaction.
>
> This explains what you're doing, but not why. The purpose of this
> change is that a subsequent patch will be adding another mode of
> operation ("atomic") to execute_commands() which differs from the
> existing mode ("non-atomic") implemented by its main loop. In its
> high-level role, execute_commands() does not need to know or care
> about the low-level details of each mode of operation. Therefore, as
> preparation for introducing a new mode, you're factoring out the
> existing mode into its own stand-alone function.
>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>> This covers the suggestion of patch 2 and 3 by Eric
>> > patch 2: Factor out the main 'for' loop of execute_commands() into a
>> > new function. This new function will eventually become
>> > execute_commands_non_atomic(). At this point, execute_commands() is
>> > pretty much in its final form with the exception of the upcoming 'if
>> > (use_atomic)' conditional.
>> > patch 3: Morph the function extracted in patch 2 into
>> > execute_commands_non_atomic() by adding transaction handling inside
>> > the 'for' loop (and applying the changes from the early part of the
>> > patch which go along with that).
>
> This patch is still rather heavyweight. My suggestion[1] for making
> these particular changes across two patches was quite deliberate. The
> problem with combining them into a single patch is that you're
> performing both code movement and functional changes at the same time.
>
> On its own, pure code movement is easy to review.
>
> On its own, code changes are as easy or difficult to review as the
> changes themselves.
I need to hammer this mantra in my brain. Sorry for messing this up again.
>
> When combined, however, the review effort is greater than the sum of
> the efforts of reviewing them separately. Partly this is because the
> combined changes have a noisier diff. If you move the code in one
> patch, and then change it in a second one, the changes pop out --
> they're quite obvious. On the other hand, when they are combined, the
> reviewer has to deliberately and painstakingly search out the changes,
> which is difficult and error-prone. Combining movement and code
> changes into a single patch also places greater cognitive load on the
> reviewer due to the necessity of keeping a more complex mental
> scoreboard relating to the different types of changes.
>
> More below.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/261706
>
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 06eb287..5f44466 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1073,6 +1076,38 @@ static void check_shallow_bugs(struct command *commands,
>> "the reported refs above");
>> }
>>
>> +static void execute_commands_loop(struct command *commands,
>> + struct shallow_info *si)
>
> Style: Indent the wrapped line to align with the text following the
> '(' in the first line.
That's true. I have found this problem myself at another patch by
Michael lately.
If you apply the patch it is correctly aligned. If you view the patch
however it is missaligned.
Because of the leading plus sign the line in which the function
signature starts is
indented by one character. The other lines starting with a tab indent
only to 8 character.
This would explain a difference of only a few chars, but not so many.
I'll check the indentation here.
>
> It's safe to say that the code which you extracted from
> execute_commands() handled the non-atomic case, and it's safe to say
> that this new function implements the non-atomic case. Therefore, it
> would be truthful to call this function execute_commands_nonatomic().
> No need to invent the name execute_commands_loop().
ok.
>
>> +{
>> + struct command *cmd;
>> + struct strbuf err = STRBUF_INIT;
>> +
>> + for (cmd = commands; cmd; cmd = cmd->next) {
>> + if (!should_process_cmd(cmd))
>> + continue;
>> +
>> + transaction = ref_transaction_begin(&err);
>> + if (!transaction) {
>> + rp_error("%s", err.buf);
>> + strbuf_reset(&err);
>> + cmd->error_string = "transaction failed to start";
>> + continue;
>> + }
>> +
>> + cmd->error_string = update(cmd, si);
>> +
>> + if (!cmd->error_string
>> + && ref_transaction_commit(transaction, &err)) {
>> + rp_error("%s", err.buf);
>> + strbuf_reset(&err);
>> + cmd->error_string = "failed to update ref";
>> + }
>> + ref_transaction_free(transaction);
>> + }
>> +
>> + strbuf_release(&err);
>> +}
>> +
>> static void execute_commands(struct command *commands,
>> const char *unpacker_error,
>> struct shallow_info *si)
>> @@ -1107,12 +1142,8 @@ static void execute_commands(struct command *commands,
>> free(head_name_to_free);
>> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>>
>> - for (cmd = commands; cmd; cmd = cmd->next) {
>> - if (!should_process_cmd(cmd))
>> - continue;
>> + execute_commands_loop(commands, si);
>>
>> - cmd->error_string = update(cmd, si);
>> - }
>> check_shallow_bugs(commands, si);
>> }
>>
>> --
>> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 6:11 ` Eric Sunshine
2014-12-30 8:41 ` Stefan Beller
@ 2014-12-30 20:33 ` Stefan Beller
2014-12-30 21:15 ` Eric Sunshine
1 sibling, 1 reply; 24+ messages in thread
From: Stefan Beller @ 2014-12-30 20:33 UTC (permalink / raw)
To: Eric Sunshine
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>> No functional changes intended.
>
> This is useful to know but is of secondary importance, thus should be
> placed after the explanation and justification of the change.
>
>> Subject: receive-pack.c: simplify execute_commands
>> This commit shortens execute_commands by moving some parts of the code
>> to extra functions.
>
> The _real_ reason for moving these bits of code into their own
> functions is that you intend to introduce additional callers in
> subsequent patches. That is what the commit message (including
> subject) should be conveying to the reader.
>
> The claimed simplification is questionable and not of particular
> importance; and it's misleading to paint it as a goal of the patch.
> Consequently, you could drop mention of it altogether.
>
> More below.
>
>> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
>> index 4e8eaf7..06eb287 100644
>> --- a/builtin/receive-pack.c
>> +++ b/builtin/receive-pack.c
>> @@ -1043,11 +1043,40 @@ static void reject_updates_to_hidden(struct command *commands)
>> }
>> }
>>
>> +static int should_process_cmd(struct command *cmd)
>> +{
>> + if (cmd->error_string)
>> + return 0;
>> + if (cmd->skip_update)
>> + return 0;
>> + return 1;
>
> Alternately, depending upon the polarity of your brain, you could
> collapse the entire function body to:
>
> return !cmd->error_string && !cmd->skip_update;
>
> or:
>
> return !(cmd->error_string || cmd->skip_update);
I did not want to change the structure of the code when moving it around as
you noted in another email that helps on reviewing.
And having the exact same conditions in an if and then return instead
of continue
makes me think it is easier to review than if I also introduce that shortening?
I can do so in a follow up.
>
>> +}
>> +
>> +static void check_shallow_bugs(struct command *commands,
>> + struct shallow_info *si)
>> +{
>> + struct command *cmd;
>> + int checked_connectivity = 1;
>> + for (cmd = commands; cmd; cmd = cmd->next) {
>> + if (!should_process_cmd(cmd))
>> + continue;
>> +
>> + if (shallow_update && si->shallow_ref[cmd->index]) {
>
> Here, inside the loop, you check 'shallow_update'...
>
>> + error("BUG: connectivity check has not been run on ref %s",
>> + cmd->ref_name);
>> + checked_connectivity = 0;
>> + }
>> + }
>> + if (shallow_update && !checked_connectivity)
>
> And here, after the loop, you check 'shallow_update'.
>
> But, if you examine the overall logic, you will find that this
> function does _nothing_ at all when 'shallow_update' is false (other
> than uselessly looping through 'commands'). Therefore, either check
> 'shallow_update' just once at the beginning of the function and exit
> early if false, or have the caller check 'shallow_update' and only
> invoke check_shallow_bugs() if true.
>
> Also, since nothing happens between them, the two conditionals inside
> the loop can be coalesced:
>
> if (should_process_cmd(cmd) && si->shallow_ref[cmd->index]) {
>
>> + error("BUG: run 'git fsck' for safety.\n"
>> + "If there are errors, try to remove "
>> + "the reported refs above");
That's a good idea. I'll definitely add that. But as said above, I
also feel that this
should rather go in a follow up patch as it twists the logic and this
patch is all
about moving the code.
>
> In v6, you considered this a fatal error in the atomic case, which
> caused the entire transaction to be rolled back. However, in this
> version, this error has no effect whatsoever on the atomic
> transaction, which is a rather significant behavioral departure. Which
> is correct? (This is a genuine question; not at all rhetorical.) If
> failing the entire transaction is the correct thing to do, then this
> is going to need some more work.
>
>> +}
>> +
>> static void execute_commands(struct command *commands,
>> const char *unpacker_error,
>> struct shallow_info *si)
>> {
>> - int checked_connectivity;
>> struct command *cmd;
>> unsigned char sha1[20];
>> struct iterate_data data;
>> @@ -1078,27 +1107,13 @@ static void execute_commands(struct command *commands,
>> free(head_name_to_free);
>> head_name = head_name_to_free = resolve_refdup("HEAD", 0, sha1, NULL);
>>
>> - checked_connectivity = 1;
>> for (cmd = commands; cmd; cmd = cmd->next) {
>> - if (cmd->error_string)
>> - continue;
>> -
>> - if (cmd->skip_update)
>> + if (!should_process_cmd(cmd))
>> continue;
>>
>> cmd->error_string = update(cmd, si);
>> - if (shallow_update && !cmd->error_string &&
>> - si->shallow_ref[cmd->index]) {
>> - error("BUG: connectivity check has not been run on ref %s",
>> - cmd->ref_name);
>> - checked_connectivity = 0;
>> - }
>> }
>> -
>> - if (shallow_update && !checked_connectivity)
>> - error("BUG: run 'git fsck' for safety.\n"
>> - "If there are errors, try to remove "
>> - "the reported refs above");
>> + check_shallow_bugs(commands, si);
>> }
>>
>> static struct command **queue_command(struct command **tail,
>> --
>> 2.2.1.62.g3f15098
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place
2014-12-30 18:45 ` Stefan Beller
@ 2014-12-30 20:33 ` Eric Sunshine
0 siblings, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 20:33 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Tue, Dec 30, 2014 at 1:45 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Dec 30, 2014 at 12:36 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>>> No functional changes intended.
>>
>> Secondary information can be demoted to the end of the commit message.
>
> I think this would help in case there is a subtle bug introduced with
> such a commit.
> If you bisect it 2 years later and then ask yourself if that subtle
> behavior was
> intentional (can easily happen if the commit message is
> short/unclear). This would then
> tell you it's definitely a bug introduced. I believe to have seen such
> a comment
> somewhere in the history, but I cannot find it again. I'll drop it
> into the notes for now.
I think you misread or misunderstood my comment, which was that
secondary information should follow primary information in the commit
message. I did not suggest moving it below the "---" line.
"No functional changes intended" is meaningful and belongs in the
commit message, but it's not as important as the explanation and
justification of the change.
>>> +static void execute_commands_loop(struct command *commands,
>>> + struct shallow_info *si)
>>
>> Style: Indent the wrapped line to align with the text following the
>> '(' in the first line.
>
> That's true. I have found this problem myself at another patch by
> Michael lately.
> If you apply the patch it is correctly aligned. If you view the patch
> however it is missaligned.
> Because of the leading plus sign the line in which the function
> signature starts is
> indented by one character. The other lines starting with a tab indent
> only to 8 character.
You're right. This one does align properly. Sorry for the noise.
(Of the two pointed out in patch 6/9, the first one also aligns
correctly; but the second does not.)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCHv8 4/9] receive-pack.c: simplify execute_commands
2014-12-30 20:33 ` Stefan Beller
@ 2014-12-30 21:15 ` Eric Sunshine
0 siblings, 0 replies; 24+ messages in thread
From: Eric Sunshine @ 2014-12-30 21:15 UTC (permalink / raw)
To: Stefan Beller
Cc: ronnie sahlberg, Michael Haggerty, Jonathan Nieder,
Junio C Hamano, Git List
On Tue, Dec 30, 2014 at 3:33 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 29, 2014 at 10:11 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Dec 29, 2014 at 9:36 PM, Stefan Beller <sbeller@google.com> wrote:
>>> +static int should_process_cmd(struct command *cmd)
>>> +{
>>> + if (cmd->error_string)
>>> + return 0;
>>> + if (cmd->skip_update)
>>> + return 0;
>>> + return 1;
>>
>> Alternately, depending upon the polarity of your brain, you could
>> collapse the entire function body to:
>>
>> return !cmd->error_string && !cmd->skip_update;
>>
>> or:
>>
>> return !(cmd->error_string || cmd->skip_update);
>
> I did not want to change the structure of the code when moving it around as
> you noted in another email that helps on reviewing.
> And having the exact same conditions in an if and then return instead
> of continue
> makes me think it is easier to review than if I also introduce that shortening?
> I can do so in a follow up.
It's a judgment call. When composing this review, I actually had a
parenthetical comment here saying that collapsing the body of the
function (as illustrated above) could be done as a patch separate from
the code movement, but I removed the comment just before sending the
email. This is such a simple change that the cognitive overhead from
combining the code movement and simplification is so slight that it
doesn't necessary warrant burdening reviewers with yet another patch.
Therefore, although I very slightly favor splitting it into two
patches, in this particular case, a single patch is not a great
burden, so I don't feel strongly one way or the other. Use your best
judgment.
>>> +}
>>> +
>>> +static void check_shallow_bugs(struct command *commands,
>>> + struct shallow_info *si)
>>> +{
>>> + struct command *cmd;
>>> + int checked_connectivity = 1;
>>> + for (cmd = commands; cmd; cmd = cmd->next) {
>>> + if (!should_process_cmd(cmd))
>>> + continue;
>>> +
>>> + if (shallow_update && si->shallow_ref[cmd->index]) {
>>
>> Here, inside the loop, you check 'shallow_update'...
>>
>>> + error("BUG: connectivity check has not been run on ref %s",
>>> + cmd->ref_name);
>>> + checked_connectivity = 0;
>>> + }
>>> + }
>>> + if (shallow_update && !checked_connectivity)
>>
>> And here, after the loop, you check 'shallow_update'.
>>
>> But, if you examine the overall logic, you will find that this
>> function does _nothing_ at all when 'shallow_update' is false (other
>> than uselessly looping through 'commands'). Therefore, either check
>> 'shallow_update' just once at the beginning of the function and exit
>> early if false, or have the caller check 'shallow_update' and only
>> invoke check_shallow_bugs() if true.
>
> That's a good idea. I'll definitely add that. But as said above, I
> also feel that this
> should rather go in a follow up patch as it twists the logic and this
> patch is all
> about moving the code.
Here, too, when composing the review, I had a parenthetical comment
stating that this improvement to the code might warrant a followup
patch after the code movement patch. However, this case is a bit
different because the combined code in the new function, although not
actively wrong, is uselessly iterating a do-nothing loop when
'shallow_update' is false, and even though not actively incorrect, it
_feels_ weird to commit such pointless code, only to fix it up later.
Due to that "weird" feeling, I didn't feel entirely comfortable
recommending splitting the change into two patches, so I removed the
comment before sending the email. Hence, this again is judgment call.
Try to find a balance which feels right, while taking the review
process into consideration.
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2014-12-30 21:15 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-30 2:36 [PATCHv8 0/9] atomic pushes Stefan Beller
2014-12-30 2:36 ` [PATCHv8 1/9] receive-pack.c: add documentation for atomic push support Stefan Beller
2014-12-30 7:08 ` Eric Sunshine
2014-12-30 8:33 ` Stefan Beller
2014-12-30 9:09 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 2/9] send-pack: rename ref_update_to_be_sent to check_to_send_update Stefan Beller
2014-12-30 2:36 ` [PATCHv8 3/9] send-pack.c: add --atomic command line argument Stefan Beller
2014-12-30 2:36 ` [PATCHv8 4/9] receive-pack.c: simplify execute_commands Stefan Beller
2014-12-30 6:11 ` Eric Sunshine
2014-12-30 8:41 ` Stefan Beller
2014-12-30 20:33 ` Stefan Beller
2014-12-30 21:15 ` Eric Sunshine
2014-12-30 7:46 ` Eric Sunshine
2014-12-30 8:42 ` Stefan Beller
2014-12-30 9:10 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 5/9] receive-pack.c: move transaction handling in a central place Stefan Beller
2014-12-30 8:36 ` Eric Sunshine
2014-12-30 18:45 ` Stefan Beller
2014-12-30 20:33 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 6/9] receive-pack.c: add execute_commands_atomic function Stefan Beller
2014-12-30 8:57 ` Eric Sunshine
2014-12-30 2:36 ` [PATCHv8 7/9] receive-pack.c: enable atomic push protocol support Stefan Beller
2014-12-30 2:36 ` [PATCHv8 8/9] push.c: add an --atomic argument Stefan Beller
2014-12-30 2:36 ` [PATCHv8 9/9] t5543-atomic-push.sh: add basic tests for atomic pushes Stefan Beller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).