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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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?

> +	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.

> +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