]> git.ipfire.org Git - thirdparty/git.git/commitdiff
remote-curl: pass on atomic capability to remote side
authorbrian m. carlson <sandals@crustytoothpaste.net>
Wed, 16 Oct 2019 23:45:34 +0000 (23:45 +0000)
committerJunio C Hamano <gitster@pobox.com>
Thu, 17 Oct 2019 07:08:22 +0000 (16:08 +0900)
When pushing more than one reference with the --atomic option, the
server is supposed to perform a single atomic transaction to update the
references, leaving them either all to succeed or all to fail.  This
works fine when pushing locally or over SSH, but when pushing over HTTP,
we fail to pass the atomic capability to the remote side.  In fact, we
have not reported this capability to any remote helpers during the life
of the feature.

Now normally, things happen to work nevertheless, since we actually
check for most types of failures, such as non-fast-forward updates, on
the client side, and just abort the entire attempt.  However, if the
server side reports a problem, such as the inability to lock a ref, the
transaction isn't atomic, because we haven't passed the appropriate
capability over and the remote side has no way of knowing that we wanted
atomic behavior.

Fix this by passing the option from the transport code through to remote
helpers, and from the HTTP remote helper down to send-pack.  With this
change, we can detect if the server side rejects the push and report
back appropriately.  Note the difference in the messages: the remote
side reports "atomic transaction failed", while our own checking rejects
pushes with the message "atomic push failed".

Document the atomic option in the remote helper documentation, so other
implementers can implement it if they like.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation/gitremote-helpers.txt
remote-curl.c
t/t5541-http-push-smart.sh
transport-helper.c
transport.h

index 43f80c80683805f97a71220005af7a0e862b8cca..03b3a6883f36886744de98a69b63140582bd895f 100644 (file)
@@ -505,6 +505,11 @@ set by Git if the remote helper has the 'option' capability.
        Indicate that only the objects wanted need to be fetched, not
        their dependents.
 
+'option atomic' {'true'|'false'}::
+       When pushing, request the remote server to update refs in a single atomic
+       transaction.  If successful, all refs will be updated, or none will.  If the
+       remote side does not support this capability, the push will fail.
+
 SEE ALSO
 --------
 linkgit:git-remote[1]
index 051f26629d7b9f6679e47f6dc353f09c16c377ae..5232ed84b66d3c8f076c7747f0943a0583410e80 100644 (file)
@@ -40,7 +40,8 @@ struct options {
                push_cert : 2,
                deepen_relative : 1,
                from_promisor : 1,
-               no_dependents : 1;
+               no_dependents : 1,
+               atomic : 1;
 };
 static struct options options;
 static struct string_list cas_options = STRING_LIST_INIT_DUP;
@@ -148,6 +149,14 @@ static int set_option(const char *name, const char *value)
                else
                        return -1;
                return 0;
+       } else if (!strcmp(name, "atomic")) {
+               if (!strcmp(value, "true"))
+                       options.atomic = 1;
+               else if (!strcmp(value, "false"))
+                       options.atomic = 0;
+               else
+                       return -1;
+               return 0;
        } else if (!strcmp(name, "push-option")) {
                if (*value != '"')
                        string_list_append(&options.push_options, value);
@@ -1196,6 +1205,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
                argv_array_push(&args, "--signed=yes");
        else if (options.push_cert == SEND_PACK_PUSH_CERT_IF_ASKED)
                argv_array_push(&args, "--signed=if-asked");
+       if (options.atomic)
+               argv_array_push(&args, "--atomic");
        if (options.verbosity == 0)
                argv_array_push(&args, "--quiet");
        else if (options.verbosity > 1)
index b86ddb60f2ea6ec1c177b8be5d056d064a1d7697..3aec962cd8d1ee0a7729bdeb27a63ebabe328e3a 100755 (executable)
@@ -184,11 +184,12 @@ test_expect_success 'push --atomic also prevents branch creation, reports collat
        test_config -C "$d" http.receivepack true &&
        up="$HTTPD_URL"/smart/atomic-branches.git &&
 
-       # Tell "$up" about two branches for now
+       # Tell "$up" about three branches for now
        test_commit atomic1 &&
        test_commit atomic2 &&
        git branch collateral &&
-       git push "$up" master collateral &&
+       git branch other &&
+       git push "$up" master collateral other &&
 
        # collateral is a valid push, but should be failed by atomic push
        git checkout collateral &&
@@ -226,6 +227,41 @@ test_expect_success 'push --atomic also prevents branch creation, reports collat
        grep "^ ! .*rejected.* collateral -> collateral .*atomic push failed" output
 '
 
+test_expect_success 'push --atomic fails on server-side errors' '
+       # Use previously set up repository
+       d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git &&
+       test_config -C "$d" http.receivepack true &&
+       up="$HTTPD_URL"/smart/atomic-branches.git &&
+
+       # break ref updates for other on the remote site
+       mkdir "$d/refs/heads/other.lock" &&
+
+       # add the new commit to other
+       git branch -f other collateral &&
+
+       # --atomic should cause entire push to be rejected
+       test_must_fail git push --atomic "$up" atomic other 2>output  &&
+
+       # the new branch should not have been created upstream
+       test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
+
+       # upstream should still reflect atomic2, the last thing we pushed
+       # successfully
+       git rev-parse atomic2 >expected &&
+       # ...to other.
+       git -C "$d" rev-parse refs/heads/other >actual &&
+       test_cmp expected actual &&
+
+       # the new branch should not have been created upstream
+       test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
+
+       # the failed refs should be indicated to the user
+       grep "^ ! .*rejected.* other -> other .*atomic transaction failed" output &&
+
+       # the collateral failure refs should be indicated to the user
+       grep "^ ! .*rejected.* atomic -> atomic .*atomic transaction failed" output
+'
+
 test_expect_success 'push --all can push to empty repo' '
        d=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&
        git init --bare "$d" &&
index 6b05a88faf59ee74840b0f9fa340a5d9397a0f5d..1ecaa9101a8ef1546711f8e3428d7bd0691ada31 100644 (file)
@@ -840,6 +840,10 @@ static void set_common_push_options(struct transport *transport,
                        die(_("helper %s does not support --signed=if-asked"), name);
        }
 
+       if (flags & TRANSPORT_PUSH_ATOMIC)
+               if (set_helper_option(transport, TRANS_OPT_ATOMIC, "true") != 0)
+                       die(_("helper %s does not support --atomic"), name);
+
        if (flags & TRANSPORT_PUSH_OPTIONS) {
                struct string_list_item *item;
                for_each_string_list_item(item, transport->push_options)
index 0b5f7806f625d888175e1c633ec2b59d8d07803b..e0131daab987f582801972fe84aa412d235c89f7 100644 (file)
@@ -208,6 +208,9 @@ void transport_check_allowed(const char *type);
 /* Filter objects for partial clone and fetch */
 #define TRANS_OPT_LIST_OBJECTS_FILTER "filter"
 
+/* Request atomic (all-or-nothing) updates when pushing */
+#define TRANS_OPT_ATOMIC "atomic"
+
 /**
  * Returns 0 if the option was used, non-zero otherwise. Prints a
  * message to stderr if the option is not used.