mbox series

[v4,0/3] advice: add "all" option to disable all hints

Message ID 20240503071706.78109-1-james@jamesliu.io (mailing list archive)
Headers show
Series advice: add "all" option to disable all hints | expand

Message

James Liu May 3, 2024, 7:17 a.m. UTC
Hi,

Thank you all for the comprehensive feedback. This is v4 of the patch
series to introduce a --no-advice option for silencing advice hints.

The main changes are:

- Two preliminary commits to reorder/clean up the options documentation
  and usage string.
- Caching the value of GIT_ADVICE.
- Adding tests which explicitly set GIT_ADVICE to false and true.

Cheers,
James

James Liu (3):
  doc: clean up usage documentation for --no-* opts
  doc: add spacing around paginate options
  advice: add --no-advice global option

 Documentation/git.txt | 18 +++++++-----
 advice.c              |  7 +++++
 environment.h         |  7 +++++
 git.c                 | 11 +++++--
 t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 101 insertions(+), 10 deletions(-)

Range-diff against v3:
1:  55d5559586 < -:  ---------- advice: add --no-advice global option
-:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for --no-* opts
-:  ---------- > 2:  1b0019026a doc: add spacing around paginate options
-:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option

Comments

Dragan Simic May 3, 2024, 7:31 a.m. UTC | #1
Hello James,

On 2024-05-03 09:17, James Liu wrote:
>  Documentation/git.txt | 18 +++++++-----
>  advice.c              |  7 +++++
>  environment.h         |  7 +++++
>  git.c                 | 11 +++++--
>  t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 101 insertions(+), 10 deletions(-)
> 
> Range-diff against v3:
> 1:  55d5559586 < -:  ---------- advice: add --no-advice global option
> -:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for
> --no-* opts
> -:  ---------- > 2:  1b0019026a doc: add spacing around paginate 
> options
> -:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option

Just a small suggestion...  Perhaps the creation factor needs adjusting
for the range diff to actually be produced.  To be clear, I don't mind
that it's missing here.
karthik nayak May 3, 2024, 2:35 p.m. UTC | #2
Hello James,

James Liu <james@jamesliu.io> writes:

> Hi,
>
> Thank you all for the comprehensive feedback. This is v4 of the patch
> series to introduce a --no-advice option for silencing advice hints.
>
> The main changes are:
>
> - Two preliminary commits to reorder/clean up the options documentation
>   and usage string.
> - Caching the value of GIT_ADVICE.
> - Adding tests which explicitly set GIT_ADVICE to false and true.
>
> Cheers,
> James
>

This version looks good to me.

Thanks
Karthik

> James Liu (3):
>   doc: clean up usage documentation for --no-* opts
>   doc: add spacing around paginate options
>   advice: add --no-advice global option
>
>  Documentation/git.txt | 18 +++++++-----
>  advice.c              |  7 +++++
>  environment.h         |  7 +++++
>  git.c                 | 11 +++++--
>  t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 101 insertions(+), 10 deletions(-)
>
> Range-diff against v3:
> 1:  55d5559586 < -:  ---------- advice: add --no-advice global option
> -:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for --no-* opts
> -:  ---------- > 2:  1b0019026a doc: add spacing around paginate options
> -:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option
> --
> 2.44.0
Junio C Hamano May 3, 2024, 5:25 p.m. UTC | #3
> Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

Huh?  Do we have "all" option that disable everything?
James Liu May 5, 2024, 11:17 p.m. UTC | #4
On Sat May 4, 2024 at 12:35 AM AEST, Karthik Nayak wrote:
> Hello James,
>
> James Liu <james@jamesliu.io> writes:
>
> > Hi,
> >
> > Thank you all for the comprehensive feedback. This is v4 of the patch
> > series to introduce a --no-advice option for silencing advice hints.
> >
> > The main changes are:
> >
> > - Two preliminary commits to reorder/clean up the options documentation
> >   and usage string.
> > - Caching the value of GIT_ADVICE.
> > - Adding tests which explicitly set GIT_ADVICE to false and true.
> >
> > Cheers,
> > James
> >
>
> This version looks good to me.
>
> Thanks
> Karthik
>

