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

[Emily Shaffer <emilyshaffer@google.com>]

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