Re: [PATCH v4] upload-pack.c: use of parse-options API

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 27, 2016, 16:59 UTC
Message-ID: <CAPig+cRipGvwqXQhY1koOLR2h=VcHqrTMhp_zbphYtaabQ0qAw@mail.gmail.com>
In-Reply-To: <20160527141628.1677-1-Antoine.Queru@ensimag.grenoble-inp.fr>

On Fri, May 27, 2016 at 10:16 AM, Antoine Queru <Antoine.Queru@ensimag.grenoble-inp.fr> wrote:

> upload-pack.c: use of parse-options API

Matthieu already mentioned that this should use imperative mood:

    upload-pack: use parse-options API

> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.

Imperative:

    Use the parse-options API rather than a hand-rolled
    option parser.

That the code becomes more compact is a nice result of this change, but not particularly important, thus not really worth a mention in the commit message.

> Description for --stateless-rpc and --advertise-refs come from
> the commit 42526b4 (Add stateless RPC options to upload-pack,
> receive-pack, 2009-10-30).

s/from the commit/from/

Show 7 quoted lines

> Signed-off-by: Antoine Queru <antoine.queru@grenoble-inp.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
> diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
> @@ -31,6 +31,19 @@ OPTIONS
> +--stateless-rpc::
> +       the programs now assume they may perform only a single read-write

s/the/The/

Also, to what "programs" does this refer? And what does "now" mean here?

Show 6 quoted lines

> +       cycle with stdin and stdout. This fits with the HTTP POST request
> +       processing model where a program may read the request, write a
> +       response, and must exit.
> +
> +--advertise-refs::
> +       When --advertise-refs is passed as a command line parameter only

The entire "When ... parameter" bit is redundant, isn't it? Why not just say "Perform only..."?

Show 5 quoted lines

> +       the initial ref advertisement is output, and the program exits
> +       immediately.  This fits with the HTTP GET request model, where
> +       no request content is received but a response must be produced.
> +
> +

Style: Drop the extra blank line.

Show 17 quoted lines

>  <directory>::
>         The repository to sync from.
>
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
> -int main(int argc, char **argv)
> +int main(int argc, const char **argv)
>  {
> -       char *dir;
> -       int i;
> +       const char *dir;
>         int strict = 0;
> +       struct option options[] = {
> +               OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
> +                        N_("quit after a single request/response exchange")),
> +               OPT_BOOL(0, "advertise-refs", &advertise_refs,
> +                        N_("only the initial ref advertisement is output, program exits immediately")),

Possible rewrite: "exit immediately after initial ref advertisement"

> +               OPT_BOOL(0, "strict", &strict,
> +                        N_("do not try <directory>/.git/ if <directory> is no Git directory")),

Use of OPT_BOOL introduces a --no-strict option which didn't exist before. Does this need to be documented? (Genuine question.)

> +               OPT_INTEGER(0, "timeout", &timeout,
> +                           N_("interrupt transfer after <n> seconds of inactivity")),
> +               OPT_END()
> +       };