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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 12, 2013, 05:59 UTC
Message-ID: <7vhaev1mao.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1376211178-17444-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 6 quoted lines

> Over the time the default value for --thin has been switched between
> on and off. As of now it's always on, even if --no-thin is given.
> Correct the code to respect --no-thin.
>
> receive-pack learns about --no-thin only for testing purposes, hence
> no document update.

Please name it "--reject-thin-pack-for-testing" to make it crystal clear that a documentation patch to "document undocumented option" is unwanted.

Show 23 quoted lines

> diff --git a/builtin/push.c b/builtin/push.c
> index 04f0eaf..333a1fb 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -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, 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.

Puzzled....