Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
Michael Haggerty mhag...@alum.mit.edu writes: If I understand correctly, after this patch the server advertises the atomic capability even though it doesn't actually have that ability until a later patch. It seems to me that the order of the patches should be reversed: don't advertise the capability before it is actually implemented. Why? Bisection. Between the two patches the server is buggy. That is a valid point. It also reminds us of another thing. We would need a way to test interoperability among the three new combinations (i.e. new and old receive-pack talking to new and old git push). We can control what the sender talks on the wire by giving or not giving --atomic option on the command line, but there should be a way for us to control what the receiver talks on the wire, i.e. receivepack.pushAtomic = false that tells us not to advertise the atomic push capability over the wire, even if you are running the updated receive-pack binary. This will not only for testing. When we discover that atomic support is undesirable for whatever reason (e.g. the transaction machinery may consume too many file descriptors without a good reason), we would need a way for users to disable it until the problem is fixed. And of course the tests should make sure that (1) git push --atomic that talks with the receiving end that has receivepack.pushAtomic set to false behaves as we desire (error out? degrade to non-atomic? --- whichever way we decide). (2) git push that talks with the receiving end with atomic enabled does not do an atomic push, i.e. try to push two refs, one that fast forwards and the other that does not, and see one ref is updated while the other ref remains intact and git push itself reports failured.; (3) git push --atomic that talks with the receiving end with atomic enabled does the atomic thing. among other things. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
On 12/19/2014 08:38 PM, Stefan Beller wrote: From: Ronnie Sahlberg sahlb...@google.com This adds support to the protocol 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. Sorry to jump in so late... If I understand correctly, after this patch the server advertises the atomic capability even though it doesn't actually have that ability until a later patch. It seems to me that the order of the patches should be reversed: don't advertise the capability before it is actually implemented. Why? Bisection. Between the two patches the server is buggy. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller sbel...@google.com wrote: From: Ronnie Sahlberg sahlb...@google.com This adds support to the protocol 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. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Stefan Beller sbel...@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 @@ -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 Not itself worth a re-send, but if you re-send for some other reason... one atomic still smacks of redundancy; an atomic sounds better. +updated or none. + allow-tip-sha1-in-want -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
On 22.12.2014 14:52, Eric Sunshine wrote: On Fri, Dec 19, 2014 at 2:38 PM, Stefan Beller sbel...@google.com wrote: From: Ronnie Sahlberg sahlb...@google.com This adds support to the protocol 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. Signed-off-by: Ronnie Sahlberg sahlb...@google.com Signed-off-by: Stefan Beller sbel...@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 @@ -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 Not itself worth a re-send, but if you re-send for some other reason... one atomic still smacks of redundancy; an atomic sounds better. I did hear you saying 'one single atomic' being too redundant. And I agree that 'one' and 'single' makes the redundancy. However I have the impression 'an atomic' is too weak. Not everybody is a careful reader picking up the subtle notions. Not everybody is english native. Or concentrated. Look at it the other way: How could it be done? * All of the refs could be updated one at a time, not atomically, so foreach ref: open refs/heads/bla: write new sha1 * All of the refs could be updated at once, not atomically: open refs pack file: write new content * All of the refs could be updated, one at a time, atomically: foreach ref: get lock write content to lock rename into place * All of the refs at once, atomically. open packed refs file lock: write new content rename into place That said, atomicity and how many transactions there are, are orthogonal to each other. That's why I'd keep pointing out 'one atomic' transaction. Thanks for all the comments. I may be doing cleanup patches for you on top of what Junio queued. +updated or none. + allow-tip-sha1-in-want -- -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] receive-pack.c: add protocol support to negotiate atomic-push
From: Ronnie Sahlberg sahlb...@google.com
This adds support to the protocol 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.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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(
Documentation/technical/protocol-capabilities.txt | 13 +++--
builtin/receive-pack.c| 6 +-
send-pack.c | 10 ++
send-pack.h | 3 ++-
4 files changed, 28 insertions(+), 4 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..e76e5d5 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;
@@ -171,7 +172,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)
@@ -1179,6 +1181,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..6646600 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;
+ 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))
