Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 6, 2020 at 11:10 PM Heba Waly via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.

> Signed-off-by: Heba Waly <heba.waly@gmail.com>
> ---
> diff --git a/advice.c b/advice.c
> @@ -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.

> diff --git a/advice.h b/advice.h
> @@ -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"?

> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -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')

> +                                       }
> +                                       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.)

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -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 &&