git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <christian.couder@gmail.com>
To: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>
Cc: git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
	Hariom Verma <hariom18599@gmail.com>,
	ZheNing Hu <adlternative@gmail.com>
Subject: Re: [PATCH] [GSOC] ref-filter: add contents:raw atom
Date: Thu, 20 May 2021 18:20:30 +0200	[thread overview]
Message-ID: <CAP8UFD0Pzdb_9+VpeLrydu8ROdVi4ygFPk367J+NWGL0P5nXdg@mail.gmail.com> (raw)
In-Reply-To: <pull.958.git.1621500593126.gitgitgadget@gmail.com>

On Thu, May 20, 2021 at 10:49 AM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: ZheNing Hu <adlternative@gmail.com>
>
> 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@gmail.com>

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!

  parent reply	other threads:[~2021-05-20 16:20 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20  8:49 [PATCH] [GSOC] ref-filter: add contents:raw atom ZheNing Hu via GitGitGadget
2021-05-20 11:29 ` Hariom verma
2021-05-20 14:34   ` ZheNing Hu
2021-05-20 16:20 ` Christian Couder [this message]
2021-05-21  4:43   ` ZheNing Hu
2021-05-21  9:10     ` Christian Couder
2021-05-21 13:12       ` ZheNing Hu
2021-05-21 13:39         ` ZheNing Hu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAP8UFD0Pzdb_9+VpeLrydu8ROdVi4ygFPk367J+NWGL0P5nXdg@mail.gmail.com \
    --to=christian.couder@gmail.com \
    --cc=adlternative@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=hariom18599@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).