[PATCH 0/2] advice: add diverging advice

[PATCH 0/2] advice: add diverging advice

[Felipe Contreras]

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

[PATCH 1/2] advice: add diverging advice for novices

[Felipe Contreras]

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 <felipe.contreras@gmail.com>
---
 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

Re: [PATCH 1/2] advice: add diverging advice for novices

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> 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."));
>  }

[PATCH 2/2] advice: make diverging advice configurable

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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

Re: [PATCH 0/2] advice: add diverging advice

[Taylor Blau]

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

Re: [PATCH 0/2] advice: add diverging advice

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> 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.

Re: [PATCH 0/2] advice: add diverging advice

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> 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.