branch: advise the user to checkout a different branch before deleting

Display a hint to the user when attempting to delete a checked out branch saying "Checkout another branch before deleting this one:
git checkout <branch_name>".

Currently the user gets an error message saying: "error: Cannot delete branch <branch_name> checked out at <path>". The hint will be displayed after the error message.

Signed-off-by Heba Waly <[email protected]>

---

t/t3200-branch.sh +0 -0

        rm -r my7 &&
        git worktree prune
 '

Quoted 8 lines · Heba Waly via GitGitGadget

Display a hint to the user when attempting to delete a checked out branch saying "Checkout another branch before deleting this one:
git checkout <branch_name>".

Currently the user gets an error message saying: "error: Cannot delete branch <branch_name> checked out at <path>". The hint will be displayed after the error message.

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 244

Quoted 9 lines

Signed-off-by Heba Waly <[email protected]>

---

builtin/branch.c +2 -0

@@ -240,6 +240,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
                                error(_("Cannot delete branch '%s' "
                                        "checked out at '%s'"),
                                      bname.buf, wt->path);
+                               advise(_("Checkout another branch before deleting this "
+                                                "one: git checkout <branch_name>"));

s/another/a different/ would make the meaning clearer.

Let's try to avoid underscores in placeholders. <branch-name> would be better, however, git-checkout documentation just calls this <branch>, so that's probably a good choice.

However, these days, I think we're promoting git-switch rather than git-checkout, so perhaps this advice should follow suit.

Finally, is this advice sufficient for newcomers when the branch the user is trying to delete is in fact checked out in a worktree other than the worktree in which the git-branch command is being invoked? That is:

    $ pwd
    /home/me/foo
    $ git branch -D bip
    Cannot delete  branch 'bip' checked out at '/home/me/bar'
    hint: Checkout another branch before deleting this one:
    hint: git checkout <branch>
    $ git checkout master # user follows advice
    $ git branch -D bip
    Cannot delete  branch 'bip' checked out at '/home/me/foo'
    hint: Checkout another branch before deleting this one:
    hint: git checkout <branch>
    $

And the user is left scratching his or her head wondering why git-branch is still showing the error despite following the instructions in the hint.

t/t3200-branch.sh test_expect_success 'test deleting branch without config' ' · line 812

Quoted 8 lines

t/t3200-branch.sh +2 -1

@@ -808,7 +808,8 @@ test_expect_success 'test deleting branch without config' '
 test_expect_success 'deleting currently checked out branch fails' '
        git worktree add -b my7 my7 &&
        test_must_fail git -C my7 branch -d my7 &&
-       test_must_fail git branch -d my7 &&
+       test_must_fail git branch -d my7 >actual.out 2>actual.err &&
+       test_i18ngrep "hint: Checkout another branch" actual.err &&

Why does this capture standard output into 'actual.out' if that file is never consulted?

t/t3200-branch.sh test_expect_success 'test deleting branch without config' ' · line 815

Quoted 3 lines

t/t3200-branch.sh +0 -0

        rm -r my7 &&
        git worktree prune
 '

branch: advise the user to checkout a different branch before deleting

Display a hint to the user when attempting to delete a checked out branch.

Currently the user gets an error message saying: "error: Cannot delete branch <branch> checked out at <path>". The hint will be displayed after the error message.

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 254

Quoted 6 lines

builtin/branch.c +6 -0

