Thanks for taking a looking and the follow-up patches!
On Sun Dec 08, 2024 at 09:08, Rubén Justo <rjusto@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> ---
>>
>> 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
