From: ZheNing Hu <adlternative@gmail.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>,
git <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>,
Hariom Verma <hariom18599@gmail.com>
Subject: Re: [PATCH] [GSOC] ref-filter: add contents:raw atom
Date: Fri, 21 May 2021 12:43:59 +0800 [thread overview]
Message-ID: <CAOLTT8S_Bu1PG+-gVK_6iUx--YrMx2hxDCTa=5sW6UJv9Oz_0Q@mail.gmail.com> (raw)
In-Reply-To: <CAP8UFD0Pzdb_9+VpeLrydu8ROdVi4ygFPk367J+NWGL0P5nXdg@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> 于2021年5月21日周五 上午12:20写道:
>
> 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).
>
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.
> > 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:...").
>
Okay, "Based-on-patch-by" would be more appropriate here.
> > @@ -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.
>
Makes sense, It might be better to use the new name: grab_contents().
> > 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!
Good suggestion. thank!
--
ZheNing Hu
next prev parent reply other threads:[~2021-05-21 4:44 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
2021-05-21 4:43 ` ZheNing Hu [this message]
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='CAOLTT8S_Bu1PG+-gVK_6iUx--YrMx2hxDCTa=5sW6UJv9Oz_0Q@mail.gmail.com' \
--to=adlternative@gmail.com \
--cc=christian.couder@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).