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

"Arsh Srivastava via GitGitGadget" <gitgitgadget@gmail.com> writes:

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.

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.

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.