{"thread":{"id":"55745","subject":"[PATCH] [GSOC] ref-filter: add contents:raw atom","startedAt":"2021-05-20T08:49:57Z","lastAt":"2021-05-21T13:39:51Z","messageCount":8,"participants":["ZheNing Hu via GitGitGadget","Hariom verma","ZheNing Hu","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"425074","messageId":"pull.958.git.1621500593126.gitgitgadget@gmail.com","threadId":"55745","inReplyTo":null,"subject":"[PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-20T08:49:52Z","receivedAt":"2021-05-20T08:49:57Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu <adlternative@gmail.com>\n\nAdd new formatting option %(contents:raw), which will\nprint all the object contents without any changes.\nIt will help further to migrate all cat-file formatting\nlogic from cat-file to ref-filter.\n\nSigned-off-by: ZheNing Hu <adlternative@gmail.com>\n---\n    [GSOC] ref-filter: add contents:raw atom\n    \n    Learn from Olga's %(raw):\n    https://github.com/git/git/pull/568/commits/bf22dae7ca387dbc92c5586c92e60cd395099399\n    \n    We can add a %(contents:raw) atom to ref-filter, which can output object\n    contents without any change.\n    \n    %(contents:raw) can work on the refs which point to blob,tree,commit,tag\n    objects.\n    \n    It also support %(*contents:raw) to dereference.\n    \n    With %(cotent:raw), we can later provide support for printing the\n    content of the \"raw\" object for cat-file --batch.\n    \n    Why not just use Olga's %(raw)? Because %(contents) can output object\n    contents already, we can reuse it, instead of using another atom %(raw).\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-958%2Fadlternative%2Fref-filter-raw-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-958/adlternative/ref-filter-raw-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/958\n\n ref-filter.c            | 27 +++++++++++++++++++++------\n t/t6300-for-each-ref.sh | 31 +++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e2eac50d9508..fc384a194c0c 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -133,7 +133,7 @@ static struct used_atom {\n \t\t\tunsigned int nobracket : 1, push : 1, push_remote : 1;\n \t\t} remote_ref;\n \t\tstruct {\n-\t\t\tenum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n+\t\t\tenum { C_RAW, C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n \t\t\t       C_SIG, C_SUB, C_SUB_SANITIZE, C_TRAILERS } option;\n \t\t\tstruct process_trailer_options trailer_opts;\n \t\t\tunsigned int nlines;\n@@ -347,6 +347,8 @@ static int contents_atom_parser(const struct ref_format *format, struct used_ato\n {\n \tif (!arg)\n \t\tatom->u.contents.option = C_BARE;\n+\telse if (!strcmp(arg, \"raw\"))\n+\t\tatom->u.contents.option = C_RAW;\n \telse if (!strcmp(arg, \"body\"))\n \t\tatom->u.contents.option = C_BODY;\n \telse if (!strcmp(arg, \"size\"))\n@@ -1292,7 +1294,8 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n }\n \n /* See grab_values */\n-static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n+static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf,\n+\t\t\t\t   unsigned long buf_size, enum object_type object_type)\n {\n \tint i;\n \tconst char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;\n@@ -1312,6 +1315,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n \t\t    !starts_with(name, \"trailers\") &&\n \t\t    !starts_with(name, \"contents\"))\n \t\t\tcontinue;\n+\t\tif (atom->u.contents.option == C_RAW) {\n+\t\t\tv->s = xmemdupz(buf, buf_size);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (object_type != OBJ_TAG && object_type != OBJ_COMMIT)\n+\t\t\tcontinue;\n+\n \t\tif (!subpos)\n \t\t\tfind_subpos(buf,\n \t\t\t\t    &subpos, &sublen,\n@@ -1374,25 +1384,30 @@ static void fill_missing_values(struct atom_value *val)\n  * pointed at by the ref itself; otherwise it is the object the\n  * ref (which is a tag) refers to.\n  */\n-static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)\n+static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data)\n {\n+\tvoid *buf = data->content;\n+\tunsigned long buf_size = data->size;\n+\n \tswitch (obj->type) {\n \tcase OBJ_TAG:\n \t\tgrab_tag_values(val, deref, obj);\n-\t\tgrab_sub_body_contents(val, deref, buf);\n+\t\tgrab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n \t\tgrab_person(\"tagger\", val, deref, buf);\n \t\tbreak;\n \tcase OBJ_COMMIT:\n \t\tgrab_commit_values(val, deref, obj);\n-\t\tgrab_sub_body_contents(val, deref, buf);\n+\t\tgrab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n \t\tgrab_person(\"author\", val, deref, buf);\n \t\tgrab_person(\"committer\", val, deref, buf);\n \t\tbreak;\n \tcase OBJ_TREE:\n \t\t/* grab_tree_values(val, deref, obj, buf, sz); */\n+\t\tgrab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n \t\tbreak;\n \tcase OBJ_BLOB:\n \t\t/* grab_blob_values(val, deref, obj, buf, sz); */\n+\t\tgrab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n \t\tbreak;\n \tdefault:\n \t\tdie(\"Eh?  Object of type %d?\", obj->type);\n@@ -1614,7 +1629,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj\n \t\t\treturn strbuf_addf_ret(err, -1, _(\"parse_object_buffer failed on %s for %s\"),\n \t\t\t\t\t       oid_to_hex(&oi->oid), ref->refname);\n \t\t}\n-\t\tgrab_values(ref->value, deref, *obj, oi->content);\n+\t\tgrab_values(ref->value, deref, *obj, oi);\n \t}\n \n \tgrab_common_values(ref->value, deref, oi);\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 9e0214076b4d..baa3a40a70b1 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -686,6 +686,17 @@ test_atom refs/tags/signed-empty contents:body ''\n test_atom refs/tags/signed-empty contents:signature \"$sig\"\n test_atom refs/tags/signed-empty contents \"$sig\"\n \n+test_expect_success 'basic atom: refs/tags/signed-empty contents:raw' '\n+\tgit cat-file tag refs/tags/signed-empty >expected &&\n+\tgit for-each-ref --format=\"%(contents:raw)\" refs/tags/signed-empty >actual &&\n+\tsanitize_pgp <expected >expected.clean &&\n+\tsanitize_pgp <actual >actual.clean &&\n+\techo \"\" >>expected.clean &&\n+\ttest_cmp expected.clean actual.clean\n+'\n+\n+test_atom refs/tags/signed-empty *contents:raw $(git cat-file commit HEAD)\n+\n test_atom refs/tags/signed-short subject 'subject line'\n test_atom refs/tags/signed-short subject:sanitize 'subject-line'\n test_atom refs/tags/signed-short contents:subject 'subject line'\n@@ -695,6 +706,15 @@ test_atom refs/tags/signed-short contents:signature \"$sig\"\n test_atom refs/tags/signed-short contents \"subject line\n $sig\"\n \n+test_expect_success 'basic atom: refs/tags/signed-short contents:raw' '\n+\tgit cat-file tag refs/tags/signed-short >expected &&\n+\tgit for-each-ref --format=\"%(contents:raw)\" refs/tags/signed-short >actual &&\n+\tsanitize_pgp <expected >expected.clean &&\n+\tsanitize_pgp <actual >actual.clean &&\n+\techo \"\" >>expected.clean &&\n+\ttest_cmp expected.clean actual.clean\n+'\n+\n test_atom refs/tags/signed-long subject 'subject line'\n test_atom refs/tags/signed-long subject:sanitize 'subject-line'\n test_atom refs/tags/signed-long contents:subject 'subject line'\n@@ -708,6 +728,15 @@ test_atom refs/tags/signed-long contents \"subject line\n body contents\n $sig\"\n \n+test_expect_success 'basic atom: refs/tags/signed-long contents:raw' '\n+\tgit cat-file tag refs/tags/signed-long >expected &&\n+\tgit for-each-ref --format=\"%(contents:raw)\" refs/tags/signed-long >actual &&\n+\tsanitize_pgp <expected >expected.clean &&\n+\tsanitize_pgp <actual >actual.clean &&\n+\techo \"\" >>expected.clean &&\n+\ttest_cmp expected.clean actual.clean\n+'\n+\n test_expect_success 'set up refs pointing to tree and blob' '\n \tgit update-ref refs/mytrees/first refs/heads/main^{tree} &&\n \tgit update-ref refs/myblobs/first refs/heads/main:one\n@@ -718,6 +747,7 @@ test_atom refs/mytrees/first contents:subject \"\"\n test_atom refs/mytrees/first body \"\"\n test_atom refs/mytrees/first contents:body \"\"\n test_atom refs/mytrees/first contents:signature \"\"\n+test_atom refs/mytrees/first contents:raw $(git cat-file tree refs/mytrees/first)\n test_atom refs/mytrees/first contents \"\"\n \n test_atom refs/myblobs/first subject \"\"\n@@ -725,6 +755,7 @@ test_atom refs/myblobs/first contents:subject \"\"\n test_atom refs/myblobs/first body \"\"\n test_atom refs/myblobs/first contents:body \"\"\n test_atom refs/myblobs/first contents:signature \"\"\n+test_atom refs/myblobs/first contents:raw $(git cat-file blob refs/myblobs/first)\n test_atom refs/myblobs/first contents \"\"\n \n test_expect_success 'set up multiple-sort tags' '\n\nbase-commit: 97eea85a0a1ec66d356567808a1e4ca2367e0ce7\n-- \ngitgitgadget\n"},{"id":"425097","messageId":"CA+CkUQ8kiCk7X2kFMqPDK2VMbhgRh-HaHynV2GjE7k=LAJhiUA@mail.gmail.com","threadId":"55745","inReplyTo":"pull.958.git.1621500593126.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"Hariom verma","fromEmail":"hariom18599@gmail.com","sentAt":"2021-05-20T11:29:30Z","receivedAt":"2021-05-20T12:29:40Z","isPatch":true,"sender":{"key":"hariom18599@gmail.com","avatar":"https://avatars.githubusercontent.com/u/37576387?v=4"},"body":"Hi ZheNing,\n\nOn Thu, May 20, 2021 at 2:19 PM ZheNing Hu via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: ZheNing Hu <adlternative@gmail.com>\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index e2eac50d9508..fc384a194c0c 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -133,7 +133,7 @@ static struct used_atom {\n>                         unsigned int nobracket : 1, push : 1, push_remote : 1;\n>                 } remote_ref;\n>                 struct {\n> -                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n> +                       enum { C_RAW, C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n\nMaybe it's better if you can preserve the alphabetical order?\n\nThanks.\n"},{"id":"425105","messageId":"CAOLTT8SVnK6HiUYhEkbt10q0ZUprSLy-01c8+TfD6BK2dRmZiA@mail.gmail.com","threadId":"55745","inReplyTo":"CA+CkUQ8kiCk7X2kFMqPDK2VMbhgRh-HaHynV2GjE7k=LAJhiUA@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-20T14:34:07Z","receivedAt":"2021-05-20T14:34:27Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Hariom verma <hariom18599@gmail.com> 于2021年5月20日周四 下午7:29写道：\n>\n> Hi ZheNing,\n>\n> On Thu, May 20, 2021 at 2:19 PM ZheNing Hu via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: ZheNing Hu <adlternative@gmail.com>\n> >\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index e2eac50d9508..fc384a194c0c 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -133,7 +133,7 @@ static struct used_atom {\n> >                         unsigned int nobracket : 1, push : 1, push_remote : 1;\n> >                 } remote_ref;\n> >                 struct {\n> > -                       enum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n> > +                       enum { C_RAW, C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n>\n> Maybe it's better if you can preserve the alphabetical order?\n>\n> Thanks.\n\nThanks, this is indeed my negligence.\n--\nZheNing Hu\n"},{"id":"425127","messageId":"CAP8UFD0Pzdb_9+VpeLrydu8ROdVi4ygFPk367J+NWGL0P5nXdg@mail.gmail.com","threadId":"55745","inReplyTo":"pull.958.git.1621500593126.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-20T16:20:30Z","receivedAt":"2021-05-20T16:20:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, May 20, 2021 at 10:49 AM ZheNing Hu via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: ZheNing Hu <adlternative@gmail.com>\n>\n> Add new formatting option %(contents:raw), which will\n> print all the object contents without any changes.\n\nMaybe you could tell how it would be different from %(contents), or in\nother words what are the changes that %(contents) makes.\n\nIsn't %(contents) only printing the commit message or the tag message\nof a commit or a tag respectively? If %(contents:raw) would print all\nthe object contents, it could feel strange that it is actually\nprinting more than %(contents).\n\nAlso is %(contents:raw) supposed to print something for a blob or a\ntree, while I guess %(contents) is printing nothing for them?\n\n> It will help further to migrate all cat-file formatting\n> logic from cat-file to ref-filter.\n>\n> Signed-off-by: ZheNing Hu <adlternative@gmail.com>\n\nIt looks like you rewrote the patch nearly completely, but if you\nbased your patch on, or got inspired by, Olga's work, it might be nice\nto acknowledge this using a trailer (for example \"Based-on-patch-by:\n...\" or \"Helped-by:...\").\n\n> ---\n>     [GSOC] ref-filter: add contents:raw atom\n>\n>     Learn from Olga's %(raw):\n>     https://github.com/git/git/pull/568/commits/bf22dae7ca387dbc92c5586c92e60cd395099399\n>\n>     We can add a %(contents:raw) atom to ref-filter, which can output object\n>     contents without any change.\n>\n>     %(contents:raw) can work on the refs which point to blob,tree,commit,tag\n>     objects.\n>\n>     It also support %(*contents:raw) to dereference.\n>\n>     With %(cotent:raw), we can later provide support for printing the\n\ns/cotent/contents/\n\n>     content of the \"raw\" object for cat-file --batch.\n\n> @@ -1292,7 +1294,8 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size\n>  }\n>\n>  /* See grab_values */\n> -static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n> +static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf,\n> +                                  unsigned long buf_size, enum object_type object_type)\n>  {\n>         int i;\n>         const char *subpos = NULL, *bodypos = NULL, *sigpos = NULL;\n> @@ -1312,6 +1315,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n>                     !starts_with(name, \"trailers\") &&\n>                     !starts_with(name, \"contents\"))\n>                         continue;\n> +               if (atom->u.contents.option == C_RAW) {\n> +                       v->s = xmemdupz(buf, buf_size);\n> +                       continue;\n> +               }\n> +               if (object_type != OBJ_TAG && object_type != OBJ_COMMIT)\n> +                       continue;\n\nWhen seeing the 2 lines above, I am guessing that, before this patch,\ngrab_sub_body_contents() couldn't actually be called when object_type\nwas OBJ_BLOB or OBJ_TREE, but you have made other changes elsewhere so\nthat now it can. As only the atom->u.contents.option == C_RAW case is\nrelevant in this case, you added this check. Let's see if I am\nright...\n\n>                 if (!subpos)\n>                         find_subpos(buf,\n>                                     &subpos, &sublen,\n> @@ -1374,25 +1384,30 @@ static void fill_missing_values(struct atom_value *val)\n>   * pointed at by the ref itself; otherwise it is the object the\n>   * ref (which is a tag) refers to.\n>   */\n> -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)\n> +static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data)\n>  {\n> +       void *buf = data->content;\n> +       unsigned long buf_size = data->size;\n> +\n>         switch (obj->type) {\n>         case OBJ_TAG:\n>                 grab_tag_values(val, deref, obj);\n> -               grab_sub_body_contents(val, deref, buf);\n> +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n>                 grab_person(\"tagger\", val, deref, buf);\n>                 break;\n>         case OBJ_COMMIT:\n>                 grab_commit_values(val, deref, obj);\n> -               grab_sub_body_contents(val, deref, buf);\n> +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n>                 grab_person(\"author\", val, deref, buf);\n>                 grab_person(\"committer\", val, deref, buf);\n>                 break;\n>         case OBJ_TREE:\n>                 /* grab_tree_values(val, deref, obj, buf, sz); */\n> +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n>                 break;\n>         case OBJ_BLOB:\n>                 /* grab_blob_values(val, deref, obj, buf, sz); */\n> +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n\n...ok, I was right above. The issue now is that I wonder if\ngrab_sub_body_contents() is still a good name for a function that can\nbe called for a blob or a tree which does not really have a body.\n\n>                 break;\n>         default:\n>                 die(\"Eh?  Object of type %d?\", obj->type);\n> @@ -1614,7 +1629,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj\n>                         return strbuf_addf_ret(err, -1, _(\"parse_object_buffer failed on %s for %s\"),\n>                                                oid_to_hex(&oi->oid), ref->refname);\n>                 }\n> -               grab_values(ref->value, deref, *obj, oi->content);\n> +               grab_values(ref->value, deref, *obj, oi);\n>         }\n>\n>         grab_common_values(ref->value, deref, oi);\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 9e0214076b4d..baa3a40a70b1 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -686,6 +686,17 @@ test_atom refs/tags/signed-empty contents:body ''\n>  test_atom refs/tags/signed-empty contents:signature \"$sig\"\n>  test_atom refs/tags/signed-empty contents \"$sig\"\n>\n> +test_expect_success 'basic atom: refs/tags/signed-empty contents:raw' '\n> +       git cat-file tag refs/tags/signed-empty >expected &&\n> +       git for-each-ref --format=\"%(contents:raw)\" refs/tags/signed-empty >actual &&\n> +       sanitize_pgp <expected >expected.clean &&\n> +       sanitize_pgp <actual >actual.clean &&\n> +       echo \"\" >>expected.clean &&\n> +       test_cmp expected.clean actual.clean\n> +'\n\nFor an empty tag %(contents:raw) should produce nothing, ok.\n\n> +test_atom refs/tags/signed-empty *contents:raw $(git cat-file commit HEAD)\n\nMaybe use single quotes around *contents:raw?\n\nThe rest looks good to me. Thanks!\n"},{"id":"425180","messageId":"CAOLTT8S_Bu1PG+-gVK_6iUx--YrMx2hxDCTa=5sW6UJv9Oz_0Q@mail.gmail.com","threadId":"55745","inReplyTo":"CAP8UFD0Pzdb_9+VpeLrydu8ROdVi4ygFPk367J+NWGL0P5nXdg@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-21T04:43:59Z","receivedAt":"2021-05-21T04:44:17Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder <christian.couder@gmail.com> 于2021年5月21日周五 上午12:20写道：\n>\n> On Thu, May 20, 2021 at 10:49 AM ZheNing Hu via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: ZheNing Hu <adlternative@gmail.com>\n> >\n> > Add new formatting option %(contents:raw), which will\n> > print all the object contents without any changes.\n>\n> Maybe you could tell how it would be different from %(contents), or in\n> other words what are the changes that %(contents) makes.\n>\n> Isn't %(contents) only printing the commit message or the tag message\n> of a commit or a tag respectively? If %(contents:raw) would print all\n> the object contents, it could feel strange that it is actually\n> printing more than %(contents).\n>\n\nOkay, some explanations are indeed missing here:\n\n%(contents) will discard the metadata part of the object file,\nand only print the data contents part of it. %(contents:raw)\ncan will not discard the metadata part of the object file, this\n means that it can print the \"raw\" content of an object.\n\nIn addition, %(contents:raw) can support print commit, blob,\ntag, tree objects contents which %(contents) can only support\ncommit,tag objects.\n\nE.g:\n\ngit for-each-ref --format=%(contents:raw) refs/heads/foo\n\nwill have the same output as:\n\ngit rev-parse refs/heads/foo | git cat-file --batch\n\n> Also is %(contents:raw) supposed to print something for a blob or a\n> tree, while I guess %(contents) is printing nothing for them?\n>\n\nNow my thoughts are:\nLet %(contents) learn to print four kinds of objects.\nand then let %(contents:raw) learn to print metadata.\nI will split it into two patches.\n\n> > It will help further to migrate all cat-file formatting\n> > logic from cat-file to ref-filter.\n> >\n> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>\n>\n> It looks like you rewrote the patch nearly completely, but if you\n> based your patch on, or got inspired by, Olga's work, it might be nice\n> to acknowledge this using a trailer (for example \"Based-on-patch-by:\n> ...\" or \"Helped-by:...\").\n>\n\nOkay, \"Based-on-patch-by\" would be more appropriate here.\n\n> > @@ -1312,6 +1315,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n> >                     !starts_with(name, \"trailers\") &&\n> >                     !starts_with(name, \"contents\"))\n> >                         continue;\n> > +               if (atom->u.contents.option == C_RAW) {\n> > +                       v->s = xmemdupz(buf, buf_size);\n> > +                       continue;\n> > +               }\n> > +               if (object_type != OBJ_TAG && object_type != OBJ_COMMIT)\n> > +                       continue;\n>\n> When seeing the 2 lines above, I am guessing that, before this patch,\n> grab_sub_body_contents() couldn't actually be called when object_type\n> was OBJ_BLOB or OBJ_TREE, but you have made other changes elsewhere so\n> that now it can. As only the atom->u.contents.option == C_RAW case is\n> relevant in this case, you added this check. Let's see if I am\n> right...\n>\n> >                 if (!subpos)\n> >                         find_subpos(buf,\n> >                                     &subpos, &sublen,\n> > @@ -1374,25 +1384,30 @@ static void fill_missing_values(struct atom_value *val)\n> >   * pointed at by the ref itself; otherwise it is the object the\n> >   * ref (which is a tag) refers to.\n> >   */\n> > -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)\n> > +static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data)\n> >  {\n> > +       void *buf = data->content;\n> > +       unsigned long buf_size = data->size;\n> > +\n> >         switch (obj->type) {\n> >         case OBJ_TAG:\n> >                 grab_tag_values(val, deref, obj);\n> > -               grab_sub_body_contents(val, deref, buf);\n> > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> >                 grab_person(\"tagger\", val, deref, buf);\n> >                 break;\n> >         case OBJ_COMMIT:\n> >                 grab_commit_values(val, deref, obj);\n> > -               grab_sub_body_contents(val, deref, buf);\n> > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> >                 grab_person(\"author\", val, deref, buf);\n> >                 grab_person(\"committer\", val, deref, buf);\n> >                 break;\n> >         case OBJ_TREE:\n> >                 /* grab_tree_values(val, deref, obj, buf, sz); */\n> > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> >                 break;\n> >         case OBJ_BLOB:\n> >                 /* grab_blob_values(val, deref, obj, buf, sz); */\n> > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n>\n> ...ok, I was right above. The issue now is that I wonder if\n> grab_sub_body_contents() is still a good name for a function that can\n> be called for a blob or a tree which does not really have a body.\n>\n\nMakes sense, It might be better to use the new name: grab_contents().\n\n> >                 break;\n> >         default:\n> >                 die(\"Eh?  Object of type %d?\", obj->type);\n> > @@ -1614,7 +1629,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj\n> >                         return strbuf_addf_ret(err, -1, _(\"parse_object_buffer failed on %s for %s\"),\n> >                                                oid_to_hex(&oi->oid), ref->refname);\n> >                 }\n> > -               grab_values(ref->value, deref, *obj, oi->content);\n> > +               grab_values(ref->value, deref, *obj, oi);\n> >         }\n> >\n> >         grab_common_values(ref->value, deref, oi);\n> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> > index 9e0214076b4d..baa3a40a70b1 100755\n> > --- a/t/t6300-for-each-ref.sh\n> > +++ b/t/t6300-for-each-ref.sh\n> > @@ -686,6 +686,17 @@ test_atom refs/tags/signed-empty contents:body ''\n> >  test_atom refs/tags/signed-empty contents:signature \"$sig\"\n> >  test_atom refs/tags/signed-empty contents \"$sig\"\n> >\n> > +test_expect_success 'basic atom: refs/tags/signed-empty contents:raw' '\n> > +       git cat-file tag refs/tags/signed-empty >expected &&\n> > +       git for-each-ref --format=\"%(contents:raw)\" refs/tags/signed-empty >actual &&\n> > +       sanitize_pgp <expected >expected.clean &&\n> > +       sanitize_pgp <actual >actual.clean &&\n> > +       echo \"\" >>expected.clean &&\n> > +       test_cmp expected.clean actual.clean\n> > +'\n>\n> For an empty tag %(contents:raw) should produce nothing, ok.\n>\n> > +test_atom refs/tags/signed-empty *contents:raw $(git cat-file commit HEAD)\n>\n> Maybe use single quotes around *contents:raw?\n>\n> The rest looks good to me. Thanks!\n\nGood suggestion. thank!\n--\nZheNing Hu\n"},{"id":"425186","messageId":"CAP8UFD1yLS9UBs0r6_GjB-K6prW7GNxR+z445HJe8cR4HYLewA@mail.gmail.com","threadId":"55745","inReplyTo":"CAOLTT8S_Bu1PG+-gVK_6iUx--YrMx2hxDCTa=5sW6UJv9Oz_0Q@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-05-21T09:10:05Z","receivedAt":"2021-05-21T09:10:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 21, 2021 at 6:44 AM ZheNing Hu <adlternative@gmail.com> wrote:\n>\n> Christian Couder <christian.couder@gmail.com> 于2021年5月21日周五 上午12:20写道：\n> >\n> > On Thu, May 20, 2021 at 10:49 AM ZheNing Hu via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> > >\n> > > From: ZheNing Hu <adlternative@gmail.com>\n> > >\n> > > Add new formatting option %(contents:raw), which will\n> > > print all the object contents without any changes.\n> >\n> > Maybe you could tell how it would be different from %(contents), or in\n> > other words what are the changes that %(contents) makes.\n> >\n> > Isn't %(contents) only printing the commit message or the tag message\n> > of a commit or a tag respectively? If %(contents:raw) would print all\n> > the object contents, it could feel strange that it is actually\n> > printing more than %(contents).\n>\n> Okay, some explanations are indeed missing here:\n\nBy the way I forgot to say that your patch might want to also properly\ndocument %(contents:raw). If the doc was updated as part of the patch,\nmaybe the whole patch would be easier to understand.\n\n> %(contents) will discard the metadata part of the object file,\n\nIt's not clear what you mean when you talk about metadata in objects.\nAre you taking about only the headers, like the \"tree XXX\", \"parent\nYYY\", etc lines in commits, and the \"object OOO\", \"type TTT\", etc\nlines in tags? Or does it also includes the lines in tree objects that\nreference other trees or blobs?\n\n> and only print the data contents part of it. %(contents:raw)\n> can will not discard the metadata part of the object file, this\n\ns/can//\n\n>  means that it can print the \"raw\" content of an object.\n\nThe above seems to be changing the definition of %(contents) (as\npreviously it was only the commit message of a commit or the tag\nmessage a tag) to explain %(contents:raw)...\n\n> In addition, %(contents:raw) can support print commit, blob,\n> tag, tree objects contents which %(contents) can only support\n> commit,tag objects.\n\n...but this is saying that the definition of %(contents) actually\ndoesn't change. This doesn't seem logical to me.\n\nIf %(contents:raw) can support any kind of object (commit, blob, tag\nor tree) then it would be logical that %(contents) also support any\nkind of object.\n\nSo if you really want to define %(contents:raw) as the raw object\ncontents, you might want to consider a preparatory patch that would\nfirst change the definition of %(contents) to be the object contents\nwithout the headers. This would keep the same output for any commit or\ntag. But for blobs and trees it would print the whole content of the\nobject in the same way as `git cat-file -p` does, instead of nothing.\n\nI think this acceptable as refs rarely point to something other than\ncommits, so people are not likely to rely on the fact that %(contents)\ncurrently prints nothing for blobs and trees.\n\n> E.g:\n>\n> git for-each-ref --format=%(contents:raw) refs/heads/foo\n>\n> will have the same output as:\n>\n> git rev-parse refs/heads/foo | git cat-file --batch\n\nOk, I think that would indeed be useful.\n\n> > Also is %(contents:raw) supposed to print something for a blob or a\n> > tree, while I guess %(contents) is printing nothing for them?\n>\n> Now my thoughts are:\n> Let %(contents) learn to print four kinds of objects.\n> and then let %(contents:raw) learn to print metadata.\n> I will split it into two patches.\n\nYeah, great!\n\n> > > It will help further to migrate all cat-file formatting\n> > > logic from cat-file to ref-filter.\n> > >\n> > > Signed-off-by: ZheNing Hu <adlternative@gmail.com>\n> >\n> > It looks like you rewrote the patch nearly completely, but if you\n> > based your patch on, or got inspired by, Olga's work, it might be nice\n> > to acknowledge this using a trailer (for example \"Based-on-patch-by:\n> > ...\" or \"Helped-by:...\").\n>\n> Okay, \"Based-on-patch-by\" would be more appropriate here.\n\nNice!\n\n> > > @@ -1312,6 +1315,13 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, void *buf)\n> > >                     !starts_with(name, \"trailers\") &&\n> > >                     !starts_with(name, \"contents\"))\n> > >                         continue;\n> > > +               if (atom->u.contents.option == C_RAW) {\n> > > +                       v->s = xmemdupz(buf, buf_size);\n> > > +                       continue;\n> > > +               }\n> > > +               if (object_type != OBJ_TAG && object_type != OBJ_COMMIT)\n> > > +                       continue;\n> >\n> > When seeing the 2 lines above, I am guessing that, before this patch,\n> > grab_sub_body_contents() couldn't actually be called when object_type\n> > was OBJ_BLOB or OBJ_TREE, but you have made other changes elsewhere so\n> > that now it can. As only the atom->u.contents.option == C_RAW case is\n> > relevant in this case, you added this check. Let's see if I am\n> > right...\n> >\n> > >                 if (!subpos)\n> > >                         find_subpos(buf,\n> > >                                     &subpos, &sublen,\n> > > @@ -1374,25 +1384,30 @@ static void fill_missing_values(struct atom_value *val)\n> > >   * pointed at by the ref itself; otherwise it is the object the\n> > >   * ref (which is a tag) refers to.\n> > >   */\n> > > -static void grab_values(struct atom_value *val, int deref, struct object *obj, void *buf)\n> > > +static void grab_values(struct atom_value *val, int deref, struct object *obj, struct expand_data *data)\n> > >  {\n> > > +       void *buf = data->content;\n> > > +       unsigned long buf_size = data->size;\n> > > +\n> > >         switch (obj->type) {\n> > >         case OBJ_TAG:\n> > >                 grab_tag_values(val, deref, obj);\n> > > -               grab_sub_body_contents(val, deref, buf);\n> > > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> > >                 grab_person(\"tagger\", val, deref, buf);\n> > >                 break;\n> > >         case OBJ_COMMIT:\n> > >                 grab_commit_values(val, deref, obj);\n> > > -               grab_sub_body_contents(val, deref, buf);\n> > > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> > >                 grab_person(\"author\", val, deref, buf);\n> > >                 grab_person(\"committer\", val, deref, buf);\n> > >                 break;\n> > >         case OBJ_TREE:\n> > >                 /* grab_tree_values(val, deref, obj, buf, sz); */\n> > > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> > >                 break;\n> > >         case OBJ_BLOB:\n> > >                 /* grab_blob_values(val, deref, obj, buf, sz); */\n> > > +               grab_sub_body_contents(val, deref, buf, buf_size, obj->type);\n> >\n> > ...ok, I was right above. The issue now is that I wonder if\n> > grab_sub_body_contents() is still a good name for a function that can\n> > be called for a blob or a tree which does not really have a body.\n>\n> Makes sense, It might be better to use the new name: grab_contents().\n\nYeah.\n"},{"id":"425216","messageId":"CAOLTT8Q9meKiHG=TvwsW49dwWtzBNnkfT03+dQrvKDLGuLiOFg@mail.gmail.com","threadId":"55745","inReplyTo":"CAP8UFD1yLS9UBs0r6_GjB-K6prW7GNxR+z445HJe8cR4HYLewA@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-21T13:12:35Z","receivedAt":"2021-05-21T13:13:07Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Christian Couder <christian.couder@gmail.com> 于2021年5月21日周五 下午5:10写道：\n>\n> > Okay, some explanations are indeed missing here:\n>\n> By the way I forgot to say that your patch might want to also properly\n> document %(contents:raw). If the doc was updated as part of the patch,\n> maybe the whole patch would be easier to understand.\n>\n\nIndeed so.\n\n> > %(contents) will discard the metadata part of the object file,\n>\n> It's not clear what you mean when you talk about metadata in objects.\n> Are you taking about only the headers, like the \"tree XXX\", \"parent\n> YYY\", etc lines in commits, and the \"object OOO\", \"type TTT\", etc\n> lines in tags? Or does it also includes the lines in tree objects that\n> reference other trees or blobs?\n>\n\nWell, the metadata here only refers to the header of the commit and\ntag objects, \"tree XXX\", \"parent YYY\"...  The lines in tree objects that\nreference other trees or blobs will not output. What we have to do here\nis to simulate the behavior of `git cat-file --batch`. But we encounter a\nvery serious problem here:\n\nNow we want to use for-each-ref to print a ref point to a tree:\n\ngit for-each-ref --format=\"%(contents:raw)\" refs/mytrees/first\n\nwill output:\n\n100644 one\n\nbut\n\ngit cat-file tree refs/mytrees/first\n\nwill output:\n\n100644 onem�cֈ��q�D�֧hJ)E-100644 two.t���0�+��VjC��eV�ӈq\n\nwhich is the compressed data, it may contains '\\0'.\n\nWhne we use `append_atom()` to add the contents of the tree object\nto the buffer, notice that it uses `strbuf_addstr()`, the underlying call\nis `strlen()`, which truncates the data we added. Can we have any good\nremedies? For example, record the length of \"v->s\" by \"v->s_size\" and\nuse `strbuf_addstr(&state->stack->output, v->s, v->s_size)`?\n\n> > and only print the data contents part of it. %(contents:raw)\n> > can will not discard the metadata part of the object file, this\n>\n> s/can//\n>\n> >  means that it can print the \"raw\" content of an object.\n>\n> The above seems to be changing the definition of %(contents) (as\n> previously it was only the commit message of a commit or the tag\n> message a tag) to explain %(contents:raw)...\n>\n> > In addition, %(contents:raw) can support print commit, blob,\n> > tag, tree objects contents which %(contents) can only support\n> > commit,tag objects.\n>\n> ...but this is saying that the definition of %(contents) actually\n> doesn't change. This doesn't seem logical to me.\n>\n> If %(contents:raw) can support any kind of object (commit, blob, tag\n> or tree) then it would be logical that %(contents) also support any\n> kind of object.\n>\n> So if you really want to define %(contents:raw) as the raw object\n> contents, you might want to consider a preparatory patch that would\n> first change the definition of %(contents) to be the object contents\n> without the headers. This would keep the same output for any commit or\n> tag. But for blobs and trees it would print the whole content of the\n> object in the same way as `git cat-file -p` does, instead of nothing.\n>\n\nYes, I agree with you. Another thing worth mentioning is:\n%(contents:raw) or %(contents) is not similar to \"git cat-file -p foo\"\nthey are more like \"git cat-file <type> foo\" or\n\"git rev-parse foo | git cat-file --batch\", \"git cat-file -p foo\" It will\nprocess the original data:\nFor example, a tree object, with \"git cat-file -p\",\nwill use \"ls-tree\" to output all the sub-trees and blobs it connects.\n\n$ git cat-file -p refs/mytrees/first\n100644 blob 6dde63d6888cec71ea82449bd6a7684a29452d7f    one\n100644 blob f719efd430d52bcfc8566a43b2eb655688d38871    two.t\n\nbut with \"git cat-file <type>\" will output uncompressed data:\n\ngit cat-file tree refs/mytrees/first\n100644 onem�cֈ��q�D�֧hJ)E-100644 two.t���0�+��VjC��eV�ӈq%\n\n> I think this acceptable as refs rarely point to something other than\n> commits, so people are not likely to rely on the fact that %(contents)\n> currently prints nothing for blobs and trees.\n>\n> > E.g:\n> >\n> > git for-each-ref --format=%(contents:raw) refs/heads/foo\n> >\n> > will have the same output as:\n> >\n> > git rev-parse refs/heads/foo | git cat-file --batch\n>\n> Ok, I think that would indeed be useful.\n>\n> > > Also is %(contents:raw) supposed to print something for a blob or a\n> > > tree, while I guess %(contents) is printing nothing for them?\n> >\n> > Now my thoughts are:\n> > Let %(contents) learn to print four kinds of objects.\n> > and then let %(contents:raw) learn to print metadata.\n> > I will split it into two patches.\n>\n> Yeah, great!\n>\n\nThanks!\n--\nZheNing Hu\n"},{"id":"425218","messageId":"CAOLTT8TVn-SPdsX+0BrrrWzhq3in6DDFxDY_i4fEhkd-dW6nqg@mail.gmail.com","threadId":"55745","inReplyTo":"CAOLTT8Q9meKiHG=TvwsW49dwWtzBNnkfT03+dQrvKDLGuLiOFg@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: add contents:raw atom","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-21T13:39:30Z","receivedAt":"2021-05-21T13:39:51Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"ZheNing Hu <adlternative@gmail.com> 于2021年5月21日周五 下午9:12写道：\n>\n>\n> Now we want to use for-each-ref to print a ref point to a tree:\n>\n> git for-each-ref --format=\"%(contents:raw)\" refs/mytrees/first\n>\n> will output:\n>\n> 100644 one\n>\n> but\n>\n> git cat-file tree refs/mytrees/first\n>\n> will output:\n>\n> 100644 onem�cֈ��q�D�֧hJ)E-100644 two.t���0�+��VjC��eV�ӈq\n>\n> which is the compressed data, it may contains '\\0'.\n>\n> Whne we use `append_atom()` to add the contents of the tree object\n> to the buffer, notice that it uses `strbuf_addstr()`, the underlying call\n> is `strlen()`, which truncates the data we added. Can we have any good\n> remedies? For example, record the length of \"v->s\" by \"v->s_size\" and\n> use `strbuf_addstr(&state->stack->output, v->s, v->s_size)`?\n>\n\nThis is not a good method, because `quote_formatting()` in `append_atom()`\nwill also cause this truncation problem.\n\n--\nZheNing Hu\n"}]}