Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option

[karthik nayak <karthik.188@gmail.com>]

On 04/16/2015 02:22 AM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>> ...
>>> -	if (argc != 3 && argc != 2)
>>> +	if (argc < 2 || argc > 4)
>>>   		usage_with_options(cat_file_usage, options);
>>
>> Hmm, why this change?
>>
>> By allowing 4 args blindly like this, you will let something like
>> these to pass:
>>
>>      $ git cat-file --textconv -t HEAD
>>      commit
>>      $ git cat-file -p -t HEAD
>>      commit
>>      $ git cat-file -s -t HEAD
>>      commit
>>      $ git cat-file -t -s HEAD
>>      879
>>
>> It is fine if you are declaring that the last one wins when these
>> mutually-exclusive options are given. "git cat-file -e -s -t HEAD"
>> should also say "commit" if that is what you are doing, but instead
>> the code with this patch complains, which is bad.
>>
>> It also is fine and is more in line with the spirit of the original
>> code if you keep the rule tight and only one of these mutually
>> exclusive options is allowed.
>>
>> In either case, this check needs to be rethought.
>
> I wonder if we want to do something like this as a preliminary step
> before your [PATCH 2/4].  Everything after the parse_options() call
> seems to protect against leftover argc depending on what they need
> already, so the only thing existing "we take only 2 or 3 args" check
> is doing is to protect against giving more than one command mode
> options, I think.  And OPT_CMDMODE() should do a much better job at
> that that kind of thing, by giving a more informative error message
> e.g.
>
>      $ git cat-file -p -e HEAD
>      error: switch 'e': incompatible with -p
>
> This is a tangent, but while we are in the vicinity, we may want to
> rethink the help message we attach to the '-e' option.  Technically
> the current message is _not_ wrong per-se, but it misses the point.
> The primary thing the option does is to check the (e)xistence of the
> named object, and the fact that it does so silently is merely a
> detail of the operation.  The current help text omits the more
> important part of what the option is.

Would you rather check '-e' and go on to check '-p' or do you merely 
just want a different message.
As far as I see whenever any option other than a '-e' is given it 
indirectly has to check for the existence of the object.

eg:
$ git cat-file -t asdfghjkl
fatal: Not a valid object name asdfghjkl

$ git cat-file -e asdfghjkl
fatal: Not a valid object name asdfghjkl

So when a user is giving the '-e' option he just expects a silent output 
if the object exists, hence we rather have the option '-e' behave as a 
mutually exclusive option and output the error message like.

$ git cat-file -p -e HEAD
error: switch 'e': incompatible with -p

what do you think?
>
>   builtin/cat-file.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 1ec7190..534991d 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -372,12 +372,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>
>   	const struct option options[] = {
>   		OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
> -		OPT_SET_INT('t', NULL, &opt, N_("show object type"), 't'),
> -		OPT_SET_INT('s', NULL, &opt, N_("show object size"), 's'),
> -		OPT_SET_INT('e', NULL, &opt,
> +		OPT_CMDMODE('t', NULL, &opt, N_("show object type"), 't'),
> +		OPT_CMDMODE('s', NULL, &opt, N_("show object size"), 's'),
> +		OPT_CMDMODE('e', NULL, &opt,
>   			    N_("exit with zero when there's no error"), 'e'),
> -		OPT_SET_INT('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
> -		OPT_SET_INT(0, "textconv", &opt,
> +		OPT_CMDMODE('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
> +		OPT_CMDMODE(0, "textconv", &opt,
>   			    N_("for blob objects, run textconv on object's content"), 'c'),
>   		OPT_BOOL( 0, "literally", &literally,
>   			  N_("allow -s and -t to work with broken/corrupt objects")),
> @@ -392,9 +392,6 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>
>   	git_config(git_cat_file_config, NULL);
>
> -	if (argc < 2 || argc > 4)
> -		usage_with_options(cat_file_usage, options);
> -
>   	argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
>
>   	if (opt) {
>
Thanks! Didn't know about the OPT_CMDMODE option.