Junio C Hamano <gits...@pobox.com> writes:
> Karthik Nayak <karthik....@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.
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) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
