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

[Junio C Hamano <gitster@pobox.com>]

"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.