[PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting

[PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting

[Heba Waly via GitGitGadget]

When a user attempts to delete a checked out branch, an error message is
displayed saying: "error: Cannot delete branch checked out at ". This patch
suggests displaying a hint after the error message advising the user to
checkout another branch first using "git checkout ".

Heba Waly (1):
  branch: advise the user to checkout a different branch before deleting

 builtin/branch.c  | 2 ++
 t/t3200-branch.sh | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)


base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/507
-- 
gitgitgadget

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

[Heba Waly via GitGitGadget]

From: Heba Waly <heba.waly@gmail.com>

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 <heba.waly@gmail.com>
---
 builtin/branch.c  | 2 ++
 t/t3200-branch.sh | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..799e967008 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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>"));
 				ret = 1;
 				continue;
 			}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 411a70b0ce..3b2812a8f4 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -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 &&
 	rm -r my7 &&
 	git worktree prune
 '
-- 
gitgitgadget

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

[Eric Sunshine]

On Wed, Jan 1, 2020 at 9:50 PM Heba Waly via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 <heba.waly@gmail.com>
> ---
> diff --git a/builtin/branch.c b/builtin/branch.c
> @@ -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.

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

>         rm -r my7 &&
>         git worktree prune
>  '

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

[Heba Waly]

On Thu, Jan 2, 2020 at 9:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Jan 1, 2020 at 9:50 PM Heba Waly via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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 <heba.waly@gmail.com>
> > ---
> > diff --git a/builtin/branch.c b/builtin/branch.c
> > @@ -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.
>
Ok.

> 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.
>
Yes.

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

I didn't know that, will change it.

> 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.
>

Understood.

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

Correct, I missed this one.

> >         rm -r my7 &&
> >         git worktree prune
> >  '

Thanks Eric, will submit an updated version soon.

Heba

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

[Heba Waly via GitGitGadget]

When a user attempts to delete a checked out branch, an error message is
displayed saying: "error: Cannot delete branch checked out at ". This patch
suggests displaying a hint after the error message advising the user to
checkout another branch first using "git checkout ".

Heba Waly (1):
  branch: advise the user to checkout a different branch before deleting

 advice.c          |  4 +++-
 advice.h          |  1 +
 builtin/branch.c  | 14 ++++++++++++++
 t/t3200-branch.sh |  6 ++++--
 4 files changed, 22 insertions(+), 3 deletions(-)


base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/507

Range-diff vs v1:

 1:  82bf24ce53 ! 1:  19a7cc1889 branch: advise the user to checkout a different branch before deleting
     @@ -3,15 +3,48 @@
          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>".
     +    branch.
      
          Currently the user gets an error message saying: "error: Cannot delete
     -    branch <branch_name> checked out at <path>". The hint will be displayed
     +    branch <branch> checked out at <path>". The hint will be displayed
          after the error message.
      
          Signed-off-by: Heba Waly <heba.waly@gmail.com>
      
     + diff --git a/advice.c b/advice.c
     + --- a/advice.c
     + +++ b/advice.c
     +@@
     + int advice_checkout_ambiguous_remote_branch_name = 1;
     + int advice_nested_tag = 1;
     + int advice_submodule_alternate_error_strategy_die = 1;
     ++int advice_delete_checkedout_branch = 1;
     + 
     + static int advice_use_color = -1;
     + static char advice_colors[][COLOR_MAXLEN] = {
     +@@
     + 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
     + 	{ "nestedTag", &advice_nested_tag },
     + 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
     +-
     ++	{ "deleteCheckedoutBranch", &advice_delete_checkedout_branch },
     ++	
     + 	/* make this an alias for backward compatibility */
     + 	{ "pushNonFastForward", &advice_push_update_rejected }
     + };
     +
     + diff --git a/advice.h b/advice.h
     + --- a/advice.h
     + +++ b/advice.h
     +@@
     + extern int advice_checkout_ambiguous_remote_branch_name;
     + extern int advice_nested_tag;
     + extern int advice_submodule_alternate_error_strategy_die;
     ++extern int advice_delete_checkedout_branch;
     + 
     + int git_default_advice_config(const char *var, const char *value);
     + __attribute__((format (printf, 1, 2)))
     +
       diff --git a/builtin/branch.c b/builtin/branch.c
       --- a/builtin/branch.c
       +++ b/builtin/branch.c
     @@ -19,8 +52,20 @@
       				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>"));
     ++				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>"));
     ++					}
     ++					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);
     ++					}
     ++				}
       				ret = 1;
       				continue;
       			}
     @@ -29,12 +74,15 @@
       --- a/t/t3200-branch.sh
       +++ b/t/t3200-branch.sh
      @@
     + 
       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 -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 &&
     ++	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 &&
       	rm -r my7 &&
       	git worktree prune
       '

