[RFC 0/3] Make git more user-friendly during a merge conflict

[RFC 0/3] Make git more user-friendly during a merge conflict

[Andrew Wong]

Users may not be aware that they need to use "git merge --abort" or "git reset
--merge" to properly abort a merge conflict. They are likely to just use "git
reset", because that "usually" cleans up the repo. But in the case where the
user had local changes, "git reset" would leave the repo in a messy state where
merged changes and local changes are mixed together.

The first two patches are just about rewording a message, and adding messages
to tell users to use "git merge --abort" to abort a merge.

We could stop here and hope that the users would read the messages, but I think
git could be a bit more user-friendly. The last patch might be a bit more
controversial. It changes the default behavior of "git reset" to default to
"git reset --merge" during a merge conflict. I imagine that's what the user
would want most of the time, and not "git reset --mixed". I haven't updated the
"git reset" docs yet, I'll update it if we decide to use this new behavior.

Comments?

Andrew Wong (3):
  wt-status: Make conflict hint message more consistent with other hints
  merge: Add hints to tell users about "git merge --abort"
  reset: Change the default behavior to use "--merge" during a merge

 builtin/merge.c | 3 ++-
 builtin/reset.c | 7 ++++++-
 wt-status.c     | 5 ++++-
 3 files changed, 12 insertions(+), 3 deletions(-)

-- 
1.9.0.6.g16e5f9a

[RFC 1/3] wt-status: Make conflict hint message more consistent with other hints

[Andrew Wong]

Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
---
 wt-status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index 4e55810..6e1ad7d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -899,7 +899,7 @@ static void show_merge_in_progress(struct wt_status *s,
 		status_printf_ln(s, color, _("You have unmerged paths."));
 		if (s->hints)
 			status_printf_ln(s, color,
-				_("  (fix conflicts and run \"git commit\")"));
+				_("  (fix conflicts, and use \"git commit\" to conclude the merge)"));
 	} else {
 		status_printf_ln(s, color,
 			_("All conflicts fixed but you are still merging."));
-- 
1.9.0.6.g16e5f9a

[RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Andrew Wong]

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

diff --git a/builtin/merge.c b/builtin/merge.c
index e576a7f..07af427 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
 	fclose(fp);
 	rerere(allow_rerere_auto);
 	printf(_("Automatic merge failed; "
-			"fix conflicts and then commit the result.\n"));
+			"fix conflicts and then commit the result.\n"
+			"To abort the merge, use \"git merge --abort\".\n"));
 	return 1;
 }
 
diff --git a/wt-status.c b/wt-status.c
index 6e1ad7d..54c2203 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -907,6 +907,9 @@ static void show_merge_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git commit\" to conclude merge)"));
 	}
+	if (s->hints)
+		status_printf_ln(s, color,
+			_("  (use \"git merge --abort\" to abort the merge)\n"));
 	wt_status_print_trailer(s);
 }
 
-- 
1.9.0.6.g16e5f9a

[RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Andrew Wong]

If the user wants to do "git reset" during a merge, the user most likely
wants to do a "git reset --merge". This is especially true during a
merge conflict and the user had local changes, because "git reset" would
leave the merged changes mixed in with the local changes. This makes
"git reset" a little more user-friendly during a merge.

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

diff --git a/builtin/reset.c b/builtin/reset.c
index 6004803..740263d 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -318,7 +318,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 					_(reset_type_names[reset_type]));
 	}
 	if (reset_type == NONE)
-		reset_type = MIXED; /* by default */
+	{
+		if(is_merge())
+			reset_type = MERGE;
+		else
+			reset_type = MIXED;
+	}
 
 	if (reset_type != SOFT && reset_type != MIXED)
 		setup_work_tree();
-- 
1.9.0.6.g16e5f9a

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Matthieu Moy]

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

> If the user wants to do "git reset" during a merge, the user most likely
> wants to do a "git reset --merge". This is especially true during a
> merge conflict and the user had local changes, because "git reset" would
> leave the merged changes mixed in with the local changes. This makes
> "git reset" a little more user-friendly during a merge.

But this breaks backward compatibility.

I sometimes run "git reset" during a merge to only reset the index and
then examine the changes introduced by the merge. With your changes,
someone doing so would abort the merge and discard the merge resolution.
I very rarely do this, but even rarely, I wouldn't like Git to start
droping data silently for me ;-).

