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

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

[Sidhant Sharma [:tk]]

This patch makes receive-pack use the parse_options API,
bringing it more in line with send-pack and push.

Helped-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Sidhant Sharma [:tk] <tigerkid001@gmail.com>
---
 builtin/receive-pack.c | 55 ++++++++++++++++++++++----------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c8e32b2..fe9a594 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -21,7 +21,10 @@
 #include "sigchain.h"
 #include "fsck.h"

-static const char receive_pack_usage[] = "git receive-pack <git-dir>";
+static const char * const receive_pack_usage[] = {
+	N_("git receive-pack <git-dir>"),
+	NULL
+};

 enum deny_action {
 	DENY_UNCONFIGURED,
@@ -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 prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
 static int fix_thin = 1;
-static int stateless_rpc;
+static int stateless_rpc = 0;
 static const char *service_dir;
 static const char *head_name;
 static void *head_name_to_free;
@@ -1707,45 +1710,35 @@ static int delete_only(struct command *commands)
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int advertise_refs = 0;
-	int i;
 	struct command *commands;
 	struct sha1_array shallow = SHA1_ARRAY_INIT;
 	struct sha1_array ref = SHA1_ARRAY_INIT;
 	struct shallow_info si;
+	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,
+		},
+		OPT_END()
+	};

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

-			if (!strcmp(arg, "--advertise-refs")) {
-				advertise_refs = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--stateless-rpc")) {
-				stateless_rpc = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--reject-thin-pack-for-testing")) {
-				fix_thin = 0;
-				continue;
-			}
+	service_dir = argv[0];

-			usage(receive_pack_usage);
-		}
-		if (service_dir)
-			usage(receive_pack_usage);
-		service_dir = arg;
-	}
 	if (!service_dir)
-		usage(receive_pack_usage);
+		usage_with_options(receive_pack_usage, options);

 	setup_path();

--
2.6.2

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

[Matthieu Moy]

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/

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

[Sidhant Sharma]

> 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.
OK, will correct the above points.

>>  	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);
> ?
>
>
Yes, I just realized that is dead code (sorry). Removing the 'if' statement would correct that? Also, is the unconditional assignment to service_dir correct in this case, or should some other test condition be added?

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?



Thanks and regards,
Sidhant Sharma  [:tk]

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

[Matthieu Moy]

Sidhant Sharma <tigerkid001@gmail.com> writes:

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

Yes.

> Also, is the unconditional assignment to service_dir correct in this
> case, or should some other test condition be added?

Since usage_msg_opt is NORETURN, it's OK: if you reach this point, you
know that argv[0] contains something.

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

format-patch has a -v2 option to let you get [PATCH v2 ...]
automatically.

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

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

[Eric Sunshine]

On Tue, Mar 1, 2016 at 12:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> 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.

It's good manners to include a link to the previous version even if
you do use --in-reply-to since not all reviewers will have the
previous thread in their mailbox.

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

[Sidhant Sharma [:tk]]

Make receive-pack use the parse_options API,
bringing it more in line with send-pack and push.

Helped-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Sidhant Sharma [:tk] <tigerkid001@gmail.com>
---

 Link to previous version: $gmane/288035

 builtin/receive-pack.c | 53 +++++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c8e32b2..220a899 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -21,7 +21,10 @@
 #include "sigchain.h"
 #include "fsck.h"

-static const char receive_pack_usage[] = "git receive-pack <git-dir>";
+static const char * const receive_pack_usage[] = {
+	N_("git receive-pack <git-dir>"),
+	NULL
+};

 enum deny_action {
 	DENY_UNCONFIGURED,
@@ -49,7 +52,7 @@ static int quiet;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
-static int fix_thin = 1;
+static int reject_thin;
 static int stateless_rpc;
 static const char *service_dir;
 static const char *head_name;
@@ -1548,7 +1551,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (fsck_objects)
 			argv_array_pushf(&child.args, "--strict%s",
 				fsck_msg_types.buf);
-		if (fix_thin)
+		if (!reject_thin)
 			argv_array_push(&child.args, "--fix-thin");
 		child.out = -1;
 		child.err = err_fd;
@@ -1707,45 +1710,29 @@ static int delete_only(struct command *commands)
 int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 {
 	int advertise_refs = 0;
-	int i;
 	struct command *commands;
 	struct sha1_array shallow = SHA1_ARRAY_INIT;
 	struct sha1_array ref = SHA1_ARRAY_INIT;
 	struct shallow_info si;

+	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()
+	};
+
 	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);

-			if (!strcmp(arg, "--advertise-refs")) {
-				advertise_refs = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--stateless-rpc")) {
-				stateless_rpc = 1;
-				continue;
-			}
-			if (!strcmp(arg, "--reject-thin-pack-for-testing")) {
-				fix_thin = 0;
-				continue;
-			}
-
-			usage(receive_pack_usage);
-		}
-		if (service_dir)
-			usage(receive_pack_usage);
-		service_dir = arg;
-	}
-	if (!service_dir)
-		usage(receive_pack_usage);
+	service_dir = argv[0];

 	setup_path();

--
2.7.2

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

[Sidhant Sharma]

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


I tried using the other algorithms, but results were same for all.


Regards,
Sidhant Sharma [:tk]

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

[Matthieu Moy]

"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
http://www-verimag.imag.fr/~moy/

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

[Junio C Hamano]

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.

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

[Sidhant Sharma]

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

Thanks for accepting my patch :)
Now that this one is complete, I was wondering what should I do next. Is there a list of more such microproject-like projects? I'd be very excited to contribute more patches.


Thanks and regards,
Sidhant Sharma [:tk]

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

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

Git can certainly live without this patch and users won't see any
difference indeed. But the slight code reduction might be worth it:

 builtin/receive-pack.c | 53 +++++++++++++++++++-------------------------------
 1 file changed, 20 insertions(+), 33 deletions(-)

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

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

[Junio C Hamano]

Sidhant Sharma <tigerkid001@gmail.com> writes:

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

Having said that, the resulting code looks more modular in that
adding a new option or extending the behaviour of an existing option
may be easier with the patch going forward, so after all we might
be better off taking it to the production.  I'll think about it a
bit more and decide.

Thanks.

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

[Duy Nguyen]

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

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

[Sidhant Sharma]

> 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.
I can make a patch with the changes, but I think the patch has been finalized.
Should I make one?


Regards,
Sidhant Sharma [:tk]