On Mon Dec 09, 2024 at 12:21, Bence Ferdinandy <bence@xxxxxxxxxxxxxx> wrote:
>
> 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
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