I'm not really convinced that this is such a good change, and if we go
this way, there should be a transition to let users stop using
argumentless "git reset" to reset the index during a merge.

The other 2 patches look good to me.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Andrew Wong]

On Wed, Feb 26, 2014 at 1:21 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> But this breaks backward compatibility.
>
> I sometimes run "git reset" during a merge to only reset the index and
> then examine the changes introduced by the merge. With your changes,
> someone doing so would abort the merge and discard the merge resolution.
> I very rarely do this, but even rarely, I wouldn't like Git to start
> droping data silently for me ;-).

I don't think it's actually dropping data though, because your changes just
come from "git merge". So you can also do the merge again.

To examine the changes, you're saying you'd do "git reset && git diff". But
without doing "git reset", couldn't you do "git diff HEAD" to get the diff?
This also has the advantage of keeping git in the merging state, so you can
decide to continue/abort the merge later on.

> I'm not really convinced that this is such a good change, and if we go
> this way, there should be a transition to let users stop using
> argumentless "git reset" to reset the index during a merge.

Yeah, this breaks compatibility, but like I said, during a merge, I don't
see a good reason to do "git reset --mixed", and not "git reset --merge".
Especially when there are local changes, "--mixed" would actually cause
more headaches than "git reset --merge", because you would lose the
distinction between merge changes and unstaged changes.

Andrew

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Jonathan Nieder]

Hi,

Andrew Wong wrote:

> The first two patches are just about rewording a message, and adding messages
> to tell users to use "git merge --abort" to abort a merge.

Sounds like a good idea.  I look forward to reading the patches.

> We could stop here and hope that the users would read the messages, but I think
> git could be a bit more user-friendly. The last patch might be a bit more
> controversial. It changes the default behavior of "git reset" to default to
> "git reset --merge" during a merge conflict. I imagine that's what the user
> would want most of the time, and not "git reset --mixed".

I don't think that's a good idea.  I'm not sure what new users would
expect; in any case, making the command context-dependent just makes
the learning process harder imho.  And for experienced users, this
would be a bad regression.

An 'advice' message pointing the user to 'git merge --abort' might
make sense, though.

What do you think?

Thanks,
Jonathan

Re: [RFC 1/3] wt-status: Make conflict hint message more consistent with other hints

[Jonathan Nieder]

Hi,

Andrew Wong wrote:

> [Subject: wt-status: Make conflict hint message more consistent with other hints]

Thanks for working on this.

Could you include a little more detail?  What other hints is this
making the message more consistent with?

Ideally the commit message would include a quick sample interaction,
so the reviewer could see the user going "Wha?" and then look at the
patch to see how it resolves the confusion.

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -899,7 +899,7 @@ static void show_merge_in_progress(struct wt_status *s,
>  		status_printf_ln(s, color, _("You have unmerged paths."));
>  		if (s->hints)
>  			status_printf_ln(s, color,
> -				_("  (fix conflicts and run \"git commit\")"));
> +				_("  (fix conflicts, and use \"git commit\" to conclude the merge)"));

Quick thoughts:

 - The comma just moves the message closer to the right margin.  I think
   it makes the message less readable.

 - What else would "git commit" do other than concluding the merge?
   What confusion is this meant to prevent?

 - Would introducing a new "git merge --continue" command help?

   Advantages: (1) the name of the command makes it obvious what
   it does; (2) the command could check that there is actually
   a merge in progress, helping the user when the state is not
   what they think; (3) consistency with "git cherry-pick --abort" /
   "git cherry-pick --continue".

   Disadvantage: redundancy (but see (2) above).

Hope that helps,
Jonathan

Re: [RFC 1/3] wt-status: Make conflict hint message more consistent with other hints

[Junio C Hamano]

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

> Signed-off-by: Andrew Wong <andrew.kw.w@gmail.com>
> ---
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 4e55810..6e1ad7d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -899,7 +899,7 @@ static void show_merge_in_progress(struct wt_status *s,
>  		status_printf_ln(s, color, _("You have unmerged paths."));
>  		if (s->hints)
>  			status_printf_ln(s, color,
> -				_("  (fix conflicts and run \"git commit\")"));
> +				_("  (fix conflicts, and use \"git commit\" to conclude the merge)"));
>  	} else {
>  		status_printf_ln(s, color,
>  			_("All conflicts fixed but you are still merging."));

I see that you are trying to match the phrasing used in the other
side of this if/else (which is outside the context of the posted
patch).  Over there we say "... to conclude merge" while the new
text says "... to conclude THE merge".  Don't we want to match them?

For those who did not look beyond the context of the patch text, as
I had to look these up to convince myself that the proposed change
is a good one.  This function is only called when we see MERGE_HEAD,
so "unmerged" here can come only from a failed merge, not other
mergy operations like am, cherry-pick, revert, etc. and telling the
user that 'commit' will conclude the merge will not be misleading
(unless you count "'git commit' will conclude a conflicted 'git pull'"
as misleading, and I of course do not).

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Jonathan Nieder]

