patch, 3 partswt-status: Make status messages more consistent with others

2/3: I've added advice.mergeHints to silent the messages that suggests "git merge--abort".

3/3: I've added a warning message when users used "git reset" during a merge. This warning will be printed if the user is in the middle of a merge. In future releases, we'll change this into an error to prevent work tree from becoming a mess.

Andrew Wong (3):
  wt-status: Make status messages more consistent with others
  merge: Advise user to use "git merge --abort" to abort merges
  reset: Print a warning when user uses "git reset" during a merge

 Documentation/config.txt |  3 +++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 builtin/merge.c          |  6 ++++++
 builtin/reset.c          | 21 +++++++++++++++++++++
 wt-status.c              | 23 +++++++++++++----------
 6 files changed, 46 insertions(+), 10 deletions(-)

This is mainly changing messages that say:
    run "git foo --bar"
to
    use "git foo --bar" to baz

Although the commands and flags are usually self-explanatory, this is more consistent with other status messages, and gives some sort of explanation to the user.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 wt-status.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Andrew Wong <andrew.kw.w@gmail.com> writes:

"git foo --bar" is fine, but "to baz" was hard to read without first realizing that 'baz' stands for some/any verb. I think rephrasing it to

	use "git foo --bar" to do baz

would reduce confusion.

The above hunk makes sense.

At first glance, I felt that none of the remainder made much sense. My reaction was: "git foo --continue" to continue? What else could the --continue option even mean?

The real value I see in these conversions is by saying "use this to continue" instead of an unconditional "run this", it implies "*IF* you wanted to continue, you can do this", meaning that user also has the option of *not* continuing. But the proposed update falls short of realizing the full potential, if that is the value we are trying to add. I'd say

	fix conflicts and then use "git am --continue" if you want
	to continue.

or an even more explicit

	fix conflicts and then use "git am --continue" if you want
	to continue; or you can "git am --abort" to discontinue.

would be an improvement, but

	fix conflicts and then use "git am --continue" to continue

is probably not quite.

Print message during "git merge" and "git status".

Add a new "mergeHints" advice to silence these messages.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 Documentation/config.txt | 3 +++
 advice.c                 | 2 ++
 advice.h                 | 1 +
 builtin/merge.c          | 6 ++++++
 wt-status.c              | 3 +++
 5 files changed, 15 insertions(+)

Andrew Wong <andrew.kw.w@gmail.com> writes:

This sounds sensible. Don't we want to have this one take effect on the places where advice.resolveConflict is used in git-pull? I.e. something like:

	do_we_advise=no
	if advice.resolveConflict is not set:
		if advice.mergeHints is set to false:
                	do_we_advise=no
		else:
                	do_we_advise=yes
	else:
        	do_we_advise=yes

        if do_we_advise == 'yes':
        	give advice in die_conflict and die_merge

During a merge, "--mixed" is most likely not what the user wants. Using "--mixed" during a merge would leave the merged changes and new files mixed in with the local changes. The user would have to manually clean up the work tree, which is non-trivial. In future releases, we want to make "git reset" error out when used in the middle of a merge. For now, we simply print out a warning to the user.

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 builtin/reset.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

On 14-03-14 12:37 AM, Andrew Wong wrote:

I know this approach was suggested earlier, but given these dangers it seems silly to give this big warning on a plain "git reset" but still go ahead and do the things the warning talks about.

Is there any issue with changing "git reset" to error-out now but letting "git reset --mixed" proceed? Something like (note the reworded warning message):

$ git reset Cowardly refusing to implicitly run 'git reset --mixed' during a merge. This would not clean up any merged changes and would not remove any new files that were created in the work tree. It would also make it impossible for git to automatically clean up the work tree later, so you would have to clean up the work tree manually. You probably meant to run 'git merge --abort' instead. $ git reset --mixed # Stoopid git! I know what I'm doing! $

This would mean that the 10% of git users who like to do "git reset" in the middle of a conflicted merge will have to teach their fingers some extra motions. But these users are all veterans, and they can more easily type in 8 extra characters (6 with completion) than new users can recover from accidentally misusing git-reset's power.

		M.

On Fri, Mar 14, 2014 at 10:33 AM, Marc Branchaud <marcnarc@xiplink.com> wrote:

Yeah, I would have preferred to have "git reset" error out right now, because the messed up work tree can be quite a pain to clean up. The main argument for issuing the warning is about maintaining compatibility.

For the users that really did mean "--merge", the warning is silly. It's basically saying "We know that you're about to mess up your work tree, but we let you mess up anyway. Learn the correct way so that you don't mess up next time".

It actually doesn't seem too bad if we did make "git reset" to error out (during a merge) right away. By erroring out, the command won't cause some irreversible damage, and users don't lose data. Yes, it breaks compatibility, but perhaps not in a bad way?

I'm really fine with either. Junio?

Andrew Wong <andrew.kw.w@gmail.com> writes:

I suspect that you meant "--mixed" instead of "--merge" here.

I do not agree with the "We know that you are about to mess up" above. Whoever is issuing that warning message may not know better than the user who is running "reset". As you wrote "most likely not what the user wants" in your proposed log message, the only thing we know is that it _often_ is a newbie mistake.

I recently needed to manually cherry-pick only one half of a patch, and the way I did so was

	git show $that_commit >P.diff
        git apply -3 P.diff
        ... conflicts are expected; that is why I used -3 in the
        ... first place
        git reset
        git diff HEAD
	edit
        ... edit away the other half I did not want to cherry-pick
        ... while fixing the conflicted parts that happened to be
        ... in the part I did want to cherry-pick

"git cherry-pick --no-commit $that_commit" could have been used as a replacement for the first two steps but being able to run the "reset" without erroring out was an essential part to make this workflow usable.

So I am OK with "eventually error out by default", but not OK with "we know better than the user and will not allow it at all".

On Fri, Mar 14, 2014 at 4:55 PM, Junio C Hamano <gitster@pobox.com> wrote:

No, I did mean "--merge". It's silly for inexperienced users because it's too late to use "--merge" by the time they realized they should not have used the default. The work tree has already become a mess. So they'd immediately think "if git was smart enough to warn me about the mess, why not prevent me from getting into the mess in the first place?"

For the experienced users, they would understand the warning, because they would be aware of the index, and the effect that "--mixed" and "--merge" have on it.

Again, I didn't mean "we know better than the user". However, from a new user's perspective, they won't understand why "git reset" gives the warning, but still "knowingly" messes up their work tree.

And "we don't know better than the user" is exactly why I think we should "eventually error out" rather than automatically switching to "--merge". As Matthieu was saying, automatically switching to "--merge" could discard conflict resolutions, which would be undesirable. So it's better for git to error out then having git decides what the user (probably) wants.

On 14-03-14 04:55 PM, Junio C Hamano wrote:

Can I interpret that as you being OK with my proposed "Cowardly refusing" approach?

		M.

Andrew Wong <andrew.kw.w@gmail.com> writes:

Two style niggles here.

"we want"?  Has any of us decided on that?

Andrew Wong <andrew.kw.w@gmail.com> writes:

Has this series been tested with existing test suite? I tentatively queued it to 'pu' but then had to revert because many tests started failing, causing me to redo the today's integration cycle for 'pu' once again.

On Mon, Mar 17, 2014 at 7:04 PM, Junio C Hamano <gitster@pobox.com> wrote:

I tested it during RFC, but missed it when I sent it as patch. The problem is here:

Andrew Wong <andrew.kw.w@gmail.com> writes:

Thanks. Will hold onto the topic branch lest I forget, but will keep it out of 'pu' in the meantime.