From: Matthieu Moy Date: Tue, 01 Mar 2016 17:57:30 GMT Subject: Re: [PATCH] builtin/receive-pack.c: use parse_options API Message-ID: In-Reply-To: <56D5D601.8030601@gmail.com> Sidhant Sharma 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/