On 04/16/2015 02:22 AM, Junio C Hamano wrote:
Junio C Hamano <gitster@xxxxxxxxx> writes:
Karthik Nayak <karthik.188@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html