From: Calvin Wan <calvinwan@google.com>
To: Jonathan Tan <jonathantanmy@google.com>, git@vger.kernel.org
Subject: Re: [PATCH v2] fetch object-info-format: client option for object-info
Date: Thu, 10 Feb 2022 14:58:40 -0800 [thread overview]
Message-ID: <CAFySSZAyyEOLMXhJNMmJTV1AJRdnc9AER1n20PhYaLZs1BZPdg@mail.gmail.com> (raw)
In-Reply-To: <20220209204153.481122-1-jonathantanmy@google.com>
> If you generally agree with my review comments
Everything you've said makes sense. Thanks for taking an in depth look
at this! I'll make the changes you suggested and then start working on
the next steps.
On Wed, Feb 9, 2022 at 12:42 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> Calvin Wan <calvinwan@google.com> writes:
> > Add ‘—object-info-format’ option to fetch. This option allows
> > the client to make an object-info [1] command request to a server
> > that supports protocol v2.
>
> Avoid using characters above 127 in commit messages (unless, say, as part of
> someone's name). They make it hard to search for, and in this case, whatever
> dash is there is wrong (it should be "--").
>
> > The transport implementation uses vtables [2], similar to how Git
> > fetches refs,
>
> You should be explicit that you're adding a new function to the vtable.
> (Whether that is what we should do is another issue: let me look at the
> patch to see.)
>
> > to determine whether a process needs to be taken over
> > before sending the object-info request.
>
> The vtable is not to determine whether a process needs to be taken over,
> but so that we support multiple protocols (HTTP, SSH, etc.). In any
> case, this detail is probably not relevant.
>
> > Different protocols
> > require different setups for making requests.
>
> This is true, but I don't see the relevance.
>
> > [1] https://lore.kernel.org/git/20210420233830.2181153-1-bga@google.com/
> > [2] https://lore.kernel.org/git/26f276956001a120cc9105b0071762c2fd4a45c5.15=
> > 13287544.git.jonathantanmy@google.com/
>
> For merged code, quote the commit, not the email.
>
> > @@ -220,6 +225,8 @@ static struct option builtin_fetch_options[] = {
> > N_("write the commit-graph after fetching")),
> > OPT_BOOL(0, "stdin", &stdin_refspecs,
> > N_("accept refspecs from stdin")),
> > + OPT_STRING_LIST(0, "object-info-format", &object_info_format, N_("option"),
> > + N_("command request arguments")),
>
> I would have expected a parameter named "format" to take a format
> string, but taking a string list of the fields we need might work too.
> In any case, maybe rename it to "--object-info" or similar.
>
> > @@ -2000,6 +2007,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> > struct remote *remote = NULL;
> > int result = 0;
> > int prune_tags_ok = 1;
> > + struct oid_array oids = OID_ARRAY_INIT;
> > + struct object_id oid;
>
> The "oids" at function level needs a more descriptive name (e.g.
> "oids_for_object_info"). The name of "oid" is fine, since it's just used
> as a temporary variable, but since it is temporary, it should be
> declared in the block where it's used. (Same for "oids", actually:
> declare it in the block it's used, and in that case you can keep the
> name since it's more tightly scoped.)
>
> > @@ -2057,6 +2066,23 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> > if (dry_run)
> > write_fetch_head = 0;
> >
> > + if (object_info_format.nr > 0) {
> > + if (argc == 0 || argc == 1) {
> > + die(_("must supply remote and object ids when using --object-info-format"));
> > + } else {
> > + remote = remote_get(argv[0]);
> > + for (i = 1; i < argc; i++) {
> > + if (get_oid(argv[i], &oid))
> > + return error(_("malformed object name '%s'"), argv[i]);
> > + oid_array_append(&oids, &oid);
> > + }
> > + }
> > + gtransport = prepare_transport(remote, 0);
> > + gtransport->server_options = &object_info_format;
> > + result = transport_fetch_object_info(gtransport, &oids);
> > + return result;
> > + }
>
> I was thinking that this should reuse the refspec parsing mechanism
> (which also supports stdin), but upon more thought, using the refspec
> parser means that we would also need to check that all refspecs are
> exact OIDs (because we wouldn't know what to do with them otherwise).
> OK, parsing the objects by ourselves looks reasonable.
>
> The assignment of object_info_format to server_options is probably not a
> good idea, though, since readers of server_options would expect server
> options, not what you're assigning. The best place to put this
> information is in smart_options. (See the negotiate_only code.)
>
> > +static void write_object_info_command_and_capabilities(struct strbuf *req_buf,
> > + const struct string_list *server_options)
> > +{
>
> [snip contents]
>
> This code is very similar to code in fetch-pack.c. If you stick to
> crafting the request in builtin/fetch.c, you should refactor
> fetch-pack.{c,h} to expose this functionality (in a preparatory commit)
> and then use that function from here.
>
> > +void send_object_info_request(int fd_out, struct object_info_args *args)
> > +{
> > + struct strbuf req_buf = STRBUF_INIT;
> > + int i;
> > +
> > + write_object_info_command_and_capabilities(&req_buf, args->server_options);
> > +
> > + if (string_list_has_string(args->server_options, "size"))
> > + packet_buf_write(&req_buf, "size");
>
> What happens if "size" is not in the list?
>
> > + printf "%s %d\n" "$object_id" "$length" >expect &&
>
> You can just write "echo $object_id $length >expect". Also, test the
> following:
> - more than one OID
> - an OID that's not on the remote
> - a malformed OID
> - a server that doesn't support protocol v2
> - a server that supports protocol v2 but not object-format
>
> (You don't have to do this for all protocols; just pick one. I prefer
> HTTP, since that's the most complex.)
>
> Other than that, the tests look good. Thanks for testing the different
> protocols.
>
> > @@ -1269,6 +1280,7 @@ static struct transport_vtable vtable = {
> > .get_refs_list = get_refs_list,
> > .fetch_refs = fetch_refs,
> > .push_refs = push_refs,
> > + .fetch_object_info = fetch_object_info,
> > .connect = connect_helper,
> > .disconnect = release_helper
> > };
>
> Adding a function to the transport vtable is not so disruptive since we
> don't have many transport vtables, but better if we can avoid this
> disruption. In this case, I think it's better to reuse fetch_refs.
> Mainly, the plumbing from transport_fetch_refs() to all the helper
> functions in fetch-pack.c already exists, so reusing fetch_refs would
> allow us to reuse that plumbing.
>
> This also means that we don't have to expose the protocol functionality
> in fetch-pack.c that you copied over to builtin/fetch.c in this patch,
> which is an added bonus.
>
> > +static int fetch_object_info(struct transport *transport, struct oid_array *oids)
> > +{
>
> [snip contents]
>
> And reusing the plumbing might mean that we don't need this function
> too.
>
> Taking a step back, there also needs to be a fallback mechanism for when
> the server doesn't support object-info.
>
> If you generally agree with my review comments, I would say that your
> next steps are:
> - investigate if we can reuse the transport_fetch_pack -> fetch-pack.c
> machinery
> - make a fallback for when the server doesn't support object-info
> (might be easier when we use the machinery, so I would start with
> that first)
next prev parent reply other threads:[~2022-02-10 22:58 UTC|newest]
Thread overview: 103+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-08 23:19 [PATCH] fetch —object-info-format: client option for object-info Calvin Wan
2022-02-08 23:56 ` [PATCH v2] " Calvin Wan
2022-02-09 12:48 ` Philip Oakley
2022-02-10 22:32 ` Calvin Wan
2022-02-09 20:41 ` [PATCH v2] fetch object-info-format: " Jonathan Tan
2022-02-10 22:58 ` Calvin Wan [this message]
2022-03-28 19:11 ` [PATCH v3 0/3] object-info: add option for retrieving object info Calvin Wan
2022-03-28 19:11 ` [PATCH v3 1/3] fetch-pack: refactor packet writing and fetch options Calvin Wan
2022-03-29 22:54 ` Junio C Hamano
2022-03-29 23:01 ` Taylor Blau
2022-03-30 21:55 ` Jonathan Tan
2022-03-28 19:11 ` [PATCH v3 2/3] transfer.advertiseObjectInfo: add object-info config Calvin Wan
2022-03-29 22:34 ` Junio C Hamano
2022-03-29 22:48 ` Calvin Wan
2022-03-29 23:09 ` Taylor Blau
2022-03-28 19:11 ` [PATCH v3 3/3] object-info: add option for retrieving object info Calvin Wan
2022-03-29 19:57 ` Junio C Hamano
2022-03-29 22:54 ` Junio C Hamano
2022-03-30 0:49 ` Junio C Hamano
2022-03-30 22:31 ` Calvin Wan
2022-03-30 22:43 ` Jonathan Tan
2022-03-30 23:42 ` Junio C Hamano
2022-03-29 23:19 ` Taylor Blau
2022-03-30 22:47 ` Calvin Wan
2022-03-30 22:06 ` John Cai
2022-03-31 19:56 ` Calvin Wan
2022-04-01 16:16 ` Junio C Hamano
2022-03-30 22:07 ` Jonathan Tan
2022-03-30 22:12 ` Josh Steadmon
2022-03-30 22:46 ` Calvin Wan
2022-03-29 20:35 ` [PATCH v3 0/3] " Junio C Hamano
2022-03-29 22:40 ` Calvin Wan
2022-03-31 1:50 ` Junio C Hamano
2022-05-02 17:08 ` [PATCH v4 0/8] cat-file: add --batch-command remote-object-info command Calvin Wan
2022-05-02 17:08 ` [PATCH v4 1/8] fetch-pack: refactor packet writing Calvin Wan
2022-05-02 17:08 ` [PATCH v4 2/8] fetch-pack: move fetch default settings Calvin Wan
2022-05-02 22:58 ` Junio C Hamano
2022-05-03 23:06 ` Jonathan Tan
2022-05-05 18:13 ` Calvin Wan
2022-05-02 17:08 ` [PATCH v4 3/8] object-store: add function to free object_info contents Calvin Wan
2022-05-02 23:23 ` Junio C Hamano
2022-05-04 19:09 ` Junio C Hamano
2022-05-05 0:15 ` Junio C Hamano
2022-05-05 16:47 ` Calvin Wan
2022-05-05 17:01 ` Junio C Hamano
2022-05-02 17:09 ` [PATCH v4 4/8] object-info: send attribute packet regardless of object ids Calvin Wan
2022-05-03 0:05 ` Junio C Hamano
2022-05-03 19:21 ` Calvin Wan
2022-05-03 23:11 ` Jonathan Tan
2022-05-02 17:09 ` [PATCH v4 5/8] transport: add client side capability to request object-info Calvin Wan
2022-05-03 0:54 ` Junio C Hamano
2022-05-03 18:58 ` Calvin Wan
2022-05-04 15:42 ` Junio C Hamano
2022-05-03 23:15 ` Jonathan Tan
2022-05-04 15:50 ` Junio C Hamano
2022-05-02 17:09 ` [PATCH v4 6/8] transport: add object-info fallback to fetch Calvin Wan
2022-05-03 23:27 ` Jonathan Tan
2022-05-02 17:09 ` [PATCH v4 7/8] cat-file: move parse_cmd and DEFAULT_FORMAT up Calvin Wan
2022-05-02 17:09 ` [PATCH v4 8/8] cat-file: add --batch-command remote-object-info command Calvin Wan
2022-05-04 21:27 ` Jonathan Tan
2022-05-05 18:13 ` Calvin Wan
2022-05-05 18:44 ` Junio C Hamano
2022-05-05 19:09 ` Junio C Hamano
2022-05-05 19:19 ` Calvin Wan
2022-07-31 15:24 ` ZheNing Hu
2022-08-08 17:43 ` Calvin Wan
2022-07-28 23:02 ` [PATCH v5 0/6] " Calvin Wan
2022-07-28 23:56 ` Junio C Hamano
2022-07-29 0:02 ` Junio C Hamano
2022-07-31 8:41 ` Phillip Wood
2022-08-04 22:57 ` Calvin Wan
2022-09-30 23:23 ` Junio C Hamano
2022-07-28 23:02 ` [PATCH v5 1/6] fetch-pack: refactor packet writing Calvin Wan
2022-07-28 23:02 ` [PATCH v5 2/6] fetch-pack: move fetch initialization Calvin Wan
2022-07-28 23:02 ` [PATCH v5 3/6] protocol-caps: initialization bug fix Calvin Wan
2022-07-29 17:51 ` Junio C Hamano
2022-07-28 23:02 ` [PATCH v5 4/6] serve: advertise object-info feature Calvin Wan
2022-07-29 17:57 ` Junio C Hamano
2022-08-01 18:28 ` Calvin Wan
2022-08-01 18:44 ` Ævar Arnfjörð Bjarmason
2022-08-01 18:47 ` Junio C Hamano
2022-08-01 18:58 ` Calvin Wan
2022-07-28 23:02 ` [PATCH v5 5/6] transport: add client support for object-info Calvin Wan
2022-07-29 18:06 ` Junio C Hamano
2022-08-04 20:28 ` Calvin Wan
2022-08-01 13:30 ` Phillip Wood
2022-08-04 22:20 ` Calvin Wan
2022-08-08 10:07 ` Phillip Wood
2022-08-01 14:26 ` Phillip Wood
2022-08-08 9:16 ` Phillip Wood
2022-07-28 23:02 ` [PATCH v5 6/6] cat-file: add remote-object-info to batch-command Calvin Wan
2022-07-29 6:25 ` Ævar Arnfjörð Bjarmason
2022-08-07 5:50 ` ZheNing Hu
2022-08-08 18:07 ` Calvin Wan
2022-08-11 10:58 ` Ævar Arnfjörð Bjarmason
2022-07-29 18:21 ` Junio C Hamano
2022-08-08 18:37 ` Calvin Wan
2022-09-28 13:12 ` Ævar Arnfjörð Bjarmason
2022-07-31 15:02 ` [PATCH v4 0/8] cat-file: add --batch-command remote-object-info command ZheNing Hu
2022-08-08 17:32 ` Calvin Wan
2022-08-13 22:17 ` Junio C Hamano
2022-02-09 19:09 ` [PATCH] fetch —object-info-format: client option for object-info John Cai
2022-02-10 22:49 ` Calvin Wan
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFySSZAyyEOLMXhJNMmJTV1AJRdnc9AER1n20PhYaLZs1BZPdg@mail.gmail.com \
--to=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.