ref-filter: add contents:raw atom

Hi ZheNing,

ref-filter.c static struct used_atom { · line 136

Quoted 13 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

ref-filter.c +1 -1

index e2eac50d9508..fc384a194c0c 100644
@@ -133,7 +133,7 @@ static struct used_atom {
                        unsigned int nobracket : 1, push : 1, push_remote : 1;
                } remote_ref;
                struct {
-                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,
+                       enum { C_RAW, C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,

Maybe it's better if you can preserve the alphabetical order?

Thanks.

From: ZheNing Hu <[email protected]>

Add new formatting option %(contents:raw), which will print all the object contents without any changes.

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

Signed-off-by ZheNing Hu <[email protected]>

---
[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

content of the "raw" object for cat-file --batch.

ref-filter.c +2 -1

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

ref-filter.c +6 -3

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

ref-filter.c +1 -1

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

t/t6300-for-each-ref.sh +0 -0

index 9e0214076b4d..baa3a40a70b1 100755
@@ -686,6 +686,17 @@ test_atom refs/tags/signed-empty contents:body ''
 test_atom refs/tags/signed-empty contents:signature "$sig"

Quoted 5 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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?

Quoted 4 lines

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

Signed-off-by ZheNing Hu <[email protected]>

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

ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1323

Quoted 10 lines

ref-filter.c +6 -0

@@ -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 30 lines

ref-filter.c +7 -3

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

ref-filter.c static void fill_missing_values(struct atom_value *val) · line 1410

Quoted 4 lines

ref-filter.c +1 -0

                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.

Quoted 18 lines

ref-filter.c +1 -1

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

t/t6300-for-each-ref.sh +0 -0

index 9e0214076b4d..baa3a40a70b1 100755
@@ -686,6 +686,17 @@ test_atom refs/tags/signed-empty contents:body ''
 test_atom refs/tags/signed-empty contents:signature "$sig"

t/t6300-for-each-ref.sh test_atom refs/tags/signed-empty contents:body '' · line 696

Quoted 10 lines

t/t6300-for-each-ref.sh +8 -0

 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.

t/t6300-for-each-ref.sh test_atom refs/tags/signed-empty contents:body '' · line 698

Quoted 1 line

t/t6300-for-each-ref.sh +1 -0

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

Quoted 12 lines · Christian Couder <[email protected]> 于2021年5月21日周五 上午12:20写道：

Quoted 5 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

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.

Quoted 4 lines

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

Quoted 2 lines

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 9 lines

Quoted 4 lines

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

Signed-off-by ZheNing Hu <[email protected]>

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 48 lines

ref-filter.c static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf) · line 1323

Quoted 10 lines

ref-filter.c +6 -0

@@ -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 30 lines

ref-filter.c +7 -3

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

Quoted 8 lines

ref-filter.c static void fill_missing_values(struct atom_value *val) · line 1410

Quoted 4 lines

ref-filter.c +1 -0

                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 2 lines

Okay, some explanations are indeed missing here:

By the way I forgot to say that your patch might want to also properly document %(contents:raw). If the doc was updated as part of the patch, maybe the whole patch would be easier to understand.

Quoted 1 line

%(contents) will discard the metadata part of the object file,

It's not clear what you mean when you talk about metadata in objects. Are you taking about only the headers, like the "tree XXX", "parent YYY", etc lines in commits, and the "object OOO", "type TTT", etc lines in tags? Or does it also includes the lines in tree objects that reference other trees or blobs?

Quoted 2 lines

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

s/can//

Quoted 1 line

means that it can print the "raw" content of an object.

The above seems to be changing the definition of %(contents) (as previously it was only the commit message of a commit or the tag message a tag) to explain %(contents:raw)...

Quoted 3 lines

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

...but this is saying that the definition of %(contents) actually doesn't change. This doesn't seem logical to me.

If %(contents:raw) can support any kind of object (commit, blob, tag or tree) then it would be logical that %(contents) also support any kind of object.

So if you really want to define %(contents:raw) as the raw object contents, you might want to consider a preparatory patch that would first change the definition of %(contents) to be the object contents without the headers. This would keep the same output for any commit or tag. But for blobs and trees it would print the whole content of the object in the same way as git cat-file -p does, instead of nothing.

I think this acceptable as refs rarely point to something other than commits, so people are not likely to rely on the fact that %(contents) currently prints nothing for blobs and trees.

Quoted 7 lines

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

Ok, I think that would indeed be useful.

Quoted 3 lines

Quoted 2 lines

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

Quoted 4 lines

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.

Yeah, great!

Now we want to use for-each-ref to print a ref point to a tree:

git for-each-ref --format="%(contents:raw)" refs/mytrees/first

will output:

100644 one

but

git cat-file tree refs/mytrees/first

will output:

100644 onem�cֈ��q�D�֧hJ)E-100644 two.t���0�+��VjC��eV�ӈq

which is the compressed data, it may contains '\0'.

Whne we use append_atom() to add the contents of the tree object to the buffer, notice that it uses strbuf_addstr(), the underlying call is strlen(), which truncates the data we added. Can we have any good remedies? For example, record the length of "v->s" by "v->s_size" and use strbuf_addstr(&state->stack->output, v->s, v->s_size)?