{"thread":{"id":"52548","subject":"[PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting","startedAt":"2020-01-02T02:49:52Z","lastAt":"2020-01-10T12:12:10Z","messageCount":18,"participants":["Heba Waly via GitGitGadget","Eric Sunshine","Heba Waly","Junio C Hamano","Emily Shaffer","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"389157","messageId":"pull.507.git.1577933387.gitgitgadget@gmail.com","threadId":"52548","inReplyTo":null,"subject":"[PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-02T02:49:46Z","receivedAt":"2020-01-02T02:49:52Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"When a user attempts to delete a checked out branch, an error message is\ndisplayed saying: \"error: Cannot delete branch checked out at \". This patch\nsuggests displaying a hint after the error message advising the user to\ncheckout another branch first using \"git checkout \".\n\nHeba Waly (1):\n  branch: advise the user to checkout a different branch before deleting\n\n builtin/branch.c  | 2 ++\n t/t3200-branch.sh | 3 ++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\n\nbase-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/507\n-- \ngitgitgadget\n"},{"id":"389158","messageId":"82bf24ce537ca9333d72c2b4698864817801f10f.1577933387.git.gitgitgadget@gmail.com","threadId":"52548","inReplyTo":"pull.507.git.1577933387.gitgitgadget@gmail.com","subject":"[PATCH 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-02T02:49:47Z","receivedAt":"2020-01-02T02:49:55Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"From: Heba Waly <heba.waly@gmail.com>\n\nDisplay a hint to the user when attempting to delete a checked out\nbranch saying \"Checkout another branch before deleting this one:\ngit checkout <branch_name>\".\n\nCurrently the user gets an error message saying: \"error: Cannot delete\nbranch <branch_name> checked out at <path>\". The hint will be displayed\nafter the error message.\n\nSigned-off-by: Heba Waly <heba.waly@gmail.com>\n---\n builtin/branch.c  | 2 ++\n t/t3200-branch.sh | 3 ++-\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..799e967008 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -240,6 +240,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t      bname.buf, wt->path);\n+\t\t\t\tadvise(_(\"Checkout another branch before deleting this \"\n+\t\t\t\t\t\t \"one: git checkout <branch_name>\"));\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 411a70b0ce..3b2812a8f4 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -808,7 +808,8 @@ test_expect_success 'test deleting branch without config' '\n test_expect_success 'deleting currently checked out branch fails' '\n \tgit worktree add -b my7 my7 &&\n \ttest_must_fail git -C my7 branch -d my7 &&\n-\ttest_must_fail git branch -d my7 &&\n+\ttest_must_fail git branch -d my7 >actual.out 2>actual.err &&\n+\ttest_i18ngrep \"hint: Checkout another branch\" actual.err &&\n \trm -r my7 &&\n \tgit worktree prune\n '\n-- \ngitgitgadget\n"},{"id":"389161","messageId":"CAPig+cS39vcy6yT3Dg2HfGVCyg2U+7t7Xj85ayM7LaAk3zTjrg@mail.gmail.com","threadId":"52548","inReplyTo":"82bf24ce537ca9333d72c2b4698864817801f10f.1577933387.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-02T08:18:29Z","receivedAt":"2020-01-02T08:18:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 1, 2020 at 9:50 PM Heba Waly via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Display a hint to the user when attempting to delete a checked out\n> branch saying \"Checkout another branch before deleting this one:\n> git checkout <branch_name>\".\n>\n> Currently the user gets an error message saying: \"error: Cannot delete\n> branch <branch_name> checked out at <path>\". The hint will be displayed\n> after the error message.\n>\n> Signed-off-by: Heba Waly <heba.waly@gmail.com>\n> ---\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> @@ -240,6 +240,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n>                                 error(_(\"Cannot delete branch '%s' \"\n>                                         \"checked out at '%s'\"),\n>                                       bname.buf, wt->path);\n> +                               advise(_(\"Checkout another branch before deleting this \"\n> +                                                \"one: git checkout <branch_name>\"));\n\ns/another/a different/ would make the meaning clearer.\n\nLet's try to avoid underscores in placeholders. <branch-name> would be\nbetter, however, git-checkout documentation just calls this <branch>,\nso that's probably a good choice.\n\nHowever, these days, I think we're promoting git-switch rather than\ngit-checkout, so perhaps this advice should follow suit.\n\nFinally, is this advice sufficient for newcomers when the branch the\nuser is trying to delete is in fact checked out in a worktree other\nthan the worktree in which the git-branch command is being invoked?\nThat is:\n\n    $ pwd\n    /home/me/foo\n    $ git branch -D bip\n    Cannot delete  branch 'bip' checked out at '/home/me/bar'\n    hint: Checkout another branch before deleting this one:\n    hint: git checkout <branch>\n    $ git checkout master # user follows advice\n    $ git branch -D bip\n    Cannot delete  branch 'bip' checked out at '/home/me/foo'\n    hint: Checkout another branch before deleting this one:\n    hint: git checkout <branch>\n    $\n\nAnd the user is left scratching his or her head wondering why\ngit-branch is still showing the error despite following the\ninstructions in the hint.\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> @@ -808,7 +808,8 @@ test_expect_success 'test deleting branch without config' '\n>  test_expect_success 'deleting currently checked out branch fails' '\n>         git worktree add -b my7 my7 &&\n>         test_must_fail git -C my7 branch -d my7 &&\n> -       test_must_fail git branch -d my7 &&\n> +       test_must_fail git branch -d my7 >actual.out 2>actual.err &&\n> +       test_i18ngrep \"hint: Checkout another branch\" actual.err &&\n\nWhy does this capture standard output into 'actual.out' if that file\nis never consulted?\n\n>         rm -r my7 &&\n>         git worktree prune\n>  '\n"},{"id":"389257","messageId":"CACg5j25bNcy66R3bCwwc1NQ1F1rEoc=QOPBteyux0Xr6xwHLSQ@mail.gmail.com","threadId":"52548","inReplyTo":"CAPig+cS39vcy6yT3Dg2HfGVCyg2U+7t7Xj85ayM7LaAk3zTjrg@mail.gmail.com","subject":"Re: [PATCH 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2020-01-06T00:42:03Z","receivedAt":"2020-01-06T00:42:20Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Thu, Jan 2, 2020 at 9:18 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Wed, Jan 1, 2020 at 9:50 PM Heba Waly via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > Display a hint to the user when attempting to delete a checked out\n> > branch saying \"Checkout another branch before deleting this one:\n> > git checkout <branch_name>\".\n> >\n> > Currently the user gets an error message saying: \"error: Cannot delete\n> > branch <branch_name> checked out at <path>\". The hint will be displayed\n> > after the error message.\n> >\n> > Signed-off-by: Heba Waly <heba.waly@gmail.com>\n> > ---\n> > diff --git a/builtin/branch.c b/builtin/branch.c\n> > @@ -240,6 +240,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> >                                 error(_(\"Cannot delete branch '%s' \"\n> >                                         \"checked out at '%s'\"),\n> >                                       bname.buf, wt->path);\n> > +                               advise(_(\"Checkout another branch before deleting this \"\n> > +                                                \"one: git checkout <branch_name>\"));\n>\n> s/another/a different/ would make the meaning clearer.\n>\nOk.\n\n> Let's try to avoid underscores in placeholders. <branch-name> would be\n> better, however, git-checkout documentation just calls this <branch>,\n> so that's probably a good choice.\n>\nYes.\n\n> However, these days, I think we're promoting git-switch rather than\n> git-checkout, so perhaps this advice should follow suit.\n>\n\nI didn't know that, will change it.\n\n> Finally, is this advice sufficient for newcomers when the branch the\n> user is trying to delete is in fact checked out in a worktree other\n> than the worktree in which the git-branch command is being invoked?\n> That is:\n>\n>     $ pwd\n>     /home/me/foo\n>     $ git branch -D bip\n>     Cannot delete  branch 'bip' checked out at '/home/me/bar'\n>     hint: Checkout another branch before deleting this one:\n>     hint: git checkout <branch>\n>     $ git checkout master # user follows advice\n>     $ git branch -D bip\n>     Cannot delete  branch 'bip' checked out at '/home/me/foo'\n>     hint: Checkout another branch before deleting this one:\n>     hint: git checkout <branch>\n>     $\n>\n> And the user is left scratching his or her head wondering why\n> git-branch is still showing the error despite following the\n> instructions in the hint.\n>\n\nUnderstood.\n\n> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> > @@ -808,7 +808,8 @@ test_expect_success 'test deleting branch without config' '\n> >  test_expect_success 'deleting currently checked out branch fails' '\n> >         git worktree add -b my7 my7 &&\n> >         test_must_fail git -C my7 branch -d my7 &&\n> > -       test_must_fail git branch -d my7 &&\n> > +       test_must_fail git branch -d my7 >actual.out 2>actual.err &&\n> > +       test_i18ngrep \"hint: Checkout another branch\" actual.err &&\n>\n> Why does this capture standard output into 'actual.out' if that file\n> is never consulted?\n>\n\nCorrect, I missed this one.\n\n> >         rm -r my7 &&\n> >         git worktree prune\n> >  '\n\nThanks Eric, will submit an updated version soon.\n\nHeba\n"},{"id":"389319","messageId":"pull.507.v2.git.1578370226.gitgitgadget@gmail.com","threadId":"52548","inReplyTo":"pull.507.git.1577933387.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T04:10:25Z","receivedAt":"2020-01-07T04:10:32Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"When a user attempts to delete a checked out branch, an error message is\ndisplayed saying: \"error: Cannot delete branch checked out at \". This patch\nsuggests displaying a hint after the error message advising the user to\ncheckout another branch first using \"git checkout \".\n\nHeba Waly (1):\n  branch: advise the user to checkout a different branch before deleting\n\n advice.c          |  4 +++-\n advice.h          |  1 +\n builtin/branch.c  | 14 ++++++++++++++\n t/t3200-branch.sh |  6 ++++--\n 4 files changed, 22 insertions(+), 3 deletions(-)\n\n\nbase-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/507\n\nRange-diff vs v1:\n\n 1:  82bf24ce53 ! 1:  19a7cc1889 branch: advise the user to checkout a different branch before deleting\n     @@ -3,15 +3,48 @@\n          branch: advise the user to checkout a different branch before deleting\n      \n          Display a hint to the user when attempting to delete a checked out\n     -    branch saying \"Checkout another branch before deleting this one:\n     -    git checkout <branch_name>\".\n     +    branch.\n      \n          Currently the user gets an error message saying: \"error: Cannot delete\n     -    branch <branch_name> checked out at <path>\". The hint will be displayed\n     +    branch <branch> checked out at <path>\". The hint will be displayed\n          after the error message.\n      \n          Signed-off-by: Heba Waly <heba.waly@gmail.com>\n      \n     + diff --git a/advice.c b/advice.c\n     + --- a/advice.c\n     + +++ b/advice.c\n     +@@\n     + int advice_checkout_ambiguous_remote_branch_name = 1;\n     + int advice_nested_tag = 1;\n     + int advice_submodule_alternate_error_strategy_die = 1;\n     ++int advice_delete_checkedout_branch = 1;\n     + \n     + static int advice_use_color = -1;\n     + static char advice_colors[][COLOR_MAXLEN] = {\n     +@@\n     + \t{ \"checkoutAmbiguousRemoteBranchName\", &advice_checkout_ambiguous_remote_branch_name },\n     + \t{ \"nestedTag\", &advice_nested_tag },\n     + \t{ \"submoduleAlternateErrorStrategyDie\", &advice_submodule_alternate_error_strategy_die },\n     +-\n     ++\t{ \"deleteCheckedoutBranch\", &advice_delete_checkedout_branch },\n     ++\t\n     + \t/* make this an alias for backward compatibility */\n     + \t{ \"pushNonFastForward\", &advice_push_update_rejected }\n     + };\n     +\n     + diff --git a/advice.h b/advice.h\n     + --- a/advice.h\n     + +++ b/advice.h\n     +@@\n     + extern int advice_checkout_ambiguous_remote_branch_name;\n     + extern int advice_nested_tag;\n     + extern int advice_submodule_alternate_error_strategy_die;\n     ++extern int advice_delete_checkedout_branch;\n     + \n     + int git_default_advice_config(const char *var, const char *value);\n     + __attribute__((format (printf, 1, 2)))\n     +\n       diff --git a/builtin/branch.c b/builtin/branch.c\n       --- a/builtin/branch.c\n       +++ b/builtin/branch.c\n     @@ -19,8 +52,20 @@\n       \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n       \t\t\t\t\t\"checked out at '%s'\"),\n       \t\t\t\t      bname.buf, wt->path);\n     -+\t\t\t\tadvise(_(\"Checkout another branch before deleting this \"\n     -+\t\t\t\t\t\t \"one: git checkout <branch_name>\"));\n     ++\t\t\t\tif (advice_delete_checkedout_branch) {\n     ++\t\t\t\t\tif (wt->is_current) {\n     ++\t\t\t\t\t\tadvise(_(\"The branch you are trying to delete is already \"\n     ++\t\t\t\t\t\t\t\"checked out, run the following command to \"\n     ++\t\t\t\t\t\t\t\"checkout a different branch then try again:\\n\"\n     ++\t\t\t\t\t\t\t\"git switch <branch>\"));\n     ++\t\t\t\t\t}\n     ++\t\t\t\t\telse {\n     ++\t\t\t\t\t\tadvise(_(\"The branch you are trying to delete is checked \"\n     ++\t\t\t\t\t\t\t\"out on another worktree, run the following command \"\n     ++\t\t\t\t\t\t\t\"to checkout a different branch then try again:\\n\"\n     ++\t\t\t\t\t\t\t\"git -C %s switch <branch>\"), wt->path);\n     ++\t\t\t\t\t}\n     ++\t\t\t\t}\n       \t\t\t\tret = 1;\n       \t\t\t\tcontinue;\n       \t\t\t}\n     @@ -29,12 +74,15 @@\n       --- a/t/t3200-branch.sh\n       +++ b/t/t3200-branch.sh\n      @@\n     + \n       test_expect_success 'deleting currently checked out branch fails' '\n       \tgit worktree add -b my7 my7 &&\n     - \ttest_must_fail git -C my7 branch -d my7 &&\n     +-\ttest_must_fail git -C my7 branch -d my7 &&\n      -\ttest_must_fail git branch -d my7 &&\n     -+\ttest_must_fail git branch -d my7 >actual.out 2>actual.err &&\n     -+\ttest_i18ngrep \"hint: Checkout another branch\" actual.err &&\n     ++\ttest_must_fail git -C my7 branch -d my7 2>output1.err &&\n     ++\ttest_must_fail git branch -d my7 2>output2.err &&\n     ++\ttest_i18ngrep \"hint: The branch you are trying to delete is already checked out\" output1.err &&\n     ++\ttest_i18ngrep \"hint: The branch you are trying to delete is checked out on another worktree\" output2.err &&\n       \trm -r my7 &&\n       \tgit worktree prune\n       '\n\n-- \ngitgitgadget\n"},{"id":"389320","messageId":"19a7cc1889d6094e4f8a94c19c43ad554662e8d8.1578370226.git.gitgitgadget@gmail.com","threadId":"52548","inReplyTo":"pull.507.v2.git.1578370226.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T04:10:26Z","receivedAt":"2020-01-07T04:10:32Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"From: Heba Waly <heba.waly@gmail.com>\n\nDisplay a hint to the user when attempting to delete a checked out\nbranch.\n\nCurrently the user gets an error message saying: \"error: Cannot delete\nbranch <branch> checked out at <path>\". The hint will be displayed\nafter the error message.\n\nSigned-off-by: Heba Waly <heba.waly@gmail.com>\n---\n advice.c          |  4 +++-\n advice.h          |  1 +\n builtin/branch.c  | 14 ++++++++++++++\n t/t3200-branch.sh |  6 ++++--\n 4 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 249c60dcf3..0a8fd2f68e 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;\n int advice_checkout_ambiguous_remote_branch_name = 1;\n int advice_nested_tag = 1;\n int advice_submodule_alternate_error_strategy_die = 1;\n+int advice_delete_checkedout_branch = 1;\n \n static int advice_use_color = -1;\n static char advice_colors[][COLOR_MAXLEN] = {\n@@ -91,7 +92,8 @@ static struct {\n \t{ \"checkoutAmbiguousRemoteBranchName\", &advice_checkout_ambiguous_remote_branch_name },\n \t{ \"nestedTag\", &advice_nested_tag },\n \t{ \"submoduleAlternateErrorStrategyDie\", &advice_submodule_alternate_error_strategy_die },\n-\n+\t{ \"deleteCheckedoutBranch\", &advice_delete_checkedout_branch },\n+\t\n \t/* make this an alias for backward compatibility */\n \t{ \"pushNonFastForward\", &advice_push_update_rejected }\n };\ndiff --git a/advice.h b/advice.h\nindex b706780614..e75c5ee33c 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;\n extern int advice_checkout_ambiguous_remote_branch_name;\n extern int advice_nested_tag;\n extern int advice_submodule_alternate_error_strategy_die;\n+extern int advice_delete_checkedout_branch;\n \n int git_default_advice_config(const char *var, const char *value);\n __attribute__((format (printf, 1, 2)))\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..d1a9443e36 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \t\t\t\terror(_(\"Cannot delete branch '%s' \"\n \t\t\t\t\t\"checked out at '%s'\"),\n \t\t\t\t      bname.buf, wt->path);\n+\t\t\t\tif (advice_delete_checkedout_branch) {\n+\t\t\t\t\tif (wt->is_current) {\n+\t\t\t\t\t\tadvise(_(\"The branch you are trying to delete is already \"\n+\t\t\t\t\t\t\t\"checked out, run the following command to \"\n+\t\t\t\t\t\t\t\"checkout a different branch then try again:\\n\"\n+\t\t\t\t\t\t\t\"git switch <branch>\"));\n+\t\t\t\t\t}\n+\t\t\t\t\telse {\n+\t\t\t\t\t\tadvise(_(\"The branch you are trying to delete is checked \"\n+\t\t\t\t\t\t\t\"out on another worktree, run the following command \"\n+\t\t\t\t\t\t\t\"to checkout a different branch then try again:\\n\"\n+\t\t\t\t\t\t\t\"git -C %s switch <branch>\"), wt->path);\n+\t\t\t\t\t}\n+\t\t\t\t}\n \t\t\t\tret = 1;\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 411a70b0ce..edb01bee45 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' '\n \n test_expect_success 'deleting currently checked out branch fails' '\n \tgit worktree add -b my7 my7 &&\n-\ttest_must_fail git -C my7 branch -d my7 &&\n-\ttest_must_fail git branch -d my7 &&\n+\ttest_must_fail git -C my7 branch -d my7 2>output1.err &&\n+\ttest_must_fail git branch -d my7 2>output2.err &&\n+\ttest_i18ngrep \"hint: The branch you are trying to delete is already checked out\" output1.err &&\n+\ttest_i18ngrep \"hint: The branch you are trying to delete is checked out on another worktree\" output2.err &&\n \trm -r my7 &&\n \tgit worktree prune\n '\n-- \ngitgitgadget\n"},{"id":"389352","messageId":"CAPig+cQ0qY8KDZrQ8khuz34DqPimorN7JHHn0Ms=KpvJYtxJoA@mail.gmail.com","threadId":"52548","inReplyTo":"19a7cc1889d6094e4f8a94c19c43ad554662e8d8.1578370226.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-07T11:16:17Z","receivedAt":"2020-01-07T11:16:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 6, 2020 at 11:10 PM Heba Waly via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> branch: advise the user to checkout a different branch before deleting\n>\n> Display a hint to the user when attempting to delete a checked out\n> branch.\n>\n> Currently the user gets an error message saying: \"error: Cannot delete\n> branch <branch> checked out at <path>\". The hint will be displayed\n> after the error message.\n\nA couple comments...\n\nThe second paragraph doesn't say anything beyond what the patch/code\nitself already says clearly (plus, there's no need to state the\nobvious), so the paragraph adds no value (yet eats up reviewer time).\nTherefore, it can be dropped.\n\nTo convince readers that the change made by the patch is indeed\nwarranted, it's always important to explain _why_ this change is being\nmade.\n\nBoth points can be addressed with a short and sweet commit message,\nperhaps like this:\n\n    branch: advise how to delete checked-out branch\n\n    Teach newcomers how to deal with Git refusing to delete a\n    checked-out branch (whether in the current worktree or some\n    other).\n\nBy the way, did you actually run across a real-world case in which\nsomeone was confused about how to resolve this situation? I ask\nbecause this almost seems like too much hand-holding, and it would be\nnice to avoid polluting Git with unnecessary advice.\n\n> Signed-off-by: Heba Waly <heba.waly@gmail.com>\n> ---\n> diff --git a/advice.c b/advice.c\n> @@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;\n> @@ -91,7 +92,8 @@ static struct {\n>         { \"submoduleAlternateErrorStrategyDie\", &advice_submodule_alternate_error_strategy_die },\n> -\n> +       { \"deleteCheckedoutBranch\", &advice_delete_checkedout_branch },\n> +\n\nWhen you see an odd-looking diff like this in which you wouldn't\nexpect any diff markers on the blank line (that is, the blank line got\ndeleted and re-added), it's a good indication that there's unwanted\ntrailing whitespace on one of the lines. In this case, you (or more\nlikely your editor automatically) added trailing whitespace to the\nblank line which doesn't belong there. Unwanted whitespace changes\nlike this make the patch noisier and more difficult for a reviewer to\nread.\n\n> diff --git a/advice.h b/advice.h\n> @@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;\n> +extern int advice_delete_checkedout_branch;\n\nIs there precedent elsewhere for spelling this \"checkedout\" rather\nthan the more natural \"checked_out\"?\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> @@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> +                               if (advice_delete_checkedout_branch) {\n> +                                       if (wt->is_current) {\n> +                                               advise(_(\"The branch you are trying to delete is already \"\n> +                                                       \"checked out, run the following command to \"\n> +                                                       \"checkout a different branch then try again:\\n\"\n> +                                                       \"git switch <branch>\"));\n\nThis advice unnecessarily repeats what the error message just above it\nalready says about the branch being checked out (thus adds no value),\nand then jumps directly into showing the user an opaque command to\nresolve the situation without explaining _how_ or _why_ the command is\nsupposed to help.\n\nAdvice messages elsewhere typically indent the example command to make\nit stand out from the explanatory prose (and often separated it from\nthe text by a blank line).\n\nA rewrite which addresses both these issues might be something like:\n\n    Switch to a different branch before trying to delete it. For\n    example:\n\n        git switch <different-branch>\n        git branch -%c <this-branch>\n\n(and fill in %c with either \"-d\" or \"-D\" depending upon the value of 'force')\n\n> +                                       }\n> +                                       else {\n> +                                               advise(_(\"The branch you are trying to delete is checked \"\n> +                                                       \"out on another worktree, run the following command \"\n> +                                                       \"to checkout a different branch then try again:\\n\"\n> +                                                       \"git -C %s switch <branch>\"), wt->path);\n\nI like the use of -C here because it makes the command self-contained,\nhowever, I also worry because wt->path is an absolute path, thus\nlikely to be quite lengthy, which means that the important part of the\nexample command (the \"switch <branch>\") can get pushed quite far away,\nthus is more easily overlooked by the reader. I wonder if it would\nmake more sense to show the 'cd' command explicitly, although doing so\nties the example to a particular shell, which may be a downside.\n\n    cd %s\n    git switch <different-branch>\n    cd -\n    git branch -%c <this-branch>\n\n(It is rather verbose and ugly, though.)\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> @@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' '\n>  test_expect_success 'deleting currently checked out branch fails' '\n>         git worktree add -b my7 my7 &&\n> -       test_must_fail git -C my7 branch -d my7 &&\n> -       test_must_fail git branch -d my7 &&\n> +       test_must_fail git -C my7 branch -d my7 2>output1.err &&\n> +       test_must_fail git branch -d my7 2>output2.err &&\n> +       test_i18ngrep \"hint: The branch you are trying to delete is already checked out\" output1.err &&\n> +       test_i18ngrep \"hint: The branch you are trying to delete is checked out on another worktree\" output2.err &&\n\nNit: Separating the 'grep' from the command which generated the error\noutput makes it harder for a reader to see at a glance what is being\ntested and to reason about it since it demands that the reader keep\ntwo distinct cases in mind rather than merely focusing on one at a\ntime. Also, doing it this way forces you to invent distinct filenames\n(by appending a number, for instance), which further leads the reader\nto wonder if there is some significance (later in the test) to keeping\nthese outputs in separate files. So, a better organization (with more\nnatural filenames) would be:\n\n    test_must_fail git -C my7 branch -d my7 2>output.err &&\n    test_i18ngrep \"hint: ...\" output.err &&\n    test_must_fail git branch -d my7 2>output.err &&\n    test_i18ngrep \"hint: ...\" output.err &&\n"},{"id":"389392","messageId":"xmqqh8176xab.fsf@gitster-ct.c.googlers.com","threadId":"52548","inReplyTo":"CAPig+cQ0qY8KDZrQ8khuz34DqPimorN7JHHn0Ms=KpvJYtxJoA@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T16:34:04Z","receivedAt":"2020-01-07T16:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n[jc: skipped all the good suggestions I agree with]\n\n>> +                                       }\n>> +                                       else {\n>> +                                               advise(_(\"The branch you are trying to delete is checked \"\n>> +                                                       \"out on another worktree, run the following command \"\n>> +                                                       \"to checkout a different branch then try again:\\n\"\n>> +                                                       \"git -C %s switch <branch>\"), wt->path);\n>\n> I like the use of -C here because it makes the command self-contained,\n> however, I also worry because wt->path is an absolute path, thus\n> likely to be quite lengthy, which means that the important part of the\n> example command (the \"switch <branch>\") can get pushed quite far away,\n> thus is more easily overlooked by the reader. I wonder if it would\n> make more sense to show the 'cd' command explicitly, although doing so\n> ties the example to a particular shell, which may be a downside.\n>\n>     cd %s\n>     git switch <different-branch>\n>     cd -\n>     git branch -%c <this-branch>\n\nNote that wt->path may have special characters that would need to be\nprotected from the user's shell (worse, the quoting convention may\nbe different depending on which shell is in use).  That is one of\nthe reasons why I would suggest to stay away from giving an advice\nthat pretends to be cut-and-paste-able without being so.  In this\ncase, <different-branch> and <this-branch> must be filled by the\nuser anyway, and the only thing worth cutting-and-pasting is the\npath to the other worktree, not the \"git -C\" or \"cd\" that users\nshould be able to come up with.\n\n\t\"The branch is checked out on another worktree at\\n\"\n\t\"path '%s'\\n\"\n\t\"and cannot be deleted.  Go there, check out some other\\n\"\n\t\"branch and try again.\"\n\nor something like that, perhaps?  \n\n> (It is rather verbose and ugly, though.)\n\nI tend to agree.  It also feels to me that it is giving too much\nhand-holding, but after all advise() may turning out to be about\ngiving that.\n\n"},{"id":"389441","messageId":"CACg5j26jyWnAtM+mZ-FuN7OQWHpKk5nADG+7J-=metJMdO6+2Q@mail.gmail.com","threadId":"52548","inReplyTo":"CAPig+cQ0qY8KDZrQ8khuz34DqPimorN7JHHn0Ms=KpvJYtxJoA@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2020-01-08T01:14:57Z","receivedAt":"2020-01-08T01:15:13Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Mon, Jan 6, 2020 at 11:10 PM Heba Waly via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > branch: advise the user to checkout a different branch before deleting\n> >\n> > Display a hint to the user when attempting to delete a checked out\n> > branch.\n> >\n> > Currently the user gets an error message saying: \"error: Cannot delete\n> > branch <branch> checked out at <path>\". The hint will be displayed\n> > after the error message.\n>\n> A couple comments...\n>\n> The second paragraph doesn't say anything beyond what the patch/code\n> itself already says clearly (plus, there's no need to state the\n> obvious), so the paragraph adds no value (yet eats up reviewer time).\n> Therefore, it can be dropped.\n>\n> To convince readers that the change made by the patch is indeed\n> warranted, it's always important to explain _why_ this change is being\n> made.\n>\n> Both points can be addressed with a short and sweet commit message,\n> perhaps like this:\n>\n>     branch: advise how to delete checked-out branch\n>\n>     Teach newcomers how to deal with Git refusing to delete a\n>     checked-out branch (whether in the current worktree or some\n>     other).\n>\n\nAgree, thanks.\n\n> By the way, did you actually run across a real-world case in which\n> someone was confused about how to resolve this situation? I ask\n> because this almost seems like too much hand-holding, and it would be\n> nice to avoid polluting Git with unnecessary advice.\n>\n\nNo I didn't. I was trying to find scenarios where git can give more\nuser-friendly messages to its users.\nI see your point though, so I don't mind not proceeding with this\npatch if the community doesn't think it's adding any value.\n\n> > Signed-off-by: Heba Waly <heba.waly@gmail.com>\n> > ---\n> > diff --git a/advice.c b/advice.c\n> > @@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1;\n> > @@ -91,7 +92,8 @@ static struct {\n> >         { \"submoduleAlternateErrorStrategyDie\", &advice_submodule_alternate_error_strategy_die },\n> > -\n> > +       { \"deleteCheckedoutBranch\", &advice_delete_checkedout_branch },\n> > +\n>\n> When you see an odd-looking diff like this in which you wouldn't\n> expect any diff markers on the blank line (that is, the blank line got\n> deleted and re-added), it's a good indication that there's unwanted\n> trailing whitespace on one of the lines. In this case, you (or more\n> likely your editor automatically) added trailing whitespace to the\n> blank line which doesn't belong there. Unwanted whitespace changes\n> like this make the patch noisier and more difficult for a reviewer to\n> read.\n>\n\nMystery solved! thanks.\n\n> > diff --git a/advice.h b/advice.h\n> > @@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated;\n> > +extern int advice_delete_checkedout_branch;\n>\n> Is there precedent elsewhere for spelling this \"checkedout\" rather\n> than the more natural \"checked_out\"?\n>\n\nNot really.\n\n> > diff --git a/builtin/branch.c b/builtin/branch.c\n> > @@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n> > +                               if (advice_delete_checkedout_branch) {\n> > +                                       if (wt->is_current) {\n> > +                                               advise(_(\"The branch you are trying to delete is already \"\n> > +                                                       \"checked out, run the following command to \"\n> > +                                                       \"checkout a different branch then try again:\\n\"\n> > +                                                       \"git switch <branch>\"));\n>\n> This advice unnecessarily repeats what the error message just above it\n> already says about the branch being checked out (thus adds no value),\n> and then jumps directly into showing the user an opaque command to\n> resolve the situation without explaining _how_ or _why_ the command is\n> supposed to help.\n>\n> Advice messages elsewhere typically indent the example command to make\n> it stand out from the explanatory prose (and often separated it from\n> the text by a blank line).\n>\n> A rewrite which addresses both these issues might be something like:\n>\n>     Switch to a different branch before trying to delete it. For\n>     example:\n>\n>         git switch <different-branch>\n>         git branch -%c <this-branch>\n>\n> (and fill in %c with either \"-d\" or \"-D\" depending upon the value of 'force')\n\nOk.\n\n> > +                                       }\n> > +                                       else {\n> > +                                               advise(_(\"The branch you are trying to delete is checked \"\n> > +                                                       \"out on another worktree, run the following command \"\n> > +                                                       \"to checkout a different branch then try again:\\n\"\n> > +                                                       \"git -C %s switch <branch>\"), wt->path);\n>\n> I like the use of -C here because it makes the command self-contained,\n> however, I also worry because wt->path is an absolute path, thus\n> likely to be quite lengthy, which means that the important part of the\n> example command (the \"switch <branch>\") can get pushed quite far away,\n> thus is more easily overlooked by the reader. I wonder if it would\n> make more sense to show the 'cd' command explicitly, although doing so\n> ties the example to a particular shell, which may be a downside.\n>\n>     cd %s\n>     git switch <different-branch>\n>     cd -\n>     git branch -%c <this-branch>\n>\n> (It is rather verbose and ugly, though.)\n>\n> > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> > @@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' '\n> >  test_expect_success 'deleting currently checked out branch fails' '\n> >         git worktree add -b my7 my7 &&\n> > -       test_must_fail git -C my7 branch -d my7 &&\n> > -       test_must_fail git branch -d my7 &&\n> > +       test_must_fail git -C my7 branch -d my7 2>output1.err &&\n> > +       test_must_fail git branch -d my7 2>output2.err &&\n> > +       test_i18ngrep \"hint: The branch you are trying to delete is already checked out\" output1.err &&\n> > +       test_i18ngrep \"hint: The branch you are trying to delete is checked out on another worktree\" output2.err &&\n>\n> Nit: Separating the 'grep' from the command which generated the error\n> output makes it harder for a reader to see at a glance what is being\n> tested and to reason about it since it demands that the reader keep\n> two distinct cases in mind rather than merely focusing on one at a\n> time. Also, doing it this way forces you to invent distinct filenames\n> (by appending a number, for instance), which further leads the reader\n> to wonder if there is some significance (later in the test) to keeping\n> these outputs in separate files. So, a better organization (with more\n> natural filenames) would be:\n>\n>     test_must_fail git -C my7 branch -d my7 2>output.err &&\n>     test_i18ngrep \"hint: ...\" output.err &&\n>     test_must_fail git branch -d my7 2>output.err &&\n>     test_i18ngrep \"hint: ...\" output.err &&\n\nAgree.\n\nThanks Eric, I'll not update this patch unless somebody thinks it's\nadding a value and worth spending more time on it.\n\nHeba\n"},{"id":"389442","messageId":"20200108014442.GC181522@google.com","threadId":"52548","inReplyTo":"xmqqh8176xab.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-01-08T01:44:42Z","receivedAt":"2020-01-08T01:44:49Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote:\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> [jc: skipped all the good suggestions I agree with]\n> \n> >> +                                       }\n> >> +                                       else {\n> >> +                                               advise(_(\"The branch you are trying to delete is checked \"\n> >> +                                                       \"out on another worktree, run the following command \"\n> >> +                                                       \"to checkout a different branch then try again:\\n\"\n> >> +                                                       \"git -C %s switch <branch>\"), wt->path);\n> >\n> > I like the use of -C here because it makes the command self-contained,\n> > however, I also worry because wt->path is an absolute path, thus\n> > likely to be quite lengthy, which means that the important part of the\n> > example command (the \"switch <branch>\") can get pushed quite far away,\n> > thus is more easily overlooked by the reader. I wonder if it would\n> > make more sense to show the 'cd' command explicitly, although doing so\n> > ties the example to a particular shell, which may be a downside.\n> >\n> >     cd %s\n> >     git switch <different-branch>\n> >     cd -\n> >     git branch -%c <this-branch>\n> \n> Note that wt->path may have special characters that would need to be\n> protected from the user's shell (worse, the quoting convention may\n> be different depending on which shell is in use).  That is one of\n> the reasons why I would suggest to stay away from giving an advice\n> that pretends to be cut-and-paste-able without being so.\n\nHm, I think you've sold me on the error of my ways trying to push for\ncopy-pasteable advices :)\nBut I wonder, how much is too much? I mean that suggesting a single Git\ncommand which takes a branch name and a pathspec is safer than\nsuggesting some complicated -C=foo or cd bar thing, right?\n\n> In this\n> case, <different-branch> and <this-branch> must be filled by the\n> user anyway, and the only thing worth cutting-and-pasting is the\n> path to the other worktree, not the \"git -C\" or \"cd\" that users\n> should be able to come up with.\n> \n> \t\"The branch is checked out on another worktree at\\n\"\n> \t\"path '%s'\\n\"\n> \t\"and cannot be deleted.  Go there, check out some other\\n\"\n> \t\"branch and try again.\"\n> \n> or something like that, perhaps?  \n> \n> > (It is rather verbose and ugly, though.)\n> \n> I tend to agree.  It also feels to me that it is giving too much\n> hand-holding, but after all advise() may turning out to be about\n> giving that.\n\nWell, if advise() isn't going to hold their hand, who is? ;)\n\nWhat I mean is, I think that's indeed what advise() is about, and the\nreason it can be disabled in config. To me, the harm of giving too much\nhand-holding seems less than the harm of giving not enough; to deal with\nthe one requires skimming past things you already know, and to deal with\nthe other requires web searching, asking people, reading documentation,\nperhaps gaining \"tips\" from questionable blogs which don't actually give\nvery useful advice. I think we were just discussing not long ago the\ngeneral quality of advice on StackOverflow, for example.\n\n - Emily\n"},{"id":"389459","messageId":"CAPig+cTDayF0hHn7wSPGNS8h2qPUYhhg9Z8fY_rLQnWmAg-NKQ@mail.gmail.com","threadId":"52548","inReplyTo":"CACg5j26jyWnAtM+mZ-FuN7OQWHpKk5nADG+7J-=metJMdO6+2Q@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-08T09:27:52Z","receivedAt":"2020-01-08T09:28:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 7, 2020 at 8:15 PM Heba Waly <heba.waly@gmail.com> wrote:\n> On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > By the way, did you actually run across a real-world case in which\n> > someone was confused about how to resolve this situation? I ask\n> > because this almost seems like too much hand-holding, and it would be\n> > nice to avoid polluting Git with unnecessary advice.\n>\n> No I didn't. I was trying to find scenarios where git can give more\n> user-friendly messages to its users.\n> I see your point though, so I don't mind not proceeding with this\n> patch if the community doesn't think it's adding any value.\n\nMy own feeling is that this level of hand-holding is unnecessary, at\nleast until we a discover a good number of real-world cases in which\npeople are baffled by how to deal with this situation. Adding the\nadvice seems simple on the surface, but every new piece of advice\nmeans having to add yet another configuration variable, writing more\ncode, more tests, and more documentation, and it needs to be\nmaintained for the life of the project. So what seems simple at first\nglance, can end up being costly in terms of developer resources. For a\nbit of advice which doesn't seem to be needed by anyone (yet), all\nthat effort seem unwarranted. Thus, my preference is to see the patch\ndropped.\n"},{"id":"389462","messageId":"CAPig+cRt1fzNzbustfq2seQMjZmd79cGW+-oxsdzFXw8D-q26A@mail.gmail.com","threadId":"52548","inReplyTo":"20200108014442.GC181522@google.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-08T10:22:10Z","receivedAt":"2020-01-08T10:22:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 7, 2020 at 8:44 PM Emily Shaffer <emilyshaffer@google.com> wrote:\n> On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote:\n> > Eric Sunshine <sunshine@sunshineco.com> writes:\n> > > (It is rather verbose and ugly, though.)\n> >\n> > I tend to agree. It also feels to me that it is giving too much\n> > hand-holding, but after all advise() may turning out to be about\n> > giving that.\n>\n> Well, if advise() isn't going to hold their hand, who is? ;)\n> What I mean is, I think that's indeed what advise() is about, and the\n> reason it can be disabled in config.\n\nGit is already drowning in configuration options. Every new option\nincreases the complexity level for the user and of Git overall,\nrequires additional code, additional tests, and additional\ndocumentation, and must be maintained for the life of the project. So,\nevery configuration option carries a cost in terms of end-user\nexperience and developer resources.\n\nIf anything, we should be trying to keep the number of configuration\noptions constant (or, even better, reduce it), rather than forever\nadding more. Thus, we should think very hard before adding any new\nconfiguration option, trying instead to discover ways in which an\nissue can be addressed without adding a configuration option (with its\nattendant costs and complexity), and only add an option as a last\nresort.\n\n> To me, the harm of giving too much hand-holding seems less than the\n> harm of giving not enough; to deal with the one requires skimming\n> past things you already know [...]\n\nPerhaps I'm too old-school and too steeped in The Unix Way, but there\nis value in silence and conciseness, and that value outweighs\nchattiness, especially when chattiness is effectively unnecessary\n(which is the case in the vast majority of error/warning messages\nemitted by Git).\n\nToo much hand-holding quickly becomes noise which gets ignored, thus\neventually drowns out the really important information. People need to\nunderstand a problem before taking action to solve it. Blindly\nfollowing some piece of advice without understanding a problem can\nlead to further problems (especially in those cases when it's not\npossible to devise advice which suitably covers all situations in\nwhich a particular problem may arise).\n\nA well-written, clear, _concise_, error message can often provide\nenough information to allow the user to understand and resolve the\nproblem without additional hand-holding. It's true that there are some\ncomplex situations (for instance, detached HEAD) for which additional\nhints can be handy for newcomers or casual users, but that should be\nthe exception, not the norm. Rather than adding advice messages for\nevery possible problem, perhaps a better use of our time would be to\nweed out poorly-worded and unhelpful error messages and improve them.\n"},{"id":"389491","messageId":"CACg5j260h88bd=W_4EzAn7B0TiU02Y8BzKDQ7w3UJiHkhL60NQ@mail.gmail.com","threadId":"52548","inReplyTo":"CAPig+cTDayF0hHn7wSPGNS8h2qPUYhhg9Z8fY_rLQnWmAg-NKQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2020-01-08T18:06:26Z","receivedAt":"2020-01-08T18:06:40Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> advice seems simple on the surface, but every new piece of advice\n> means having to add yet another configuration variable, writing more\n> code, more tests, and more documentation\n\nThis raises a question though, do we really need a new configuration\nfor every new advice?\nSo a user who's not interested in receiving advice will have to\ndisable every single advice config? It doesn't seem scalable to me.\nI imagine a user will either want to enable or disable the advice\nfeature all together. Why don't we have only one `enable_advice`\nconfiguration that controls all the advice messages?\n\nThanks,\nHeba\n"},{"id":"389496","messageId":"nycvar.QRO.7.76.6.2001081945490.46@tvgsbejvaqbjf.bet","threadId":"52548","inReplyTo":"CACg5j260h88bd=W_4EzAn7B0TiU02Y8BzKDQ7w3UJiHkhL60NQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-08T19:01:12Z","receivedAt":"2020-01-08T19:01:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 9 Jan 2020, Heba Waly wrote:\n\n> On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> > advice seems simple on the surface, but every new piece of advice\n> > means having to add yet another configuration variable, writing more\n> > code, more tests, and more documentation\n\nFWIW I disagree that we need to reduce the number of config settings.\nPretty much all of them have a good reason to exist.\n\nI _could_ however see some sort of categorisation as a valuable goal,\nwhich would potentially make it easier to have chapters in the `git\nconfig` documentation where earlier chapters describe common settings\nand the later chapters describe subsequently more obscure settings.\n\n> This raises a question though, do we really need a new configuration for\n> every new advice?\n\nI would keep it this way, if only for consistency (a department in which\nGit still has a lot of room for improvement).\n\n> So a user who's not interested in receiving advice will have to\n> disable every single advice config? It doesn't seem scalable to me.\n> I imagine a user will either want to enable or disable the advice\n> feature all together. Why don't we have only one `enable_advice`\n> configuration that controls all the advice messages?\n\nThis is the first time I hear about anybody wanting to disable any advice\n;-)\n\nIf this is desired, it should be easy enough:\n\n-- snip --\ndiff --git a/advice.c b/advice.c\nindex 3ee0ee2d8fb..28e48d5410b 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -138,6 +138,13 @@ int git_default_advice_config(const char *var, const char *value)\n \tif (!skip_prefix(var, \"advice.\", &k))\n \t\treturn 0;\n\n+\tif (!strcmp(k, \"suppressall\")) {\n+\t\tif (git_config_bool(var, value))\n+\t\t\tfor (i = 0; i < ARRAY_SIZE(advice_config); i++)\n+\t\t\t\t*advice_config[i].preference = 0;\n+\t\treturn 0;\n+\t}\n+\n \tfor (i = 0; i < ARRAY_SIZE(advice_config); i++) {\n \t\tif (strcasecmp(k, advice_config[i].name))\n \t\t\tcontinue;\n-- snap --\n\nI don't really think that this is desired, though. Git has earned a\nreputation for being hard to use, so I was personally delighted when we\nstarted introducing the advise feature, and I have actually heard a couple\nusers say good things whenever Git learns to help them without having to\nask another human being (and feeling dumb as a consequence).\n\nCiao,\nDscho\n"},{"id":"389497","messageId":"xmqqwoa122h1.fsf@gitster-ct.c.googlers.com","threadId":"52548","inReplyTo":"CACg5j260h88bd=W_4EzAn7B0TiU02Y8BzKDQ7w3UJiHkhL60NQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-08T19:05:30Z","receivedAt":"2020-01-08T19:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heba Waly <heba.waly@gmail.com> writes:\n\n> On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>\n>> advice seems simple on the surface, but every new piece of advice\n>> means having to add yet another configuration variable, writing more\n>> code, more tests, and more documentation\n>\n> This raises a question though, do we really need a new configuration\n> for every new advice?\n> So a user who's not interested in receiving advice will have to\n> disable every single advice config? It doesn't seem scalable to me.\n> I imagine a user will either want to enable or disable the advice\n> feature all together. Why don't we have only one `enable_advice`\n> configuration that controls all the advice messages?\n\nThe advice mechanism was a way to help new people learn the system\nby giving a bit of extra help messages that would become annoying\nonce they learned that part of the system, so by default they are\non, and can be turned off once they learn enough about the specific\nsituation that gives one kind of advise.  Hence, \"[advice] !all\" to\ndecline any and all advice message, including anything that would be\nintroduced in the future, is somewhat a foreign concept in that\npicture.\n\nHaving said that, I am not opposed to add support for such an\noverall \"turn all off\" (or on for that matter).  Totally untested,\nbut something along this line, perhaps?  The idea is that\n\n - the config keys may come in any order;\n\n - once advice.all is set to either true or false, we set all the\n   advice.* variables to the given value,\n\n - for any other advice.* config, we interpret it only if we haven't\n   seen advice.all\n\n\n\n advice.c | 19 +++++++++++++++----\n 1 file changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex 098ac0abea..b9a8fe1360 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -3,6 +3,8 @@\n #include \"color.h\"\n #include \"help.h\"\n \n+static int advice_all_seen = -1; /* not seen yet */\n+\n int advice_fetch_show_forced_updates = 1;\n int advice_push_update_rejected = 1;\n int advice_push_non_ff_current = 1;\n@@ -142,13 +144,22 @@ int git_default_advice_config(const char *var, const char *value)\n \tif (!skip_prefix(var, \"advice.\", &k))\n \t\treturn 0;\n \n-\tfor (i = 0; i < ARRAY_SIZE(advice_config); i++) {\n-\t\tif (strcasecmp(k, advice_config[i].name))\n-\t\t\tcontinue;\n-\t\t*advice_config[i].preference = git_config_bool(var, value);\n+\tif (!strcmp(var, \"advise.all\")) {\n+\t\tadvice_all_seen = git_config_bool(var, value);\n+\t\tfor (i = 0; i < ARRAY_SIZE(advice_config); i++)\n+\t\t\t*advice_config[i].preference = advice_all_seen;\n \t\treturn 0;\n \t}\n \n+\tif (advice_all_seen < 0) {\n+\t\tfor (i = 0; i < ARRAY_SIZE(advice_config); i++) {\n+\t\t\tif (strcasecmp(k, advice_config[i].name))\n+\t\t\t\tcontinue;\n+\t\t\t*advice_config[i].preference = git_config_bool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n \treturn 0;\n }\n \n"},{"id":"389499","messageId":"xmqqsgkp220d.fsf@gitster-ct.c.googlers.com","threadId":"52548","inReplyTo":"nycvar.QRO.7.76.6.2001081945490.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-08T19:15:30Z","receivedAt":"2020-01-08T19:15:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> This is the first time I hear about anybody wanting to disable any advice\n> ...\n> I don't really think that this is desired, though.\n\nMe neither.  We seem to have come up with more-or-less the same\nillustration, but such a global \"turn all off\" needs to be explained\nvery well before we let users blindly use it, I think.\n"},{"id":"389577","messageId":"CACg5j27Cj75W=95-4T8bv351xD8K5SiyA6-9JvS-n2NmR-0z+g@mail.gmail.com","threadId":"52548","inReplyTo":"CAPig+cTDayF0hHn7wSPGNS8h2qPUYhhg9Z8fY_rLQnWmAg-NKQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2020-01-10T12:09:19Z","receivedAt":"2020-01-10T12:09:34Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> My own feeling is that this level of hand-holding is unnecessary, at\n> least until we a discover a good number of real-world cases in which\n> people are baffled by how to deal with this situation. Adding the\n> advice seems simple on the surface, but every new piece of advice\n> means having to add yet another configuration variable, writing more\n> code, more tests, and more documentation, and it needs to be\n> maintained for the life of the project. So what seems simple at first\n> glance, can end up being costly in terms of developer resources. For a\n> bit of advice which doesn't seem to be needed by anyone (yet), all\n> that effort seem unwarranted. Thus, my preference is to see the patch\n> dropped.\n\nThat's Ok. We can drop it.\n\nThanks,\nHeba\n"},{"id":"389578","messageId":"CACg5j271GjTqt-VBrtxsdssj7X5hJOV8f2xgufgV05Zn8eXsGw@mail.gmail.com","threadId":"52548","inReplyTo":"xmqqsgkp220d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting","fromName":"Heba Waly","fromEmail":"heba.waly@gmail.com","sentAt":"2020-01-10T12:11:57Z","receivedAt":"2020-01-10T12:12:10Z","isPatch":true,"sender":{"key":"heba.waly@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1539076?v=4"},"body":"On Thu, Jan 9, 2020 at 8:15 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n> > This is the first time I hear about anybody wanting to disable any advice\n> > ...\n> > I don't really think that this is desired, though.\n>\n> Me neither.  We seem to have come up with more-or-less the same\n> illustration, but such a global \"turn all off\" needs to be explained\n> very well before we let users blindly use it, I think.\n\nThank you Dscho and Junio.\n"}]}