Re: [PATCH 2/2] cat-file: add --literally option.
On Wed, Feb 25, 2015 at 6:08 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> cat-file: add --literally option.
Style: drop terminating period.
> Add --literally option which when used with -t option
> gives the type of the object given, irrelevant of the type
> and its contents.
Confusing: "gives the type [...] irrelevant of the type"?
Show 11 quoted lines
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index df99df4..1db94fe 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -107,6 +107,29 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> return 0;
> }
>
> +static int cat_one_file_literally(int opt, const char *exp_type, const char *obj_name)
Unused arguments: 'opt' and 'exp_type'
This function duplicates enough functionality of cat_one_file() that it almost seems that it could be folded into the 't' case in that function, but for the sake of review, let's assume that you'll keep it separate.
Show 5 quoted lines
> +{
> + unsigned char sha1[20];
> + unsigned char type[32];
> + struct object_context obj_context;
> + struct object_info oi = {NULL};> + int retval = 0;
> +
> + if (get_sha1_with_context(obj_name, 0, sha1, &obj_context))
You never take advantage of the extra context in 'obj_context', so perhaps get_sha1() would be preferable?
Show 5 quoted lines
> + die("Not a valid object name %s", obj_name);
> +
> + retval = has_sha1_file(sha1);
> + if (!retval)
> + return retval;Shouldn't this case be emitting some sort of diagnostic rather than exiting silently?
> + if(sha1_object_type_literally(sha1, type))
> + die("git cat-file -t --literally %s: invalid object", obj_name);Inconsistent error message style with the die() just above this one. (cat_one_file() has its own inconsistencies, but new code need not follow suit.)
Show 15 quoted lines
> +
> + printf("%s\n", type);
> +
> + return retval;
> +}
> +
> struct expand_data {
> unsigned char sha1[20];
> enum object_type type;
> @@ -324,7 +347,7 @@ static int batch_objects(struct batch_options *opt)
>
> static const char * const cat_file_usage[] = {
> N_("git cat-file (-t | -s | -e | -p | <type> | --textconv) <object>"),
> - N_("git cat-file (--batch | --batch-check) < <list-of-objects>"),
> + N_("git cat-file (-t|-s|-e|-p|<type>|--textconv|-t --literally) <object>"),This is strange. According to this diff, you've removed the --batch and --batch-check options; and you're showing the -t, -s, -e, etc. options twice.
Show 8 quoted lines
> NULL
> };
>
> @@ -359,6 +382,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
> int opt = 0;
> const char *exp_type = NULL, *obj_name = NULL;
> struct batch_options batch = {0};
> + int literally = 0;Mental note: 'literally' is initialized to false (0).
Show 8 quoted lines
> const struct option options[] = {
> OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
> @@ -369,6 +393,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
> OPT_SET_INT('p', NULL, &opt, N_("pretty-print object's content"), 'p'),
> OPT_SET_INT(0, "textconv", &opt,
> N_("for blob objects, run textconv on object's content"), 'c'),
> + OPT_BOOL( 0, "literally", &literally,
> + N_("show the type of the gicen loose object, use for debugging")),Show 9 quoted lines
> { OPTION_CALLBACK, 0, "batch", &batch, "format",
> N_("show info and content of objects fed from the standard input"),
> PARSE_OPT_OPTARG, batch_option_callback },
> @@ -380,7 +406,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
>
> git_config(git_cat_file_config, NULL);
>
> - if (argc != 3 && argc != 2)
> + if (argc != 3 && argc != 2 && (!literally && argc != 4))This can't be correct. At this point, 'literally' is unconditionally false (0). It's value won't (potentially) change until the options are processed, which doesn't happen until parse_options() is invoked _after_ this conditional.
Show 16 quoted lines
> usage_with_options(cat_file_usage, options);
>
> argc = parse_options(argc, argv, prefix, options, cat_file_usage, 0);
> @@ -405,5 +431,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)
> if (batch.enabled)
> return batch_objects(&batch);
>
> + if (literally && opt == 't')
> + return cat_one_file_literally(opt, exp_type, obj_name);
> + else if (literally)
> + usage_with_options(cat_file_usage, options);
> +
> return cat_one_file(opt, exp_type, obj_name);
> }
> --
> 2.3.1.129.g11acff1.dirty
karthik nayak
