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

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

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