All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Kim Altintop <kim@eagain.st>
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, 9 Aug 2021 12:40:48 -0700	[thread overview]
Message-ID: <YRGEwFauT5QjMSNO@google.com> (raw)
In-Reply-To: <20210809175530.75326-2-kim@eagain.st>

Hi,

Kim Altintop wrote:

> Assembling a "raw" fetch command to be fed directly to "test-tool serve-v2"
> is extracted into a test helper.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Kim Altintop <kim@eagain.st>
> ---
>  t/t5703-upload-pack-ref-in-want.sh | 107 ++++++++++++++++++++---------
>  1 file changed, 74 insertions(+), 33 deletions(-)

Thanks!  Interesting.

> diff --git a/t/t5703-upload-pack-ref-in-want.sh b/t/t5703-upload-pack-ref-in-want.sh
> index e9e471621d..cd4744b016 100755
> --- a/t/t5703-upload-pack-ref-in-want.sh
> +++ b/t/t5703-upload-pack-ref-in-want.sh
> @@ -40,6 +40,54 @@ write_command () {
>  	fi
>  }
> 
> +# Write a complete fetch command to stdout, suitable for use with `test-tool
> +# pkt-line`. "want-ref", "want", and "have" values can be given in this order,
> +# with sections separated by "--".
> +#
> +# Examples:
> +#
> +# write_fetch_command refs/heads/main
> +#
> +# write_fetch_command \
> +#	refs/heads/main \
> +#	-- \
> +#	-- \
> +#	$(git rev-parse x)
> +#
> +# write_fetch_command \
> +#	--
> +#	$(git rev-parse a) \
> +#	--
> +#	$(git rev-parse b)
> +write_fetch_command () {

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?

> +	write_command fetch &&
> +	echo "0001" &&
> +	echo "no-progress" || return
> +    while :

Whitespace seems off.  Junio covers this in his review so I'll ignore
other instances.

[...]
> @@ -97,15 +145,13 @@ test_expect_success 'basic want-ref' '
>  	EOF
>  	git rev-parse f >expected_commits &&
> 
> -	oid=$(git rev-parse a) &&
>  	test-tool pkt-line pack >in <<-EOF &&
> -	$(write_command fetch)
> -	0001
> -	no-progress
> -	want-ref refs/heads/main
> -	have $oid
> -	done
> -	0000
> +	$(write_fetch_command \
> +		refs/heads/main \
> +		-- \
> +		-- \
> +		$(git rev-parse a) \
> +	)
>  	EOF
> 
>  	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).

Thanks and hope that helps,
Jonathan

  parent reply	other threads:[~2021-08-09 19:40 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 [this message]
2021-08-09 21:43             ` Junio C Hamano
2021-08-09 21:56             ` Kim Altintop
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=YRGEwFauT5QjMSNO@google.com \
    --to=jrnieder@gmail.com \
    --cc=bwilliams.eng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jonathantanmy@google.com \
    --cc=kim@eagain.st \
    --subject='Re: [PATCH 1/3] t5730: introduce fetch command helper' \
    /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

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.