Re: [PATCH v3] push: respect --no-thin
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:58:24
Nguyễn Thái Ngọc Duy [off-list ref] writes:
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.
quoted hunk
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....