"Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@xxxxxxxxx>
writes:
> 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?
> 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
