Hi, Thanks for your patch.
"Sidhant Sharma [:tk]" <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
