Re: [PATCH] Simplified the invocation of command action in submodule

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2008, 10:27 UTC
Message-ID: <7vsl17i99r.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <47849B60.2060000@viscovery.net>

Johannes Sixt <j.sixt@viscovery.net> writes:

Show 16 quoted lines

> Imran M Yousuf schrieb:
>> On Jan 9, 2008 2:59 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>> - Previously, passing --cached to add, init, or update was an error, now
>>> it is not.
>> 
>> The usage statement and this behaviour is rather contradicting. The
>> usage says that --cached can be used with all commands; so I am not
>> sure whether using --cached with add should be an error or not. IMHO,
>> if the previous implementation was right than the USAGE has to be
>> changed, and if the previous implementation was incorrect, than if the
>> default command is set to status than current implementation is right.
>
> I prefer that the usage statement lists one line per sub-command with the
> flags that apply only to the sub-command. IOW, a usage statement that
> suggests that a flag applies to all sub-commands when in reality it
> doesn't is bogus, IMHO.

I view the usage emitted by a command primarily as a quick reminder for people who are _already_ familiar with the command to help "was the option this command takes --foo or --bar? I can never remember which X-<" situation. The usage string is not a replacement of the manual page. For that reason, I generally prefer short and sweet one line usage for the whole command, even if it does not exactly capture mutually incompatible option combinations, _as long as_ the command itself is simple enough.

As you said, however, git-submodule is a command dispatcher on its own, and what its subcommands do are quite different, to the point that they probably should not even be sharing the option parser. One line per subcommand feels more appropriate.

By the way, Imran, if the current implementation declares a combination of "add" and "--cached" an error, and a new implementation does not, that's called a regression. Unless you can prove that the combination makes sense and the existing behaviour is a bug, in which case you can say the new implementation fixes the bug.

In this case, module_add does not even pay attention to $cached in the existing code. The choice is between (1) silently ignore user's expectation that "add --cached" would do something different from "add" without "--cached", or (2) tell the user that the combination does not make sense and error out. To people who _know_ what the command does, the choice between the two does not make much difference (they do not give ignored option, nor trigger the error), but to new people the latter is often easier to use.