+                                       }
+                                       else {
+                                               advise(_("The branch you are trying to delete is checked "
+                                                       "out on another worktree, run the following command "
+                                                       "to checkout a different branch then try again:\n"
+                                                       "git -C %s switch <branch>"), wt->path);

I like the use of -C here because it makes the command self-contained, however, I also worry because wt->path is an absolute path, thus likely to be quite lengthy, which means that the important part of the example command (the "switch <branch>") can get pushed quite far away, thus is more easily overlooked by the reader. I wonder if it would make more sense to show the 'cd' command explicitly, although doing so ties the example to a particular shell, which may be a downside.

cd %s
git switch <different-branch>
cd -
git branch -%c <this-branch>

(It is rather verbose and ugly, though.)

Eric Sunshine <[email protected]> writes:

[jc: skipped all the good suggestions I agree with]

Quoted 15 lines

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 254

Quoted 6 lines

builtin/branch.c +6 -0

+                                       }
+                                       else {
+                                               advise(_("The branch you are trying to delete is checked "
+                                                       "out on another worktree, run the following command "
+                                                       "to checkout a different branch then try again:\n"
+                                                       "git -C %s switch <branch>"), wt->path);

I like the use of -C here because it makes the command self-contained, however, I also worry because wt->path is an absolute path, thus likely to be quite lengthy, which means that the important part of the example command (the "switch <branch>") can get pushed quite far away, thus is more easily overlooked by the reader. I wonder if it would make more sense to show the 'cd' command explicitly, although doing so ties the example to a particular shell, which may be a downside.

Quoted 4 lines

cd %s
git switch <different-branch>
cd -
git branch -%c <this-branch>

Note that wt->path may have special characters that would need to be protected from the user's shell (worse, the quoting convention may be different depending on which shell is in use). That is one of the reasons why I would suggest to stay away from giving an advice that pretends to be cut-and-paste-able without being so.

In this
case, <different-branch> and <this-branch> must be filled by the user anyway, and the only thing worth cutting-and-pasting is the path to the other worktree, not the "git -C" or "cd" that users should be able to come up with.

"The branch is checked out on another worktree at\n" "path '%s'\n"
"and cannot be deleted. Go there, check out some other\n" "branch and try again."

or something like that, perhaps?

Quoted 1 line

(It is rather verbose and ugly, though.)

I tend to agree. It also feels to me that it is giving too much hand-holding, but after all advise() may turning out to be about giving that.

Quoted 6 lines · Junio C Hamano

Quoted 1 line · Eric Sunshine

(It is rather verbose and ugly, though.)

I tend to agree. It also feels to me that it is giving too much hand-holding, but after all advise() may turning out to be about giving that.

Well, if advise() isn't going to hold their hand, who is? ;) What I mean is, I think that's indeed what advise() is about, and the reason it can be disabled in config.

To me, the harm of giving too much hand-holding seems less than the harm of giving not enough; to deal with the one requires skimming past things you already know [...]

Quoted 8 lines · Heba Waly via GitGitGadget

branch: advise the user to checkout a different branch before deleting

Display a hint to the user when attempting to delete a checked out branch.

Currently the user gets an error message saying: "error: Cannot delete branch <branch> checked out at <path>". The hint will be displayed after the error message.

A couple comments...

The second paragraph doesn't say anything beyond what the patch/code itself already says clearly (plus, there's no need to state the obvious), so the paragraph adds no value (yet eats up reviewer time). Therefore, it can be dropped.

To convince readers that the change made by the patch is indeed warranted, it's always important to explain _why_ this change is being made.

Both points can be addressed with a short and sweet commit message, perhaps like this:

branch: advise how to delete checked-out branch

Teach newcomers how to deal with Git refusing to delete a checked-out branch (whether in the current worktree or some other).

By the way, did you actually run across a real-world case in which someone was confused about how to resolve this situation? I ask because this almost seems like too much hand-holding, and it would be nice to avoid polluting Git with unnecessary advice.

