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

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2016, 19:26 UTC
Message-ID: <xmqqd1o38i0m.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160530145347.15643-1-antoine.queru@ensimag.grenoble-inp.fr>

Antoine Queru <antoine.queru@ensimag.grenoble-inp.fr> writes:

> From: Antoine Queru <Antoine.Queru@ensimag.grenoble-inp.fr>

Don't you want to be known to the project as the email that matches your Signed-off-by: line?

Show 12 quoted lines

> Use the parse-options API rather than a hand-rolled option parser.
>
> Description for --stateless-rpc and --advertise-refs come from
> 42526b4 (Add stateless RPC options to upload-pack,
> receive-pack, 2009-10-30).
>
> Signed-off-by: Antoine Queru <antoine.queru@grenoble-inp.fr>
> Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
> ---
> ---strict::
> +--[no-]strict::
>  	Do not try <directory>/.git/ if <directory> is no Git directory.

Not a new problem, but "is no Git ..." may technically be correct, but it would be easier to read if phrased "is not a Git ..." or something like that.

	I am NOT asking _you_ to change/fix that in this patch.  It
	is just a note for "a low hanging fruit" for people to pick
	up with a separate patch.

Show 10 quoted lines

> +--stateless-rpc::
> +	Perform only a single read-write 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::
> +	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.
> +

Good.

Show 15 quoted lines

> diff --git a/upload-pack.c b/upload-pack.c
> index dc802a0..083d068 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -14,8 +14,12 @@
>  #include "sigchain.h"
>  #include "version.h"
>  #include "string-list.h"
> +#include "parse-options.h"
>  
> -static const char upload_pack_usage[] = "git upload-pack [--strict] [--timeout=<n>] <dir>";
> +static const char * const upload_pack_usage[] = {
> +	N_("git upload-pack [options] <dir>"),
> +	NULL
> +};

Output from "git grep -e '\[option' -e '\[<option' -- \*.c" tells me that "[<options>]" would be more in line with the established convention than "[options]".

	I personally wish if all these hits from the above grep
	consistently spelled it as "[options]", because there is not
	much gained by enclosing the word in <> to highlight it as a
	placeholder.  An argument that "it is done for uniformity
	with descriptions for other non option arguments, i.e. not
	to special case 'options'" would not hold water, e.g. in

        builtin/merge.c:        N_("git merge [<options>] [<commit>...]"),

	<options> is still special-cased in that it implies multiple
	things, unlike "<commit>..." notation that has to explicitly
	say that can have multiple.  [<options>...] would have been
	justifiable with the "make it uniform with non-option args",
	though.

	BUT this patch is not about "make usage string better"
	patch, so I do NOT want you to switch upload-pack's usage
	string to use the "options is special anyway, so let's not
	waste two display columns with enclosing <>" convention.

So, in conclusion, "git upload-pack [<options>] <dir>".

The remainder of the patch looked OK to me.

Thanks.