From: Taylor Blau <me@ttaylorr.com>
To: Calvin Wan <calvinwan@google.com>
Cc: git@vger.kernel.org, jonathantanmy@google.com,
philipoakley@iee.email, johncai86@gmail.com
Subject: Re: [PATCH v3 3/3] object-info: add option for retrieving object info
Date: Tue, 29 Mar 2022 19:19:25 -0400 [thread overview]
Message-ID: <YkOT/TCKPK3LT9gh@nand.local> (raw)
In-Reply-To: <20220328191112.3092139-4-calvinwan@google.com>
Hi Calvin,
Junio took a deeper look below, but here are a few small code-hygiene
issues that I noticed while taking a quicker look through this patch.
On Mon, Mar 28, 2022 at 07:11:12PM +0000, Calvin Wan wrote:
> Sometimes it is useful to get information about an object without
> having to download it completely. The server logic has already been
> implemented as “a2ba162cda (object-info: support for retrieving object
> info, 2021-04-20)”. This patch implements the client option for
> it. Currently, only 'size' is supported, however future patches can
> implement additional options.
>
> Add ‘--object-info’ option to fetch. This option allows the client to
The single-quotes here look like smart quotes. I don't think we forbid
these explicitly within commit messages, but using the standard '
(ASCII 0x27) character is more common.
> make an object-info command request to a server that supports protocol
> v2. If the server is v2, but does not allow for the object-info
> command request, fetch the objects as if it were a standard fetch
> (however without changing any refs). Therefore, hook `fetch
> object-info` into transport_fetch_refs() to easily fallback if needed.
>
> A previous patch added the `transfer.advertiseObjectInfo` config
> option, of which this patch utilizes to test the fallback.
>
> ---
> Documentation/fetch-options.txt | 5 ++
> Documentation/git-fetch.txt | 1 +
> builtin/fetch.c | 36 ++++++++-
> fetch-pack.c | 42 +++++++++-
> fetch-pack.h | 9 +++
> t/t5583-fetch-object-info.sh | 138 ++++++++++++++++++++++++++++++++
> transport-helper.c | 8 +-
> transport-internal.h | 1 +
> transport.c | 75 ++++++++++++++++-
> transport.h | 9 +++
> 10 files changed, 315 insertions(+), 9 deletions(-)
> create mode 100755 t/t5583-fetch-object-info.sh
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index f903683189..f1ca95c32d 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -299,3 +299,8 @@ endif::git-pull[]
> -6::
> --ipv6::
> Use IPv6 addresses only, ignoring IPv4 addresses.
> +
> +--object-info=[<arguments>]::
> + Fetches only the relevant information passed in arguments from a
> + specific remote and set of object ids. Object 'size' is currently
> + the only supported argument.
> diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
> index 550c16ca61..a13d2ba7ad 100644
> --- a/Documentation/git-fetch.txt
> +++ b/Documentation/git-fetch.txt
> @@ -13,6 +13,7 @@ SYNOPSIS
> 'git fetch' [<options>] <group>
> 'git fetch' --multiple [<options>] [(<repository> | <group>)...]
> 'git fetch' --all [<options>]
> +'git fetch' --object-info=[<arguments>] <remote> [<object-ids>]
>
>
> DESCRIPTION
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 4d12c2fd4d..0b21bebfca 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -29,6 +29,9 @@
> #include "commit-graph.h"
> #include "shallow.h"
> #include "worktree.h"
> +#include "protocol.h"
> +#include "pkt-line.h"
> +#include "connect.h"
>
> #define FORCED_UPDATES_DELAY_WARNING_IN_MS (10 * 1000)
>
> @@ -37,6 +40,7 @@ static const char * const builtin_fetch_usage[] = {
> N_("git fetch [<options>] <group>"),
> N_("git fetch --multiple [<options>] [(<repository> | <group>)...]"),
> N_("git fetch --all [<options>]"),
> + N_("git fetch --object-info=[<arguments>] <remote> [<object-ids>]"),
> NULL
> };
>
> @@ -86,6 +90,7 @@ static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
> static int fetch_write_commit_graph = -1;
> static int stdin_refspecs = 0;
> static int negotiate_only;
> +static struct string_list object_info_options = STRING_LIST_INIT_NODUP;
>
> static int git_fetch_config(const char *k, const char *v, void *cb)
> {
> @@ -221,6 +226,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", &object_info_options, N_("option"),
> + N_("command request arguments")),
> OPT_END()
> };
>
> @@ -2087,6 +2094,7 @@ 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 object_info_oids = OID_ARRAY_INIT;
>
> packet_trace_identity("fetch");
>
> @@ -2168,7 +2176,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> if (dry_run)
> write_fetch_head = 0;
>
> - if (all) {
> + if (object_info_options.nr > 0) {
Small nit; since object_info_options.nr should never be negative, we
should instead write "if (object_info_options)".
> + if (argc == 0 || argc == 1) {
> + die(_("must supply remote and object ids when using --object-info"));
> + } else {
> + struct object_id oid;
> + remote = remote_get(argv[0]);
> + for (i = 1; i < argc; i++) {
> + if (get_oid(argv[i], &oid))
> + return error(_("malformed object id '%s'"), argv[i]);
> + oid_array_append(&object_info_oids, &oid);
> + }
> + }
> + } else if (all) {
> if (argc == 1)
> die(_("fetch --all does not take a repository argument"));
> else if (argc > 1)
> @@ -2199,7 +2219,19 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
> }
> }
>
> - if (negotiate_only) {
> + if (object_info_options.nr > 0) {
> + if (!remote)
> + die(_("must supply remote when using --object-info"));
> + gtransport = prepare_transport(remote, 1);
> + if (gtransport->smart_options) {
> + gtransport->smart_options->object_info = 1;
> + gtransport->smart_options->object_info_oids = &object_info_oids;
> + gtransport->smart_options->object_info_options = &object_info_options;
> + }
> + if (server_options.nr)
> + gtransport->server_options = &server_options;
> + result = transport_fetch_refs(gtransport, NULL);
> + } else if (negotiate_only) {
> struct oidset acked_commits = OIDSET_INIT;
> struct oidset_iter iter;
> const struct object_id *oid;
> diff --git a/fetch-pack.c b/fetch-pack.c
> index b709a61baf..36e3d1c5d0 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1269,6 +1269,27 @@ static void write_command_and_capabilities(struct strbuf *req_buf,
> packet_buf_delim(req_buf);
> }
>
> +void send_object_info_request(int fd_out, struct object_info_args *args)
> +{
> + struct strbuf req_buf = STRBUF_INIT;
> + int i;
I don't think this would ever be practically exploit-able, since we'd
run past the argument limit long before we hit INT_MAX, but this should
be a size_t instead of an int to match the type of args->oid->nr.
> +
> + write_command_and_capabilities(&req_buf, args->server_options, "object-info");
> +
> + if (string_list_has_string(args->object_info_options, "size"))
> + packet_buf_write(&req_buf, "size");
> +
> + for (i = 0; i < args->oids->nr; i++) {
> + packet_buf_write(&req_buf, "oid %s\n", oid_to_hex(&args->oids->oid[i]));
> + }
Small nit, the braces around this for-loop are not required.
> static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,
> struct fetch_pack_args *args,
> const struct ref *wants, struct oidset *common,
> @@ -1604,6 +1625,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> if (args->depth > 0 || args->deepen_since || args->deepen_not)
> args->deepen = 1;
>
> + if (args->object_info) {
> + state = FETCH_SEND_REQUEST;
> + }
> +
Ditto here.
> while (state != FETCH_DONE) {
> switch (state) {
> case FETCH_CHECK_LOCAL:
> @@ -1613,7 +1638,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> /* Filter 'ref' by 'sought' and those that aren't local */
> mark_complete_and_common_ref(negotiator, args, &ref);
> filter_refs(args, &ref, sought, nr_sought);
> - if (everything_local(args, &ref))
> + if (!args->object_info && everything_local(args, &ref))
> state = FETCH_DONE;
> else
> state = FETCH_SEND_REQUEST;
> @@ -1999,8 +2024,19 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
> }
> args->connectivity_checked = 1;
> }
> -
> - update_shallow(args, sought, nr_sought, &si);
> +
> + if (args->object_info) {
> + struct ref *ref_cpy_reader = ref_cpy;
> + unsigned long size = 0;
> + while (ref_cpy_reader) {
> + oid_object_info(the_repository, &(ref_cpy_reader->old_oid), &size);
> + printf("%s %li\n", oid_to_hex(&(ref_cpy_reader->old_oid)), size);
Both expressions like "&(x)" can be written instead as "&x" without the
outer parenthesis.
Likewise, we do not use %li, that line should instead read:
printf("%s %"PRIuMAX"\n", oid_to_hex(&ref_cpy_reader->old_oid), (uintmax_t)size);
Thanks,
Taylor
next prev parent reply other threads:[~2022-03-29 23:19 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
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 [this message]
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=YkOT/TCKPK3LT9gh@nand.local \
--to=me@ttaylorr.com \
--cc=calvinwan@google.com \
--cc=git@vger.kernel.org \
--cc=johncai86@gmail.com \
--cc=jonathantanmy@google.com \
--cc=philipoakley@iee.email \
/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.