Re: [PATCH v3] push: respect --no-thin

From: Duy Nguyen <pclouds@gmail.com>
Date: Aug 12, 2013, 11:30 UTC
Message-ID: <CACsJy8C-c1TXRiYBO7Mm0jGZvttqzRTONTkskuoU3_kZxHhuCQ@mail.gmail.com>
In-Reply-To: <7vhaev1mao.fsf@alter.siamese.dyndns.org>

On Mon, Aug 12, 2013 at 12:59 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 31 quoted lines

>> @@ -15,7 +15,7 @@ static const char * const push_usage[] = {
>>       NULL,
>>  };
>>
>> -static int thin;
>> +static int thin = 1;
>>  static int deleterefs;
>>  static const char *receivepack;
>>  static int verbosity;
>> @@ -313,8 +313,7 @@ static int push_with_options(struct transport *transport, int flags)
>>       if (receivepack)
>>               transport_set_option(transport,
>>                                    TRANS_OPT_RECEIVEPACK, receivepack);
>> -     if (thin)
>> -             transport_set_option(transport, TRANS_OPT_THIN, "yes");
>> +     transport_set_option(transport, TRANS_OPT_THIN, thin ? "yes" : NULL);
>>
>>       if (verbosity > 0)
>>               fprintf(stderr, _("Pushing to %s\n"), transport->url);
>
> Hmm, I am utterly confused.
>
> How does the original code have thin as an non-overridable default?
> The variable is initialized to 0, parse-options specifies it as
> OPT_BOOLEAN, and TRANS_OPT_THIN is set only if "thin" is set.
>
> Updated code flips the default to "1" but unconditionally uses
> TRANS_OPT_THIN to set it to either "yes" or NULL.  While it is not
> wrong per-se, do_push() (i.e. the caller of this function) grabs the
> transport from transport_get() which initializes the transport with
> the thin option disabled by default,

transport_get() actually sets thin option to 1 by default. If I don't misread the code, the first version of transport.c already flipped "thin" from 0 (in push.c) to 1 (in transport.c), see 9b28851 (Push code for transport library - 2007-09-10). The funny thing is that commit was just one day after Shawn flipped 'thin' from 1 to 0 in push.c in a4503a1.

> so it is not immediately
> obvious to me why "always call TRANS_OPT_THIN but set it explicitly
> to NULL when --no-thin is asked" is necessary or improvement.

-- 
Duy