[PATCH] branch: error description when deleting a not fully merged branch

[PATCH] branch: error description when deleting a not fully merged branch

[Rubén Justo]

The error message we show when the user tries to delete a not fully
merged branch describes the error and gives a hint to the user:

	error: the branch 'foo' is not fully merged.
	If you are sure you want to delete it, run 'git branch -D foo'.

Let's move the hint part so that it takes advantage of the advice
machinery:

	error: the branch 'foo' is not fully merged
	hint: If you are sure you want to delete it, run 'git branch -D foo'
	hint: Disable this message with "git config advice.forceDeleteBranch false"

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This change is a pending NEEDSWORK from a recent series about adjusting
the error messages in branch.c

Unfortunately the full message now becomes a three line message.

Hopefully we can find a way in the near future to keep it at two.

 Documentation/config/advice.txt | 3 +++
 advice.c                        | 1 +
 advice.h                        | 3 ++-
 builtin/branch.c                | 9 ++++++---
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 4d7e5d8759..5814d659b9 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -142,4 +142,7 @@ advice.*::
 		Advice shown when a user tries to create a worktree from an
 		invalid reference, to instruct how to create a new unborn
 		branch instead.
+	forceDeleteBranch::
+		Advice shown when a user tries to delete a not fully merged
+		branch without the force option set.
 --
diff --git a/advice.c b/advice.c
index 50c79443ba..e91f5d7ab8 100644
--- a/advice.c
+++ b/advice.c
@@ -79,6 +79,7 @@ static struct {
 	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
 	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
+	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch", 1 },
 };
 
 static const char turn_off_instructions[] =
diff --git a/advice.h b/advice.h
index 2affbe1426..5bef900142 100644
--- a/advice.h
+++ b/advice.h
@@ -10,7 +10,7 @@ struct string_list;
  * Add the new config variable to Documentation/config/advice.txt.
  * Call advise_if_enabled to print your advice.
  */
- enum advice_type {
+enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
@@ -50,6 +50,7 @@ struct string_list;
 	ADVICE_WAITING_FOR_EDITOR,
 	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_WORKTREE_ADD_ORPHAN,
+	ADVICE_FORCE_DELETE_BRANCH,
 };
 
 int git_default_advice_config(const char *var, const char *value);
diff --git a/builtin/branch.c b/builtin/branch.c
index 0a32d1b6c8..2240433bc8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -24,6 +24,7 @@
 #include "ref-filter.h"
 #include "worktree.h"
 #include "help.h"
+#include "advice.h"
 #include "commit-reach.h"
 
 static const char * const builtin_branch_usage[] = {
@@ -190,9 +191,11 @@ static int check_branch_commit(const char *branchname, const char *refname,
 		return -1;
 	}
 	if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
-		error(_("the branch '%s' is not fully merged.\n"
-		      "If you are sure you want to delete it, "
-		      "run 'git branch -D %s'"), branchname, branchname);
+		error(_("the branch '%s' is not fully merged"),
+		      branchname);
+		advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
+				  _("If you are sure you want to delete it, "
+				  "run 'git branch -D %s'"), branchname);
 		return -1;
 	}
 	return 0;
-- 
2.42.0

Re: [PATCH] branch: error description when deleting a not fully merged branch

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> The error message we show when the user tries to delete a not fully
> merged branch describes the error and gives a hint to the user:
>
> 	error: the branch 'foo' is not fully merged.
> 	If you are sure you want to delete it, run 'git branch -D foo'.
>
> Let's move the hint part so that it takes advantage of the advice
> machinery:
>
> 	error: the branch 'foo' is not fully merged
> 	hint: If you are sure you want to delete it, run 'git branch -D foo'
> 	hint: Disable this message with "git config advice.forceDeleteBranch false"

This is probably one sensible step forward, so let's queue it as-is.

But with reservations for longer-term future direction.  Stepping
back a bit, when 'foo' is not fully merged and the user used "branch
-d" on it, is it sensible for us to suggest use of "branch -D"?

