Re: [PATCH 8/8] [GSOC] cat-file: re-implement --textconv, --filters options

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Jun 12 2021, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <adlternative@gmail.com>

>  	opt->format.format = format.buf;
> +	if (opt->cmdmode == 'c')
> +		opt->format.use_textconv = 1;
> +	if (opt->cmdmode == 'w')
> +		opt->format.use_filters = 1;
> +


Style nit: if/if -> if/else if, both can't be true.

> +		/* get the type and size */
> +		if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
> +					OBJECT_INFO_LOOKUP_REPLACE))
> +			return strbuf_addf_ret(err, 1, _("%s missing"),
> +					       oid_to_hex(&oi->oid));
> +
> +		oi->info.sizep = NULL;
> +		oi->info.typep = NULL;
> +		oi->info.contentp = temp_contentp;

Here we have a call function and error if bad, then proceed to populate
stuff (good)...

> +
> +		if (use_textconv) {
> +			act_oi = *oi;
> +
> +			if(!ref->rest)
> +				return strbuf_addf_ret(err, -1, _("missing path for '%s'"),
> +						       oid_to_hex(&act_oi.oid));
> +			if (act_oi.type == OBJ_BLOB) {
> +				if (textconv_object(the_repository,
> +						    ref->rest, 0100644, &act_oi.oid,
> +						    1, (char **)(&act_oi.content), &act_oi.size)) {
> +					actual_oi = &act_oi;
> +					goto success;
> +				}
> +			}
> +		}


Maybe change the if (A) { if (B) {} ) to if (A && B) {} ?

>  	}
>  	if (oid_object_info_extended(the_repository, &oi->oid, &oi->info,
>  				     OBJECT_INFO_LOOKUP_REPLACE))
> @@ -1748,19 +1786,43 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
>  		BUG("Object size is less than zero.");
>  
>  	if (oi->info.contentp) {
> -		*obj = parse_object_buffer(the_repository, &oi->oid, oi->type, oi->size, oi->content, &eaten);
> +		if (use_filters) {
> +			if(!ref->rest)
> +				return strbuf_addf_ret(err, -1, _("missing path for '%s'"),
> +						       oid_to_hex(&oi->oid));
> +			if (oi->type == OBJ_BLOB) {
> +				struct strbuf strbuf = STRBUF_INIT;
> +				struct checkout_metadata meta;
> +				act_oi = *oi;
> +
> +				init_checkout_metadata(&meta, NULL, NULL, &act_oi.oid);
> +				if (convert_to_working_tree(&the_index, ref->rest, act_oi.content, act_oi.size, &strbuf, &meta)) {
> +					act_oi.size = strbuf.len;
> +					act_oi.content = strbuf_detach(&strbuf, NULL);
> +					actual_oi = &act_oi;
> +				} else {
> +					die("could not convert '%s' %s",
> +					    oid_to_hex(&oi->oid), ref->rest);
> +				}

... but here instead of "if (!x) { bad } do stuff" we have if (x) {do
stuff} else { bad }". Better to get the die out of the way, and avoid
the indentation on the "do stuff" IMO.


> -#define REF_FORMAT_INIT { NULL, NULL, 0, 0, 0, -1 }
> +#define REF_FORMAT_INIT { NULL, NULL, 0, 0, 0, 0, 0, -1 }

Not a new problem, but an earlier cleanup to simply change this to
designated initializers would be welcome, see recent work of mine in
fsck.h for an example.

I.e. we keep churning on changing this *_INIT just to populate the one
-1 field at the end, can also be simply:

    #define FOO_INIT { .that_field = 1 }