On Wed, Jan 8, 2020 at 12:16 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Mon, Jan 6, 2020 at 11:10 PM Heba Waly via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