advice.c static struct { · line 94

Quoted 9 lines

Signed-off-by Heba Waly <[email protected]>

---

advice.c +2 -1

@@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;

@@ -91,7 +92,8 @@ static struct {
        { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-
+       { "deleteCheckedoutBranch", &advice_delete_checkedout_branch },
+

When you see an odd-looking diff like this in which you wouldn't expect any diff markers on the blank line (that is, the blank line got deleted and re-added), it's a good indication that there's unwanted trailing whitespace on one of the lines. In this case, you (or more likely your editor automatically) added trailing whitespace to the blank line which doesn't belong there. Unwanted whitespace changes like this make the patch noisier and more difficult for a reviewer to read.

advice.h extern int advice_graft_file_deprecated; · line 31

Quoted 3 lines

advice.h +1 -0

@@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;
+extern int advice_delete_checkedout_branch;

Is there precedent elsewhere for spelling this "checkedout" rather than the more natural "checked_out"?

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 245

Quoted 8 lines

builtin/branch.c +6 -0

@@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
+                               if (advice_delete_checkedout_branch) {
+                                       if (wt->is_current) {
+                                               advise(_("The branch you are trying to delete is already "
+                                                       "checked out, run the following command to "
+                                                       "checkout a different branch then try again:\n"
+                                                       "git switch <branch>"));

This advice unnecessarily repeats what the error message just above it already says about the branch being checked out (thus adds no value), and then jumps directly into showing the user an opaque command to resolve the situation without explaining _how_ or _why_ the command is supposed to help.

Advice messages elsewhere typically indent the example command to make it stand out from the explanatory prose (and often separated it from the text by a blank line).

A rewrite which addresses both these issues might be something like:

Switch to a different branch before trying to delete it. For example:

git switch <different-branch>
git branch -%c <this-branch>

(and fill in %c with either "-d" or "-D" depending upon the value of 'force')

builtin/branch.c static int delete_branches(int argc, const char **argv, int force, int kinds, · line 254

Quoted 6 lines

builtin/branch.c +6 -0

+                                       }
+                                       else {
+                                               advise(_("The branch you are trying to delete is checked "
+                                                       "out on another worktree, run the following command "
+                                                       "to checkout a different branch then try again:\n"
+                                                       "git -C %s switch <branch>"), wt->path);

I like the use of -C here because it makes the command self-contained, however, I also worry because wt->path is an absolute path, thus likely to be quite lengthy, which means that the important part of the example command (the "switch <branch>") can get pushed quite far away, thus is more easily overlooked by the reader. I wonder if it would make more sense to show the 'cd' command explicitly, although doing so ties the example to a particular shell, which may be a downside.

cd %s
git switch <different-branch>
cd -
git branch -%c <this-branch>

(It is rather verbose and ugly, though.)

t/t3200-branch.sh test_expect_success 'test deleting branch without config' ' · line 812

Quoted 10 lines

t/t3200-branch.sh +4 -2

@@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' '
 test_expect_success 'deleting currently checked out branch fails' '
        git worktree add -b my7 my7 &&
-       test_must_fail git -C my7 branch -d my7 &&
-       test_must_fail git branch -d my7 &&
+       test_must_fail git -C my7 branch -d my7 2>output1.err &&
+       test_must_fail git branch -d my7 2>output2.err &&
+       test_i18ngrep "hint: The branch you are trying to delete is already checked out" output1.err &&
+       test_i18ngrep "hint: The branch you are trying to delete is checked out on another worktree" output2.err &&

Nit: Separating the 'grep' from the command which generated the error output makes it harder for a reader to see at a glance what is being tested and to reason about it since it demands that the reader keep two distinct cases in mind rather than merely focusing on one at a time. Also, doing it this way forces you to invent distinct filenames (by appending a number, for instance), which further leads the reader to wonder if there is some significance (later in the test) to keeping these outputs in separate files. So, a better organization (with more natural filenames) would be:

    test_must_fail git -C my7 branch -d my7 2>output.err &&
    test_i18ngrep "hint: ..." output.err &&
    test_must_fail git branch -d my7 2>output.err &&
    test_i18ngrep "hint: ..." output.err &&

Quoted 4 lines · Eric Sunshine

By the way, did you actually run across a real-world case in which someone was confused about how to resolve this situation? I ask because this almost seems like too much hand-holding, and it would be nice to avoid polluting Git with unnecessary advice.

No I didn't. I was trying to find scenarios where git can give more user-friendly messages to its users.
I see your point though, so I don't mind not proceeding with this patch if the community doesn't think it's adding any value.

advice seems simple on the surface, but every new piece of advice means having to add yet another configuration variable, writing more code, more tests, and more documentation

Quoted 3 lines · Eric Sunshine

advice seems simple on the surface, but every new piece of advice means having to add yet another configuration variable, writing more code, more tests, and more documentation

This raises a question though, do we really need a new configuration for every new advice?

So a user who's not interested in receiving advice will have to disable every single advice config? It doesn't seem scalable to me. I imagine a user will either want to enable or disable the advice feature all together. Why don't we have only one enable_advice configuration that controls all the advice messages?

This is the first time I hear about anybody wanting to disable any advice
...
I don't really think that this is desired, though.

Quoted 3 lines · Johannes Schindelin

This is the first time I hear about anybody wanting to disable any advice
...
I don't really think that this is desired, though.

Me neither. We seem to have come up with more-or-less the same illustration, but such a global "turn all off" needs to be explained very well before we let users blindly use it, I think.

Quoted 4 lines · Eric Sunshine

advice seems simple on the surface, but every new piece of advice means having to add yet another configuration variable, writing more code, more tests, and more documentation

This raises a question though, do we really need a new configuration for every new advice?
So a user who's not interested in receiving advice will have to disable every single advice config? It doesn't seem scalable to me. I imagine a user will either want to enable or disable the advice feature all together. Why don't we have only one enable_advice configuration that controls all the advice messages?

My own feeling is that this level of hand-holding is unnecessary, at least until we a discover a good number of real-world cases in which people are baffled by how to deal with this situation. Adding the advice seems simple on the surface, but every new piece of advice means having to add yet another configuration variable, writing more code, more tests, and more documentation, and it needs to be maintained for the life of the project. So what seems simple at first glance, can end up being costly in terms of developer resources. For a bit of advice which doesn't seem to be needed by anyone (yet), all that effort seem unwarranted. Thus, my preference is to see the patch dropped.