Re: [PATCH] transport-helper: enforce atomic in push_refs_with_push
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 2, 2019, 19:06 UTC
- Message-ID
- <xmqq1rz845pc.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190702005340.66615-1-emilyshaffer@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
Show 21 quoted lines
> diff --git a/transport-helper.c b/transport-helper.c
> index c7e17ec9cb..6b05a88faf 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,
> {
> int force_all = flags & TRANSPORT_PUSH_FORCE;
> int mirror = flags & TRANSPORT_PUSH_MIRROR;
> + int atomic = flags & TRANSPORT_PUSH_ATOMIC;
> struct helper_data *data = transport->data;
> struct strbuf buf = STRBUF_INIT;
> struct ref *ref;
> @@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,
> case REF_STATUS_REJECT_NONFASTFORWARD:
> case REF_STATUS_REJECT_STALE:
> case REF_STATUS_REJECT_ALREADY_EXISTS:
> + if (atomic) {
> + string_list_clear(&cas_options, 0);
> + return 0;
> + } else
> + continue;Ah, this looks vaguely familiar. Thanks for resurrecting the topic.
The clearing is merely to avoid leaks, and the primary change to the function is to immediately return 0.
Show 10 quoted lines
> case REF_STATUS_UPTODATE: > continue; > default: > diff --git a/transport.c b/transport.c > index f1fcd2c4b0..f4d6b38f9d 100644 > --- a/transport.c > +++ b/transport.c > @@ -1226,10 +1226,23 @@ int transport_push(struct repository *r, > err = push_had_errors(remote_refs); > ret = push_ret | err;
Here, before reporting the push result, when we are doing ATOMIC, we tweak the result we are going to report to atomic-push-failed.
Show 11 quoted lines
> + if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
> + for (struct ref *it = remote_refs; it; it = it->next)
> + switch (it->status) {
> + case REF_STATUS_NONE:
> + case REF_STATUS_UPTODATE:
> + case REF_STATUS_OK:
> + it->status = REF_STATUS_ATOMIC_PUSH_FAILED;
> + default:
> + continue;
> + }
> + }This roughly corresponds to what send-pack.c::atomic_push_failure() does. Here, we avoid overwriting a status that already signals a failure. The list of "good" statuses used here match what is used at the end of send_pack.c::send_pack(), which decides the final outcome of "git push" for the native transport.
Looks good.
By the way, I rearranged the patch as I happen to agree with Dscho that the additional {} was unwarranted and made it harder to review. It is clear that we need to tweak the status before reporting.
Show 8 quoted lines
> > if (!quiet || err) > transport_print_push_status(transport->url, remote_refs, > verbose | porcelain, porcelain, > reject_reasons); > > if (flags & TRANSPORT_PUSH_SET_UPSTREAM) > set_upstreams(transport, remote_refs, pretend);