Re: [PATCH] builtin/remote.c: teach `-v` to list filters for promisor remotes

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2022, 21:17 UTC
Message-ID: <xmqqczgy6zk5.fsf@gitster.g>
In-Reply-To: <pull.1227.git.1651324796892.gitgitgadget@gmail.com>

"Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

>  	if (remote->url_nr > 0) {
> +		struct strbuf promisor_config = STRBUF_INIT;
> +		const char *partial_clone_filter = NULL;
> +
> +		strbuf_addf(&promisor_config, "remote.%s.partialclonefilter", remote->name);
>  		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
> +		if (!git_config_get_string_tmp(promisor_config.buf, &partial_clone_filter))
> +			strbuf_addf(&url_buf, " [%s]", partial_clone_filter);
> +
> +		strbuf_release(&promisor_config);
>  		string_list_append(list, remote->name)->util =
>  				strbuf_detach(&url_buf, NULL);

Three comments and a half on the code:

 - Is it likely that to new readers it would be obvious that what is
   in the [square brackets] is the list-objects-filter used?  When we
   want to add new kinds of information other than the URL and the
   list-objects-filter, what is our plan to add them?

 - The presentation order is <remote-name> then <direction> (fetch
   or push) and then optionally <list-objects-filter>.

   (a) shouldn't the output format be described in the
       doucmentation?

   (b) does it make sense to append new information like this, or
       is it more logical to keep the <direction> at the end?

 - Now url_buf no longer contains the url of the remote, but it still
   is called url_buf.  It is merely a "temporary string" now.  Is it
   a good idea to either rename it, stop reusing the same thing for
   different purposes, or do something else?

 - By adding this unconditionally, we would break the scripts that
   read the output from this command and expect there won't be extra
   information after the <direction>.  It may be a good thing (they
   are not prepared to see the list-objects-filter, and the breakage
   may serve as a reminder that they need to update these scripts
   when they see breakage), or it may be an irritating regression.

But stepping back a bit.

Why do we want to give this in the "remote -v" output in the first place? When a reader really cares, they can ask "git config" for this extra piece of information. When you have more than one remote, "git remote -v" that gives the URL is a good way to remind which nickname you'd want to give to "git pull" or "git push". If it makes sense to add the extra <list-objects-filtrer> information, that would mean that there are probably two remote nicknames that refer to the same URL (i.e. "remote -v" readers cannot tell them apart without extra information), but how likely is that, I wonder?

Show 24 quoted lines

> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> index 4a3778d04a8..bf8f3644d3c 100755
> --- a/t/t5616-partial-clone.sh
> +++ b/t/t5616-partial-clone.sh
> @@ -49,6 +49,17 @@ test_expect_success 'do partial clone 1' '
>  	test "$(git -C pc1 config --local remote.origin.partialclonefilter)" = "blob:none"
>  '
>  
> +test_expect_success 'filters for promisor remotes is listed by git remote -v' '
> +	git clone --filter=blob:none "file://$(pwd)/srv.bare" pc2 &&
> +	git -C pc2 remote -v >out &&
> +	grep "[blob:none]" out &&
> +
> +	git -C pc2 config remote.origin.partialCloneFilter object:type=commit &&
> +	git -C pc2 remote -v >out &&
> +	grep "[object:type=commit]" out &&
> +	rm -rf pc2
> +'
> +
>  test_expect_success 'verify that .promisor file contains refs fetched' '
>  	ls pc1/.git/objects/pack/pack-*.promisor >promisorlist &&
>  	test_line_count = 1 promisorlist &&
>
> base-commit: 0f828332d5ac36fc63b7d8202652efa152809856