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