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.
>
> 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.
Rubén Justo (3):
advice: enhance `detach_advice()` to `detach_advice_if_enabled()`
commit: use `advise_if_enabled()` in `read_graft_file()`
object-name: advice to avoid refs that resemble hashes
advice.c | 8 +++-----
advice.h | 2 +-
builtin/checkout.c | 5 ++---
builtin/clone.c | 3 +--
commit.c | 17 +++++++----------
object-name.c | 9 ++++-----
t/t1512-rev-parse-disambiguation.sh | 15 ++++++++++++++-
7 files changed, 32 insertions(+), 27 deletions(-)
