[PATCHv2 3/6] send-pack.c: add --atomic command line argument

[Stefan Beller]

From: Ronnie Sahlberg sahlb...@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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---

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);

 Documentation/git-send-pack.txt |  7 ++-
 builtin/send-pack.c |  6 +-
 remote.h|  3 ++-
 send-pack.c | 36 ++--
 transport.c |  4 
 5 files changed, 51 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;

Re: [PATCHv2 3/6] send-pack.c: add --atomic command line argument

[Junio C Hamano]

Stefan Beller sbel...@google.com writes:

  * 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?

I suspect that you completely mis-read me.

If you add an extra arg that is *specifically* for atomic push, like
this:

-static int update_to_send(...);
+static int update_to_send(..., int *atomic_push_failed);

what signal does it send to the next person who may want to add
a new nuclear push option?  Should the patch look like

-static int update_to_send(..., int *atomic_push_failed);
+static int update_to_send(..., int *atomic_push_failed, int 
*nuclear_push_failed);

by adding yet another separate variable for error reporting?

I would have understood if the new variable was named something like
failure_reason, which may be set to PUSH_FAILURE_ATOMIC when an
atomic push failure was detected.  Making the helper return the
reason why the push failed would be another way, like you did in the
2/6 patch in this round.

Either way, the next person would know to add a code to do his
nuclear push and set the reason to PUSH_FAILURE_NUCLEAR when it
fails, instead of piling yet another error reporting variable that
is specific to the feature.

This is all about code maintainability, which is very different from
who cares about error reporting.

 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;
 ...
   for (ref = remote_refs; ref; ref = ref-next) {
 - if (no_ref_update_to_be_sent(ref, args))
 + int reject_reason;
 + if ((reject_reason = no_ref_update_to_be_sent(ref, args))) {

We avoid assignment inside a conditional.

 + /* 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  reject_reason == 2)
 + return atomic_push_failure(args,
 +remote_refs,
 +ref);
   continue;

Perhaps

switch (check_to_send_update(...)) {
case 0: /* no error */
break;
case -REF_STATUS_ATOMIC_PUSH_FAILED:
return atomic_push_failure(args, remote_refs, ref);
break;
default:
continue;
}

or something?

--
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