| Message ID | 04c3556f-0242-4ac3-b94a-be824cd2004a@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | branch: error description when deleting a not fully merged branch | expand |
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;
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.
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.
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;
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(-)