Andrew Wong wrote:

> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
>  	fclose(fp);
>  	rerere(allow_rerere_auto);
>  	printf(_("Automatic merge failed; "
> -			"fix conflicts and then commit the result.\n"));
> +			"fix conflicts and then commit the result.\n"
> +			"To abort the merge, use \"git merge --abort\".\n"));

Seems reasonable, but I worry about the command growing too noisy.

Could this be guarded by an advice.<something> setting?  (See advice.*
in git-config(1) for what I mean.)

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -907,6 +907,9 @@ static void show_merge_in_progress(struct wt_status *s,
>  			status_printf_ln(s, color,
>  				_("  (use \"git commit\" to conclude merge)"));
>  	}
> +	if (s->hints)
> +		status_printf_ln(s, color,
> +			_("  (use \"git merge --abort\" to abort the merge)\n"));

Perhaps:

	...
		_("  (or use \"git merge --abort\" to abort the merge)"));

to clarify that this is an alternative to the advice immediately above.

`status_printf_ln` already prints a newline, so the translated message
shouldn't include an extra one.

Thanks,
Jonathan

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Jonathan Nieder]

Andrew Wong wrote:

> Yeah, this breaks compatibility, but like I said, during a merge, I don't
> see a good reason to do "git reset --mixed", and not "git reset --merge".

Yeah, in principle if it had a different behavior, then plain "git
reset" could be useful during a merge, but as is, I tend to use the
form with a path ("git reset -- .") to avoid losing MERGE_HEAD.

I really don't like the idea of making "git reset" modal, though.  I'd
rather that reset --mixed print some advice about how to recover from
the mistake, which would also have the advantage of allowing scripts
that for whatever reason used "git reset" in this situation to
continue to work.

Thanks,
Jonathan

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Andrew Wong <andrew.kw.w@gmail.com> writes:
>
>> If the user wants to do "git reset" during a merge, the user most likely
>> wants to do a "git reset --merge". This is especially true during a
>> merge conflict and the user had local changes, because "git reset" would
>> leave the merged changes mixed in with the local changes. This makes
>> "git reset" a little more user-friendly during a merge.
>
> But this breaks backward compatibility.
>
> I sometimes run "git reset" during a merge to only reset the index and
> then examine the changes introduced by the merge.

Hmmmm, wouldn't it a better way to do this to run "git diff HEAD"
without any resetting of the index, though?  Having said that...

> With your changes,
> someone doing so would abort the merge and discard the merge resolution.
>
> I very rarely do this, but even rarely, I wouldn't like Git to start
> droping data silently for me ;-).

...this indeed may be a concern that deserves a bit more thought.

> I'm not really convinced that this is such a good change, and if we go
> this way, there should be a transition to let users stop using
> argumentless "git reset" to reset the index during a merge.

If the only reason somebody might want to "reset --mixed" is to make
the next "git diff" behave as if it were "git diff HEAD", I would be
willing to say that we should:

 - start warning against "reset" (no mode specifier) and "reset --mixed"
   when the index is unmerged *and* MERGE_HEAD exists; and then

 - after a few releases start erroring out when such a "reset" is
   given; and then

 - use this patch to intelligently choose the default mode.

Another downside of "reset --mixed" during a conflicted merge (or
other mergy operations, e.g. "am -3") is that new files are left in
the working tree, making the next attempt to redo the mergy
operation immediately fail until you remove them, so in practice,
the only mode I'd use with a conflicted index (be it from a
conflicted merge or any other mergy operation) is "reset --hard".

So forbidding "reset --mixed" when the index is unmerged (even when
there is no MERGE_HEAD) may be a good idea in the long run.

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Matthieu Moy]

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

