builtin/remote.c: teach -v to list filters for promisor remotes

[Abhra303]

Fixes #1211 [1]

Changes since v3:

• tests are moved to t5505-remote.sh
• Documentation improved
• Added Closes trailer in the commit message

Changes since v2:

• added more test cases
• fixed broken indentations

Changes since v1:

• updated documentation
• renamed url_buf into remote_info_buf

[1] #1211

cc: Philip Oakley philipoakley@iee.email
cc: Junio C Hamano gitster@pobox.com
cc: Philippe Blain levraiphilippeblain@gmail.com
cc: Taylor Blau me@ttaylorr.com

[phil-blain]

Hi @Abhra303, thanks for having a go at that! I encourage you to send it to the list for review :)

[Abhra303]

@phil-blain , Yes I will. I was waiting for the tests to complete.

Thanks :)

[Abhra303]

/submit

[gitgitgadget]

Submitted as pull.1227.git.1651324796892.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1227/Abhra303/promisor_remote-v1

To fetch this version to local tag pr-1227/Abhra303/promisor_remote-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1227/Abhra303/promisor_remote-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Abhradeep Chakraborty via GitGitGadget" <gitgitgadget@gmail.com>
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

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Sorry for the late response.

Junio C Hamano <gitster@pobox.com> wrote:

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

I do think that new readers can easily understand the meaning of the
text inside the [square brackets]. These square brackets (with the
list-objects-filter inside it) will be shown only if the remote is
a promisor remote. So, users who don't use promisor remotes, will not
be affected. Those who used the filters can only notice the change.
They can easily understand it. In fact, I think it would give them an
option to quickly check which are the promisor remotes and which are not.
Though this change should be properly documented (which I forgot to
add) so that they can be sure about it.

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

Yeah, it should be documented. I forgot it :|
Will add it in the next version.

I think it is better to keep <list-objects-filter> at the end.
Because I think, people first want to check whether the remote
is (fetch) or (push). After that, they might want to know about the
filter. Another point is that <list-objects-filter> is optional
(i.e. only for promisor remotes). It would not make sense to put an
optional info in between two permanent info (in this case,
<remote-name> and <direction>). It would be difficult for scripts
which parse the output of `git remote -v` on the basis of string
positions.

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

Hmm, this can be a subject for discussion. Yes, it is true that the
name `url_buf` is not suitable for the additional info it contains ( in
the proposed change). I did it to use less memory. I think renaming it
to `remote_info_buf` or similar is a better idea.

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

I agree. Frankly speaking, I have no counter argument for this. I can
tell that the proposed change will be beneficial for the users who use
promisor remotes along with other remotes. So, may be we can accept the
short term consequences of it. What we can do is we can provide a proper
documentation so that if anything bad happen to those scripts, devs can
see the documentation and update the scripts accordingly.

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

`remote -v` helps users to get the overall idea of the remotes. We can
see how many remotes are there, which remote name corresponds to which
url etc. That is we can get a summary of remotes. Having that said, does
not it make sense to add the extra <list-objects-filter> here? Users
can easily understand which are promisor remotes ( along with their
filter type) and which are not. Of course, they can use git config for
that. But it would be a tidious job to check the the type of remotes
(i.e. which are promisor remotes and which are not) one by one. If the
user try to search for the promisor remotes in the config file, he/she
have to go through the other configuration settings (irrelevant to him/her
at that time) to reach the `[remote]` section. Isn't it?

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

I think, having a proper documentation about the new changes is the
answer to it.


Thanks :)

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes:

> Sorry for the late response.
>
> Junio C Hamano <gitster@pobox.com> wrote:
>
>> 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? 
>
> I do think that new readers can easily understand the meaning of the
> text inside the [square brackets]. These square brackets (with the
> list-objects-filter inside it) will be shown only if the remote is
> a promisor remote. So, users who don't use promisor remotes, will not
> be affected. Those who used the filters can only notice the change.
> They can easily understand it. In fact, I think it would give them an
> option to quickly check which are the promisor remotes and which are not.
> Though this change should be properly documented (which I forgot to
> add) so that they can be sure about it.

