* [PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting @ 2020-01-02 2:49 Heba Waly via GitGitGadget 2020-01-02 2:49 ` [PATCH 1/1] " Heba Waly via GitGitGadget 2020-01-07 4:10 ` [PATCH v2 0/1] [Outreachy] [RFC] " Heba Waly via GitGitGadget 0 siblings, 2 replies; 18+ messages in thread From: Heba Waly via GitGitGadget @ 2020-01-02 2:49 UTC (permalink / raw) To: git; +Cc: Heba Waly, Junio C Hamano When a user attempts to delete a checked out branch, an error message is displayed saying: "error: Cannot delete branch checked out at ". This patch suggests displaying a hint after the error message advising the user to checkout another branch first using "git checkout ". Heba Waly (1): branch: advise the user to checkout a different branch before deleting builtin/branch.c | 2 ++ t/t3200-branch.sh | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/507 -- gitgitgadget ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-02 2:49 [PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting Heba Waly via GitGitGadget @ 2020-01-02 2:49 ` Heba Waly via GitGitGadget 2020-01-02 8:18 ` Eric Sunshine 2020-01-07 4:10 ` [PATCH v2 0/1] [Outreachy] [RFC] " Heba Waly via GitGitGadget 1 sibling, 1 reply; 18+ messages in thread From: Heba Waly via GitGitGadget @ 2020-01-02 2:49 UTC (permalink / raw) To: git; +Cc: Heba Waly, Junio C Hamano, Heba Waly From: Heba Waly <heba.waly@gmail.com> Display a hint to the user when attempting to delete a checked out branch saying "Checkout another branch before deleting this one: git checkout <branch_name>". Currently the user gets an error message saying: "error: Cannot delete branch <branch_name> checked out at <path>". The hint will be displayed after the error message. Signed-off-by: Heba Waly <heba.waly@gmail.com> --- builtin/branch.c | 2 ++ t/t3200-branch.sh | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/builtin/branch.c b/builtin/branch.c index d8297f80ff..799e967008 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -240,6 +240,8 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, error(_("Cannot delete branch '%s' " "checked out at '%s'"), bname.buf, wt->path); + advise(_("Checkout another branch before deleting this " + "one: git checkout <branch_name>")); ret = 1; continue; } diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 411a70b0ce..3b2812a8f4 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -808,7 +808,8 @@ test_expect_success 'test deleting branch without config' ' test_expect_success 'deleting currently checked out branch fails' ' git worktree add -b my7 my7 && test_must_fail git -C my7 branch -d my7 && - test_must_fail git branch -d my7 && + test_must_fail git branch -d my7 >actual.out 2>actual.err && + test_i18ngrep "hint: Checkout another branch" actual.err && rm -r my7 && git worktree prune ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-02 2:49 ` [PATCH 1/1] " Heba Waly via GitGitGadget @ 2020-01-02 8:18 ` Eric Sunshine 2020-01-06 0:42 ` Heba Waly 0 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2020-01-02 8:18 UTC (permalink / raw) To: Heba Waly via GitGitGadget; +Cc: Git List, Heba Waly, Junio C Hamano 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 > ' ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-02 8:18 ` Eric Sunshine @ 2020-01-06 0:42 ` Heba Waly 0 siblings, 0 replies; 18+ messages in thread From: Heba Waly @ 2020-01-06 0:42 UTC (permalink / raw) To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting 2020-01-02 2:49 [PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting Heba Waly via GitGitGadget 2020-01-02 2:49 ` [PATCH 1/1] " Heba Waly via GitGitGadget @ 2020-01-07 4:10 ` Heba Waly via GitGitGadget 2020-01-07 4:10 ` [PATCH v2 1/1] " Heba Waly via GitGitGadget 1 sibling, 1 reply; 18+ messages in thread From: Heba Waly via GitGitGadget @ 2020-01-07 4:10 UTC (permalink / raw) To: git; +Cc: Heba Waly, Junio C Hamano When a user attempts to delete a checked out branch, an error message is displayed saying: "error: Cannot delete branch checked out at ". This patch suggests displaying a hint after the error message advising the user to checkout another branch first using "git checkout ". Heba Waly (1): branch: advise the user to checkout a different branch before deleting advice.c | 4 +++- advice.h | 1 + builtin/branch.c | 14 ++++++++++++++ t/t3200-branch.sh | 6 ++++-- 4 files changed, 22 insertions(+), 3 deletions(-) base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-507%2FHebaWaly%2Fdelete_branch_hint-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-507/HebaWaly/delete_branch_hint-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/507 Range-diff vs v1: 1: 82bf24ce53 ! 1: 19a7cc1889 branch: advise the user to checkout a different branch before deleting @@ -3,15 +3,48 @@ branch: advise the user to checkout a different branch before deleting Display a hint to the user when attempting to delete a checked out - branch saying "Checkout another branch before deleting this one: - git checkout <branch_name>". + branch. Currently the user gets an error message saying: "error: Cannot delete - branch <branch_name> checked out at <path>". The hint will be displayed + branch <branch> checked out at <path>". The hint will be displayed after the error message. Signed-off-by: Heba Waly <heba.waly@gmail.com> + diff --git a/advice.c b/advice.c + --- a/advice.c + +++ b/advice.c +@@ + int advice_checkout_ambiguous_remote_branch_name = 1; + int advice_nested_tag = 1; + int advice_submodule_alternate_error_strategy_die = 1; ++int advice_delete_checkedout_branch = 1; + + static int advice_use_color = -1; + static char advice_colors[][COLOR_MAXLEN] = { +@@ + { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name }, + { "nestedTag", &advice_nested_tag }, + { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die }, +- ++ { "deleteCheckedoutBranch", &advice_delete_checkedout_branch }, ++ + /* make this an alias for backward compatibility */ + { "pushNonFastForward", &advice_push_update_rejected } + }; + + diff --git a/advice.h b/advice.h + --- a/advice.h + +++ b/advice.h +@@ + extern int advice_checkout_ambiguous_remote_branch_name; + extern int advice_nested_tag; + extern int advice_submodule_alternate_error_strategy_die; ++extern int advice_delete_checkedout_branch; + + int git_default_advice_config(const char *var, const char *value); + __attribute__((format (printf, 1, 2))) + diff --git a/builtin/branch.c b/builtin/branch.c --- a/builtin/branch.c +++ b/builtin/branch.c @@ -19,8 +52,20 @@ error(_("Cannot delete branch '%s' " "checked out at '%s'"), bname.buf, wt->path); -+ advise(_("Checkout another branch before deleting this " -+ "one: git checkout <branch_name>")); ++ if (advice_delete_checkedout_branch) { ++ if (wt->is_current) { ++ advise(_("The branch you are trying to delete is already " ++ "checked out, run the following command to " ++ "checkout a different branch then try again:\n" ++ "git switch <branch>")); ++ } ++ else { ++ advise(_("The branch you are trying to delete is checked " ++ "out on another worktree, run the following command " ++ "to checkout a different branch then try again:\n" ++ "git -C %s switch <branch>"), wt->path); ++ } ++ } ret = 1; continue; } @@ -29,12 +74,15 @@ --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ + test_expect_success 'deleting currently checked out branch fails' ' git worktree add -b my7 my7 && - test_must_fail git -C my7 branch -d my7 && +- test_must_fail git -C my7 branch -d my7 && - test_must_fail git branch -d my7 && -+ test_must_fail git branch -d my7 >actual.out 2>actual.err && -+ test_i18ngrep "hint: Checkout another branch" actual.err && ++ test_must_fail git -C my7 branch -d my7 2>output1.err && ++ test_must_fail git branch -d my7 2>output2.err && ++ test_i18ngrep "hint: The branch you are trying to delete is already checked out" output1.err && ++ test_i18ngrep "hint: The branch you are trying to delete is checked out on another worktree" output2.err && rm -r my7 && git worktree prune ' -- gitgitgadget ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-07 4:10 ` [PATCH v2 0/1] [Outreachy] [RFC] " Heba Waly via GitGitGadget @ 2020-01-07 4:10 ` Heba Waly via GitGitGadget 2020-01-07 11:16 ` Eric Sunshine 0 siblings, 1 reply; 18+ messages in thread From: Heba Waly via GitGitGadget @ 2020-01-07 4:10 UTC (permalink / raw) To: git; +Cc: Heba Waly, Junio C Hamano, Heba Waly From: Heba Waly <heba.waly@gmail.com> Display a hint to the user when attempting to delete a checked out branch. Currently the user gets an error message saying: "error: Cannot delete branch <branch> checked out at <path>". The hint will be displayed after the error message. Signed-off-by: Heba Waly <heba.waly@gmail.com> --- advice.c | 4 +++- advice.h | 1 + builtin/branch.c | 14 ++++++++++++++ t/t3200-branch.sh | 6 ++++-- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/advice.c b/advice.c index 249c60dcf3..0a8fd2f68e 100644 --- a/advice.c +++ b/advice.c @@ -31,6 +31,7 @@ int advice_graft_file_deprecated = 1; int advice_checkout_ambiguous_remote_branch_name = 1; int advice_nested_tag = 1; int advice_submodule_alternate_error_strategy_die = 1; +int advice_delete_checkedout_branch = 1; static int advice_use_color = -1; static char advice_colors[][COLOR_MAXLEN] = { @@ -91,7 +92,8 @@ static struct { { "checkoutAmbiguousRemoteBranchName", &advice_checkout_ambiguous_remote_branch_name }, { "nestedTag", &advice_nested_tag }, { "submoduleAlternateErrorStrategyDie", &advice_submodule_alternate_error_strategy_die }, - + { "deleteCheckedoutBranch", &advice_delete_checkedout_branch }, + /* make this an alias for backward compatibility */ { "pushNonFastForward", &advice_push_update_rejected } }; diff --git a/advice.h b/advice.h index b706780614..e75c5ee33c 100644 --- a/advice.h +++ b/advice.h @@ -31,6 +31,7 @@ extern int advice_graft_file_deprecated; extern int advice_checkout_ambiguous_remote_branch_name; extern int advice_nested_tag; extern int advice_submodule_alternate_error_strategy_die; +extern int advice_delete_checkedout_branch; int git_default_advice_config(const char *var, const char *value); __attribute__((format (printf, 1, 2))) diff --git a/builtin/branch.c b/builtin/branch.c index d8297f80ff..d1a9443e36 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -240,6 +240,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds, error(_("Cannot delete branch '%s' " "checked out at '%s'"), bname.buf, wt->path); + if (advice_delete_checkedout_branch) { + if (wt->is_current) { + advise(_("The branch you are trying to delete is already " + "checked out, run the following command to " + "checkout a different branch then try again:\n" + "git switch <branch>")); + } + else { + advise(_("The branch you are trying to delete is checked " + "out on another worktree, run the following command " + "to checkout a different branch then try again:\n" + "git -C %s switch <branch>"), wt->path); + } + } ret = 1; continue; } diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 411a70b0ce..edb01bee45 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -807,8 +807,10 @@ test_expect_success 'test deleting branch without config' ' test_expect_success 'deleting currently checked out branch fails' ' git worktree add -b my7 my7 && - test_must_fail git -C my7 branch -d my7 && - test_must_fail git branch -d my7 && + test_must_fail git -C my7 branch -d my7 2>output1.err && + test_must_fail git branch -d my7 2>output2.err && + test_i18ngrep "hint: The branch you are trying to delete is already checked out" output1.err && + test_i18ngrep "hint: The branch you are trying to delete is checked out on another worktree" output2.err && rm -r my7 && git worktree prune ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-07 4:10 ` [PATCH v2 1/1] " Heba Waly via GitGitGadget @ 2020-01-07 11:16 ` Eric Sunshine 2020-01-07 16:34 ` Junio C Hamano 2020-01-08 1:14 ` Heba Waly 0 siblings, 2 replies; 18+ messages in thread From: Eric Sunshine @ 2020-01-07 11:16 UTC (permalink / raw) To: Heba Waly via GitGitGadget; +Cc: Git List, Heba Waly, Junio C Hamano 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 && ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-07 11:16 ` Eric Sunshine @ 2020-01-07 16:34 ` Junio C Hamano 2020-01-08 1:44 ` Emily Shaffer 2020-01-08 1:14 ` Heba Waly 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2020-01-07 16:34 UTC (permalink / raw) To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Heba Waly 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-07 16:34 ` Junio C Hamano @ 2020-01-08 1:44 ` Emily Shaffer 2020-01-08 10:22 ` Eric Sunshine 0 siblings, 1 reply; 18+ messages in thread From: Emily Shaffer @ 2020-01-08 1:44 UTC (permalink / raw) To: Junio C Hamano Cc: Eric Sunshine, Heba Waly via GitGitGadget, Git List, Heba Waly 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 1:44 ` Emily Shaffer @ 2020-01-08 10:22 ` Eric Sunshine 0 siblings, 0 replies; 18+ messages in thread From: Eric Sunshine @ 2020-01-08 10:22 UTC (permalink / raw) To: Emily Shaffer Cc: Junio C Hamano, Heba Waly via GitGitGadget, Git List, Heba Waly On Tue, Jan 7, 2020 at 8:44 PM Emily Shaffer <emilyshaffer@google.com> wrote: > On Tue, Jan 07, 2020 at 08:34:04AM -0800, Junio C Hamano wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > (It is rather verbose and ugly, though.) > > > > I tend to agree. It also feels to me that it is giving too much > > hand-holding, but after all advise() may turning out to be about > > giving that. > > Well, if advise() isn't going to hold their hand, who is? ;) > What I mean is, I think that's indeed what advise() is about, and the > reason it can be disabled in config. Git is already drowning in configuration options. Every new option increases the complexity level for the user and of Git overall, requires additional code, additional tests, and additional documentation, and must be maintained for the life of the project. So, every configuration option carries a cost in terms of end-user experience and developer resources. If anything, we should be trying to keep the number of configuration options constant (or, even better, reduce it), rather than forever adding more. Thus, we should think very hard before adding any new configuration option, trying instead to discover ways in which an issue can be addressed without adding a configuration option (with its attendant costs and complexity), and only add an option as a last resort. > To me, the harm of giving too much hand-holding seems less than the > harm of giving not enough; to deal with the one requires skimming > past things you already know [...] Perhaps I'm too old-school and too steeped in The Unix Way, but there is value in silence and conciseness, and that value outweighs chattiness, especially when chattiness is effectively unnecessary (which is the case in the vast majority of error/warning messages emitted by Git). Too much hand-holding quickly becomes noise which gets ignored, thus eventually drowns out the really important information. People need to understand a problem before taking action to solve it. Blindly following some piece of advice without understanding a problem can lead to further problems (especially in those cases when it's not possible to devise advice which suitably covers all situations in which a particular problem may arise). A well-written, clear, _concise_, error message can often provide enough information to allow the user to understand and resolve the problem without additional hand-holding. It's true that there are some complex situations (for instance, detached HEAD) for which additional hints can be handy for newcomers or casual users, but that should be the exception, not the norm. Rather than adding advice messages for every possible problem, perhaps a better use of our time would be to weed out poorly-worded and unhelpful error messages and improve them. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-07 11:16 ` Eric Sunshine 2020-01-07 16:34 ` Junio C Hamano @ 2020-01-08 1:14 ` Heba Waly 2020-01-08 9:27 ` Eric Sunshine 1 sibling, 1 reply; 18+ messages in thread From: Heba Waly @ 2020-01-08 1:14 UTC (permalink / raw) To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 1:14 ` Heba Waly @ 2020-01-08 9:27 ` Eric Sunshine 2020-01-08 18:06 ` Heba Waly 2020-01-10 12:09 ` Heba Waly 0 siblings, 2 replies; 18+ messages in thread From: Eric Sunshine @ 2020-01-08 9:27 UTC (permalink / raw) To: Heba Waly; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 9:27 ` Eric Sunshine @ 2020-01-08 18:06 ` Heba Waly 2020-01-08 19:01 ` Johannes Schindelin 2020-01-08 19:05 ` Junio C Hamano 2020-01-10 12:09 ` Heba Waly 1 sibling, 2 replies; 18+ messages in thread From: Heba Waly @ 2020-01-08 18:06 UTC (permalink / raw) To: Eric Sunshine Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano, Emily Shaffer On Wed, Jan 8, 2020 at 10:28 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > advice seems simple on the surface, but every new piece of advice > means having to add yet another configuration variable, writing more > code, more tests, and more documentation This raises a question though, do we really need a new configuration for every new advice? So a user who's not interested in receiving advice will have to disable every single advice config? It doesn't seem scalable to me. I imagine a user will either want to enable or disable the advice feature all together. Why don't we have only one `enable_advice` configuration that controls all the advice messages? Thanks, Heba ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 18:06 ` Heba Waly @ 2020-01-08 19:01 ` Johannes Schindelin 2020-01-08 19:15 ` Junio C Hamano 2020-01-08 19:05 ` Junio C Hamano 1 sibling, 1 reply; 18+ messages in thread From: Johannes Schindelin @ 2020-01-08 19:01 UTC (permalink / raw) To: Heba Waly Cc: Eric Sunshine, Heba Waly via GitGitGadget, Git List, Junio C Hamano, Emily Shaffer 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 19:01 ` Johannes Schindelin @ 2020-01-08 19:15 ` Junio C Hamano 2020-01-10 12:11 ` Heba Waly 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2020-01-08 19:15 UTC (permalink / raw) To: Johannes Schindelin Cc: Heba Waly, Eric Sunshine, Heba Waly via GitGitGadget, Git List, Emily Shaffer 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 19:15 ` Junio C Hamano @ 2020-01-10 12:11 ` Heba Waly 0 siblings, 0 replies; 18+ messages in thread From: Heba Waly @ 2020-01-10 12:11 UTC (permalink / raw) To: Junio C Hamano Cc: Johannes Schindelin, Eric Sunshine, Heba Waly via GitGitGadget, Git List, Emily Shaffer 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 18:06 ` Heba Waly 2020-01-08 19:01 ` Johannes Schindelin @ 2020-01-08 19:05 ` Junio C Hamano 1 sibling, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2020-01-08 19:05 UTC (permalink / raw) To: Heba Waly Cc: Eric Sunshine, Heba Waly via GitGitGadget, Git List, Emily Shaffer 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; } ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/1] branch: advise the user to checkout a different branch before deleting 2020-01-08 9:27 ` Eric Sunshine 2020-01-08 18:06 ` Heba Waly @ 2020-01-10 12:09 ` Heba Waly 1 sibling, 0 replies; 18+ messages in thread From: Heba Waly @ 2020-01-10 12:09 UTC (permalink / raw) To: Eric Sunshine; +Cc: Heba Waly via GitGitGadget, Git List, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-01-10 12:12 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-02 2:49 [PATCH 0/1] [Outreachy] [RFC] branch: advise the user to checkout a different branch before deleting Heba Waly via GitGitGadget 2020-01-02 2:49 ` [PATCH 1/1] " Heba Waly via GitGitGadget 2020-01-02 8:18 ` Eric Sunshine 2020-01-06 0:42 ` Heba Waly 2020-01-07 4:10 ` [PATCH v2 0/1] [Outreachy] [RFC] " Heba Waly via GitGitGadget 2020-01-07 4:10 ` [PATCH v2 1/1] " Heba Waly via GitGitGadget 2020-01-07 11:16 ` Eric Sunshine 2020-01-07 16:34 ` Junio C Hamano 2020-01-08 1:44 ` Emily Shaffer 2020-01-08 10:22 ` Eric Sunshine 2020-01-08 1:14 ` Heba Waly 2020-01-08 9:27 ` Eric Sunshine 2020-01-08 18:06 ` Heba Waly 2020-01-08 19:01 ` Johannes Schindelin 2020-01-08 19:15 ` Junio C Hamano 2020-01-10 12:11 ` Heba Waly 2020-01-08 19:05 ` Junio C Hamano 2020-01-10 12:09 ` Heba Waly
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).