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

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.

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....

On Mon, Aug 12, 2013 at 12:59 PM, Junio C Hamano [off-list ref] wrote:

quoted

@@ -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