Re: [PATCH] advice: use global config for default branch name
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 10, 2026, 04:28 UTC
- Message-ID
- <xmqq5x0d4tmh.fsf@gitster.g>
- In-Reply-To
- <20260909195132.GA182066@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Wed, Sep 09, 2026 at 11:51:03AM -0700, Junio C Hamano wrote: > >> > will. So there are many missed opportunities for offering the turn-off >> > instructions. Nobody seems to have complained, which makes me wonder if >> > the turn-off instructions would be annoyingly chatty if we printed them >> > all the time. Most of those calls predate the addition if the turn-off >> > instructions and advise_if_enabled(), which was added in 2020. I wonder >> > how people would feel if we converted them all and started printing the >> > turn-off instructions everywhere. >> >> Depends on how we do so, I guess. Do you mean we should rewrite >> advise() call above to advice_if_enabled(), even though the check >> for ADVICE_FOO token appear redundant? > > I mean we could mechanically rewrite: > > if (advice_enabled(ADVICE_FOO)) > advise(...); > > to: > > advise_if_enabled(ADVICE_FOO, ...);
Surely, and I think we are pretty much on the same page. Such a mechanical rewrite is not too bad. Here is what I came up with:
$ edit tools/coccinelle/advice.cocci
$ make coccicheck
$ git add -N tools/coccinelle/advice.cocci
$ git apply .build/tools/coccinelle/ALL.cocci.patch
$ git add -p Some of the hunks I simply accepted with (y), but most of them
needed (e)dit to make them presentable; otherwise we ended up
with too many overly long lines and losing some comments.--- >8 --- Subject: [PATCH] advice: use advise_if_enabled() more
One very common pattern is
if (advice_enabled(ADVICE_FOO))
advise(_("MESSAGE FOR FOO"));but we have a perfect short-hand for that. Using coccinelle, rewrite the above as
advise_if_enabled(ADVICE_FOO, _("MESSAGE FOR FOR"));Signed-off-by: Junio C Hamano <gitster@pobox.com> --- advice.c | 13 ++++--------- branch.c | 6 +++--- builtin/am.c | 4 ++-- builtin/checkout.c | 4 ++-- builtin/submodule--helper.c | 4 ++-- sequencer.c | 9 ++++----- tools/coccinelle/advice.cocci | 7 +++++++ 7 files changed, 24 insertions(+), 23 deletions(-) create mode 100644 tools/coccinelle/advice.cocci
diff --git a/advice.c b/advice.c index 63bf8b0c5f..c60b33ee33 100644 --- a/advice.c +++ b/advice.c @@ -216,13 +216,8 @@ int error_resolve_conflict(const char *me) else BUG("Unhandled conflict reason '%s'", me); - if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) - /* - * Message used both when 'git commit' fails and when - * other commands doing a merge do. - */ - advise(_("Fix them up in the work tree, and then use 'git add/rm <file>'\n" - "as appropriate to mark resolution and make a commit.")); + advice_if_enabled(ADVICE_RESOLVE_CONFLICT, + _("Fix them up in the work tree, and then use 'git add/rm <file>'\n" "as appropriate to mark resolution and make a commit.")); return -1; } @@ -235,8 +230,8 @@ void NORETURN die_resolve_conflict(const char *me) void NORETURN die_conclude_merge(void) { error(_("You have not concluded your merge (MERGE_HEAD exists).")); - if (advice_enabled(ADVICE_RESOLVE_CONFLICT)) - advise(_("Please, commit your changes before merging.")); + advice_if_enabled(ADVICE_RESOLVE_CONFLICT, + _("Please, commit your changes before merging.")); die(_("Exiting because of unfinished merge.")); } diff --git a/branch.c b/branch.c index 22f4f46b96..a87facd311 100644 --- a/branch.c +++ b/branch.c @@ -812,9 +812,9 @@ void create_branches_recursively(struct repository *r, const char *name, int code = die_message( _("submodule '%s': unable to find submodule"), submodule_entry_list.entries[i].submodule->name); - if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED)) - advise(_("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"), - start_committish); + advice_if_enabled(ADVICE_SUBMODULES_NOT_UPDATED, + _("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"), + start_committish); exit(code); } diff --git a/builtin/am.c b/builtin/am.c index e9623b8307..6039b69475 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1910,8 +1910,8 @@ static void am_run(struct am_state *state, int resume) printf_ln(_("Patch failed at %s %.*s"), msgnum(state), linelen(state->msg), state->msg); - if (advice_enabled(ADVICE_AM_WORK_DIR)) - advise(_("Use 'git am --show-current-patch=diff' to see the failed patch")); + advice_if_enabled(ADVICE_AM_WORK_DIR, + _("Use 'git am --show-current-patch=diff' to see the failed patch")); die_user_resolve(state); } diff --git a/builtin/checkout.c b/builtin/checkout.c index 2bc21aa49b..34f05d2381 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1612,8 +1612,8 @@ static void die_expecting_a_branch(const struct branch_info *branch_info) */ code = die_message(_("a branch is expected, got '%s'"), branch_info->name); - if (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD)) - advise(_("If you want to detach HEAD at the commit, try again with the --detach option.")); + advice_if_enabled(ADVICE_SUGGEST_DETACHING_HEAD, + _("If you want to detach HEAD at the commit, try again with the --detach option.")); exit(code); } diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index e7cd3225fa..5e4989a9aa 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1806,8 +1806,8 @@ static int add_possible_reference_from_superproject( } else { switch (sas->error_mode) { case SUBMODULE_ALTERNATE_ERROR_DIE: - if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE)) - advise(_(alternate_error_advice)); + advice_if_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE, + _(alternate_error_advice)); die(_("submodule '%s' cannot add alternate: %s"), sas->submodule_name, err.buf); case SUBMODULE_ALTERNATE_ERROR_INFO: diff --git a/sequencer.c b/sequencer.c index 65afd100d9..6d8be0c036 100644 --- a/sequencer.c +++ b/sequencer.c @@ -624,8 +624,8 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts) error(_("your local changes would be overwritten by %s."), _(action_name(opts))); - if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)) - advise(_("commit your changes or stash them to proceed.")); + advice_if_enabled(ADVICE_COMMIT_BEFORE_MERGE, + _("commit your changes or stash them to proceed.")); return -1; } @@ -3497,9 +3497,8 @@ static int create_seq_dir(struct repository *r) } if (in_progress_error) { error("%s", in_progress_error); - if (advice_enabled(ADVICE_SEQUENCER_IN_USE)) - advise(in_progress_advice, - advise_skip ? "--skip | " : ""); + advice_if_enabled(ADVICE_SEQUENCER_IN_USE, in_progress_advice, + advise_skip ? "--skip | " : ""); return -1; } if (mkdir(git_path_seq_dir(), 0777) < 0) diff --git a/tools/coccinelle/advice.cocci b/tools/coccinelle/advice.cocci new file mode 100644 index 0000000000..da4851c5d0 --- /dev/null +++ b/tools/coccinelle/advice.cocci @@ -0,0 +1,7 @@ +@@ +expression A; +expression list args; +@@ +-if (advice_enabled(A)) +- advise(args); ++advice_if_enabled(A, args);
-- 2.55.0-967-gab67bff200