[Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting

[HebaWaly]

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

[HebaWaly]

/submit

[gitgitgadget]

Submitted as pull.507.git.1577933387.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Heba Waly wrote (reply to this):

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

[HebaWaly]

/submit

[gitgitgadget]

Submitted as pull.507.v2.git.1578370226.gitgitgadget@gmail.com

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Heba Waly wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Emily Shaffer wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

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.

[gitgitgadget]

On the Git mailing list, Heba Waly wrote (reply to this):

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

[gitgitgadget]

On the Git mailing list, Heba Waly wrote (reply to this):

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.