-- 
gitgitgadget

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

[Heba Waly via GitGitGadget]

From: Heba Waly <heba.waly@gmail.com>

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.

Signed-off-by: Heba Waly <heba.waly@gmail.com>
---
 advice.c          |  4 +++-
 advice.h          |  1 +
 builtin/branch.c  | 14 ++++++++++++++
 t/t3200-branch.sh |  6 ++++--
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/advice.c b/advice.c
index 249c60dcf3..0a8fd2f68e 100644
--- a/advice.c
+++ b/advice.c
@@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;
 int advice_checkout_ambiguous_remote_branch_name = 1;
 int advice_nested_tag = 1;
 int advice_submodule_alternate_error_strategy_die = 1;
+int advice_delete_checkedout_branch = 1;
 
 static int advice_use_color = -1;
 static char advice_colors[][COLOR_MAXLEN] = {
@@ -91,7 +92,8 @@ static struct {
 	{ "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name },
 	{ "nestedTag", &advice_nested_tag },
 	{ "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die },
-
+	{ "deleteCheckedoutBranch", &advice_delete_checkedout_branch },
+	
 	/* make this an alias for backward compatibility */
 	{ "pushNonFastForward", &advice_push_update_rejected }
 };
diff --git a/advice.h b/advice.h
index b706780614..e75c5ee33c 100644
--- a/advice.h
+++ b/advice.h
@@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;
 extern int advice_checkout_ambiguous_remote_branch_name;
 extern int advice_nested_tag;
 extern int advice_submodule_alternate_error_strategy_die;
+extern int advice_delete_checkedout_branch;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/builtin/branch.c b/builtin/branch.c
index d8297f80ff..d1a9443e36 100644
--- 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,
 				error(_("Cannot delete branch '%s' "
 					"checked out at '%s'"),
 				      bname.buf, wt->path);
+				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>"));
+					}
+					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);
+					}
+				}
 				ret = 1;
 				continue;
 			}
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 411a70b0ce..edb01bee45 100755
--- 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 &&
 	rm -r my7 &&
 	git worktree prune
 '
-- 
gitgitgadget

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

[Eric Sunshine]

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 &&

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

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?  

> (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.

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

[Emily Shaffer]

On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> [jc: skipped all the good suggestions I agree with]
> 
> >> +                                       }
> >> +                                       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>
> 
> 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.

Hm, I think you've sold me on the error of my ways trying to push for
copy-pasteable advices :)
But I wonder, how much is too much? I mean that suggesting a single Git
command which takes a branch name and a pathspec is safer than
suggesting some complicated -C=foo or cd bar thing, right?

> 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?  
> 
> > (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, and to deal with
the other requires web searching, asking people, reading documentation,
perhaps gaining "tips" from questionable blogs which don't actually give
very useful advice. I think we were just discussing not long ago the
general quality of advice on StackOverflow, for example.

 - Emily

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

[Eric Sunshine]

On Tue, Jan 7, 2020 at 8:44 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote:
> > Eric Sunshine <sunshine@sunshineco.com> writes:
> > > (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.

Git is already drowning in configuration options. Every new option
increases the complexity level for the user and of Git overall,
requires additional code, additional tests, and additional
documentation, and must be maintained for the life of the project. So,
every configuration option carries a cost in terms of end-user
experience and developer resources.

If anything, we should be trying to keep the number of configuration
options constant (or, even better, reduce it), rather than forever
adding more. Thus, we should think very hard before adding any new
configuration option, trying instead to discover ways in which an
issue can be addressed without adding a configuration option (with its
attendant costs and complexity), and only add an option as a last
resort.

> 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 [...]

Perhaps I'm too old-school and too steeped in The Unix Way, but there
is value in silence and conciseness, and that value outweighs
chattiness, especially when chattiness is effectively unnecessary
(which is the case in the vast majority of error/warning messages
emitted by Git).

Too much hand-holding quickly becomes noise which gets ignored, thus
eventually drowns out the really important information. People need to
understand a problem before taking action to solve it. Blindly
following some piece of advice without understanding a problem can
lead to further problems (especially in those cases when it's not
possible to devise advice which suitably covers all situations in
which a particular problem may arise).

A well-written, clear, _concise_, error message can often provide
enough information to allow the user to understand and resolve the
problem without additional hand-holding. It's true that there are some
complex situations (for instance, detached HEAD) for which additional
hints can be handy for newcomers or casual users, but that should be
the exception, not the norm. Rather than adding advice messages for
every possible problem, perhaps a better use of our time would be to
weed out poorly-worded and unhelpful error messages and improve them.

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

[Heba Waly]

On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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).
>

Agree, thanks.

> 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.

> > 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.
>

Mystery solved! thanks.

> > 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"?
>

Not really.

> > 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')

Ok.

> > +                                       }
> > +                                       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 &&

