Re: [PATCH 8/8] [GSOC] cat-file: re-implement --textconv, --filters options
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年6月17日周四 下午3:22写道：
Show 16 quoted lines
>
>
> 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.
>
Show 33 quoted lines
> > + /* 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) {} ?
>Maybe it's something like this:
@@ -1762,19 +1762,17 @@ static int get_object(struct ref_array_item
*ref, int deref, struct object **obj
oi->info.typep = NULL;
oi->info.contentp = temp_contentp;
- if (use_textconv) {
- act_oi = *oi;
+ if (use_textconv && !ref->rest)
+ return strbuf_addf_ret(err, -1, _("missing
path for '%s'"),
+ oid_to_hex(&act_oi.oid));
- 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;
- }
+ if (use_textconv && oi->type == OBJ_BLOB) {
+ act_oi = *oi;
+ 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;
}
}
}
> > }
> > 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.
>
And this part:
@@ -1786,25 +1784,21 @@ 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) {
- 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);
- }
- }
+ if (use_filters && !ref->rest)
+ return strbuf_addf_ret(err, -1, _("missing
path for '%s'"),
+ oid_to_hex(&oi->oid));
+ if (use_filters && 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))
+ die("could not convert '%s' %s",
+ oid_to_hex(&oi->oid), ref->rest);
+ act_oi.size = strbuf.len;
+ act_oi.content = strbuf_detach(&strbuf, NULL);
+ actual_oi = &act_oi;
}
>
> > -#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 }
>
I agree, this method is better. We don't need to care about which members
so many "0" point to.
Thanks.
--
ZheNing Hu
ZheNing Hu <adlternative@gmail.com>