On Thu, May 20, 2021 at 10:49 AM ZheNing Hu via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote: > > From: ZheNing Hu <adlternative@xxxxxxxxx> > > Add new formatting option %(contents:raw), which will > print all the object contents without any changes. Maybe you could tell how it would be different from %(contents), or in other words what are the changes that %(contents) makes. Isn't %(contents) only printing the commit message or the tag message of a commit or a tag respectively? If %(contents:raw) would print all the object contents, it could feel strange that it is actually printing more than %(contents). Also is %(contents:raw) supposed to print something for a blob or a tree, while I guess %(contents) is printing nothing for them? > It will help further to migrate all cat-file formatting > logic from cat-file to ref-filter. > > Signed-off-by: ZheNing Hu <adlternative@xxxxxxxxx> It looks like you rewrote the patch nearly completely, but if you based your patch on, or got inspired by, Olga's work, it might be nice to acknowledge this using a trailer (for example "Based-on-patch-by: ..." or "Helped-by:..."). > --- > [GSOC] ref-filter: add contents:raw atom > > Learn from Olga's %(raw): > https://github.com/git/git/pull/568/commits/bf22dae7ca387dbc92c5586c92e60cd395099399 > > We can add a %(contents:raw) atom to ref-filter, which can output object > contents without any change. > > %(contents:raw) can work on the refs which point to blob,tree,commit,tag > objects. > > It also support %(*contents:raw) to dereference. > > With %(cotent:raw), we can later provide support for printing the s/cotent/contents/ > content of the "raw" object for cat-file --batch. > @@ -1292,7 +1294,8 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size > } > > /* See grab_values */ > -static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) > +static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf, > + unsigned long buf_size, enum object_type object_type) > { > int i; > const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL; > @@ -1312,6 +1315,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) > !starts_with(name, "trailers") && > !starts_with(name, "contents")) > continue; > + if (atom->u.contents.option == C_RAW) { > + v->s = xmemdupz(buf, buf_size); > + continue; > + } > + if (object_type != OBJ_TAG && object_type != OBJ_COMMIT) > + continue; When seeing the 2 lines above, I am guessing that, before this patch, grab_sub_body_contents() couldn't actually be called when object_type was OBJ_BLOB or OBJ_TREE, but you have made other changes elsewhere so that now it can. As only the atom->u.contents.option == C_RAW case is relevant in this case, you added this check. Let's see if I am right... > if (!subpos) > find_subpos(buf, > &subpos, &sublen, > @@ -1374,25 +1384,30 @@ static void fill_missing_values(struct atom_value *val) > * pointed at by the ref itself; otherwise it is the object the > * ref (which is a tag) refers to. > */ > -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf) > +static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data) > { > + void *buf = data->content; > + unsigned long buf_size = data->size; > + > switch (obj->type) { > case OBJ_TAG: > grab_tag_values(val, deref, obj); > - grab_sub_body_contents(val, deref, buf); > + grab_sub_body_contents(val, deref, buf, buf_size, obj->type); > grab_person("tagger", val, deref, buf); > break; > case OBJ_COMMIT: > grab_commit_values(val, deref, obj); > - grab_sub_body_contents(val, deref, buf); > + grab_sub_body_contents(val, deref, buf, buf_size, obj->type); > grab_person("author", val, deref, buf); > grab_person("committer", val, deref, buf); > break; > case OBJ_TREE: > /* grab_tree_values(val, deref, obj, buf, sz); */ > + grab_sub_body_contents(val, deref, buf, buf_size, obj->type); > break; > case OBJ_BLOB: > /* grab_blob_values(val, deref, obj, buf, sz); */ > + grab_sub_body_contents(val, deref, buf, buf_size, obj->type); ...ok, I was right above. The issue now is that I wonder if grab_sub_body_contents() is still a good name for a function that can be called for a blob or a tree which does not really have a body. > break; > default: > die("Eh? Object of type %d?", obj->type); > @@ -1614,7 +1629,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj > return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"), > oid_to_hex(&oi->oid), ref->refname); > } > - grab_values(ref->value, deref, *obj, oi->content); > + grab_values(ref->value, deref, *obj, oi); > } > > grab_common_values(ref->value, deref, oi); > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh > index 9e0214076b4d..baa3a40a70b1 100755 > --- a/t/t6300-for-each-ref.sh > +++ b/t/t6300-for-each-ref.sh > @@ -686,6 +686,17 @@ test_atom refs/tags/signed-empty contents:body '' > test_atom refs/tags/signed-empty contents:signature "$sig" > test_atom refs/tags/signed-empty contents "$sig" > > +test_expect_success 'basic atom: refs/tags/signed-empty contents:raw' ' > + git cat-file tag refs/tags/signed-empty >expected && > + git for-each-ref --format="%(contents:raw)" refs/tags/signed-empty >actual && > + sanitize_pgp <expected >expected.clean && > + sanitize_pgp <actual >actual.clean && > + echo "" >>expected.clean && > + test_cmp expected.clean actual.clean > +' For an empty tag %(contents:raw) should produce nothing, ok. > +test_atom refs/tags/signed-empty *contents:raw $(git cat-file commit HEAD) Maybe use single quotes around *contents:raw? The rest looks good to me. Thanks!