> On Wed, Feb 26, 2014 at 1:21 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> But this breaks backward compatibility.
>>
>> I sometimes run "git reset" during a merge to only reset the index and
>> then examine the changes introduced by the merge. With your changes,
>> someone doing so would abort the merge and discard the merge resolution.
>> I very rarely do this, but even rarely, I wouldn't like Git to start
>> droping data silently for me ;-).
>
> I don't think it's actually dropping data though, because your changes just
> come from "git merge". So you can also do the merge again.

But you can't repeat your merge conflicts resolution.

> To examine the changes, you're saying you'd do "git reset && git diff". But
> without doing "git reset", couldn't you do "git diff HEAD" to get the
> diff?

I could. The point is, sometimes I don't.

If you were to design "git reset"'s interface from scratch, your
proposal would make sense. But we're talking about a change, and you
can't expect that users never use the current behavior. At the very
least, there should be a warning telling the user that the behavior
changed, and I'm really afraid that the warning goes along the lines of
"I've thought you'd prefer me to discard your unsaved changes, please
rewrite them if you actually didn't want me to".

>> I'm not really convinced that this is such a good change, and if we go
>> this way, there should be a transition to let users stop using
>> argumentless "git reset" to reset the index during a merge.
>
> Yeah, this breaks compatibility, but like I said, during a merge, I don't
> see a good reason to do "git reset --mixed",

The point with backward compatibility is not to know whether users have
a good reason to, but whether you can guarantee that no one ever does
it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC 1/3] wt-status: Make conflict hint message more consistent with other hints

[Andrew Wong]

On Wed, Feb 26, 2014 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I see that you are trying to match the phrasing used in the other
> side of this if/else (which is outside the context of the posted
> patch).  Over there we say "... to conclude merge" while the new
> text says "... to conclude THE merge".  Don't we want to match them?

Ah, good catch. My mind just read it as "conclude THE merge", even
though the word wasn't there. Let's add the "the" in. :)

> For those who did not look beyond the context of the patch text, as
> I had to look these up to convince myself that the proposed change
> is a good one.  This function is only called when we see MERGE_HEAD,
> so "unmerged" here can come only from a failed merge, not other
> mergy operations like am, cherry-pick, revert, etc. and telling the
> user that 'commit' will conclude the merge will not be misleading
> (unless you count "'git commit' will conclude a conflicted 'git pull'"
> as misleading, and I of course do not).

I'll update the commit message to explain that I'm match the other
side of the if/else.

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Andrew Wong]

On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Seems reasonable, but I worry about the command growing too noisy.
>
> Could this be guarded by an advice.<something> setting?  (See advice.*
> in git-config(1) for what I mean.)

Ah, good idea. This seems to belong to advice.resolveConflict. I'll
add it there.

> `status_printf_ln` already prints a newline, so the translated message
> shouldn't include an extra one.

Good catch. Thanks!

Andrew

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Andrew Wong]

On Wed, Feb 26, 2014 at 3:48 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> I really don't like the idea of making "git reset" modal, though.  I'd
> rather that reset --mixed print some advice about how to recover from
> the mistake, which would also have the advantage of allowing scripts
> that for whatever reason used "git reset" in this situation to
> continue to work.

In the case where user had unstaged changes before running "git
merge", there's no way to recover from the mistake. Their worktree is
left with a mix of both the merge changes and their original unstaged
changes. As Junio pointed out, new files will also be left in the
worktree, so the next attempt to "git merge" will fail until the files
are removed. There's no way to recover from it except to have the user
manually clean out the merge changes and new files manually. That's
why "git reset --mixed" doesn't seem sensible during a merge.

That said, I do feel it might not be a good idea to have the default
behavior of "git reset" change depending on the context. What Junio
suggested might be a better approach. To have "git reset" error out
instead may be a better alternative, since that doesn't silently do
something else and break compatibility.

Andrew

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Andrew Wong]

On Wed, Feb 26, 2014 at 3:57 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> If you were to design "git reset"'s interface from scratch, your
> proposal would make sense. But we're talking about a change, and you
> can't expect that users never use the current behavior. At the very
> least, there should be a warning telling the user that the behavior
> changed, and I'm really afraid that the warning goes along the lines of
> "I've thought you'd prefer me to discard your unsaved changes, please
> rewrite them if you actually didn't want me to".
>
>>> I'm not really convinced that this is such a good change, and if we go
>>> this way, there should be a transition to let users stop using
>>> argumentless "git reset" to reset the index during a merge.
>>
>> Yeah, this breaks compatibility, but like I said, during a merge, I don't
>> see a good reason to do "git reset --mixed",
>
> The point with backward compatibility is not to know whether users have
> a good reason to, but whether you can guarantee that no one ever does
> it.