You forgot to answer more important half of the question.  It would
be easy for you to know what the string inside brackets means
because you are so obsessed with the promisor remote to write this
patch ;-) But when we need to add even more pieces of information in
the future, will it stay so?  Can "[some-random-string]" easily be
identified as a list-objects-filter by those who do not care
particularly about promisor remotes (e.g. those who wanted to see
the URL to tell multiple remote nicknames apart) when the line has
even more piece of information in the future?

At some point, we'd need to either (1) stop adding too many details
to avoid cluttering the output line, or (2) start labeling each
piece of information to make it easy for the readers to identify
which one is which [*].  We need to ask ourselves why now is not
that "some point" already.

    Side note: and the strategy to add new pieces of information
    need to take the same approach between the two, and that is why
    we need "what is the plan to add new pieces of information?"
    answered.

> (i.e. which are promisor remotes and which are not) one by one. If the
> user try to search for the promisor remotes in the config file, he/she
> have to go through the other configuration settings (irrelevant to him/her
> at that time) to reach the `[remote]` section. Isn't it?

Sorry, but the question does not make much sense to me.  Why is a
piece of information you get from "git config" irrelevant if you get
it in order to figure out what you want to know, i.e.  what promisor
remote do we rely on?

>> ...  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?
>
> I think, having a proper documentation about the new changes is the
> answer to it.

The question is "what can readers achieve by having this extra
information in 'remote -v' output".  Do you have to duck the
question because you cannot answer in a simple sentence, and instead
readers must read reams of documentation pages?  I doubt it would be
that obscure.

I wanted to like the patch, the changed text is simple enough, but
quite honestly, the lack of clarity in the answers to the most basic
"why do we want this? what is this good for? how does this help the
users?" questions, I am not yet succeeding to do so.

Thanks.

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Junio C Hamano <gitster@pobox.com> wrote:

> You forgot to answer more important half of the question.  It would
> be easy for you to know what the string inside brackets means
> because you are so obsessed with the promisor remote to write this
> patch ;-) But when we need to add even more pieces of information in
> the future, will it stay so?  Can "[some-random-string]" easily be
> identified as a list-objects-filter by those who do not care
> particularly about promisor remotes (e.g. those who wanted to see
> the URL to tell multiple remote nicknames apart) when the line has
> even more piece of information in the future?
>
> At some point, we'd need to either (1) stop adding too many details
> to avoid cluttering the output line, or (2) start labeling each
> piece of information to make it easy for the readers to identify
> which one is which [*].  We need to ask ourselves why now is not
> that "some point" already.
>
>     Side note: and the strategy to add new pieces of information
>     need to take the same approach between the two, and that is why
>     we need "what is the plan to add new pieces of information?"
>     answered.

I am sorry if I failed to explain you what I really wanted to mean.
Yes, I forgot to answer the last question which is "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?".

So let me answer this now. As `-v` flag gives a kind of overall summary
of the remotes, users expect that the most important and most basic
information should be listed in the output of `remote -v`. So, there
may be some other more important informations in the future that we
have to add to `remote -v` output. In that case, method (1) would not
be a great idea I think (unless a new flag has been created). method
(2) is better.

> > (i.e. which are promisor remotes and which are not) one by one. If the
> > user try to search for the promisor remotes in the config file, he/she
> > have to go through the other configuration settings (irrelevant to him/her
> > at that time) to reach the `[remote]` section. Isn't it?
>
> Sorry, but the question does not make much sense to me.  Why is a
> piece of information you get from "git config" irrelevant if you get
> it in order to figure out what you want to know, i.e.  what promisor
> remote do we rely on?

Let me explain what I really meant here - I am guessing that you have no
problem with the upper part of that para.

