git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Philippe Blain <levraiphilippeblain@gmail.com>
To: Abhradeep Chakraborty via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Philip Oakley <philipoakley@iee.email>,
	Junio C Hamano <gitster@pobox.com>,
	Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Subject: Re: [PATCH v3] builtin/remote.c: teach `-v` to list filters for promisor remotes
Date: Sun, 8 May 2022 11:33:51 -0400	[thread overview]
Message-ID: <aa9884d5-b69a-bfd2-4235-a30326bd65f6@gmail.com> (raw)
In-Reply-To: <pull.1227.v3.git.1651933221216.gitgitgadget@gmail.com>

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.

  reply	other threads:[~2022-05-08 15:33 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-30 13:19 [PATCH] builtin/remote.c: teach `-v` to list filters for promisor remotes Abhradeep Chakraborty via GitGitGadget
2022-04-30 21:17 ` Junio C Hamano
2022-05-01 15:57   ` Abhradeep Chakraborty
2022-05-01 16:14     ` Junio C Hamano
2022-05-01 19:38       ` Abhradeep Chakraborty
2022-05-02 10:33         ` Philip Oakley
2022-05-02 14:56           ` Abhradeep Chakraborty
2022-05-03 15:20 ` [PATCH v2] " Abhradeep Chakraborty via GitGitGadget
2022-05-04 17:10   ` Junio C Hamano
2022-05-05 14:12     ` Abhradeep Chakraborty
2022-05-07 14:20   ` [PATCH v3] " Abhradeep Chakraborty via GitGitGadget
2022-05-08 15:33     ` Philippe Blain [this message]
2022-05-09 16:29       ` Junio C Hamano
2022-05-09 16:45         ` Philippe Blain
2022-05-08 15:44     ` Philippe Blain
2022-05-09  9:13       ` Abhradeep Chakraborty
2022-05-09 11:32     ` [PATCH v4] " Abhradeep Chakraborty via GitGitGadget
2022-05-09 15:34       ` Taylor Blau
2022-05-09 17:01         ` Philippe Blain
2022-05-09 17:52           ` Junio C Hamano
2022-05-13 13:49             ` Abhradeep Chakraborty
2022-05-13 18:37               ` Junio C Hamano
2022-05-16 15:38                 ` Abhradeep Chakraborty
2022-05-09 17:21         ` Abhradeep Chakraborty
2022-05-09 22:22           ` Taylor Blau
2022-05-09 17:44         ` Abhradeep Chakraborty

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aa9884d5-b69a-bfd2-4235-a30326bd65f6@gmail.com \
    --to=levraiphilippeblain@gmail.com \
    --cc=chakrabortyabhradeep79@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=philipoakley@iee.email \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).