Yeah, I do see what you mean. But the problem of using "git reset
--mixed" during a merge is problematic too. It leaves you with a mix
of merge changes and local changes. As Junio pointed out, new files
will also be left in the worktree. So users would have to clean all
that up manually. Perhaps what Junio suggested is a better approach.
Slowly phase out this behavior by printing out warnings. Then
eventually erroring out in this situation, and then finally switch to
a new behavior, whatever that may be.

Andrew

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Stephen Leake]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi,
>
> Andrew Wong wrote:
>
>> The first two patches are just about rewording a message, and adding messages
>> to tell users to use "git merge --abort" to abort a merge.
>
> Sounds like a good idea.  I look forward to reading the patches.
>
>> We could stop here and hope that the users would read the messages, but I think
>> git could be a bit more user-friendly. The last patch might be a bit more
>> controversial. It changes the default behavior of "git reset" to default to
>> "git reset --merge" during a merge conflict. I imagine that's what the user
>> would want most of the time, and not "git reset --mixed".
>
> I don't think that's a good idea.  I'm not sure what new users would
> expect; 

As a newbie, I would like to know how to abort the merge, so I like this
message. 

> in any case, making the command context-dependent just makes
> the learning process harder imho.  

I like commands that "do the right thing". So no, this would not be
confusing. 

> And for experienced users, this would be a bad regression.

Backward incompatibility is a real concern.

It might be best if "git reset" (with _no_ option) be made to error out,
so all users have to specify what they want.

The transition process Junio proposed sounds good to me.

-- 
-- Stephe

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Charles Bailey]

On Fri, Feb 28, 2014 at 03:01:53AM -0600, Stephen Leake wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
> > And for experienced users, this would be a bad regression.
> 
> Backward incompatibility is a real concern.
> 
> It might be best if "git reset" (with _no_ option) be made to error out,
> so all users have to specify what they want.

This is just as much of a regression (if less dangerous) as changing
the default behaviour of git reset to touch the working tree.

'git reset' is a very, very common action for me and simply means
'reset [my index] [to HEAD]'. I frequently find myself resetting so
that I can stage something a bit different to what I had originally
intended.

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[David Kastrup]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> I like commands that "do the right thing". So no, this would not be
> confusing.

I _hate_ commands that think they know better than to do what they are
told.  In particular when doing destructive things.  And just because
_you_ like them does not mean they are not confusing.

In the long run, it is much more confusing if you come to rely on some
commands doing "the right thing" while in other cases, the actually
written thing is done.

"do the right thing" commands also tend to do the wrong thing
occasionally with potentially disastrous results when they are used in
scripts where the followup actions rely on the actual result.

-- 
David Kastrup

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Stephen Leake]

David Kastrup <dak@gnu.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> I like commands that "do the right thing". So no, this would not be
>> confusing.
>
> I _hate_ commands that think they know better than to do what they are
> told.  In particular when doing destructive things.  And just because
> _you_ like them does not mean they are not confusing.

Ok, I should have said "not confusing for me".

People differ.

> In the long run, it is much more confusing if you come to rely on some
> commands doing "the right thing" while in other cases, the actually
> written thing is done.

There should always be the option of telling git exactly what to do. In
my emacs front end, the command that "does the right thing" is _called_
"do-the-right-thing". All of the other commands do exactly as told.

In this case, it is only "git reset" that would do the right thing,
since you did _not_ tell it specifically what to do.

Relying on a default is always problematic, in my experience; I much
prefer "no default" to "a default that people voted on 10 years ago, and
now we are stuck with it".

> "do the right thing" commands also tend to do the wrong thing
> occasionally with potentially disastrous results when they are used in
> scripts where the followup actions rely on the actual result.

That is bad, and should not be allowed. On the other hand, I have yet to
see an actual use case of bad behavior in this discussion.

-- 
-- Stephe

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[David Kastrup]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> "do the right thing" commands also tend to do the wrong thing
>> occasionally with potentially disastrous results when they are used
>> in scripts where the followup actions rely on the actual result.
>
> That is bad, and should not be allowed. On the other hand, I have yet
> to see an actual use case of bad behavior in this discussion.

Huh.

