Re: [PATCH 2/8] send-pack.c: add an --atomic-push command line argument

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Oct 30, 2014, 21:21 UTC
Message-ID: <CAL=YDWkUzC9bJo_iN6sPqS6JkAe7XWxtn99tLQYvfv1qwpBovw@mail.gmail.com>
In-Reply-To: <xmqqioj1uxx1.fsf@gitster.dls.corp.google.com>

On Thu, Oct 30, 2014 at 1:04 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
>> index 2a0de42..8f64feb 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-push] [<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-push::
>> +     With atomic-push all refs are updated in one single atomic transaction.
>> +     This means that if any of the refs fails then the entire push will
>> +     fail without changing any refs.
>
> Whenever you say "This means that", please read it twice to see if
> everything before and including that phrase can be removed.  It is a
> sign that you found what you wrote before it is not understandable
> and what follows is the version that would be understood by the
> readers.

I reworded it for the next re-roll.

Show 9 quoted lines

>
>> @@ -203,6 +203,13 @@ 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:
>> +             if (atomic_push_failed && args->use_atomic_push) {
>
> Hmph.  Can we even get atomis_push_failed when args->use_atomic_push
> is not set?

It can, but it looks wrong to have one variable passed in via an argument and the other variable being a global.

I changed it to be
    if (atomic_push_failed) {
instead and only pass in a non-NULL atomic_push_failed by the caller
iff args->use_atomic_push is true.

Show 11 quoted lines

>
>> +                     fprintf(stderr, "Atomic push is not possible "
>> +                             "for ref %s. Status:%d\n", ref->name,
>> +                             ref->status);
>> +                     *atomic_push_failed = 1;
>> +             }
>> +             /* fallthrough */
>
> Is this "is not possible" (hence we do not let the user use it), or
> "failed" (i.e. we tried and we failed)?
>

I reworded it to say failed instead.

Will be part of next re-roll.

Thanks Ronnie Sahlberg