Re: [PATCH] promisor-remote: add promisor.quiet configuration option

From: Junio C Hamano <gitster@pobox.com>
Date: May 23, 2024, 22:23 UTC
Message-ID: <xmqqsey8qia7.fsf@gitster.g>
In-Reply-To: <20240523131926.1959245-1-tom@compton.nu>

Tom Hughes <tom@compton.nu> writes:

> Add a configuration optione to allow output from the promisor
> fetching objects to be suppressed/

"optione" -> "option", "suppressed/" -> "suppressed.".

> This allows us to stop commands like git blame being swamped
> with progress messages and gc notifications from the promisor
> when used in a partial clone.

"git blame" -> "'git blame'", perhaps.

It is an interesting observation. I thought "git blame" was quite bad at streaming (i.e., until it learned the origin of each and every line, it never produced any output the user asked for), which actually would make it a non issue that the output the user wanted gets mixed with the progress messages and other garbage. Unless the user understands that "git blame" is not spending time itself, but is waiting for necessary blobs to be fetched from the promisor, and is expected to wait unusally longer than the fully local case, having to stare at a blank/unchanging screen would make it uneasy for the end-user and that is why we have progress eye-candy.

I am OK for promisor.quiet being optional, but I am torn when I imagine what comes next. On one hand, I myself probably would find it neat to make these lazy fetches happen completely silently as if nothing strange is happening from the point of view of end-users (except for some operations may be unusually slow compared to fully local repository). On the other hand, I suspect people will be tempted to push it to be on by default at which time it may hurt unsuspecting (new) users who may have been helped by progress bars.

Show 9 quoted lines

> diff --git a/Documentation/config/promisor.txt b/Documentation/config/promisor.txt
> new file mode 100644
> index 0000000000..98c5cb2ec2
> --- /dev/null
> +++ b/Documentation/config/promisor.txt
> @@ -0,0 +1,3 @@
> +promisor.quiet::
> +	If set to "true" assume `--quiet` when fetching additional
> +	objects for a partial clone.

OK.

> diff --git a/promisor-remote.c b/promisor-remote.c

The implementation is absolutely trivial and straight-forward.

> +test_expect_success TTY 'promisor.quiet=false works' '

Do not say "works"---recall the best practice of writing a good bug reports. Stating your expectation more explicitly, e.g. "shows progress messages" or somesuch.

> +	rm -rf server server2 repo &&
> +	rm -rf server server3 repo &&

Why remove the same thing twice?

Show 6 quoted lines

> +	test_create_repo server &&
> +	test_commit -C server foo &&
> +	git -C server repack -a -d --write-bitmap-index &&
> +
> +	git clone "file://$(pwd)/server" repo &&
> +	git hash-object repo/foo.t >blobhash &&

Do you need a temporary file, or would

	blobhash=$(git hash-object repo/foo.t) &&

work just fine?  Of course you'd later have to say

	... git -C repo cat-file -p $blobhash

instead of "$(cat blobhash)". Even simpler, I wonder if you can remove this hash-object invocation, and then do

	... git -C repo cat-file -p :foo.t

> +	rm -rf repo/.git/objects/* &&

This! IS! BAD! for the reason stated later ...

> +	git -C server config uploadpack.allowanysha1inwant 1 &&
> +	git -C server config uploadpack.allowfilter 1 &&

... but these are OK and expected, ...

> +	git -C repo config core.repositoryformatversion 1 &&
> +	git -C repo config extensions.partialclone "origin" &&

... and this is way too different from what would happen in the real life.

I'd prefer not to see manual destruction of $GIT_DIR/objects/* or manual futzing of repository format version and extensions. These configuration variables are *NOT* for end-users to futz with, and the tests should not be doing so either.

Can't we prepare the "repo" only by creating a partial clone in an usual way?

> +	git -C repo config promisor.quiet "false" &&

This of course is good, as this is what the test wants to check.

> +	test_terminal git -C repo cat-file -p $(cat blobhash) 2>err &&

It seems that exactly the same set of comments apply to the next one, so I'll refrain from repeating myself.

Thanks.

Show 25 quoted lines

> +test_expect_success TTY 'promisor.quiet=true works' '
> +	rm -rf server server2 repo &&
> +	rm -rf server server3 repo &&
> +	test_create_repo server &&
> +	test_commit -C server foo &&
> +	git -C server repack -a -d --write-bitmap-index &&
> +
> +	git clone "file://$(pwd)/server" repo &&
> +	git hash-object repo/foo.t >blobhash &&
> +	rm -rf repo/.git/objects/* &&
> +
> +	git -C server config uploadpack.allowanysha1inwant 1 &&
> +	git -C server config uploadpack.allowfilter 1 &&
> +	git -C repo config core.repositoryformatversion 1 &&
> +	git -C repo config extensions.partialclone "origin" &&
> +	git -C repo config promisor.quiet "true" &&
> +
> +	test_terminal git -C repo cat-file -p $(cat blobhash) 2>err &&
> +
> +	# Ensure that no progress messages are written
> +	! grep "Receiving objects" err
> +'
> +
>  . "$TEST_DIRECTORY"/lib-httpd.sh
>  start_httpd