patchtag: Use OPT_BOOL instead of OPT_BOOLEAN to allow one action multiple times

As of b04ba2bb (parse-options: deprecate OPT_BOOLEAN, 2011-09-27), the OPT_BOOLEAN was deprecated. While I am going to replace the OPT_BOOLEAN by the proposed OPT_BOOL or the OPT_COUNTUP to keep existing behavior, this commit is actually a bug fix!

In line 499 we have:
	if (list + delete + verify > 1)
		usage_with_options(git_tag_usage, options);
Now if we give one of the options twice, we'll get the usage information.
(i.e. 'git tag --verify --verify <tagname>' and
'git --delete --delete <tagname>' yield usage information and do not
do the intended command.)

This could have been fixed by rewriting the line to
	if (!!list + !!delete + !!verify > 1)
		usage_with_options(git_tag_usage, options);
or as it happened in this patch by having the parameters not
counting up for each occurrence, but the OPT_BOOL just setting the
variables to either 0 if the option is not given or 1 if the option is
given multiple times.

However we could discuss if the negated options do make sense here, or if
we don't want to allow them here, as this seems valid (before and after
this patch):
	git tag --no-verify --delete <tagname>

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/tag.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Stefan Beller <stefanbeller@googlemail.com> writes:

Makes twisted sort of sense ;-).

It probably does not. As you hinted in your earlier patch, we may want to introduce a "only can set to true" boolean used solely to specify these things. They are disguised as "options", but are in fact command operation modes that are often mutually exclusive.

For these operation modes that are mutually exclusive, there are multiple possible implementations:

 * One OPT_BOOL_NONEG per option; the code ensures the mutual
   exclusion with "if (list + delete + verify > 1)";

 * One OPT_BIT per option in a single variable; the code ensures the
   mutual exclusion with count_bits, which may be a lot more
   cumbersome;

 * OPT_SET_INT that updates a single variable to enum; instead of
   making it an error to give two conflicting modes, this would give
   us the last-one-wins rule.

Unlike usual "options", we generally do not want the last-one-wins semantics for command operation modes, I think.

Perhaps we would want something like this?

-- >8 --
Subject: [PATCH] parse-options: add OPT_CMDMODE()

This can be used to define a set of mutually exclusive "command mode" options, and automatically catch use of more than one from that set as an error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 parse-options.h |  3 +++
 2 files changed, 56 insertions(+), 5 deletions(-)

On 07/30/13 21:24, Junio C Hamano wrote:

Your approach seems more like what we really want, however I'd have
some points:
 * Is it a good idea to have so many different OPT_MODE or
   OPTION_MODE defines? In my attempts I tried to reuse existing
   OPTION_s to not pollute the parsing infrastructure with more
   lines of code. ;)

 * You can only have one OPTION_CMDMODE in one argv vector right?
   I searched through the commands (... > 1) and did not find any
   places, where we'd want to have multiple 'groups' of exclusive 
   commands, such as (either A or B) and/or (either C or D)
   This cmd_mode would just all a (either A, B, C or D), but that
   should be good for now.

 * This command mode could also be used for builtin/branch:
	if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)
		usage_with_options(builtin_branch_usage, options);  
   as well as commit:
	if (!!also + !!only + !!all + !!interactive > 1)
		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
   as well as for checkout:
	if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
		die(_("-b, -B and --orphan are mutually exclusive"));
   So if we'd introduce this command mode, I'd be happy to supply patches
   for branch, commit and checkout to use the new exclusive mechanism.
 
So I think I like it.
Reviewed-by: Stefan Beller <stefanbeller@googlemail.com>

Stefan Beller <stefanbeller@googlemail.com> writes:

That is not what I intended, at least.

	int one = 0, two = 0;
	struct option options[] = {
        OPT_CMDMODE('a', NULL, &one, N_("set one to a"), 'a'),
        OPT_CMDMODE('b', NULL, &one, N_("set one to b"), 'b'),
        OPT_CMDMODE('c', NULL, &two, N_("set two to c"), 'c'),
        OPT_CMDMODE('d', NULL, &two, N_("set two to d"), 'd'),
        OPT_END()
        }

