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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2015, 20:20 UTC
Message-ID: <xmqqtwwhjg8b.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1429117174-4968-1-git-send-email-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 9 quoted lines

>  	case 's':
> -		type = sha1_object_info(sha1, &size);
> -		if (type > 0) {
> -			printf("%lu\n", size);
> -			return 0;
> -		}
> -		break;
> +		oi.sizep = &size;
> +		if ((type = sha1_object_info_extended(sha1, &oi, flags)) < 0)

Do you still need to assign to type here?

> +			die("git cat-file: could not get object info");
> +		printf("%lu\n", size);
> +		return 0;

Show 33 quoted lines

> @@ -323,7 +332,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 (-t [--literally]|-s [--literally]|-e|-p|<type>|--textconv) <object>"),
>  	N_("git cat-file (--batch | --batch-check) < <list-of-objects>"),
>  	NULL
>  };
> @@ -359,6 +368,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;
>  
>  	const struct option options[] = {
>  		OPT_GROUP(N_("<type> can be one of: blob, tree, commit, tag")),
> @@ -369,6 +379,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_("allow -s and -t to work with broken/corrupt objects")),
>  		{ 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 +392,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 < 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.