Thanks for the review Karthik!

Cheers,
James
James Liu May 5, 2024, 11:20 p.m. UTC | #5
On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
> > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
>
> Huh?  Do we have "all" option that disable everything?

Oops, I think I mistakenly copied the subject from the original cover
letter. I don't suppose this warrants a v5, does it?

Cheers,
James
James Liu May 6, 2024, 1:10 a.m. UTC | #6
On Mon May 6, 2024 at 9:20 AM AEST, James Liu wrote:
> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
> > > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
> >
> > Huh?  Do we have "all" option that disable everything?
>
> Oops, I think I mistakenly copied the subject from the original cover
> letter. I don't suppose this warrants a v5, does it?
>
> Cheers,
> James

Ahh, I didn't see Karthik's feedback. Will prepare a new version.

Cheers,
James
Junio C Hamano May 6, 2024, 4:41 p.m. UTC | #7
"James Liu" <james@jamesliu.io> writes:

> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
>> > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
>>
>> Huh?  Do we have "all" option that disable everything?
>
> Oops, I think I mistakenly copied the subject from the original cover
> letter. I don't suppose this warrants a v5, does it?

No, I was just teasing you ;-).
Junio C Hamano May 6, 2024, 4:47 p.m. UTC | #8
"James Liu" <james@jamesliu.io> writes:

> On Mon May 6, 2024 at 9:20 AM AEST, James Liu wrote:
>> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
>> > > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
>> >
>> > Huh?  Do we have "all" option that disable everything?
>>
>> Oops, I think I mistakenly copied the subject from the original cover
>> letter. I don't suppose this warrants a v5, does it?
>>
>> Cheers,
>> James
>
> Ahh, I didn't see Karthik's feedback. Will prepare a new version.

I count two messages from Karthik for v4, one of them being "this
round looks good", to which you said "thanks".

If you mean the other comment about a non-sentence in the proposed
log message of [2/3], the copy I have in 'seen' says this:

    doc: add spacing around paginate options

    Make the documentation page consistent with the usage string printed by
    "git help git" and consistent with the description of "[-v | --version]"
    option.

    Signed-off-by: James Liu <james@jamesliu.io>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

If that looks good enough to both of you, then we can probably
declare victory and mark the topic for 'next'.

Thanks.
James Liu May 6, 2024, 11:08 p.m. UTC | #9
On Tue May 7, 2024 at 2:47 AM AEST, Junio C Hamano wrote:
> I count two messages from Karthik for v4, one of them being "this
> round looks good", to which you said "thanks".
>
> If you mean the other comment about a non-sentence in the proposed
> log message of [2/3], the copy I have in 'seen' says this:
>
>     doc: add spacing around paginate options
>
>     Make the documentation page consistent with the usage string printed by
>     "git help git" and consistent with the description of "[-v | --version]"
>     option.
>
>     Signed-off-by: James Liu <james@jamesliu.io>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> If that looks good enough to both of you, then we can probably
> declare victory and mark the topic for 'next'.
>
> Thanks.

Yes I'm happy with that. Thanks Junio!

Cheers,
James
karthik nayak May 7, 2024, 6:23 a.m. UTC | #10
Hello,

"James Liu" <james@jamesliu.io> writes:

> On Tue May 7, 2024 at 2:47 AM AEST, Junio C Hamano wrote:
>> I count two messages from Karthik for v4, one of them being "this
>> round looks good", to which you said "thanks".
>>
>> If you mean the other comment about a non-sentence in the proposed
>> log message of [2/3], the copy I have in 'seen' says this:
>>
>>     doc: add spacing around paginate options
>>
>>     Make the documentation page consistent with the usage string printed by
>>     "git help git" and consistent with the description of "[-v | --version]"
>>     option.
>>
>>     Signed-off-by: James Liu <james@jamesliu.io>
>>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> If that looks good enough to both of you, then we can probably
>> declare victory and mark the topic for 'next'.
>>
>> Thanks.
>
> Yes I'm happy with that. Thanks Junio!
>
> Cheers,
> James

Looks good to me too :)

Thanks
Karthik