<http://permalink.gmane.org/gmane.comp.version-control.git/242744>

-- 
David Kastrup

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Stephen Leake]

David Kastrup <dak@gnu.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> "do the right thing" commands also tend to do the wrong thing
>>> occasionally with potentially disastrous results when they are used
>>> in scripts where the followup actions rely on the actual result.
>>
>> That is bad, and should not be allowed. On the other hand, I have yet
>> to see an actual use case of bad behavior in this discussion.
>
> Huh.
>
> <http://permalink.gmane.org/gmane.comp.version-control.git/242744>

That's about backward incompatibility, which is bad, but not what I was
talking about above.

Specifically, the proposed change is:

'git reset' will have different default actions depending on context:

- if a merge is not in progress, it will do 'git reset --mixed'

- if a merge is in progress, it will do 'git reset --merge'

Is there a use case where this will do the wrong thing?

Of course, I fully understand that not being able to come up with a
"wrong thing" use case is not the same as proving it cannot happen,
especially for a system as complex as git.

So it would be ok to say "we don't do that so we are not exposed to
unintended consequences".

But "wrong thing" use cases are more convincing :).

-- 
-- Stephe

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[David Kastrup]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>>
>>> David Kastrup <dak@gnu.org> writes:
>>>
>>>> "do the right thing" commands also tend to do the wrong thing
>>>> occasionally with potentially disastrous results when they are used
>>>> in scripts where the followup actions rely on the actual result.
>>>
>>> That is bad, and should not be allowed. On the other hand, I have yet
>>> to see an actual use case of bad behavior in this discussion.
>>
>> Huh.
>>
>> <http://permalink.gmane.org/gmane.comp.version-control.git/242744>
>
> That's about backward incompatibility, which is bad, but not what I was
> talking about above.

No, it isn't.  I quote:

    I sometimes run "git reset" during a merge to only reset the index
    and then examine the changes introduced by the merge. With your
    changes, someone doing so would abort the merge and discard the
    merge resolution.  I very rarely do this, but even rarely, I
    wouldn't like Git to start droping data silently for me ;-).

You should not make statements like "I have yet to see an actual use
case of bad behavior in this discussion" when you actually mean "I have
not yet seen anything I would be interested in doing myself".

-- 
David Kastrup

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Stephen Leake]

David Kastrup <dak@gnu.org> writes:

> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> Stephen Leake <stephen_leake@stephe-leake.org> writes:
>>>
>>>> David Kastrup <dak@gnu.org> writes:
>>>>
>>>>> "do the right thing" commands also tend to do the wrong thing
>>>>> occasionally with potentially disastrous results when they are used
>>>>> in scripts where the followup actions rely on the actual result.
>>>>
>>>> That is bad, and should not be allowed. On the other hand, I have yet
>>>> to see an actual use case of bad behavior in this discussion.
>>>
>>> Huh.
>>>
>>> <http://permalink.gmane.org/gmane.comp.version-control.git/242744>
>>
>> That's about backward incompatibility, which is bad, but not what I was
>> talking about above.
>
> No, it isn't.  I quote:
>
>     I sometimes run "git reset" during a merge to only reset the index
>     and then examine the changes introduced by the merge. With your
>     changes, someone doing so would abort the merge and discard the
>     merge resolution.  I very rarely do this, but even rarely, I
>     wouldn't like Git to start droping data silently for me ;-).
>
> You should not make statements like "I have yet to see an actual use
> case of bad behavior in this discussion" when you actually mean "I have
> not yet seen anything I would be interested in doing myself".

Clearly I misunderstood your point. Merely repeating the same statement
that I misunderstood, and adding a misunderstanding of what I said, is
not helpful.

So let me see if I can expand on your use case:

- you do 'git merge', which results in conflicts

- you edit some workspace files to resolve some of those conflicts 

    (I added this step later, since it was implied but not explicit)