should give you two independent sets of modes, one and two.

The only reason I needed to add an extra parameter to get_value() was so that I can tell the former two and the latter two belong to different groups, and that is done by looking at the address of the variable. In opt_command_mode_error(), opt->value == that->value is used as a condition to see if the other option is possibly the one that was used previously, which conflicted with us.

On 07/30/13 21:24, Junio C Hamano wrote:

Here is just another idea: 
	if (cmdmode == 'v')
This may be hard to read, (What is 'v'? I cannot remember 
all the alphabet ;)) So maybe we could have an enum instead of
the last parameter? 
OPT_CMDMODE( short, long, variable, description, enum)

Also the variable would then only need to be an enum accepting variable, and not an integer accepting all integer range, so we'd also catch typos or wrong values in such a case:

Stefan Beller <stefanbeller@googlemail.com> writes:

I actually was thinking about going totally in the opposite direction.

People who grew up with the old Unix tradition getopt(3) are used to code like this:

        while ((opt = getopt(ac, av, "abcde")) != -1) {
                switch (opt) {
                case 'a': perform_a(); break;
                case 'b': perform_b(); break;
                ...
                }
        }

In other words, the "enum" is most convenient if it matches the "short" option, so instead of having to repeat ourselves over and over, like I did in that illustration patch for builtin/tag.c, e.g.

		OPT_CMDMODE('d', "delete", &cmdmode, N_("delete tags"), 'd'),
		OPT_CMDMODE('v', "verify", &cmdmode, N_("verify tags"), 'v'),

we could just do

