Re: [PATCH v7] unpack-trees: suggest using 'git stash' when checkout fails

>
> "Arsh Srivastava via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > When a branch switch fails due to local changes and
> > new users who are not familiar with the error message often
> > get confused about how to move ahead and resolve the issue as
> > the previous error message only suggests to commit or stash the changes
> > but doesn't explain how to do that or what the next steps are.
>
> The first paragraph is a bit of a run-on and has a misplaced "and";
> I cannot quite read and understand this overly long single sentence.
>
> Perhaps the early part can become a bit easier to read with
> punctuations, and cutting the sentence into two, e.g.,
>
>   When a branch switch fails due to local changes, new users who
>   are unfamiliar with the error message often get confused about how
>   to move ahead and resolve the issue.
>
> Also it is misleading to say "previous" error message.  We talk
> about the current code in the present tense, to highlight what the
> problem in the current code is.  The _current_ message stops at
> hinting the commands to be used without giving wordy instructions
> that are best left to manuals.  You may view it as a weakness (which
> may motivate this patch to be written).  But I personally am not so
> sure that adding words to the existing message would necessarily
> make it more clear.
>
> > This patch enhances the error message with more specific
> > instructions in a concise manner to help users understand
> > how to resolve the issue and move their local changes
> > safely to the other branch using stash.
>
> As Documentation/SubmittingPatches says, let's instruct the code to
> "be like so" in imperative mood.  E.g., "Enhance the error
> message..." instead of "This patch enhances...".
>
> By the way, the updated message seems much less concise than the
> original.
>
> >       msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
> >             ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
> > -               "Please commit your changes or stash them before you switch branches.")
> > -           : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
> > +               "To move you local changes safely to the other branch,\n"
> > +               "Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge."
> > +               )
> > +           : _("Your local changes to the following files would be overwritten by checkout:\n%%s"
> > +               "Please commit your changes or stash them before you switch branches.");
>
> These were already overly long, but the updated one is way too long
> to be read on end-user's terminal.  The source lines are overly
> long, too.
>
> The original was this:
>
>         msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>               ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
>                   "Please commit your changes or stash them before you switch branches.")
>               : _("Your local changes to the following files would be overwritten by checkout:\n%%s");
>
> Note that when advice is *NOT* enabled, we only gave
>
> _("Your local changes to the following files would be overwritten by checkout:\n%%s");
>
> without any "advise" in the output.  That is what !advice_enabled() means.
>
> The updated code does this:
>
>         msg = advice_enabled(ADVICE_COMMIT_BEFORE_MERGE)
>               ? _("Your local changes to the following files would be overwritten by checkout:\n%%s"
>                   "To move you local changes safely to the other branch,\n"
>                   "Please try 'git stash' followed by 'git checkout <branch>' followed by 'git stash pop' for safe merge."
>                   )
>               : _("Your local changes to the following files would be overwritten by checkout:\n%%s"
>                   "Please commit your changes or stash them before you switch branches.");
>
> to those users who decline the advice, we now show "Please
> commit...".  That is not what !advice_enabled() should trigger, is
> it?
>
> Also "To move you" -> "To move your".
>
> Also the advice lost the other possiblity of first committing the
> work in progress on the original branch before switching, yet the
> new advice message is quite wordy.
>
> Also, using "for safe merge" when the user is performing a
> "checkout" might be slightly confusing, even if 'stash pop' involves
> a merge under the hood.
>
> A more concise version might say:
>
>   Try 'git stash && git checkout <branch> && git stash pop' to carry
>   your changes to the new branch, or commit your work before switching.
>
> But as I already said, I think the current text may already strike
> the right balance between being clear and being concise.
>
> Thanks.
>