Re: [PATCH 2/8] tag: Refactor the options handling code to be less bizarro

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/builtin/tag.c b/builtin/tag.c
> index ad29be6923..0bba3fd070 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -454,10 +454,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	}
>  	create_tag_object = (opt.sign || annotate || msg.given || msgfile);
>  
> -	if (argc == 0 && !cmdmode)
> +	if (argc == 0 && !cmdmode && !create_tag_object)
>  		cmdmode = 'l';

So with this change, if we cannot infer that we are creating a tag
object from other options, we leave cmdmode to its original 0.

> -	if ((create_tag_object || force) && (cmdmode != 0))
> +	if ((create_tag_object || force) && (cmdmode || (!cmdmode && !argc)))
>  		usage_with_options(git_tag_usage, options);

And then immediately after that, we complain by detecting that we
know we are creating a tag and a non-zero cmdmode is in effect
(i.e. 'l', 'd' or 'v', none of which is about creating a tag).  The
way we used to detect that we are doing something other than tag
creation was by seeing cmdmode is set to anything.  Because of your
earlier change, that no longer is true.  You need to separately
check (!cmdmode && !argc).

By following the logic that way, I can see how this change at this
step is a no-op, but I have to say that the code with this patch
looks much more bizarre than the original.

I am not sure why you want to do the first change at this step in
the first place.  Is it because you'd want to take over (!cmdmode &&
!argc) condtion to default to 'list'?  With the change in 4/8 and
5/8, you are ensuring that cmdmode is set to 'l' for these new cases
before the code hits the check to call usage_with_options().  And at
that point, you can use the original "are we creating and !cmdmode
says we are not?  That's contradiction" logic without making it more
bizarre with this patch, no?