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

From: Philippe Blain <levraiphilippeblain@gmail.com>
Date: May 8, 2022, 15:44 UTC
Message-ID: <f15e2673-ddc3-27ff-d31c-7fa32af27ae7@gmail.com>
In-Reply-To: <pull.1227.v3.git.1651933221216.gitgitgadget@gmail.com>

Forgot to comment on the patch itself :P

Le 2022-05-07 à 10:20, Abhradeep Chakraborty via GitGitGadget a écrit :

> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
>

>  Documentation/git-remote.txt |  3 +++
>  builtin/remote.c             | 18 +++++++++++++-----
>  t/t5616-partial-clone.sh     | 28 ++++++++++++++++++++++++++++

I think the tests woud fit better in t5505-remote.sh, since the patch really adds a feature to the 'git remote' command.

Show 11 quoted lines

>  3 files changed, 44 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index cde9614e362..a125bd839f7 100644
> --- a/Documentation/git-remote.txt
> +++ b/Documentation/git-remote.txt
> @@ -35,6 +35,9 @@ OPTIONS
>  -v::
>  --verbose::
>  	Be a little more verbose and show remote url after name.
> +	For promisor remotes it will show an extra information

I found it sligtly awkward to use the future tense here. Maybe just:

    For promisor remotes, also show which filter
    (`blob:none` etc.) that promisor remote use, wrapped in square brackets.

And technically, it's not really the remote that "uses" the filter, but more the local Git client. So maybe something like this would be more accurate and simpler:

    For promisor remotes, also show which filter (`blob:none` etc.)
    are configured, wrapped in square brackets.

And even then "wrapped in square brackets" *could* be dropped, I think.

Apart from that, the patch and test look good, thanks for working on that!

Cheers, Philippe.