Hi,
Thanks for your patch.
"Sidhant Sharma [:tk]" <tigerkid001@xxxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html