Re: [PATCH v4 8/8] cat-file: add --batch-command remote-object-info command

From: Jonathan Tan <jonathantanmy@google.com>
Date: May 4, 2022, 21:27 UTC
Message-ID: <20220504212738.162853-1-jonathantanmy@google.com>
In-Reply-To: <20220502170904.2770649-9-calvinwan@google.com>

Thanks for the patch. I'll limit myself to high-level comments for now, since the design may need to change if we want to support mixing remote-object-info with other commands in a --batch-command --buffer invocation.

Calvin Wan <calvinwan@google.com> writes:

Show 15 quoted lines

> Since the `info` command in cat-file --batch-command prints object info
> for a given object, it is natural to add another command in cat-file
> --batch-command to print object info for a given object from a remote.
> Add `remote-object-info` to cat-file --batch-command.
> 
> While `info` takes object ids one at a time, this creates overhead when
> making requests to a server so `remote-object-info` instead can take
> multiple object ids at once.
> 
> cat-file --batch-command is generally implemented in the following
> manner:
> 
>  - Receive and parse input from user
>  - Call respective function attached to command
>  - Set batch mode state, get object info, print object info

You mention below that this only works in --buffer mode, so I don't think that this is relevant.

Show 12 quoted lines

> In --buffer mode, this changes to:
> 
>  - Receive and parse input from user
>  - Store respective function attached to command in a queue
>  - After flush, loop through commands in queue
>     - Call respective function attached to command
>     - Set batch mode state, get object info, print object info
> 
> Notice how the getting and printing of object info is accomplished one
> at a time. As described above, this creates a problem for making
> requests to a server. Therefore, `remote-object-info` is implemented in
> the following manner:

If you do want this command to work with --buffer and without, probably clearer to replace "Notice" with "Notice that when cat-file is invoked both without --buffer and with it". (Having said that, it makes sense to me to only make it work with --buffer - making one request per OID would probably not be something that the user would want.)

Show 29 quoted lines

>  - Receive and parse input from user
>  If command is `remote-object-info`:
>     - Get object info from remote
>     - Loop through object info
>         - Call respective function attached to `info`
>         - Set batch mode state, use passed in object info, print object
>           info
>  Else:
>     - Call respective function attached to command
>     - Parse input, get object info, print object info
> 
> And finally for --buffer mode `remote-object-info`:
>  - Receive and parse input from user
>  - Store respective function attached to command in a queue
>  - After flush, loop through commands in queue:
>     If command is `remote-object-info`:
>         - Get object info from remote
>         - Loop through object info
>             - Call respective function attached to `info`
>             - Set batch mode state, use passed in object info, print
>               object info
>     Else:
>         - Call respective function attached to command
>         - Set batch mode state, get object info, print object info
> 
> To summarize, `remote-object-info` gets object info from the remote and
> then generates multiples `info` commands with the object info passed in.
> It cannot be implemented similarly to `info` and `content` because of
> the overhead with remote commenication.

What happens if remote-object-info is mixed with other commands?

Show 11 quoted lines

> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
> index 24a811f0ef..0d9e8e6214 100644
> --- a/Documentation/git-cat-file.txt
> +++ b/Documentation/git-cat-file.txt
> @@ -115,6 +115,10 @@ info <object>::
>  	Print object info for object reference `<object>`. This corresponds to the
>  	output of `--batch-check`.
>  
> +remote-object-info <remote> [<object>]::
> +	Print object info for object references `<object>` at specified <remote>.
> +	This command may only be combined with `--buffer`.

Here you mention that we need --buffer.

Show 14 quoted lines

> @@ -253,13 +258,14 @@ newline. The available atoms are:
>  
>  `objectsize:disk`::
>  	The size, in bytes, that the object takes up on disk. See the
> -	note about on-disk sizes in the `CAVEATS` section below.
> +	note about on-disk sizes in the `CAVEATS` section below. Not
> +	supported by `remote-object-info`.
>  
>  `deltabase`::
>  	If the object is stored as a delta on-disk, this expands to the
>  	full hex representation of the delta base object name.
>  	Otherwise, expands to the null OID (all zeroes). See `CAVEATS`
> -	below.
> +	below. Not supported by `remote-object-info`.

OK - makes sense that these are not supported by remote-object-info, because the remote may not even store objects in delta format.

Show 12 quoted lines

> @@ -32,11 +35,14 @@ struct batch_options {
>  	int unordered;
>  	int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */
>  	const char *format;
> +	int use_remote_info;
>  };
>  
>  #define DEFAULT_FORMAT "%(objectname) %(objecttype) %(objectsize)"
>  
>  static const char *force_path;
> +static struct object_info *remote_object_info;
> +static struct oid_array object_info_oids = OID_ARRAY_INIT;

Hmm...I would have expected the information to appear in the same data structure used to store other command invocations like "contents", "info", and "flush", not separately like this. This design doesn't seem to support mixing commands.

Show 6 quoted lines

> @@ -538,6 +611,7 @@ typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
>  struct queued_cmd {
>  	parse_cmd_fn_t fn;
>  	char *line;
> +	const char *name;
>  };

Rather than store a pointer like this (and thus needing to keep track of the lifetime of the memory pointed to by this pointer), if we only need to know whether this queued_cmd is a remote-object-info call, we can just use "unsigned is_remote_object_info : 1".

Show 7 quoted lines

> @@ -565,9 +639,49 @@ static const struct parse_cmd {
>  } commands[] = {
>  	{ "contents", parse_cmd_contents, 1},
>  	{ "info", parse_cmd_info, 1},
> +	{ "remote-object-info", parse_cmd_info, 1},
>  	{ "flush", NULL, 0},
>  };

Double parse_cmd_info?