Re: [PATCH v3 6/6] cat-file: add remote-object-info to batch-command

[Peijian Ju <eric.peijian@gmail.com>]

On Wed, Oct 23, 2024 at 5:49 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Thu, Sep 26, 2024 at 3:39 AM Eric Ju <eric.peijian@gmail.com> wrote:
>
> > 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 and print each object info
> >     Else:
> >         - Call respective function attached to command
> >         - Get object info, print object info
> >
> > To summarize, `remote-object-info` gets object info from the remote and
> > then loop through the object info passed in, print the info.
>
> Maybe: s/print the info/printing the info/

Thank you. Fixed in V4.

>
> > In order for remote-object-info to avoid remote communication overhead
> > in the non-buffer mode, the objects are passed in as such:
> >
> > remote-object-info <remote> <oid> <oid> ... <oid>
> >
> > rather than
> >
> > remote-object-info <remote> <oid>
> > remote-object-info <remote> <oid>
> > ...
> > remote-object-info <remote> <oid>
>
> [...]
>
> >  If no format is specified, the default format is `%(objectname)
> > -%(objecttype) %(objectsize)`.
> > +%(objecttype) %(objectsize)`, except for `remote-object-info` commands which use
> > +`%(objectname) %(objectsize)` for now because "%(objecttype)" is not supported yet.
> > +When "%(objecttype)" is supported, default format should be unified.
>
> I think we should warn more clearly and strongly that users should
> take into account that the default format will change. So they should
> better not rely on the current format in their code.
>
> Maybe something like:
>
> `%(objectname) %(objectsize)` for now because "%(objecttype)" is not
> supported yet.
> WARNING: When "%(objecttype)" is supported, default format WILL be unified, so
> DO NOT RELY on the current default format to stay the same!!!
>

Thank you. The warning is added to v4.

> >  If `--batch` is specified, or if `--batch-command` is used with the `contents`
> >  command, the object information is followed by the object contents (consisting
>
> [...]
>
> > diff --git a/t/t1017-cat-file-remote-object-info.sh b/t/t1017-cat-file-remote-object-info.sh
> > new file mode 100755
> > index 0000000000..6826ff7a59
> > --- /dev/null
> > +++ b/t/t1017-cat-file-remote-object-info.sh
> > @@ -0,0 +1,750 @@
> > +#!/bin/sh
> > +
> > +test_description='git cat-file --batch-command with remote-object-info command'
> > +
> > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> > +
> > +. ./test-lib.sh
> > +
> > +echo_without_newline () {
> > +    printf '%s' "$*"
> > +}
> > +
> > +echo_without_newline_nul () {
> > +    echo_without_newline "$@" | tr '\n' '\0'
> > +}
> > +
> > +strlen () {
> > +    echo_without_newline "$1" | wc -c | sed -e 's/^ *//'
> > +}
>
> The above functions have been copied verbatim from t1006-cat-file.sh.
> I think this is worth a comment or a TODO before these functions
> saying that common code might want to be unified in the future.
>
> Maybe something like:
>
> # TODO: refactor these functions which were copied from
> t1006-cat-file.sh into a new common file, maybe "lib-cat-file.sh"
>

Thank you. I added "lib-cat-file.sh" in v4, and let both tests
t1006-cat-file.sh
and t1017-cat-file-remote-object-info.sh refer to it.


> Except the above nits and another one I found in patch 4/6, the rest
> of this patch series looks good to me.
>
> Thanks!