If we forget about my proposed change, there are two possible ways to find
out the info about promisor remotes - 
	(1) Use `git config --get remote.<remote-name>.partialCloneFilter`

	   This command gives an output only if <remote-name> is a promisor
	   remote. So in case the user forget which one is a promisor
	   remote, he/she has to try this command with each and every
	   <remote-name> to find out which is/are the promisor remote(s).

	(2) Open the git config file (either manually or by running `git
	    config --edit`

	    In this case, the user has to go through all the settings until
	    the [remote "<remote-name>"] section is found. E.g. let's say
	    below is the config file - 

	    [core]
        	repositoryformatversion = 0
        	filemode = true
        	bare = false
        	logallrefupdates = true
        	ignorecase = true
        	precomposeunicode = true
	    [remote "upstream"]
        	url = https://github.com/git/git.git
        	fetch = +refs/heads/*:refs/remotes/upstream/*
	    [branch "master"]
        	remote = upstream
        	merge = refs/heads/master
	    [remote "origin"]
        	url = https://github.com/Abhra303/git.git
        	fetch = +refs/heads/*:refs/remotes/origin/*
		partialCloneFilter = blob:none

	    To find out whether "origin" is promisor or not, he has to go
	    to the [remote "origin"] section. Here all the configuations
	    under `[core]`, `[remote "upstream"]` and `[branch "master"]
	    are irrelevant to him/her at that time (because he/she is not
	    interested to know about those configuration settings at that
	    time).

The proposed change is simpler compared to the above as it lists down all
the remotes along with their list-objects-filter. Another point is that
it's important for an user to know which one is a promisor remote and what
filter type they use. If we go with the current implementation the output
would be let's say - 
origin <remote-url> (fetch)
origin <remote-url> (push)
upstream <remote-url> (fetch)
upstream <remote-url> (push)

By seeing the above output anyone may assume that all the remotes are
normal remotes. If the user now try to run `git pull origin` and suddenly
he/she discover that some blobs are not downloaded. He/she run the above
mentioned (1) command and find that this is a promisor remote!

Here `remote -v` didn't warn the user about the origin remote being an
promisor remote. Instead it makes him/her assume that all are normal
remotes. Providing only these three info (i.e. <remote-name>, <remote-url>
and <direction>) is not sufficient - it only shows the half of the picture.


> The question is "what can readers achieve by having this extra
> information in 'remote -v' output".  Do you have to duck the
> question because you cannot answer in a simple sentence, and instead
> readers must read reams of documentation pages?  I doubt it would be
> that obscure.

Sorry, I misunderstood that section of your first comment. I think
I hopefully answered this question in the above portion of this comment.
Providing only the three information about remotes is not sufficient
as it do not distinguish between promisor remotes and normal remotes.
In that sense, it will add simplicity and the user would be much more
clear about the remotes(i.e. which is promisor remote and which is not).

> I wanted to like the patch, the changed text is simple enough, but
> quite honestly, the lack of clarity in the answers to the most basic
> "why do we want this? what is this good for? how does this help the
> users?" questions, I am not yet succeeding to do so.

My bad! Hope I am now able to answer all the questions you asked. Let
me know if you still struggle to get my point.

Thanks :)

[gitgitgadget]

On the Git mailing list, Philip Oakley wrote (reply to this):

On 01/05/2022 20:38, Abhradeep Chakraborty wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
>
>> You forgot to answer more important half of the question.  It would
>> be easy for you to know what the string inside brackets means
>> because you are so obsessed with the promisor remote to write this
>> patch ;-) But when we need to add even more pieces of information in
>> the future, will it stay so?  Can "[some-random-string]" easily be
>> identified as a list-objects-filter by those who do not care
>> particularly about promisor remotes (e.g. those who wanted to see
>> the URL to tell multiple remote nicknames apart) when the line has
>> even more piece of information in the future?
>>
>> At some point, we'd need to either (1) stop adding too many details
>> to avoid cluttering the output line, or (2) start labeling each
>> piece of information to make it easy for the readers to identify
>> which one is which [*].  We need to ask ourselves why now is not
>> that "some point" already.
>>
>>     Side note: and the strategy to add new pieces of information
>>     need to take the same approach between the two, and that is why
>>     we need "what is the plan to add new pieces of information?"
>>     answered.
> I am sorry if I failed to explain you what I really wanted to mean.
> Yes, I forgot to answer the last question which is "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?".
>
> So let me answer this now. As `-v` flag gives a kind of overall summary
> of the remotes, users expect that the most important and most basic
> information should be listed in the output of `remote -v`. So, there
> may be some other more important informations in the future that we
> have to add to `remote -v` output. In that case, method (1) would not
> be a great idea I think (unless a new flag has been created). method
> (2) is better.

When I use the `git remote` command, I use the -vv variant to what what
I need, i.e. its more than `-v`, so maybe adding an extra
`--show-partial-filter` option may be necessary (with a more compact
name ;-).

There will also be a similar desire (IIUC) to match the sparse/cone mode
repos to their remotes, i.e. to remind a user that what is held at the
remote isn't the same as held locally.
>
>>> (i.e. which are promisor remotes and which are not) one by one. If the
>>> user try to search for the promisor remotes in the config file, he/she
>>> have to go through the other configuration settings (irrelevant to him/her
>>> at that time) to reach the `[remote]` section. Isn't it?
>> Sorry, but the question does not make much sense to me.  Why is a
>> piece of information you get from "git config" irrelevant if you get
>> it in order to figure out what you want to know, i.e.  what promisor
>> remote do we rely on?
> Let me explain what I really meant here - I am guessing that you have no
> problem with the upper part of that para.
>
> If we forget about my proposed change, there are two possible ways to find
> out the info about promisor remotes - 
> 	(1) Use `git config --get remote.<remote-name>.partialCloneFilter`
>
> 	   This command gives an output only if <remote-name> is a promisor
> 	   remote. So in case the user forget which one is a promisor
> 	   remote, he/she has to try this command with each and every
> 	   <remote-name> to find out which is/are the promisor remote(s).

I hear your pain here. I had the same issue with the branch description.
(https://stackoverflow.com/questions/15058844/print-branch-description).
It's the same 'extract from config' problem.
 
```You can display the branch description using a git config command.

To show all branch descriptions, I have the alias

brshow = config --get-regexp 'branch.*.description'

, and for the current HEAD I have

brshow1 = !git config --get "branch.$(git rev-parse --abbrev-ref
HEAD).description". ```

so it is possible to generalise the config query, if hard to discover.
<https://stackoverflow.com/a/15062356/717355>
>
> 	(2) Open the git config file (either manually or by running `git
> 	    config --edit`
>
> 	    In this case, the user has to go through all the settings until
> 	    the [remote "<remote-name>"] section is found. E.g. let's say
> 	    below is the config file - 
>
> 	    [core]
>         	repositoryformatversion = 0
>         	filemode = true
>         	bare = false
>         	logallrefupdates = true
>         	ignorecase = true
>         	precomposeunicode = true
> 	    [remote "upstream"]
>         	url = https://github.com/git/git.git
>         	fetch = +refs/heads/*:refs/remotes/upstream/*
> 	    [branch "master"]
>         	remote = upstream
>         	merge = refs/heads/master
> 	    [remote "origin"]
>         	url = https://github.com/Abhra303/git.git
>         	fetch = +refs/heads/*:refs/remotes/origin/*
> 		partialCloneFilter = blob:none
>
> 	    To find out whether "origin" is promisor or not, he has to go
> 	    to the [remote "origin"] section. Here all the configuations
> 	    under `[core]`, `[remote "upstream"]` and `[branch "master"]
> 	    are irrelevant to him/her at that time (because he/she is not
> 	    interested to know about those configuration settings at that
> 	    time).
>
> The proposed change is simpler compared to the above as it lists down all
> the remotes along with their list-objects-filter. Another point is that
> it's important for an user to know which one is a promisor remote and what
> filter type they use. If we go with the current implementation the output
> would be let's say - 
> origin <remote-url> (fetch)
> origin <remote-url> (push)
> upstream <remote-url> (fetch)
> upstream <remote-url> (push)
>
> By seeing the above output anyone may assume that all the remotes are
> normal remotes. If the user now try to run `git pull origin` and suddenly
> he/she discover that some blobs are not downloaded. He/she run the above
> mentioned (1) command and find that this is a promisor remote!
>
> Here `remote -v` didn't warn the user about the origin remote being an
> promisor remote. Instead it makes him/her assume that all are normal
> remotes. Providing only these three info (i.e. <remote-name>, <remote-url>
> and <direction>) is not sufficient - it only shows the half of the picture.
>
>
>> The question is "what can readers achieve by having this extra
>> information in 'remote -v' output".  Do you have to duck the
>> question because you cannot answer in a simple sentence, and instead
>> readers must read reams of documentation pages?  I doubt it would be
>> that obscure.
> Sorry, I misunderstood that section of your first comment. I think
> I hopefully answered this question in the above portion of this comment.
> Providing only the three information about remotes is not sufficient
> as it do not distinguish between promisor remotes and normal remotes.
> In that sense, it will add simplicity and the user would be much more
> clear about the remotes(i.e. which is promisor remote and which is not).
>
>> I wanted to like the patch, the changed text is simple enough, but
>> quite honestly, the lack of clarity in the answers to the most basic
>> "why do we want this? what is this good for? how does this help the
>> users?" questions, I am not yet succeeding to do so.
> My bad! Hope I am now able to answer all the questions you asked. Let
> me know if you still struggle to get my point.
>
> Thanks :)

[gitgitgadget]

User Philip Oakley <philipoakley@iee.email> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Philip Oakley <philipoakley@iee.email> wrote:

> When I use the `git remote` command, I use the -vv variant to what what
> I need, i.e. its more than `-v`, so maybe adding an extra
> `--show-partial-filter` option may be necessary (with a more compact
> name ;-).

If adding new informations to `-v` is not possible (to avoid messy output),
atleast including it to `-vv` makes sense to me (though I am not sure if 
`git remote -vv` is currently implemented). 

> There will also be a similar desire (IIUC) to match the sparse/cone mode
> repos to their remotes, i.e. to remind a user that what is held at the
> remote isn't the same as held locally.

Yeah, maybe.

> I hear your pain here. I had the same issue with the branch description.
> (https://stackoverflow.com/questions/15058844/print-branch-description).
> It's the same 'extract from config' problem.
>
> ```You can display the branch description using a git config command.
>
> To show all branch descriptions, I have the alias
>
> brshow = config --get-regexp 'branch.*.description'
>
> , and for the current HEAD I have
>
> brshow1 = !git config --get "branch.$(git rev-parse --abbrev-ref
> HEAD).description". ```
>
> so it is possible to generalise the config query, if hard to discover.
> <https://stackoverflow.com/a/15062356/717355>

Thanks for the info. I tried your suggestion and it worked. But still,
it is better to include <list-object-filter> in the output. This is
because of the second point I mentioned in my previous comment. Users
can be much more clear about the types of available remotes overall.
IMO specifying filters for remotes is far more important than the
branch description. The behaviour of `git fetch` depends on it. If
we can specify `(fetch)` in the output then why not the filter of that
`fetch` on which the behaviour of `fetch` functionality highly depends?

Thanks :)

[Abhra303]

/submit

[gitgitgadget]

Submitted as pull.1227.v2.git.1651591253333.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1227/Abhra303/promisor_remote-v2

To fetch this version to local tag pr-1227/Abhra303/promisor_remote-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1227/Abhra303/promisor_remote-v2

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
> index cde9614e362..71a0e85990d 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
> +  (wrapped in square brackets) describing which filter
> +  (`blob:none` etc.) that promisor remote use.
>  	NOTE: This must be placed between `remote` and subcommand.

Broken indentation.  You can save embarrassment by double checking
what you committed by sending e-mail to yourself (or checking output
from "git show") before sending it to the list.

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5f4cde9d784..d4b69fe7789 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1185,14 +1185,22 @@ static int show_push_info_item(struct string_list_item *item, void *cb_data)
>  static int get_one_entry(struct remote *remote, void *priv)
>  {
>  	struct string_list *list = priv;
> -	struct strbuf url_buf = STRBUF_INIT;
> +	struct strbuf remote_info_buf = STRBUF_INIT;
>  	const char **url;
>  	int i, url_nr;
>  
>  	if (remote->url_nr > 0) {
> -		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
> +		struct strbuf promisor_config = STRBUF_INIT;
> +		const char *partial_clone_filter = NULL;
> +
> +		strbuf_addf(&promisor_config, "remote.%s.partialclonefilter", remote->name);
> +		strbuf_addf(&remote_info_buf, "%s (fetch)", remote->url[0]);
> +		if (!git_config_get_string_tmp(promisor_config.buf, &partial_clone_filter))
> +			strbuf_addf(&remote_info_buf, " [%s]", partial_clone_filter);
> +
> +		strbuf_release(&promisor_config);
>  		string_list_append(list, remote->name)->util =
> -				strbuf_detach(&url_buf, NULL);
> +				strbuf_detach(&remote_info_buf, NULL);

It is unfortunate that the "we borrow without copying" variant of
git_config_get_string() is called git_config_get_string_tmp(), which
is an utterly misleading name that might confuse readers into
mistaking it may make a temporary copy for the caller to release.
Perhaps we would want to rename it to git_config_peek_string() or
something, but that is totally outside the topic, of course.

In any case, what I wanted to say is that I just made sure that the
value in the partial_clone_filter variable is not leaked.

Looking good.

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

I doubt that these "grep" do what you think it is doing.  It would
say "I am happy" on any line that has one of these characters listed
inside the [].

Do not clean up with an extra "&& clean up" step at the end of
&&-cascade.  Instead use test_when_finished to make sure that after
any failure in the cascade the clean-up step would still trigger.

	test_expect_success 'title' '
		test_when_finished "rm -fr pc2" &&
		git clone ... &&
		...
		grep "srv.bare (fetch) \[object:type=commit\]" out
	'

or something.

Having tests that show how this new feature works is of course
necessary, but we must have negative tests that ensure that it does
*not* trigger when it should not.  E.g. the new [filter-spec] should
not be given for a remote if the user didn't ask for "-v", or the
remote is not a promisor.

Thanks.

[gitgitgadget]

This branch is now known as ac/remote-v-with-object-list-filters.

[gitgitgadget]

This patch series was integrated into seen via git@ffd5311.

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Junio C Hamano <gitster@pobox.com> wrote:

> Broken indentation.  You can save embarrassment by double checking
> what you committed by sending e-mail to yourself (or checking output
> from "git show") before sending it to the list.

Thanks for the suggestions. Will keep it in mind next time.

> I doubt that these "grep" do what you think it is doing.  It would
> say "I am happy" on any line that has one of these characters listed
> inside the [].
>
> Do not clean up with an extra "&& clean up" step at the end of
> &&-cascade.  Instead use test_when_finished to make sure that after
> any failure in the cascade the clean-up step would still trigger.
>
>	test_expect_success 'title' '
>		test_when_finished "rm -fr pc2" &&
>		git clone ... &&
>		...
> 		grep "srv.bare (fetch) \[object:type=commit\]" out
> 	'
>
> or something.
>
> Having tests that show how this new feature works is of course
> necessary, but we must have negative tests that ensure that it does
> *not* trigger when it should not.  E.g. the new [filter-spec] should
> not be given for a remote if the user didn't ask for "-v", or the
> remote is not a promisor.

Got it. Will send the necessary changes by the day after tommorow.

Thanks :)

[gitgitgadget]

There was a status update in the "New Topics" section about the branch ac/remote-v-with-object-list-filters on the Git mailing list:

source: <pull.1227.v2.git.1651591253333.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@8001664.

[Abhra303]

/preview

[gitgitgadget]

Preview email sent as pull.1227.v3.git.1651932390964.gitgitgadget@gmail.com

[Abhra303]

/submit

[gitgitgadget]

Submitted as pull.1227.v3.git.1651933221216.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1227/Abhra303/promisor_remote-v3

To fetch this version to local tag pr-1227/Abhra303/promisor_remote-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1227/Abhra303/promisor_remote-v3

[gitgitgadget]

On the Git mailing list, Philippe Blain wrote (reply to this):

Hi Abhradeep,

Le 2022-05-07 à 10:20, Abhradeep Chakraborty via GitGitGadget a écrit :
> From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> 
> `git remote -v` (`--verbose`) lists down the names of remotes along with
> their urls. 

small nit: I would capitalize URLs.

> It would be beneficial for users to also specify the filter
> types for promisor remotes. Something like this -
> 
> 	origin	remote-url (fetch) [blob:none]
> 	origin	remote-url (push)
> 
> Teach `git remote -v` to also specify the filters for promisor remotes.
> 
> Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
> ---
>     builtin/remote.c: teach -v to list filters for promisor remotes
>     
>     Fixes #1211 [1]

I don't think this matters much, but if Junio is OK with that, it would
be nice to include the reference to the GitGitGadget issue in the commit
message itself, though with its full URL, something like:

Closes: https://github.com/gitgitgadget/git/issues/1211

as another trailer before your signed-off-by. By including it in the 
commit message we allow the issue to be closed automatically when your topic
branch is merged to 'master'. By using the full link we make sure that GitHub 
knows we are targetting that issue specifically, not any other issue or PR in 
any fork of Git with the same number.

>     
>     In the previous version, documentation is updated (describing the
>     proposed change) and url_buf is renamed into remote_info_buf. In this
>     varsion, some more test cases are added and broken indentations are
>     fixed.

Again, small nit to make it easier for reviewers: usually we prefer to see
what has changed since the previous version first, and then (if you want, 
it's not strictly necessary) what changed in the other previous versions. 
It's not necessary since if we want that info we can refer to the cover letters
of the previous iterations directly. And ideally, in bullet points. So something like:

Changes since v2:
- added more test cases
- fixed broken indentations

Changes since v1:
- updated documentation
- renamed url_buf into remote_info_buf

>     
>     [1] https://github.com/gitgitgadget/git/issues/1211
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1227%2FAbhra303%2Fpromisor_remote-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1227/Abhra303/promisor_remote-v3
> Pull-Request: https://github.com/gitgitgadget/git/pull/1227

Thanks,

Philippe.

[gitgitgadget]

User Philippe Blain <levraiphilippeblain@gmail.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Philippe Blain wrote (reply to this):

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. 

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

[gitgitgadget]

This patch series was integrated into seen via git@bc7d1df.

[gitgitgadget]

This patch series was integrated into seen via git@b585b80.

[gitgitgadget]

This patch series was integrated into seen via git@e6ba461.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ac/remote-v-with-object-list-filters on the Git mailing list:

"git remote -v" now shows the list-objects-filter used during
fetching from the remote, if available.
source: <pull.1227.v4.git.1652095969026.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@482f593.

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Junio C Hamano <gitster@pobox.com> wrote:

> Isn't it where we already are with "remote -v", though?  I am not
> sure addition of excess information that may not be universally
> useful is a very welcome change, even with "remote -v -v".  I am not
> worried about showing the "list-object-filter", but I worry about
> managing temptations of future developers to add other stuff.

If future developers come up with some really useful stuff (i.e. 
universally useful), I think those should be added in the output
irrespective of the no of existing info in the output. If the
output becomes messy, we should focus on how we can make the output
clear may be using tabular format.

Else you can drop the idea and suggest them to introduce a new flag
(depending on the situation). If you still have some doubt about my
PR i.e. if you can not determine which category my PR belongs to, I
can go with adding `show-partial-clone` flag. The downside would
be that `remote -v` will not give the full summary in case of partial
clone.

If you like the tabular format approach, I am further going to propose
a table format -

+---------------+----------------------------------------------+
|  remote name  |          remote info                         |
+---------------+--------+--------+----------------------------+
|               |        | url    | https://blah.com/blah.git  |
|  origin       |        +--------+----------------------------+
|               |        | filter | blob:none                  |
|               | fetch  +--------+----------------------------+
|               |        | .                                   |
|               |        | .     (some important data)         |
|               +--------+--------+----------------------------+
|               |        | url    | https://blah.com/blah.git  |
|               | push   +--------+----------------------------+
|               |        | ... (some important data)           |
+---------------+--------+-------------------------------------+

In this way, user can see the summary of all remotes with visual ease.
Of course it is not suitable for scripting. In that case we can use
a new flag `--raw` which will let `-v` to provide a space/tab seperated
sequence of info (similar to current format).

Let me know if you (as in all) like/dislike my view and give your
arguments regarding my proposal.

Thanks :) 

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes:

> Else you can drop the idea and suggest them to introduce a new flag
> (depending on the situation). If you still have some doubt about my
> PR i.e. if you can not determine which category my PR belongs to, I
> can go with adding `show-partial-clone` flag. The downside would
> be that `remote -v` will not give the full summary in case of partial
> clone.

If majority of partial-clone users find it unnecessary noise, then
it may be an upside to give only reduced summary that is less than
full that may be given by `remote -v -v`.

Worse downside of adding it as an option is that it invites more
options.  It is less worse to add new ones to `remote -v -v` (or to
`remote -v`, or not adding it at all) than adding another option, I
would think.

Perhaps tagged output that can be easier to parse would be better
"extensible" output format for adding more random pieces of
information than going tabular.  I dunno.

[gitgitgadget]

This patch series was integrated into seen via git@78012c5.

[gitgitgadget]

On the Git mailing list, Abhradeep Chakraborty wrote (reply to this):

Junio C Hamano <gitster@pobox.com> wrote:

> If majority of partial-clone users find it unnecessary noise, then
> it may be an upside to give only reduced summary that is less than
> full that may be given by `remote -v -v`.

Should I add this to `remote -v -v` then?  `remote -vv` is currently
not implemented I guess.

> Perhaps tagged output that can be easier to parse would be better
> "extensible" output format for adding more random pieces of
> information than going tabular.  I dunno.

I am not sure what exactly you are refering by 'tagged output' but
it is true that tabular form is hard to parse. That's why I suggested
`--raw` flag which would be used for parsing. It would give the info
following the currently implemented format.

If you like the tagged output format, then should we implement `-vv` which
would give the output as the tagged output format and also can be
extended?

[gitgitgadget]

This patch series was integrated into seen via git@82d8600.

[gitgitgadget]

This patch series was integrated into seen via git@599fbfa.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ac/remote-v-with-object-list-filters on the Git mailing list:

"git remote -v" now shows the list-objects-filter used during
fetching from the remote, if available.

Will merge to 'next'?
source: <pull.1227.v4.git.1652095969026.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@6a7b2b2.

[gitgitgadget]

This patch series was integrated into seen via git@3603ceb.

[gitgitgadget]

This patch series was integrated into seen via git@90310b4.

[gitgitgadget]

This patch series was integrated into seen via git@0b5e3e7.

[gitgitgadget]

This patch series was integrated into seen via git@ebd9661.

[gitgitgadget]

This patch series was integrated into next via git@8d2dc10.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ac/remote-v-with-object-list-filters on the Git mailing list:

"git remote -v" now shows the list-objects-filter used during
fetching from the remote, if available.

Will merge to 'master'.
source: <pull.1227.v4.git.1652095969026.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@e118784.

[gitgitgadget]

This patch series was integrated into seen via git@c16a9d4.

[gitgitgadget]

This patch series was integrated into seen via git@6f4fd80.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch ac/remote-v-with-object-list-filters on the Git mailing list:

"git remote -v" now shows the list-objects-filter used during
fetching from the remote, if available.

Will merge to 'master'.
source: <pull.1227.v4.git.1652095969026.gitgitgadget@gmail.com>

[gitgitgadget]

This patch series was integrated into seen via git@2785b71.

[gitgitgadget]

This patch series was integrated into master via git@2785b71.

[gitgitgadget]

This patch series was integrated into next via git@2785b71.

[gitgitgadget]

Closed via 2785b71.