Re: [PATCH v2] advice: suggest using subcommand "git config set"
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Dec 8, 2024, 08:08 UTC
- Message-ID
- <0e139151-7162-42b3-afae-248c28bf4c4b@gmail.com>
- In-Reply-To
- <20241205122225.1184215-1-bence@ferdinandy.com>
On Thu, Dec 05, 2024 at 01:21:58PM +0100, Bence Ferdinandy wrote:
Show 9 quoted lines
> 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.
Show 42 quoted lines
>
> 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.
Show 13 quoted lines
> > 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.
Show 13 quoted lines
> 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.
Show 13 quoted lines
> 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(-)