- you do 'git reset', intending 'git reset --mixed' (because that is the
  current default)

    Actually, I can't find a precise definition of 'git reset'. Here is
    the synopsis from the man page for 'git-reset' (from git 1.7.9):

       git reset [-q] [<commit>] [--] <paths>...
       git reset (--patch | -p) [<commit>] [--] [<paths>...]
       git reset (--soft | --mixed | --hard | --merge | --keep) [-q] [<commit>]

    In 'git reset', there is no path, so it must be the second or third
    form. But those _require_ one of the -- options. So 'git reset' is
    illegal. Clearly something is wrong here; apparently the third line
    should be:

       git reset [--soft | --mixed | --hard | --merge | --keep] [-q] [<commit>]

    with '--mixed' as the default, as is stated later. (perhaps the
    original intent was to not have a default for the third form, but
    that got changed sometime?).

    This command "resets the index" but not the working tree. I'm not
    clear what "reset the index" means here; does it mean "remove all
    entries from the index", or "reset the index to some previous
    state"? In other man pages, "reset" can have either meaning
    depending on context.

- then you "examine changes introduced by the merge". I don't know what
  this means in detail. 

    Before resetting the index, you could diff a workspace file against
    either HEAD or index. Now you can only diff against HEAD, so I don't
    understand why you wanted to reset the index. That's not relevant to
    this use case; I'll just accept that resetting the index is a useful
    thing to do here. But I would like to understand why.

- with the "do the right thing" patch, 'git reset' does 'git reset
  --merge' instead

    That "Resets the index and updates the files in the working tree
    that are different between <commit> and HEAD". 

    "<commit>" in this case defaults to HEAD, so the working tree is
    not changed.

So as I understand it, this does _not_ lose your conflict resolutions.

In fact, it now seems that 'git reset --mixed' is always the same as
'git reset --merge'. So I must be missing something!

-- 
-- Stephe

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Matthieu Moy]

Stephen Leake <stephen_leake@stephe-leake.org> writes:

> So as I understand it, this does _not_ lose your conflict resolutions.

Well, then maybe it's time to try the command before continuing
commenting on its behavior ;-).

$ git status
[...]
        both modified:      foo.txt
[...]
$ git diff
diff --cc foo.txt
index 595f399,996c0e1..0000000
--- a/foo.txt
+++ b/foo.txt
@@@ -1,1 -1,1 +1,1 @@@
- content1
 -content2
++resolved
$ git reset --merge
$ git status
On branch master
nothing to commit, working directory clean
$

> In fact, it now seems that 'git reset --mixed' is always the same as
> 'git reset --merge'. So I must be missing something!

"git reset --merge" is an alias for "git merge --abort" (IIRC, it's
actually the other way around). Essentially, it reverts, or tries to
revert everything (worktree and index) as it was before the merge. That
includes throwing away conflict resolution.

Now, I do agree that the documentation of "git reset" is terrible, and I
actually think that the command does too many different things (putting
"git reset" and "git reset --hard" so close to each other is not a good
idead IMHO: the first is a harmless command I use very often, and the
second is one of the most destructive operation Git has).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC 0/3] Make git more user-friendly during a merge conflict

[Stephen Leake]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> $ git status
> On branch master
> nothing to commit, working directory clean
> $

ok, you've lost your conflict resolutions.

>> In fact, it now seems that 'git reset --mixed' is always the same as
>> 'git reset --merge'. So I must be missing something!
>
> "git reset --merge" is an alias for "git merge --abort" (IIRC, it's
> actually the other way around). Essentially, it reverts, or tries to
> revert everything (worktree and index) as it was before the merge. That
> includes throwing away conflict resolution.

Ok.

> Now, I do agree that the documentation of "git reset" is terrible, 

Ok, good.

So is this a sufficient bug report to request that the documentation be
fixed? (I obviously don't know enough to even think about submitting a
patch).

-- 
-- Stephe

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Andrew Wong]

On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Andrew Wong wrote:
>
>> --- a/builtin/merge.c
>> +++ b/builtin/merge.c
>> @@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
>>       fclose(fp);
>>       rerere(allow_rerere_auto);
>>       printf(_("Automatic merge failed; "
>> -                     "fix conflicts and then commit the result.\n"));
>> +                     "fix conflicts and then commit the result.\n"
>> +                     "To abort the merge, use \"git merge --abort\".\n"));
>
> Seems reasonable, but I worry about the command growing too noisy.
>
> Could this be guarded by an advice.<something> setting?  (See advice.*
> in git-config(1) for what I mean.)

I was planning to use advice.resolveConflict, but as I went through
merge.c, I noticed there could be a few other situations where we
could print out the same message:
1. when prepare_to_commit() fails, due to hook error, editor error, or
empty commit message
2. "git commit --no-commit"

This means contexts are no longer only about "resolving conflict", so
I was thinking of renaming advice.resolveConflict to something like
advice.mergeHints.

