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

[Junio C Hamano <gitster@pobox.com>]

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

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