Re: [PATCH v3 6/6] cat-file: add remote-object-info to batch-command
- From
Peijian Ju <eric.peijian@gmail.com>
- Date
- Oct 24, 2024, 20:28 UTC
- Message-ID
- <CAN2LT1Dk7A-y9ffNTjf+UP6VKZZgFOephFSAk8eDUi06UjteYg@mail.gmail.com>
- In-Reply-To
- <CAP8UFD24jDT1yFNTp-T==-avccd4QjrwRtfJ_-wPx78H31VSiw@mail.gmail.com>
On Wed, Oct 23, 2024 at 5:49 AM Christian Couder <christian.couder@gmail.com> wrote:
Show 18 quoted lines
> > 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.
Show 32 quoted lines
> > > 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.
Show 41 quoted lines
> > 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!