Any thoughts?

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Junio C Hamano]

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

> On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Andrew Wong wrote:
>>
>>> --- a/builtin/merge.c
>>> +++ b/builtin/merge.c
>>> @@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
>>>       fclose(fp);
>>>       rerere(allow_rerere_auto);
>>>       printf(_("Automatic merge failed; "
>>> -                     "fix conflicts and then commit the result.\n"));
>>> +                     "fix conflicts and then commit the result.\n"
>>> +                     "To abort the merge, use \"git merge --abort\".\n"));
>>
>> Seems reasonable, but I worry about the command growing too noisy.
>>
>> Could this be guarded by an advice.<something> setting?  (See advice.*
>> in git-config(1) for what I mean.)
>
> I was planning to use advice.resolveConflict, but as I went through
> merge.c, I noticed there could be a few other situations where we
> could print out the same message:
> 1. when prepare_to_commit() fails, due to hook error, editor error, or
> empty commit message
> 2. "git commit --no-commit"
>
> This means contexts are no longer only about "resolving conflict", so
> I was thinking of renaming advice.resolveConflict to something like
> advice.mergeHints.
>
> Any thoughts?

I have no strong opinion on the naming, other than that I doubt this
particular new "how to abort" message is worth the headache associated
with the "rename" which involves transition planning of deprecating
the old, supporting both for a while and then removing the old.

The existing message above in "suggest-conflicts" is about hinting
the user to first resolve the conflict before attempting to
continue, and that is perfectly in line with the existing use of
advice.resolveConfict in die_conflict() in git-pull that tells the
user there is an unresolved conflict.

On the other hand, the additional "how to abort" message does not
have to be limited to "you have conflicted paths in the index" case.

If the user said "git merge" while another "git merge" is still
outstanding, we would want to say "You have not concluded your
previous merge" and die, and you presumably want to add the same
"how to abort" message there.  Such a codepath is unlikely to be
covered by existing advice.resolveConflict, and it sounds more
natural, at least to me, to use a separate variable to squelch only
the new "how to abort" part.

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Matthieu Moy]

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

> On Wed, Feb 26, 2014 at 3:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Andrew Wong wrote:
>>
>>> --- a/builtin/merge.c
>>> +++ b/builtin/merge.c
>>> @@ -909,7 +909,8 @@ static int suggest_conflicts(int renormalizing)
>>> +                     "fix conflicts and then commit the result.\n"
>>> +                     "To abort the merge, use \"git merge --abort\".\n"));
[...]
> This means contexts are no longer only about "resolving conflict", so
> I was thinking of renaming advice.resolveConflict to something like
> advice.mergeHints.
>
> Any thoughts?

If the advice suggests merge --abort, then why not advice.abortMerge ?

But mergeHints makes sense to me also, as it would potentially encompass
more cases.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Andrew Wong]

On Wed, Mar 5, 2014 at 1:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> If the user said "git merge" while another "git merge" is still
> outstanding, we would want to say "You have not concluded your
> previous merge" and die, and you presumably want to add the same
> "how to abort" message there.  Such a codepath is unlikely to be
> covered by existing advice.resolveConflict, and it sounds more
> natural, at least to me, to use a separate variable to squelch only
> the new "how to abort" part.

Alright, I don't mind using a separate variable. Though I think it'd
be good to keep the variable name slightly more general though, like
"advice.mergeHints", so that we could use the variable to cover other
merge hints in the future as well. Having one advice config/variable
for every single situation seems a bit overkill, and we would end up
with too many variables.

Re: [RFC 2/3] merge: Add hints to tell users about "git merge --abort"

[Junio C Hamano]

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

> ... merge hints in the future as well.

I actually wish we did not have to add any hints in the first place.

> Having one advice config/variable
> for every single situation seems a bit overkill, and we would end up
> with too many variables.

That goes without saying.

Re: [RFC 3/3] reset: Change the default behavior to use "--merge" during a merge

[Andrew Wong]

On 02/26/14 15:53, Junio C Hamano wrote:
>  - start warning against "reset" (no mode specifier) and "reset --mixed"
>    when the index is unmerged *and* MERGE_HEAD exists; and then
Why do we also want to check if index is unmerged? This situation can
happen regardless of having conflicts or not (--no-commit, aborting the
editor, etc.). As long as the user is in the middle of a merge
(MERGE_HEAD exists), shouldn't they be warned if they used "git reset"?