patchbuiltin/receive-pack.c: use parse_options API

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

> +	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,
> +		},

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

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

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

>>> +	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);
>> ?
>>
>>
> Yes, I just realized that is dead code (sorry). Removing the 'if'
> statement would correct that?

> Sidhant Sharma <tigerkid001@gmail.com> writes:
>> Another thing I'd like to ask is when I prepare the next patch, should
>> it be sent as reply in this thread, or as a new thread?
>
> No strict rule on that, but I usually use --in-reply-to on the root of
> the thread for previous iteration. If you don't, include a link (e.g.
> gmane) to the previous iteration in the cover-letter.

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

> "Sidhant Sharma [:tk]" <tigerkid001@gmail.com> writes:
>
>> Make receive-pack use the parse_options API,
>> bringing it more in line with send-pack and push.
>
> Thanks. This version looks good to me.

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> "Sidhant Sharma [:tk]" <tigerkid001@gmail.com> writes:
>>
>>> Make receive-pack use the parse_options API,
>>> bringing it more in line with send-pack and push.
>> Thanks. This version looks good to me.
> I'll queue this with your "Reviewed-by:" to 'pu', just as a
> Microproject reward ;-).  Given that the program will never see an
> interactive use from a command line, however, I am not sure if it is
> worth actually merging it down thru 'next' to 'master'.
>
> Thanks.

>> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>>
>>> Thanks. This version looks good to me.
>> I'll queue this with your "Reviewed-by:" to 'pu', just as a
>> Microproject reward ;-).  Given that the program will never see an
>> interactive use from a command line, however, I am not sure if it is
>> worth actually merging it down thru 'next' to 'master'.

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> "Sidhant Sharma [:tk]" <tigerkid001@gmail.com> writes:
>>
>>> Make receive-pack use the parse_options API,
>>> bringing it more in line with send-pack and push.
>>
>> Thanks. This version looks good to me.
>
> I'll queue this with your "Reviewed-by:" to 'pu', just as a
> Microproject reward ;-).  Given that the program will never see an
> interactive use from a command line, however, I am not sure if it is
> worth actually merging it down thru 'next' to 'master'.

> +       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),
> +               OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL),
> +               OPT_END()
> +       };

> On Wed, Mar 2, 2016 at 3:21 AM, Sidhant Sharma [:tk]
> <tigerkid001@gmail.com> wrote:
>> +       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),
>> +               OPT_HIDDEN_BOOL(0, "reject-thin-pack-for-testing", &reject_thin, NULL),
>> +               OPT_END()
>> +       };
> If the patch is already final, don't bother. If not, I think I prefer
> to keep all these options visible (except the "...-for-testing"). This
> command is never executed directly by mere mortals. The ones that run
> it need to know all about these hidden tricks because they're
> implementing new transports.
>
> Another side note. I'm not so sure if we should N_() and _() strings
> in this command (same reasoning, the command's user is very likely
> developers, not true users). But it does not harm to i18n-ize the
> command either. Slightly more work for translators, of course.