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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 19, 2016, 16:10 UTC
Message-ID: <431335092.10912437.1463674220365.JavaMail.zimbra@imag.fr>
In-Reply-To: <20160519153903.22104-1-Antoine.Queru@ensimag.grenoble-inp.fr>

Antoine.Queru@ensimag.grenoble-inp.fr wrote:

> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.
> Description for -stateless-rpc and --advertise-refs
> come from the commit (gmane/131517)

Please, use a real commit id instead of a Gmane link.

We don't know how long Gmane will remain up, but a self reference from Git's history to itself will always remain valid.

The following alias is handy for this:

[alias]
        whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short

In your case it would allow writing:

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

> diff v1 v2:

We usually say "diff" to refer to an actual diff. I'd write "changes since v1" here.

> +		OPT_BOOL(0, "stateless-rpc", &stateless_rpc,
> +			 N_("may perform only a single read-write cycle with stdin and stdout")),

It's weird to define what an option does with "may". It's a property of --stateless-rpc, but does not really define it.

Show 11 quoted lines

> +	if (argc != 1)
> +		usage_with_options(upload_pack_usage, options);
>  
> -	setup_path();
> +	if (timeout)
> +		daemon_mode = 1;
>  
> -	dir = argv[i];
> +	setup_path();
>  
> +	dir = argv[0];

Not a problem with your code, but the patch shows "setup_path()" as moved while it is not really. Maybe using "send-email --patience" or some other diff option could make the patch nicer. Not really important as it does not change the final state.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/