Especially now this is a "hint" to help less experienced folks, it
may be helpful to suggest how the user can answer "If you are sure
you want to delete" part.  As this knows what unique commits on the
branch being deleted are about to be lost, one way to do so may be
to tell the user about them ("you are about to lose 'branch: error
description when deleting a not fully merged branch' and other 47
commits that are not merged the target branch 'main'", for example).

Another possibility is to suggest merging the branch into the
target, instead of suggesting a destructive "deletion", but I
suspect that it goes too far second-guessing the end-user intention.

Thanks.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>
> This change is a pending NEEDSWORK from a recent series about adjusting
> the error messages in branch.c
>
> Unfortunately the full message now becomes a three line message.
>
> Hopefully we can find a way in the near future to keep it at two.
>
>  Documentation/config/advice.txt | 3 +++
>  advice.c                        | 1 +
>  advice.h                        | 3 ++-
>  builtin/branch.c                | 9 ++++++---
>  4 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
> index 4d7e5d8759..5814d659b9 100644
> --- a/Documentation/config/advice.txt
> +++ b/Documentation/config/advice.txt
> @@ -142,4 +142,7 @@ advice.*::
>  		Advice shown when a user tries to create a worktree from an
>  		invalid reference, to instruct how to create a new unborn
>  		branch instead.
> +	forceDeleteBranch::
> +		Advice shown when a user tries to delete a not fully merged
> +		branch without the force option set.
>  --
> diff --git a/advice.c b/advice.c
> index 50c79443ba..e91f5d7ab8 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -79,6 +79,7 @@ static struct {
>  	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
>  	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
>  	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
> +	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch", 1 },
>  };
>  
>  static const char turn_off_instructions[] =
> diff --git a/advice.h b/advice.h
> index 2affbe1426..5bef900142 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -10,7 +10,7 @@ struct string_list;
>   * Add the new config variable to Documentation/config/advice.txt.
>   * Call advise_if_enabled to print your advice.
>   */
> - enum advice_type {
> +enum advice_type {
>  	ADVICE_ADD_EMBEDDED_REPO,
>  	ADVICE_ADD_EMPTY_PATHSPEC,
>  	ADVICE_ADD_IGNORED_FILE,
> @@ -50,6 +50,7 @@ struct string_list;
>  	ADVICE_WAITING_FOR_EDITOR,
>  	ADVICE_SKIPPED_CHERRY_PICKS,
>  	ADVICE_WORKTREE_ADD_ORPHAN,
> +	ADVICE_FORCE_DELETE_BRANCH,
>  };
>  
>  int git_default_advice_config(const char *var, const char *value);
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0a32d1b6c8..2240433bc8 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -24,6 +24,7 @@
>  #include "ref-filter.h"
>  #include "worktree.h"
>  #include "help.h"
> +#include "advice.h"
>  #include "commit-reach.h"
>  
>  static const char * const builtin_branch_usage[] = {
> @@ -190,9 +191,11 @@ static int check_branch_commit(const char *branchname, const char *refname,
>  		return -1;
>  	}
>  	if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
> -		error(_("the branch '%s' is not fully merged.\n"
> -		      "If you are sure you want to delete it, "
> -		      "run 'git branch -D %s'"), branchname, branchname);
> +		error(_("the branch '%s' is not fully merged"),
> +		      branchname);
> +		advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
> +				  _("If you are sure you want to delete it, "
> +				  "run 'git branch -D %s'"), branchname);
>  		return -1;
>  	}
>  	return 0;

Re: [PATCH] branch: error description when deleting a not fully merged branch

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This is probably one sensible step forward, so let's queue it as-is.
>
> But with reservations for longer-term future direction.  Stepping
> back a bit, when 'foo' is not fully merged and the user used "branch
> -d" on it, is it sensible for us to suggest use of "branch -D"?
>
> Especially now this is a "hint" to help less experienced folks, it
> may be helpful to suggest how the user can answer "If you are sure
> you want to delete" part.  As this knows what unique commits on the
> branch being deleted are about to be lost, one way to do so may be
> to tell the user about them ("you are about to lose 'branch: error
> description when deleting a not fully merged branch' and other 47
> commits that are not merged the target branch 'main'", for example).
>
> Another possibility is to suggest merging the branch into the
> target, instead of suggesting a destructive "deletion", but I
> suspect that it goes too far second-guessing the end-user intention.

The longer-term concerns aside, if you are inclined, we might want
to have this as a two step series, where [1/2] does a clean-up of
existing source file, i.e. losing the unwanted leading space from "
enum advice_type {" in advice.h and sort the advice.*:: list in
Documentation/config/advice.txt.  It is optional to sort the
advice_setting[] list in advice.c and "enum advice_type" in
advice.h, as they are not end-user facing, and we should be using
the defined constant without relying on their exact values.  But
keeping the config/advice.txt sorted would help readers more easily
locate which configuration variable to use to squelch a message.

And [2/2] does the rest.

Also I forgot that in the version I queued, I fixed the title to

    branch: make the advice to force-deleting a conditional one

Thanks.

[PATCH v2 0/3] branch: error description when deleting a not fully merged branch

[Rubén Justo]

The error message we show when the user tries to delete a not fully
merged branch describes the error and gives a hint to the user:

	error: the branch 'foo' is not fully merged.
	If you are sure you want to delete it, run 'git branch -D foo'.

Let's move the hint part so that it is displayed using the advice
machinery:

	error: the branch 'foo' is not fully merged
	hint: If you are sure you want to delete it, run 'git branch -D foo'
	hint: Disable this message with "git config advice.forceDeleteBranch false"

Rubén Justo (3):
  advice: sort the advice related lists
  advice: fix an unexpected leading space
  branch: make the advice to force-deleting a conditional one

 Documentation/config/advice.txt | 157 ++++++++++++++++----------------
 advice.c                        |  14 ++-
 advice.h                        |  15 +--
 builtin/branch.c                |   8 +-
 4 files changed, 99 insertions(+), 95 deletions(-)

-- 
2.42.0

[PATCH v2 1/3] advice: sort the advice related lists

[Rubén Justo]

Let's keep the advice related lists sorted to make them more
digestible.

A multi-line comment has also been changed; that produces the unexpected
'insertion != deletion' in this supposedly 'only sort lines' commit.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 Documentation/config/advice.txt | 154 ++++++++++++++++----------------
 advice.c                        |  13 ++-
 advice.h                        |  12 +--
 3 files changed, 88 insertions(+), 91 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 4d7e5d8759..e0deaf3144 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -4,27 +4,56 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	addEmbeddedRepo::
+		Advice on what to do when you've accidentally added one
+		git repo inside of another.
+	addEmptyPathspec::
+		Advice shown if a user runs the add command without providing
+		the pathspec parameter.
+	addIgnoredFile::
+		Advice shown if a user attempts to add an ignored file to
+		the index.
+	amWorkDir::
+		Advice that shows the location of the patch file when
+		linkgit:git-am[1] fails to apply it.
 	ambiguousFetchRefspec::
 		Advice shown when a fetch refspec for multiple remotes maps to
 		the same remote-tracking branch namespace and causes branch
 		tracking set-up to fail.
+	checkoutAmbiguousRemoteBranchName::
+		Advice shown when the argument to
+		linkgit:git-checkout[1] and linkgit:git-switch[1]
+		ambiguously resolves to a
+		remote tracking branch on more than one remote in
+		situations where an unambiguous argument would have
+		otherwise caused a remote-tracking branch to be
+		checked out. See the `checkout.defaultRemote`
+		configuration variable for how to set a given remote
+		to be used by default in some situations where this
+		advice would be printed.
+	commitBeforeMerge::
+		Advice shown when linkgit:git-merge[1] refuses to
+		merge to avoid overwriting local changes.
+	detachedHead::
+		Advice shown when you used
+		linkgit:git-switch[1] or linkgit:git-checkout[1]
+		to move to the detached HEAD state, to instruct how to
+		create a local branch after the fact.
+	diverging::
+		Advice shown when a fast-forward is not possible.
 	fetchShowForcedUpdates::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
 		that the check is disabled.
-	pushUpdateRejected::
-		Set this variable to 'false' if you want to disable
-		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
-		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
-		simultaneously.
-	pushNonFFCurrent::
-		Advice shown when linkgit:git-push[1] fails due to a
-		non-fast-forward update to the current branch.
-	pushNonFFMatching::
-		Advice shown when you ran linkgit:git-push[1] and pushed
-		'matching refs' explicitly (i.e. you used ':', or
-		specified a refspec that isn't your current branch) and
-		it resulted in a non-fast-forward error.
+	ignoredHook::
+		Advice shown if a hook is ignored because the hook is not
+		set as executable.
+	implicitIdentity::
+		Advice on how to set your identity configuration when
+		your information is guessed from the system username and
+		domain name.
+	nestedTag::
+		Advice shown if a user attempts to recursively tag a tag object.
 	pushAlreadyExists::
 		Shown when linkgit:git-push[1] rejects an update that
 		does not qualify for fast-forwarding (e.g., a tag.)
@@ -37,6 +66,18 @@ advice.*::
 		tries to overwrite a remote ref that points at an
 		object that is not a commit-ish, or make the remote
 		ref point at an object that is not a commit-ish.
+	pushNonFFCurrent::
+		Advice shown when linkgit:git-push[1] fails due to a
+		non-fast-forward update to the current branch.
+	pushNonFFMatching::
+		Advice shown when you ran linkgit:git-push[1] and pushed
+		'matching refs' explicitly (i.e. you used ':', or
+		specified a refspec that isn't your current branch) and
+		it resulted in a non-fast-forward error.
+	pushRefNeedsUpdate::
+		Shown when linkgit:git-push[1] rejects a forced update of
+		a branch when its remote-tracking ref has updates that we
+		do not have locally.
 	pushUnqualifiedRefname::
 		Shown when linkgit:git-push[1] gives up trying to
 		guess based on the source and destination refs what
@@ -44,10 +85,23 @@ advice.*::
 		we can still suggest that the user push to either
 		refs/heads/* or refs/tags/* based on the type of the
 		source object.
-	pushRefNeedsUpdate::
-		Shown when linkgit:git-push[1] rejects a forced update of
-		a branch when its remote-tracking ref has updates that we
-		do not have locally.
+	pushUpdateRejected::
+		Set this variable to 'false' if you want to disable
+		'pushNonFFCurrent', 'pushNonFFMatching', 'pushAlreadyExists',
+		'pushFetchFirst', 'pushNeedsForce', and 'pushRefNeedsUpdate'
+		simultaneously.
+	resetNoRefresh::
+		Advice to consider using the `--no-refresh` option to
+		linkgit:git-reset[1] when the command takes more than 2 seconds
+		to refresh the index after reset.
+	resolveConflict::
+		Advice shown by various commands when conflicts
+		prevent the operation from being performed.
+	rmHints::
+		In case of failure in the output of linkgit:git-rm[1],
+		show directions on how to proceed from the current state.
+	sequencerInUse::
+		Advice shown when a sequencer command is already in progress.
 	skippedCherryPicks::
 		Shown when linkgit:git-rebase[1] skips a commit that has already
 		been cherry-picked onto the upstream branch.
@@ -68,76 +122,22 @@ advice.*::
 		Advise to consider using the `-u` option to linkgit:git-status[1]
 		when the command takes more than 2 seconds to enumerate untracked
 		files.
-	commitBeforeMerge::
-		Advice shown when linkgit:git-merge[1] refuses to
-		merge to avoid overwriting local changes.
-	resetNoRefresh::
-		Advice to consider using the `--no-refresh` option to
-		linkgit:git-reset[1] when the command takes more than 2 seconds
-		to refresh the index after reset.
-	resolveConflict::
-		Advice shown by various commands when conflicts
-		prevent the operation from being performed.
-	sequencerInUse::
-		Advice shown when a sequencer command is already in progress.
-	implicitIdentity::
-		Advice on how to set your identity configuration when
-		your information is guessed from the system username and
-		domain name.
-	detachedHead::
-		Advice shown when you used
-		linkgit:git-switch[1] or linkgit:git-checkout[1]
-		to move to the detached HEAD state, to instruct how to
-		create a local branch after the fact.
-	suggestDetachingHead::
-		Advice shown when linkgit:git-switch[1] refuses to detach HEAD
-		without the explicit `--detach` option.
-	checkoutAmbiguousRemoteBranchName::
-		Advice shown when the argument to
-		linkgit:git-checkout[1] and linkgit:git-switch[1]
-		ambiguously resolves to a
-		remote tracking branch on more than one remote in
-		situations where an unambiguous argument would have
-		otherwise caused a remote-tracking branch to be
-		checked out. See the `checkout.defaultRemote`
-		configuration variable for how to set a given remote
-		to be used by default in some situations where this
-		advice would be printed.
-	amWorkDir::
-		Advice that shows the location of the patch file when
-		linkgit:git-am[1] fails to apply it.
-	rmHints::
-		In case of failure in the output of linkgit:git-rm[1],
-		show directions on how to proceed from the current state.
-	addEmbeddedRepo::
-		Advice on what to do when you've accidentally added one
-		git repo inside of another.
-	ignoredHook::
-		Advice shown if a hook is ignored because the hook is not
-		set as executable.
-	waitingForEditor::
-		Print a message to the terminal whenever Git is waiting for
-		editor input from the user.
-	nestedTag::
-		Advice shown if a user attempts to recursively tag a tag object.
 	submoduleAlternateErrorStrategyDie::
 		Advice shown when a submodule.alternateErrorStrategy option
 		configured to "die" causes a fatal error.
 	submodulesNotUpdated::
 		Advice shown when a user runs a submodule command that fails
 		because `git submodule update --init` was not run.
-	addIgnoredFile::
-		Advice shown if a user attempts to add an ignored file to
-		the index.
-	addEmptyPathspec::
-		Advice shown if a user runs the add command without providing
-		the pathspec parameter.
+	suggestDetachingHead::
+		Advice shown when linkgit:git-switch[1] refuses to detach HEAD
+		without the explicit `--detach` option.
 	updateSparsePath::
 		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.
+	waitingForEditor::
+		Print a message to the terminal whenever Git is waiting for
+		editor input from the user.
 	worktreeAddOrphan::
 		Advice shown when a user tries to create a worktree from an
 		invalid reference, to instruct how to create a new unborn
diff --git a/advice.c b/advice.c
index 50c79443ba..03322caba0 100644
--- a/advice.c
+++ b/advice.c
@@ -40,12 +40,11 @@ static struct {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo", 1 },
 	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec", 1 },
 	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile", 1 },
-	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
 	[ADVICE_AMBIGUOUS_FETCH_REFSPEC]		= { "ambiguousFetchRefspec", 1 },
+	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir", 1 },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName", 1 },
 	[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 },
@@ -56,15 +55,12 @@ static struct {
 	[ADVICE_PUSH_ALREADY_EXISTS]			= { "pushAlreadyExists", 1 },
 	[ADVICE_PUSH_FETCH_FIRST]			= { "pushFetchFirst", 1 },
 	[ADVICE_PUSH_NEEDS_FORCE]			= { "pushNeedsForce", 1 },
-	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate", 1 },
-
-	/* make this an alias for backward compatibility */
-	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 },
-
 	[ADVICE_PUSH_NON_FF_CURRENT]			= { "pushNonFFCurrent", 1 },
 	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching", 1 },
+	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate", 1 },
 	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName", 1 },
 	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected", 1 },
+	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward", 1 }, /* backwards compatibility */
 	[ADVICE_RESET_NO_REFRESH_WARNING]		= { "resetNoRefresh", 1 },
 	[ADVICE_RESOLVE_CONFLICT]			= { "resolveConflict", 1 },
 	[ADVICE_RM_HINTS]				= { "rmHints", 1 },
@@ -74,8 +70,9 @@ static struct {
 	[ADVICE_STATUS_AHEAD_BEHIND_WARNING]		= { "statusAheadBehindWarning", 1 },
 	[ADVICE_STATUS_HINTS]				= { "statusHints", 1 },
 	[ADVICE_STATUS_U_OPTION]			= { "statusUoption", 1 },
-	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
 	[ADVICE_SUBMODULES_NOT_UPDATED] 		= { "submodulesNotUpdated", 1 },
+	[ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE] = { "submoduleAlternateErrorStrategyDie", 1 },
+	[ADVICE_SUGGEST_DETACHING_HEAD]			= { "suggestDetachingHead", 1 },
 	[ADVICE_UPDATE_SPARSE_PATH]			= { "updateSparsePath", 1 },
 	[ADVICE_WAITING_FOR_EDITOR]			= { "waitingForEditor", 1 },
 	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan", 1 },
diff --git a/advice.h b/advice.h
index 2affbe1426..9396bcdcf1 100644
--- a/advice.h
+++ b/advice.h
@@ -14,13 +14,12 @@ struct string_list;
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
-	ADVICE_AM_WORK_DIR,
 	ADVICE_AMBIGUOUS_FETCH_REFSPEC,
+	ADVICE_AM_WORK_DIR,
 	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,
 	ADVICE_IGNORED_HOOK,
@@ -32,23 +31,24 @@ struct string_list;
 	ADVICE_PUSH_NEEDS_FORCE,
 	ADVICE_PUSH_NON_FF_CURRENT,
 	ADVICE_PUSH_NON_FF_MATCHING,
+	ADVICE_PUSH_REF_NEEDS_UPDATE,
 	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
-	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
 	ADVICE_PUSH_UPDATE_REJECTED,
-	ADVICE_PUSH_REF_NEEDS_UPDATE,
+	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
 	ADVICE_RESET_NO_REFRESH_WARNING,
 	ADVICE_RESOLVE_CONFLICT,
 	ADVICE_RM_HINTS,
 	ADVICE_SEQUENCER_IN_USE,
 	ADVICE_SET_UPSTREAM_FAILURE,
+	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_STATUS_AHEAD_BEHIND_WARNING,
 	ADVICE_STATUS_HINTS,
 	ADVICE_STATUS_U_OPTION,
-	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
 	ADVICE_SUBMODULES_NOT_UPDATED,
+	ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+	ADVICE_SUGGEST_DETACHING_HEAD,
 	ADVICE_UPDATE_SPARSE_PATH,
 	ADVICE_WAITING_FOR_EDITOR,
-	ADVICE_SKIPPED_CHERRY_PICKS,
 	ADVICE_WORKTREE_ADD_ORPHAN,
 };
 
-- 
2.42.0

[PATCH v2 2/3] advice: fix an unexpected leading space

[Rubén Justo]

This space was introduced, presumably unintentionally, in b3b18d1621
(advice: revamp advise API, 2020-03-02)

I notice this space due to confuse diff outputs while doing some
changes to enum advice_type.

As a reference, a recent change we have to that enum is:

    $ git show 35f0383

[ ... ]
diff --git a/advice.h b/advice.h
index 0f584163f5..2affbe1426 100644
--- a/advice.h
+++ b/advice.h
@@ -49,6 +49,7 @@ struct string_list;
        ADVICE_UPDATE_SPARSE_PATH,
        ADVICE_WAITING_FOR_EDITOR,
        ADVICE_SKIPPED_CHERRY_PICKS,
+       ADVICE_WORKTREE_ADD_ORPHAN,
 };

Note the hunk header, instead of a much more expected:

@@ -49,6 +49,7 @@ enum advice_type

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 advice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/advice.h b/advice.h
index 9396bcdcf1..74d44d1156 100644
--- a/advice.h
+++ b/advice.h
@@ -10,7 +10,7 @@ struct string_list;
  * Add the new config variable to Documentation/config/advice.txt.
  * Call advise_if_enabled to print your advice.
  */
- enum advice_type {
+enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
-- 
2.42.0

[PATCH v2 3/3] branch: make the advice to force-deleting a conditional one

[Rubén Justo]

The error message we show when the user tries to delete a not fully
merged branch describes the error and gives a hint to the user:

	error: the branch 'foo' is not fully merged.
	If you are sure you want to delete it, run 'git branch -D foo'.

Let's move the hint part so that it is displayed using the advice
machinery:

	error: the branch 'foo' is not fully merged
	hint: If you are sure you want to delete it, run 'git branch -D foo'
	hint: Disable this message with "git config advice.forceDeleteBranch false"

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 Documentation/config/advice.txt | 3 +++
 advice.c                        | 1 +
 advice.h                        | 1 +
 builtin/branch.c                | 8 +++++---
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index e0deaf3144..25c0917524 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -45,6 +45,9 @@ advice.*::
 		Advice shown when linkgit:git-fetch[1] takes a long time
 		to calculate forced updates after ref updates, or to warn
 		that the check is disabled.
+	forceDeleteBranch::
+		Advice shown when a user tries to delete a not fully merged
+		branch without the force option set.
 	ignoredHook::
 		Advice shown if a hook is ignored because the hook is not
 		set as executable.
diff --git a/advice.c b/advice.c
index 03322caba0..f6e4c2f302 100644
--- a/advice.c
+++ b/advice.c
@@ -47,6 +47,7 @@ static struct {
 	[ADVICE_DETACHED_HEAD]				= { "detachedHead", 1 },
 	[ADVICE_DIVERGING]				= { "diverging", 1 },
 	[ADVICE_FETCH_SHOW_FORCED_UPDATES]		= { "fetchShowForcedUpdates", 1 },
+	[ADVICE_FORCE_DELETE_BRANCH]			= { "forceDeleteBranch", 1 },
 	[ADVICE_GRAFT_FILE_DEPRECATED]			= { "graftFileDeprecated", 1 },
 	[ADVICE_IGNORED_HOOK]				= { "ignoredHook", 1 },
 	[ADVICE_IMPLICIT_IDENTITY]			= { "implicitIdentity", 1 },
diff --git a/advice.h b/advice.h
index 74d44d1156..9d4f49ae38 100644
--- a/advice.h
+++ b/advice.h
@@ -21,6 +21,7 @@ enum advice_type {
 	ADVICE_DETACHED_HEAD,
 	ADVICE_DIVERGING,
 	ADVICE_FETCH_SHOW_FORCED_UPDATES,
+	ADVICE_FORCE_DELETE_BRANCH,
 	ADVICE_GRAFT_FILE_DEPRECATED,
 	ADVICE_IGNORED_HOOK,
 	ADVICE_IMPLICIT_IDENTITY,
diff --git a/builtin/branch.c b/builtin/branch.c
index 0a32d1b6c8..cfb63cce5f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -24,6 +24,7 @@
 #include "ref-filter.h"
 #include "worktree.h"
 #include "help.h"
+#include "advice.h"
 #include "commit-reach.h"
 
 static const char * const builtin_branch_usage[] = {
@@ -190,9 +191,10 @@ static int check_branch_commit(const char *branchname, const char *refname,
 		return -1;
 	}
 	if (!force && !branch_merged(kinds, branchname, rev, head_rev)) {
-		error(_("the branch '%s' is not fully merged.\n"
-		      "If you are sure you want to delete it, "
-		      "run 'git branch -D %s'"), branchname, branchname);
+		error(_("the branch '%s' is not fully merged"), branchname);
+		advise_if_enabled(ADVICE_FORCE_DELETE_BRANCH,
+				  _("If you are sure you want to delete it, "
+				  "run 'git branch -D %s'"), branchname);
 		return -1;
 	}
 	return 0;
-- 
2.42.0

Re: [PATCH] branch: error description when deleting a not fully merged branch

[Rubén Justo]

On 10-ene-2024 09:48:52, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > The error message we show when the user tries to delete a not fully
> > merged branch describes the error and gives a hint to the user:
> >
> > 	error: the branch 'foo' is not fully merged.
> > 	If you are sure you want to delete it, run 'git branch -D foo'.
> >
> > Let's move the hint part so that it takes advantage of the advice
> > machinery:
> >
> > 	error: the branch 'foo' is not fully merged
> > 	hint: If you are sure you want to delete it, run 'git branch -D foo'
> > 	hint: Disable this message with "git config advice.forceDeleteBranch false"
> 
> This is probably one sensible step forward, so let's queue it as-is.

Thanks.

> But with reservations for longer-term future direction.  Stepping
> back a bit, when 'foo' is not fully merged and the user used "branch
> -d" on it, is it sensible for us to suggest use of "branch -D"

Maybe the user hits here because he's doing "branch -d" and so I would
find a more clear suggestion: "branch -d foo -f".

Or to be more generic, not suggesting a command line but a description
that explains how to use "the force" ... :) sorry for the joke

Anyway, I think you mean to suggest a less destructive approach.  Which
is fine by me.

> Especially now this is a "hint" to help less experienced folks, it
> may be helpful to suggest how the user can answer "If you are sure
> you want to delete" part.  As this knows what unique commits on the
> branch being deleted are about to be lost, one way to do so may be
> to tell the user about them ("you are about to lose 'branch: error
> description when deleting a not fully merged branch' and other 47
> commits that are not merged the target branch 'main'", for example).

That's an interesting idea.  Maybe the hint becomes more informative
than a simple advice ...  maybe a more-verbose error is needed ...  just
thinking out loud ...

> 
> Another possibility is to suggest merging the branch into the
> target, instead of suggesting a destructive "deletion", but I
> suspect that it goes too far second-guessing the end-user intention.
> 
> Thanks.

Thank you.

Re: [PATCH v2 2/3] advice: fix an unexpected leading space

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> [ ... ]
> diff --git a/advice.h b/advice.h
> index 0f584163f5..2affbe1426 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -49,6 +49,7 @@ struct string_list;
>         ADVICE_UPDATE_SPARSE_PATH,
>         ADVICE_WAITING_FOR_EDITOR,
>         ADVICE_SKIPPED_CHERRY_PICKS,
> +       ADVICE_WORKTREE_ADD_ORPHAN,
>  };
>
> Note the hunk header, instead of a much more expected:
>
> @@ -49,6 +49,7 @@ enum advice_type

Next time, don't include "diff" output in the proposed log message
without indenting.  It makes it hard to read and parse.

The attached is what I queued in my tree.

Thanks.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------
From: Rubén Justo <rjusto@gmail.com>
Subject: [PATCH] advice: fix an unexpected leading space

This space was introduced, presumably unintentionally, in b3b18d1621
(advice: revamp advise API, 2020-03-02)

I notice this space due to confuse diff outputs while doing some
changes to enum advice_type.

As a reference, a recent change we have to that enum is:

    $ git show 35f0383

    ...
    diff --git a/advice.h b/advice.h
    index 0f584163f5..2affbe1426 100644
    --- a/advice.h
    +++ b/advice.h
    @@ -49,6 +49,7 @@ struct string_list;
	    ADVICE_UPDATE_SPARSE_PATH,
	    ADVICE_WAITING_FOR_EDITOR,
	    ADVICE_SKIPPED_CHERRY_PICKS,
    +       ADVICE_WORKTREE_ADD_ORPHAN,
     };

Note the hunk header, instead of a much more expected:

    @@ -49,6 +49,7 @@ enum advice_type

Signed-off-by: Rubén Justo <rjusto@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 advice.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/advice.h b/advice.h
index 9396bcdcf1..74d44d1156 100644
--- a/advice.h
+++ b/advice.h
@@ -10,7 +10,7 @@ struct string_list;
  * Add the new config variable to Documentation/config/advice.txt.
  * Call advise_if_enabled to print your advice.
  */
- enum advice_type {
+enum advice_type {
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
-- 
2.43.0-283-ga54a84b333

Re: [PATCH v2 2/3] advice: fix an unexpected leading space

[Rubén Justo]

On 11-ene-2024 17:21:22, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > [ ... ]
> > diff --git a/advice.h b/advice.h
> > index 0f584163f5..2affbe1426 100644
> > --- a/advice.h
> > +++ b/advice.h
> > @@ -49,6 +49,7 @@ struct string_list;
> >         ADVICE_UPDATE_SPARSE_PATH,
> >         ADVICE_WAITING_FOR_EDITOR,
> >         ADVICE_SKIPPED_CHERRY_PICKS,
> > +       ADVICE_WORKTREE_ADD_ORPHAN,
> >  };
> >
> > Note the hunk header, instead of a much more expected:
> >
> > @@ -49,6 +49,7 @@ enum advice_type
> 
> Next time, don't include "diff" output in the proposed log message
> without indenting.  It makes it hard to read and parse.

My fault.  Sorry.

Is there any way to make git-format-patch issue a warning or refuse to
continue when the resulting patch is not going to be accepted by git-am?

> 
> The attached is what I queued in my tree.

Thanks.

Re: [PATCH v2 2/3] advice: fix an unexpected leading space

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> On 11-ene-2024 17:21:22, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> 
>> > [ ... ]
>> > diff --git a/advice.h b/advice.h
>> > index 0f584163f5..2affbe1426 100644
>> > --- a/advice.h
>> > +++ b/advice.h
>> > @@ -49,6 +49,7 @@ struct string_list;
>> >         ADVICE_UPDATE_SPARSE_PATH,
>> >         ADVICE_WAITING_FOR_EDITOR,
>> >         ADVICE_SKIPPED_CHERRY_PICKS,
>> > +       ADVICE_WORKTREE_ADD_ORPHAN,
>> >  };
>> >
>> > Note the hunk header, instead of a much more expected:
>> >
>> > @@ -49,6 +49,7 @@ enum advice_type
>> 
>> Next time, don't include "diff" output in the proposed log message
>> without indenting.  It makes it hard to read and parse.
>
> My fault.  Sorry.
>
> Is there any way to make git-format-patch issue a warning or refuse to
> continue when the resulting patch is not going to be accepted by git-am?

I meant it as primarily to help human readers, but you are right, it
will break "am".  How about doing it in the pre-commit hook?

Re: [PATCH v2 2/3] advice: fix an unexpected leading space

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...  How about doing it in the pre-commit hook?

Sorry, as it runs before obtaining the proposed commit log message
and making a commit, pre-commit is a wrong one to use.  I meant to
say commit-msg hook that is used to verify the contents of the
proposed commit log message.