#define OPT_CMDMODE(s, l, v, h) \
    { OPTION_CMDMODE, (s), (l), (v), NULL, \
      (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }

On 07/31/13 00:28, Junio C Hamano wrote:

I agree that's a better proposal than mine.

Stefan Beller <stefanbeller@googlemail.com> writes:

By the way, I haven't convinced myself that it is a good idea in general to encourage more use of command mode options, so I am a bit reluctant to add this before knowing which direction in the longer term we are going.

 - Some large-ish Git subcommands, like "git submodule", use the
   mode word (e.g. "git submodule status") to specify the operation
   mode (youe could consider "status" a subsubcommand that
   "submodule" subcommand takes).  These commands typically began
   their life from day one with the mode words.

 - On the other hand, many Git subcommands, like "git tag", have
   "the primary operation mode" (e.g. "create a new one" is the
   primary operation mode for "git tag"), and use command mode
   options to specify other operation modes (e.g. "--delete").
   These commands started as single purpose commands (i.e. to
   perform their "primary operation") but have organically grown
   over time and acquired command mode options to invoke their
   secondary operations.

As an end user, you need to learn which style each command takes, which is an unnecessary burden at the UI level. In the longer term, we may want to consider picking a single style, and migrating everybody to it. If I have to vote today, I would say we should teach "git submodule" to also take command mode options (e.g. "git submodule --status" will be understood the same way as "git submodule status"), make them issue warnings when mode words are used and encourage users to use command mode options instead, and optionally remove the support of mode words at a large version bump like 3.0.

One clear advantage mode words have over command mode options is that there is no room for end user confusion. The first word after "git subcmd" is the mode word, and you will not even dream of asking "what would 'git submodule add del foo' do?" as it is nonsensical. The command mode options, on the other hand, gives too much useless flexibility to ask for nonsense, e.g. "git tag --delete --verify", "git tag --no-delete --delete", etc., and extra code needs to detect and reject combinations. But commands that took mode options cannot be easily migrated to take mode words without hurting existing users and scripts (e.g. "git tag delete master" can never be a request to delete the tag 'master', as it is a request to create a tag whose name is 'delete' that points at the same object as 'master' points at).

(Going through old mail today, sorry for the late reply)

On 08/01/2013 12:10 AM, Junio C Hamano wrote:> Stefan Beller 
<stefanbeller@googlemail.com> writes:
 >
 >> On 07/31/13 00:28, Junio C Hamano wrote:
 >>>
 >>> we could just do
 >>>
 >>> #define OPT_CMDMODE(s, l, v, h) \
 >>>      { OPTION_CMDMODE, (s), (l), (v), NULL, \
 >>>        (h), PARSE_OPT_NOARG|PARSE_OPT_NONEG, NULL, (s) }
 >>>
 >>
 >> I agree that's a better proposal than mine.
 >
 > By the way, I haven't convinced myself that it is a good idea in
 > general to encourage more use of command mode options, so I am a bit
 > reluctant to add this before knowing which direction in the longer
 > term we are going.
 >
 >   - Some large-ish Git subcommands, like "git submodule", use the
 >     mode word (e.g. "git submodule status") to specify the operation
 >     mode (youe could consider "status" a subsubcommand that
 >     "submodule" subcommand takes).  These commands typically began
 >     their life from day one with the mode words.
 >
 >   - On the other hand, many Git subcommands, like "git tag", have
 >     "the primary operation mode" (e.g. "create a new one" is the
 >     primary operation mode for "git tag"), and use command mode
 >     options to specify other operation modes (e.g. "--delete").
 >     These commands started as single purpose commands (i.e. to
 >     perform their "primary operation") but have organically grown
 >     over time and acquired command mode options to invoke their
 >     secondary operations.
 >
 > As an end user, you need to learn which style each command takes,
 > which is an unnecessary burden at the UI level.  In the longer term,
 > we may want to consider picking a single style, and migrating
 > everybody to it.  If I have to vote today, I would say we should
 > teach "git submodule" to also take command mode options (e.g. "git
 > submodule --status" will be understood the same way as "git
 > submodule status"), make them issue warnings when mode words are
 > used and encourage users to use command mode options instead, and
 > optionally remove the support of mode words at a large version bump
 > like 3.0.
 >
 > One clear advantage mode words have over command mode options is
 > that there is no room for end user confusion.  The first word after
 > "git subcmd" is the mode word, and you will not even dream of asking
 > "what would 'git submodule add del foo' do?" as it is nonsensical.
 > The command mode options, on the other hand, gives too much useless
 > flexibility to ask for nonsense, e.g. "git tag --delete --verify",
 > "git tag --no-delete --delete", etc., and extra code needs to detect
 > and reject combinations.  But commands that took mode options cannot
 > be easily migrated to take mode words without hurting existing users
 > and scripts (e.g. "git tag delete master" can never be a request to
 > delete the tag 'master', as it is a request to create a tag whose
 > name is 'delete' that points at the same object as 'master' points
 > at).
 >
Why not encourage the use of a standardized '--action' option instead?
This can work with lesser compatibility headaches for both the commands
taking mode options and the commands taking mode words:

   "git submodule init"   becomes  "git submodule --action=init"
   "git tag --delete TAG" becomes  "git tag --action=delete TAGNAME"

Commands that have a "primary operation mode" can keep the use of --action optional, while commands that have no such sensible primary mode can make it mandatory (again, this gives more compatibility with the existing syntax). And the old syntax can be supported in parallel to the new one for a potentially indefinite time; albeit, if I were to propose a timetable, I'd got for:

   - Git 2.0: the new syntax is introduced
   - Git 2.x: any use the old syntax starts to trigger warnings
     (which can be silenced with a config option)
   - Git 3.0: any use the old syntax starts to trigger fatal
     errors (which can be turned into mandatory warnings with
     a config option)
   - Git 4.0: any use the old syntax starts to trigger
     mandatory fatal errors.
   - Git 4.x: Remove any handling of the old syntax.

Just my 2 cents,
   Stefano

Stefano Lattarini wrote:

Because it's an unpleasant UI. :)

That looks like a bad change in both cases --- it involves more typing without much upside to go along with it. But

	"git submodule init"   gains synonym "git submodule --init"
	"git tag --delete TAG" stays as      "git tag --delete TAG"

looks fine to me.

In the long run, we could require that for new commands the 'action' option must come immediately after the git command name if that makes things easier to learn.

Thanks for some food for thought.

My two cents, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

I agree 100% with the above that illustrates why --action=<name> is a bad idea. As I already said, adding action-option like --init, if doing so might help people, I am not opposed to it.

Thanks.