Re: [PATCH] [GSOC] ref-filter: add contents:raw atom

Christian Couder [off-list ref] 于2021年5月21日周五 上午12:20写道：

On Thu, May 20, 2021 at 10:49 AM ZheNing Hu via GitGitGadget
[off-list ref] wrote:

quoted

From: ZheNing Hu <redacted>

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).

Okay, some explanations are indeed missing here:

%(contents) will discard the metadata part of the object file,
and only print the data contents part of it. %(contents:raw)
can will not discard the metadata part of the object file, this
 means that it can print the "raw" content of an object.

In addition, %(contents:raw) can support print commit, blob,
tag, tree objects contents which %(contents) can only support
commit,tag objects.

E.g:

git for-each-ref --format=%(contents:raw) refs/heads/foo

will have the same output as:

git rev-parse refs/heads/foo | git cat-file --batch

Also is %(contents:raw) supposed to print something for a blob or a
tree, while I guess %(contents) is printing nothing for them?

Now my thoughts are:
Let %(contents) learn to print four kinds of objects.
and then let %(contents:raw) learn to print metadata.
I will split it into two patches.

quoted

It will help further to migrate all cat-file formatting
logic from cat-file to ref-filter.

Signed-off-by: ZheNing Hu <redacted>

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:...").

Okay, "Based-on-patch-by" would be more appropriate here.

quoted

@@ -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...

quoted

                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.

Makes sense, It might be better to use the new name: grab_contents().

quoted

                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.

quoted

+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!

Good suggestion. thank!
--
ZheNing Hu