Agree.

Thanks Eric, I'll not update this patch unless somebody thinks it's
adding a value and worth spending more time on it.

Heba

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

[Eric Sunshine]

On Tue, Jan 7, 2020 at 8:15 PM Heba Waly <heba.waly@gmail.com> wrote:
> On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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.

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.

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

[Heba Waly]

On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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?

Thanks,
Heba

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

[Johannes Schindelin]

Hi,

On Thu, 9 Jan 2020, Heba Waly wrote:

> On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> > 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

FWIW I disagree that we need to reduce the number of config settings.
Pretty much all of them have a good reason to exist.

I _could_ however see some sort of categorisation as a valuable goal,
which would potentially make it easier to have chapters in the `git
config` documentation where earlier chapters describe common settings
and the later chapters describe subsequently more obscure settings.

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

I would keep it this way, if only for consistency (a department in which
Git still has a lot of room for improvement).

> 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
;-)

If this is desired, it should be easy enough:

-- snip --
diff --git a/advice.c b/advice.c
index 3ee0ee2d8fb..28e48d5410b 100644
--- a/advice.c
+++ b/advice.c
@@ -138,6 +138,13 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;

+	if (!strcmp(k, "suppressall")) {
+		if (git_config_bool(var, value))
+			for (i = 0; i < ARRAY_SIZE(advice_config); i++)
+				*advice_config[i].preference = 0;
+		return 0;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
 		if (strcasecmp(k, advice_config[i].name))
 			continue;
-- snap --

I don't really think that this is desired, though. Git has earned a
reputation for being hard to use, so I was personally delighted when we
started introducing the advise feature, and I have actually heard a couple
users say good things whenever Git learns to help them without having to
ask another human being (and feeling dumb as a consequence).

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> 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.

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

[Heba Waly]

On Thu, Jan 9, 2020 at 8:15 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.

Thank you Dscho and Junio.

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

[Junio C Hamano]

Heba Waly <heba.waly@gmail.com> writes:

> On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>>
>> 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?

The advice mechanism was a way to help new people learn the system
by giving a bit of extra help messages that would become annoying
once they learned that part of the system, so by default they are
on, and can be turned off once they learn enough about the specific
situation that gives one kind of advise.  Hence, "[advice] !all" to
decline any and all advice message, including anything that would be
introduced in the future, is somewhat a foreign concept in that
picture.

Having said that, I am not opposed to add support for such an
overall "turn all off" (or on for that matter).  Totally untested,
but something along this line, perhaps?  The idea is that

 - the config keys may come in any order;

 - once advice.all is set to either true or false, we set all the
   advice.* variables to the given value,

 - for any other advice.* config, we interpret it only if we haven't
   seen advice.all



 advice.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/advice.c b/advice.c
index 098ac0abea..b9a8fe1360 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,8 @@
 #include "color.h"
 #include "help.h"
 
+static int advice_all_seen = -1; /* not seen yet */
+
 int advice_fetch_show_forced_updates = 1;
 int advice_push_update_rejected = 1;
 int advice_push_non_ff_current = 1;
@@ -142,13 +144,22 @@ int git_default_advice_config(const char *var, const char *value)
 	if (!skip_prefix(var, "advice.", &k))
 		return 0;
 
-	for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
-		if (strcasecmp(k, advice_config[i].name))
-			continue;
-		*advice_config[i].preference = git_config_bool(var, value);
+	if (!strcmp(var, "advise.all")) {
+		advice_all_seen = git_config_bool(var, value);
+		for (i = 0; i < ARRAY_SIZE(advice_config); i++)
+			*advice_config[i].preference = advice_all_seen;
 		return 0;
 	}
 
+	if (advice_all_seen < 0) {
+		for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
+			if (strcasecmp(k, advice_config[i].name))
+				continue;
+			*advice_config[i].preference = git_config_bool(var, value);
+			return 0;
+		}
+	}
+
 	return 0;
 }
 

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

[Heba Waly]

On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> 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.

That's Ok. We can drop it.

Thanks,
Heba