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

[Eric Sunshine <sunshine@sunshineco.com>]

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/

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

> +       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..."?

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

>  <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()
> +       };