From: karthik nayak Date: Thu, 16 Apr 2015 07:26:33 GMT Subject: Re: [PATCH v8 2/4] cat-file: teach cat-file a '--literally' option Message-ID: <552F6429.9050304@gmail.com> In-Reply-To: On 04/16/2015 02:22 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Karthik Nayak 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_(" 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.