Re: [PATCH v2] advice: suggest using subcommand "git config set"

["Bence Ferdinandy" <bence@ferdinandy.com>]

On Mon Dec 09, 2024 at 12:21, Bence Ferdinandy <bence@ferdinandy.com> wrote:
>
> Thanks for taking a looking and the follow-up patches!
>
> On Sun Dec 08, 2024 at 09:08, Rubén Justo <rjusto@gmail.com> wrote:
>> On Thu, Dec 05, 2024 at 01:21:58PM +0100, Bence Ferdinandy wrote:
>>
>>> The advice message currently suggests using "git config advice..." to
>>> disable advice messages, but since
>>> 
>>> 00bbdde141 (builtin/config: introduce "set" subcommand, 2024-05-06)
>>> 
>>> we have the "set" subcommand for config. Since using the subcommand is
>>> more in-line with the modern interface, any advice should be promoting
>>> its usage. Change the disable advice message to use the subcommand
>>> instead.
>>
>> It's very consistent to keep our messages updated with respect to
>> changes in the user interface.  So this patch is a step in the right
>> direction.  Thanks for working on this.
>>
>>> Change all uses of "git config advice" in the tests to use the
>>> subcommand.
>>
>> Maybe this should be done in a separate patch.
>
> So I was a bit lazy here, since sed changed both the expected test outputs and
> the usage, so that could certainly be split into two patches to be prudent.
>
>>
>>> 
>>> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
>>> ---
>>> 
>>> Notes:
>>>     For the tests I just indiscriminately ran:
>>>     sed -i "s/git config advice\./git config set advice./" t[0-9]*.sh
>>>     
>>>     v2: - fixed 3 hardcoded "git config advice" type messages
>>>         - made the motiviation more explicit
>>> 
>>>  advice.c                        | 2 +-
>>>  commit.c                        | 2 +-
>>>  hook.c                          | 2 +-
>>>  object-name.c                   | 2 +-
>>>  t/t0018-advice.sh               | 2 +-
>>>  t/t3200-branch.sh               | 2 +-
>>>  t/t3404-rebase-interactive.sh   | 6 +++---
>>>  t/t3501-revert-cherry-pick.sh   | 2 +-
>>>  t/t3507-cherry-pick-conflict.sh | 6 +++---
>>>  t/t3510-cherry-pick-sequence.sh | 2 +-
>>>  t/t3511-cherry-pick-x.sh        | 2 +-
>>>  t/t3602-rm-sparse-checkout.sh   | 2 +-
>>>  t/t3700-add.sh                  | 6 +++---
>>>  t/t3705-add-sparse-checkout.sh  | 2 +-
>>>  t/t7002-mv-sparse-checkout.sh   | 4 ++--
>>>  t/t7004-tag.sh                  | 2 +-
>>>  t/t7201-co.sh                   | 4 ++--
>>>  t/t7400-submodule-basic.sh      | 2 +-
>>>  t/t7508-status.sh               | 2 +-
>>>  19 files changed, 27 insertions(+), 27 deletions(-)
>>> 
>>> diff --git a/advice.c b/advice.c
>>> index 6b879d805c..f7a5130c2c 100644
>>> --- a/advice.c
>>> +++ b/advice.c
>>> @@ -93,7 +93,7 @@ static struct {
>>>  
>>>  static const char turn_off_instructions[] =
>>>  N_("\n"
>>> -   "Disable this message with \"git config advice.%s false\"");
>>> +   "Disable this message with \"git config set advice.%s false\"");
>>
>> The main goal of this patch.  Good.
>>
>>>  
>>>  static void vadvise(const char *advice, int display_instructions,
>>>  		    const char *key, va_list params)
>>> diff --git a/commit.c b/commit.c
>>> index cc03a93036..35ab9bead5 100644
>>> --- a/commit.c
>>> +++ b/commit.c
>>> @@ -276,7 +276,7 @@ static int read_graft_file(struct repository *r, const char *graft_file)
>>>  			 "to convert the grafts into replace refs.\n"
>>>  			 "\n"
>>>  			 "Turn this message off by running\n"
>>> -			 "\"git config advice.graftFileDeprecated false\""));
>>> +			 "\"git config set advice.graftFileDeprecated false\""));
>>
>> OK.
>>
>> However, instead of solidifying this message, perhaps we could take
>> advantage of `advise_if_enabled()` here.  That way, we simplify the
>> code a bit while we also automatically get the new help message, which
>> you are already adjusting in advice.c.
>>
>> More on this below.
>>
>>>  	while (!strbuf_getwholeline(&buf, fp, '\n')) {
>>>  		/* The format is just "Commit Parent1 Parent2 ...\n" */
>>>  		struct commit_graft *graft = read_graft_line(&buf);
>>> diff --git a/hook.c b/hook.c
>>> index a9320cb0ce..9ddbdee06d 100644
>>> --- a/hook.c
>>> +++ b/hook.c
>>> @@ -39,7 +39,7 @@ const char *find_hook(struct repository *r, const char *name)
>>>  				advise(_("The '%s' hook was ignored because "
>>>  					 "it's not set as executable.\n"
>>>  					 "You can disable this warning with "
>>> -					 "`git config advice.ignoredHook false`."),
>>> +					 "`git config set advice.ignoredHook false`."),
>>
>> This message is more of a warning than advice.  I don't think we want
>> to use the same approach here as above, because:
>>
>>     hint: The 'foo' hook was ignored because it's not set as executable.
>>     hint: Disable this message with [...]
>>
>> looks weird.
>>
>> So, your change is enough and right.  OK.
>>
>>>  				       path.buf);
>>>  			}
>>>  		}
>>> diff --git a/object-name.c b/object-name.c
>>> index c892fbe80a..0fa9008b76 100644
>>> --- a/object-name.c
>>> +++ b/object-name.c
>>> @@ -952,7 +952,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
>>>  	"\n"
>>>  	"where \"$br\" is somehow empty and a 40-hex ref is created. Please\n"
>>>  	"examine these refs and maybe delete them. Turn this message off by\n"
>>> -	"running \"git config advice.objectNameWarning false\"");
>>> +	"running \"git config set advice.objectNameWarning false\"");
>>
>> Here, however, I think we should also switch to `advise_if_enabled()`.
>>
>> [...]
>>
>> The rest of the patch looks good.  I think it's desirable to separate
>> the changes in the advice messages from the uses of "git config set"
>> in the tests, as I commented at the beginning of this message.  But I
>> don't have a strong opinion on it.
>>
>> I'll reply to this message with the changes I've suggested about using
>> `advise_if_enabled()`.  If you agree with the changes, feel free to
>> use them as you wish.
>
> Imho my patch is a "no-brainer" in that it doesn't really change anything about
> code or behaviour, while what you sent does, so I think the best way to go with
> this would be to first just switch to `config set` with already existing stuff
> and then open up the question of changing them in a more meaningful way. In
> general of course it seems like a good idea to bring advice messages under one
> interface, but there's more in there that I don't think I could argue for or
> against with any confidence.
>
> I'll send a v3 with the test usages changes split out.
>
> Thanks,
> Bence

I started to split the commit, but realized that I only updated "git config
advice\." to "git config set advice." in the tests. If I split the around five
instances of actually using "git config advice" in the code, then it starts to
make a lot less sense for why it is only for "advice" and not for all the other
uses of "git config" in the tests. So I'm now inclined to think that I either
leave the patch as is, or simple just remove the parts that are not updating
expected test outcomes and leave updating usage of "git config" in tests for
a later as it would likely be a larger effort to clean up everything to use
explicit set/get. This cleanup would also only make sense if there are plans to
deprecate the old implicit setting syntax at some point.

So should I remove the changes to usage in tests or just leave the patch as is?

Best,
Bence