From: Kim Altintop <kim@eagain.st>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, jonathantanmy@google.com,
bwilliams.eng@gmail.com
Subject: Re: [PATCH 1/3] t5730: introduce fetch command helper
Date: Mon, 09 Aug 2021 21:56:49 +0000 [thread overview]
Message-ID: <CDFBG4S23B1M.141X9FIPP7YHS@schmidt> (raw)
In-Reply-To: <YRGEwFauT5QjMSNO@google.com>
Thanks for chiming in!
On Mon Aug 9, 2021 at 9:40 PM CEST, Jonathan Nieder wrote:
> Hm, for comparison let me see what this looks like without the helper:
> after some prior step
>
> object_format=$(test_oid algo) && # probably just once in a setup step
> x=$(git rev-parse x) &&
>
> we can write
>
> cat <<-EOF &&
> command=fetch
> object-format=$object_format
> 0001
> no-progress
> want-ref refs/heads/main
> have $x
> done
> 0000
> EOF
>
> I find that a little _easier_ to read than a write_fetch_command call,
> because I don't have to chase the definition and x is labeled as a
> 'have'.
>
> Is there some additional motivation for this helper?
It was suggested in earlier review rounds. I think it does improve readability
as quite some lines need to be repeated everywhere a fetch command is assembled.
I agree though that not having some sort of "named arguments" is a bit
detrimental.
>
> > test-tool serve-v2 --stateless-rpc >out <in &&
> > @@ -121,16 +167,14 @@ test_expect_success 'multiple want-ref lines' '
> > EOF
> > git rev-parse c d >expected_commits &&
> >
> > - oid=$(git rev-parse b) &&
> > test-tool pkt-line pack >in <<-EOF &&
> > - $(write_command fetch)
> > - 0001
> > - no-progress
> > - want-ref refs/heads/o/foo
> > - want-ref refs/heads/o/bar
> > - have $oid
> > - done
> > - 0000
> > + $(write_fetch_command \
> > + refs/heads/o/foo \
> > + refs/heads/o/bar \
> > + -- \
> > + -- \
> > + $(git rev-parse b) \
> > + )
> > EOF
>
> Here the entirety of the input to "test-tool pkt-line pack" is the
> entirety of the output from write_fetch_command, which would suggest
> either
>
> a. making write_fetch_command pipe its output to "test-tool pkt-line
> pack", or
>
> b. using a pipe instead of a command substitution, like
> "write_fetch_command ... | test-tool pkt-line pack >in"
>
> (although as mentioned above, I think it's simpler to inline the
> write_fetch_command and even the write_command as well).
Yes, although I believe a pipe cannot be used as we don't have bash's `set -o
pipefail` (ie. the exit status will always be the status of the last command in
the pipe, even if an earlier one failed).
Perhaps an alternative would be:
write_fetch_command () {
write_command fetch &&
echo "0001" &&
echo "no-progress" &&
cat /dev/stdin &&
echo "done" &&
echo "0000"
}
Which would then be called like so:
write_fetch_command >pkt_cmd <<-EOF &&
want-ref refs/heads/main
have $(git rev-parse a)
EOF
test-tool pkt-line pack <pkt_cmd >in &&
test-tool serve-v2 --stateless-rpc >out <in &&
I'm not sure how portable that is, though. Maybe using `while read -r` instead
of `cat /dev/stdin`?
next prev parent reply other threads:[~2021-08-09 21:57 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 13:59 [PATCH] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-07-30 14:04 ` Kim Altintop
2021-07-30 18:57 ` Junio C Hamano
2021-07-30 21:08 ` Kim Altintop
2021-07-31 20:36 ` [PATCH v2] " Kim Altintop
2021-08-02 21:06 ` Jonathan Tan
2021-08-04 20:36 ` Kim Altintop
2021-08-04 20:42 ` [PATCH v3] " Kim Altintop
2021-08-04 21:00 ` [PATCH v4] " Kim Altintop
2021-08-09 17:56 ` [PATCH 0/3] upload-pack: " Kim Altintop
2021-08-09 17:56 ` [PATCH 1/3] t5730: introduce fetch command helper Kim Altintop
2021-08-09 19:16 ` Junio C Hamano
2021-08-09 21:18 ` Kim Altintop
2021-08-09 19:40 ` Jonathan Nieder
2021-08-09 21:43 ` Junio C Hamano
2021-08-09 21:56 ` Kim Altintop [this message]
2021-08-09 22:03 ` Junio C Hamano
2021-08-09 23:01 ` Jonathan Nieder
2021-08-10 9:44 ` Kim Altintop
2021-08-09 17:57 ` [PATCH 2/3] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-08-09 17:57 ` [PATCH 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces Kim Altintop
2021-08-10 9:49 ` Kim Altintop
2021-08-13 6:23 ` [PATCH v6 0/3] upload-pack: treat want-ref relative to namespace Kim Altintop
2021-08-14 21:46 ` Johannes Schindelin
2021-08-15 17:59 ` Junio C Hamano
2021-08-15 19:35 ` Kim Altintop
2021-08-16 12:39 ` Johannes Schindelin
2021-08-13 6:23 ` [PATCH v6 1/3] t5730: introduce fetch command helper Kim Altintop
2021-08-13 6:23 ` [PATCH v6 2/3] upload-pack.c: treat want-ref relative to namespace Kim Altintop
2021-08-13 6:23 ` [PATCH v6 3/3] docs: clarify the interaction of transfer.hideRefs and namespaces Kim Altintop
2021-08-04 21:15 ` [PATCH v3] upload-pack.c: treat want-ref relative to namespace Junio C Hamano
2021-08-04 22:04 ` Kim Altintop
2021-08-04 22:17 ` Eric Sunshine
2021-08-04 22:17 ` Junio C Hamano
2021-08-04 22:23 ` Junio C Hamano
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=CDFBG4S23B1M.141X9FIPP7YHS@schmidt \
--to=kim@eagain.st \
--cc=bwilliams.eng@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).