public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Matthieu Moy <Matthieu.Moy@grenoble-inp•fr>
To: "Sidhant Sharma \[\:tk\]" <tigerkid001@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] builtin/receive-pack.c: use parse_options API
Date: Tue, 01 Mar 2016 18:22:30 +0100	[thread overview]
Message-ID: <vpq60x62jvt.fsf@anie.imag.fr> (raw)
In-Reply-To: <1456846560-9223-1-git-send-email-tigerkid001@gmail.com> (Sidhant Sharma's message of "Tue, 1 Mar 2016 21:06:00 +0530")

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.

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

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

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

next prev parent reply	other threads:[~2016-03-01 17:22 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-01 15:36 [PATCH] builtin/receive-pack.c: use parse_options API Sidhant Sharma [:tk]
2016-03-01 17:22 ` Matthieu Moy [this message]
2016-03-01 17:48   ` Sidhant Sharma
2016-03-01 17:57     ` Matthieu Moy
2016-03-01 18:54       ` Eric Sunshine
2016-03-01 20:21 ` [PATCH v2] " Sidhant Sharma [:tk]
2016-03-01 20:31   ` Sidhant Sharma
2016-03-01 20:39   ` Matthieu Moy
2016-03-01 22:05     ` Junio C Hamano
2016-03-02  5:18       ` Sidhant Sharma
2016-03-02  8:51         ` Junio C Hamano
2016-03-02  8:23       ` Matthieu Moy
2016-03-02  9:53   ` Duy Nguyen
2016-03-02 13:53     ` Sidhant Sharma

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=vpq60x62jvt.fsf@anie.imag.fr \
    --to=matthieu.moy@grenoble-inp$(echo .)fr \
    --cc=git@vger$(echo .)kernel.org \
    --cc=tigerkid001@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox