Re: [PATCH] builtin/receive-pack.c: use parse_options API

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Mar 1, 2016, 17:22 UTC
Message-ID: <vpq60x62jvt.fsf@anie.imag.fr>
In-Reply-To: <1456846560-9223-1-git-send-email-tigerkid001@gmail.com>

Hi,

Thanks for your patch.

"Sidhant Sharma [:tk]" <tigerkid001@gmail.com> writes:

> This patch makes receive-pack use the parse_options API,

We usually avoid saying "this patch" and use imperative tone: talk to your patch and give it orders like "Make receive-pack use the parse_options API ...". Or just skip that part which is already in the title.

Show 6 quoted lines

> @@ -45,12 +48,12 @@ static int unpack_limit = 100;
>  static int report_status;
>  static int use_sideband;
>  static int use_atomic;
> -static int quiet;
> +static int quiet = 0;

static int are already initialized to 0, you don't need this explicit "= 0". In the codebase of Git, we prever omiting the initialization.

Show 9 quoted lines

> +	struct option options[] = {
> +		OPT__QUIET(&quiet, N_("quiet")),
> +		OPT_HIDDEN_BOOL(0, "stateless-rpc", &stateless_rpc, NULL),
> +		OPT_HIDDEN_BOOL(0, "advertise-refs", &advertise_refs, NULL),
> +		/* Hidden OPT_BOOL option */
> +		{
> +			OPTION_SET_INT, 0, "reject-thin-pack-for-testing", &fix_thin, NULL,
> +			NULL, PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0,
> +		},

After seeing the patch, I think the code would be clearer by using something like

	OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL)

and then use !reject_thin where the patch was using fix_thin. Turns 5 lines into one here, and you just pay a ! later in terms of readability.

Starting from here, the patch is a bit painful to read because the diff heuristics grouped hunks in a strange way. You may try "git format-patch --patience" or --minimal or --histogram to see if it gives a better result. The final commit would be the same, but it may make review easier.

(Not blaming you, just pointing a potentially useful hint, don't worry)

Show 16 quoted lines

>  	packet_trace_identity("receive-pack");
>
> -	argv++;
> -	for (i = 1; i < argc; i++) {
> -		const char *arg = *argv++;
> +	argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0);
>
> -		if (*arg == '-') {
> -			if (!strcmp(arg, "--quiet")) {
> -				quiet = 1;
> -				continue;
> -			}
> +	if (argc > 1)
> +		usage_msg_opt(_("Too many arguments."), receive_pack_usage, options);
> +	if (argc == 0)
> +		usage_msg_opt(_("You must specify a directory."), receive_pack_usage, options);

Before that, the loop was ensuring that service_dir was assigned once and only once, and now you check that you have one non-option arg and assign it unconditionally:

> +	service_dir = argv[0];

... so isn't this "if" dead code:

>  	if (!service_dir)
> -		usage(receive_pack_usage);
> +		usage_with_options(receive_pack_usage, options);

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