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

[Junio C Hamano <gitster@pobox.com>]

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;