# [PATCH 0/2] advice: add diverging advice 7 messages from 2023-03-08 to 2023-03-09. Participants: Felipe Contreras, Taylor Blau, Junio C Hamano. Thread: https://gitlist.dev/t/59355 ## Felipe Contreras, 2023-03-08 02:48 Subject: [PATCH 0/2] advice: add diverging advice Message-ID: <20230308024834.1562386-1-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20230308024834.1562386-1-felipe.contreras%40gmail.com ``` Not all our users are experts in git who understand their configurations perfectly, some might be stuck in a simple error: Not possible to fast-forward, aborting. That's not very friendly. Let's add a bit more advice in case the user doesn't know what's going on. Felipe Contreras (2): advice: add diverging advice for novices advice: make diverging advice configurable Documentation/config/advice.txt | 2 ++ advice.c | 9 +++++++++ advice.h | 1 + 3 files changed, 12 insertions(+) -- 2.39.2 ``` ## Felipe Contreras, 2023-03-08 02:48 Subject: [PATCH 1/2] advice: add diverging advice for novices Message-ID: <20230308024834.1562386-2-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20230308024834.1562386-2-felipe.contreras%40gmail.com In-Reply-To: <20230308024834.1562386-1-felipe.contreras@gmail.com> ``` The user might not necessarily know why ff only was configured, maybe an admin did it, or the installer (Git for Windows), or perhaps they just followed some online advice. This can happen not only on pull.ff=only, but merge.ff=only too. Even worse if the user has configured pull.rebase=false and merge.ff=only, because in those cases a diverging merge will constantly keep failing. There's no trivial way to get out of this other than `git merge --no-ff`. Let's not assume our users are experts in git who completely understand all their configurations. Signed-off-by: Felipe Contreras --- advice.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/advice.c b/advice.c index fd18968943..c3fb631f93 100644 --- a/advice.c +++ b/advice.c @@ -217,6 +217,13 @@ void NORETURN die_conclude_merge(void) void NORETURN die_ff_impossible(void) { + advise(_("Diverging branches can't be fast-forwarded, you need to either:\n" + "\n" + "\tgit merge --no-ff\n" + "\n" + "or:\n" + "\n" + "\tgit rebase\n")); die(_("Not possible to fast-forward, aborting.")); } -- 2.39.2 ``` ## Felipe Contreras, 2023-03-08 02:48 Subject: [PATCH 2/2] advice: make diverging advice configurable Message-ID: <20230308024834.1562386-3-felipe.contreras@gmail.com> URL: https://gitlist.dev/e/20230308024834.1562386-3-felipe.contreras%40gmail.com In-Reply-To: <20230308024834.1562386-1-felipe.contreras@gmail.com> ``` Signed-off-by: Felipe Contreras --- Documentation/config/advice.txt | 2 ++ advice.c | 4 +++- advice.h | 1 + 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt index a00d0100a8..c96b5b2e5d 100644 --- a/Documentation/config/advice.txt +++ b/Documentation/config/advice.txt @@ -136,4 +136,6 @@ advice.*:: Advice shown when either linkgit:git-add[1] or linkgit:git-rm[1] is asked to update index entries outside the current sparse checkout. + diverging:: + Advice shown when a fast-forward is not possible. -- diff --git a/advice.c b/advice.c index c3fb631f93..fbc59f9c6d 100644 --- a/advice.c +++ b/advice.c @@ -44,6 +44,7 @@ static struct { [ADVICE_COMMIT_BEFORE_MERGE] = { "commitBeforeMerge", 1 }, [ADVICE_DETACHED_HEAD] = { "detachedHead", 1 }, [ADVICE_SUGGEST_DETACHING_HEAD] = { "suggestDetachingHead", 1 }, + [ADVICE_DIVERGING] = { "diverging", 1 }, [ADVICE_FETCH_SHOW_FORCED_UPDATES] = { "fetchShowForcedUpdates", 1 }, [ADVICE_GRAFT_FILE_DEPRECATED] = { "graftFileDeprecated", 1 }, [ADVICE_IGNORED_HOOK] = { "ignoredHook", 1 }, @@ -217,7 +218,8 @@ void NORETURN die_conclude_merge(void) void NORETURN die_ff_impossible(void) { - advise(_("Diverging branches can't be fast-forwarded, you need to either:\n" + advise_if_enabled(ADVICE_DIVERGING, + _("Diverging branches can't be fast-forwarded, you need to either:\n" "\n" "\tgit merge --no-ff\n" "\n" diff --git a/advice.h b/advice.h index 07e0f76833..41b5bc127c 100644 --- a/advice.h +++ b/advice.h @@ -21,6 +21,7 @@ struct string_list; ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME, ADVICE_COMMIT_BEFORE_MERGE, ADVICE_DETACHED_HEAD, + ADVICE_DIVERGING, ADVICE_SUGGEST_DETACHING_HEAD, ADVICE_FETCH_SHOW_FORCED_UPDATES, ADVICE_GRAFT_FILE_DEPRECATED, -- 2.39.2 ``` ## Taylor Blau, 2023-03-08 16:03 Subject: Re: [PATCH 0/2] advice: add diverging advice Message-ID: URL: https://gitlist.dev/e/ZAix68A9e6RHz69y%40nand.local In-Reply-To: <20230308024834.1562386-1-felipe.contreras@gmail.com> ``` Hi Felipe, On Tue, Mar 07, 2023 at 08:48:32PM -0600, Felipe Contreras wrote: > Not all our users are experts in git who understand their configurations > perfectly, some might be stuck in a simple error: > > Not possible to fast-forward, aborting. > > That's not very friendly. > > Let's add a bit more advice in case the user doesn't know what's going > on. Thanks for improving this case. I think the new advice() is reasonable and will be helpful for users who might otherwise get stuck here. I don't think that splitting it into two separate patches was strictly necessary. If I were queuing, I'd probably squash the two together, but I leave that one to Junio to figure out ;-). Thanks, Taylor ``` ## Junio C Hamano, 2023-03-08 17:17 Subject: Re: [PATCH 0/2] advice: add diverging advice Message-ID: URL: https://gitlist.dev/e/xmqqjzzrupqo.fsf%40gitster.g In-Reply-To: ``` Taylor Blau writes: > I don't think that splitting it into two separate patches was strictly > necessary. If I were queuing, I'd probably squash the two together, ... One advantage of sending a topic like this as two patches is that, if the review discussion leads to a consensus that the new help message should be given unconditionally to everybody, only [1/2] can be queued while dropping [2/2]. But the point of advise() is to serve as a training wheel that can be disabled by users who no longer need it, so need for such a "flexibility" may not appear in topics that add new advise() calls all that often, I would imagine. I am willing to do the squashing on the receiving end. The effect of two patches combined together is a good improvement, and the proposed log message for the first one covers why we want to do both of these two. Thanks, both, for writing and reviewing. ``` ## Junio C Hamano, 2023-03-08 17:17 Subject: Re: [PATCH 1/2] advice: add diverging advice for novices Message-ID: URL: https://gitlist.dev/e/xmqqcz5jupqh.fsf%40gitster.g In-Reply-To: <20230308024834.1562386-2-felipe.contreras@gmail.com> ``` Felipe Contreras writes: > The user might not necessarily know why ff only was configured, maybe an > admin did it, or the installer (Git for Windows), or perhaps they just > followed some online advice. > > This can happen not only on pull.ff=only, but merge.ff=only too. > > Even worse if the user has configured pull.rebase=false and > merge.ff=only, because in those cases a diverging merge will constantly > keep failing. There's no trivial way to get out of this other than > `git merge --no-ff`. A good description. Without this explained, the instruction to run "git merge" with "--no-ff" in the text would have been puzzling to readers. At least I was initially puzzled as I read the patch text before the proposed log message. > void NORETURN die_ff_impossible(void) > { > + advise(_("Diverging branches can't be fast-forwarded, you need to either:\n" > + "\n" > + "\tgit merge --no-ff\n" > + "\n" > + "or:\n" > + "\n" > + "\tgit rebase\n")); > die(_("Not possible to fast-forward, aborting.")); > } ``` ## Junio C Hamano, 2023-03-09 23:44 Subject: Re: [PATCH 0/2] advice: add diverging advice Message-ID: URL: https://gitlist.dev/e/xmqqv8j97ami.fsf%40gitster.g In-Reply-To: ``` Taylor Blau writes: > On Tue, Mar 07, 2023 at 08:48:32PM -0600, Felipe Contreras wrote: >> Not all our users are experts in git who understand their configurations >> perfectly, some might be stuck in a simple error: >> >> Not possible to fast-forward, aborting. >> >> That's not very friendly. >> >> Let's add a bit more advice in case the user doesn't know what's going >> on. > > Thanks for improving this case. I think the new advice() is reasonable > and will be helpful for users who might otherwise get stuck here. We may want to further tweak the advise() message depending on the caller [*], but that can come on top after these two patches settle. [Footnote] * When a failed "git pull" gave the new advise() message, the user may not be advanced enough to adjust the advice to run "git merge --no-ff" to the given situation, for example. ```