threads / patch / 59355

patch, 2 partsadvice: add diverging advice

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

## tl;dr

7 messages between Mar 8, 2023 and Mar 9, 2023. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Felipe Contreras· Mar 8, 2023, 02:48 UTC · lore

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· Mar 8, 2023, 02:48 UTC · re: Felipe Contreras · lore

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

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(+)

Show changes to advice.c +7 −0

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

Junio C Hamano· Mar 8, 2023, 17:17 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 10 quoted lines

> 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.

Show 11 quoted lines

>  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."));
>  }

Felipe Contreras· Mar 8, 2023, 02:48 UTC · re: Felipe Contreras · lore

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

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(-)

Show changes to 3 files +6 −1

Documentation/config/advice.txt, advice.c, advice.h

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· Mar 8, 2023, 16:03 UTC · re: Felipe Contreras · lore

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

Hi Felipe,

On Tue, Mar 07, 2023 at 08:48:32PM -0600, Felipe Contreras wrote:

Show 9 quoted lines

> 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· Mar 8, 2023, 17:17 UTC · re: Taylor Blau · lore

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

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.

Junio C Hamano· Mar 9, 2023, 23:44 UTC · re: Taylor Blau · lore

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

Taylor Blau <me@ttaylorr.com> writes:

Show 13 quoted lines

> 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.