[PATCH] reset: --unmerge

[PATCH] reset: --unmerge

[Junio C Hamano]

The procedure to resolve a merge conflict typically goes like this:

 - first open the file in the editor, and with the help of conflict
   markers come up with a resolution.

 - save the file.

 - look at the output from "git diff" to see the combined diff to
   double check if the resolution makes sense.

 - perform other tests, like trying to build the result with "make".

 - finally "git add file" to mark that you are done.

and repeating the above until you are done with all the conflicted
paths.  If you, for whatever reason, accidentally "git add file" by
mistake until you are convinced that you resolved it correctly (e.g.
doing "git add file" immediately after saving, without a chance to
peruse the output from "git diff"), there is no good way to recover.
There is "git checkout -m file" but that overwrites the working tree
file to reproduce the conflicted state, which is not exactly what
you want.  You only want to reproduce the conflicted state in the
index, so that you can inspect the (proposed) merge resolution you
already have in your working tree.

Add "git reset --unmerge <paths>" command that does just that.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Not thought through thoroughly yet about details, such as "should
   it always require at least one pathspec?".

 builtin/reset.c           |  8 +++++++-
 t/t7107-reset-unmerged.sh | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/builtin/reset.c b/builtin/reset.c
index 9020ec66c8..3aa9e0b34a 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -21,6 +21,7 @@
 #include "parse-options.h"
 #include "unpack-trees.h"
 #include "cache-tree.h"
+#include "resolve-undo.h"
 
 static const char * const git_reset_usage[] = {
 	N_("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]"),
@@ -272,6 +273,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	struct object_id oid;
 	struct pathspec pathspec;
 	int intent_to_add = 0;
+	int unmerge = 0;
 	const struct option options[] = {
 		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
 		OPT_SET_INT(0, "mixed", &reset_type,
@@ -286,6 +288,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('p', "patch", &patch_mode, N_("select hunks interactively")),
 		OPT_BOOL('N', "intent-to-add", &intent_to_add,
 				N_("record only the fact that removed paths will be added later")),
+		OPT_BOOL(0, "unmerge", &unmerge,
+			 N_("recover conflicted stages from an earlier 'git add'")),
 		OPT_END()
 	};
 
@@ -357,7 +361,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 		hold_locked_index(lock, 1);
 		if (reset_type == MIXED) {
 			int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
-			if (read_from_tree(&pathspec, oid.hash, intent_to_add))
+			if (unmerge)
+				unmerge_cache(&pathspec);
+			else if (read_from_tree(&pathspec, oid.hash, intent_to_add))
 				return 1;
 			if (get_git_work_tree())
 				refresh_index(&the_index, flags, NULL, NULL,
diff --git a/t/t7107-reset-unmerged.sh b/t/t7107-reset-unmerged.sh
new file mode 100755
index 0000000000..57b2e27150
--- /dev/null
+++ b/t/t7107-reset-unmerged.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='git reset with paths'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo one >file &&
+	git add file &&
+	git commit -m "one" &&
+	git tag initial &&
+
+	echo two >file &&
+	git commit -a -m "two" &&
+
+	git checkout -b side initial &&
+	echo three >file &&
+	git commit -a -m "three"
+'
+
+test_expect_success "cause conflict, resolve, and unresolve" '
+	git reset --hard &&
+	git checkout master &&
+	test_must_fail git merge side &&
+
+	git ls-files -u >expect &&
+
+	echo four >file &&
+	git add file &&
+
+	git reset --unmerge -- file &&
+	git ls-files -u >actual &&
+	test_cmp expect actual
+'
+
+test_done

Re: [PATCH] reset: --unmerge

[Duy Nguyen]

On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The procedure to resolve a merge conflict typically goes like this:
>
>  - first open the file in the editor, and with the help of conflict
>    markers come up with a resolution.
>
>  - save the file.
>
>  - look at the output from "git diff" to see the combined diff to
>    double check if the resolution makes sense.
>
>  - perform other tests, like trying to build the result with "make".
>
>  - finally "git add file" to mark that you are done.
>
> and repeating the above until you are done with all the conflicted
> paths.  If you, for whatever reason, accidentally "git add file" by
> mistake until you are convinced that you resolved it correctly (e.g.
> doing "git add file" immediately after saving, without a chance to
> peruse the output from "git diff"), there is no good way to recover.

I made this exact mistake on a giant, half-resolved merge conflict the
other day and panicked. While I prepared myself to script update-index
to restore stages 2 and 3, I found "update-index --unresolve". It
sounds like this "reset --unmerge" is the same functionality, right?
I'm not objecting this patch though, update-index is not something a
casual user should use.

> There is "git checkout -m file" but that overwrites the working tree
> file to reproduce the conflicted state, which is not exactly what
> you want.  You only want to reproduce the conflicted state in the
> index, so that you can inspect the (proposed) merge resolution you
> already have in your working tree.
-- 
Duy

Re: [PATCH] reset: --unmerge

[Duy Nguyen]

On Tue, Oct 25, 2016 at 6:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> The procedure to resolve a merge conflict typically goes like this:
>>
>>  - first open the file in the editor, and with the help of conflict
>>    markers come up with a resolution.
>>
>>  - save the file.
>>
>>  - look at the output from "git diff" to see the combined diff to
>>    double check if the resolution makes sense.
>>
>>  - perform other tests, like trying to build the result with "make".
>>
>>  - finally "git add file" to mark that you are done.

BTW making git-add (and "git commit -a") refuse files with conflict
markers present could prevent this mistake earlier and is probably a
better option because the user won't have to discover 'reset
--unmerge'. If the user likes to add the file with conflict markers
anyway (because those look like conflict markers but are in fact not)
then they can go with "git add -f" or similar.
-- 
Duy

Re: [PATCH] reset: --unmerge

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> BTW making git-add (and "git commit -a") refuse files with conflict
> markers present could prevent this mistake earlier and is probably a
> better option because the user won't have to discover 'reset
> --unmerge'.

That may help some users, but you are solving a different problem.

I do not say "save" unless I know the editor buffer contents is not
ready.  "This is ready to be saved" however is different from "This
resolution is correct", and I need the unmerged states in the index
to verify, namely by looking at "git diff" (no other parameters)
output that shows only the paths with unmerged stages and in the
compact combined diff format.

Somebody with a bright idea decided that vc-git-resolve-conflicts
variable should be on by default in Emacs 25.1 X-<, which causes
"save" after resolving conflicts to automatically run "git add", to
destroy this valuable tool.  My knee-jerk reaction, of course, to
such a default is "that's brain dead", but perhaps they did so for
some good reason that I fail to fathom.

Re: [PATCH] reset: --unmerge

[Duy Nguyen]

On Wed, Oct 26, 2016 at 6:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Somebody with a bright idea decided that vc-git-resolve-conflicts
> variable should be on by default in Emacs 25.1 X-<,

Oh good, I have an excuse to stick to 24.5.1 for a while longer then.

>  which causes
> "save" after resolving conflicts to automatically run "git add", to
> destroy this valuable tool.  My knee-jerk reaction, of course, to
> such a default is "that's brain dead", but perhaps they did so for
> some good reason that I fail to fathom.

I was curious. The default is t since the variable's introduction [1].
Interestingly the thread/bug that resulted in that commit started with
"report this bug to git" [2]. Something about git-stash. I quote the
original mail here in case anyone wants to look into it (not sure if
it's actually reported here before, I don't pay much attention to
git-stash mails)

-- 8< --
Bad news, everyone!

When a stash contains changes for several files, and "stash pop"
encounters conflicts only in some of them, the rest of the files are
stages automatically.

At least, that happens with Git 2.1.0 on my machine, and some
commenters here: http://stackoverflow.com/a/1237337/615245

So then when we unstage the files which had conflicts after resolving
those, the result is mixed. Which doesn't look right.

What shall we do? Unstage the automatically-staged files? Revert the
changes from this bug? It seems Git really wants the changes staged
after the conflict resolution.
-- 8< --

[1] http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=45651154473c7d2f16230da765d034ecfde7968a
[2] https://lists.gnu.org/archive/html/bug-gnu-emacs/2015-05/msg00433.html
-- 
Duy

Re: [PATCH] reset: --unmerge

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Interestingly the thread/bug that resulted in that commit started with
> "report this bug to git" [2]. Something about git-stash. I quote the
> original mail here in case anyone wants to look into it (not sure if
> it's actually reported here before, I don't pay much attention to
> git-stash mails)
>
> -- 8< --
> Bad news, everyone!
>
> When a stash contains changes for several files, and "stash pop"
> encounters conflicts only in some of them, the rest of the files are
> stages automatically.

It indeed is curious.

That is the designed behaviour for _ANY_ mergy operation, and not
limited to "stash pop".

A clean application is added to the index so that you can find out
about them from "diff --cached", while conflicted ones keep their
unmerged stages so that the conflict can be resolved in the working
tree files.  There is no bad news here.

Once you resolve the conflict, you would add the final contents to
the working tree, but as anybody who knows how "git diff" after
resolving conflicts in the working tree files is useful would know,
"saving the editor buffer after removing conflict markers" is not a
valid signal that the user is confident that the contents is final.

> At least, that happens with Git 2.1.0 on my machine, and some
> commenters here: http://stackoverflow.com/a/1237337/615245
>
> So then when we unstage the files which had conflicts after resolving
> those, the result is mixed. Which doesn't look right.

Whoever wrote this does not understand how mergy operations in Git
works, I guess.

> What shall we do? Unstage the automatically-staged files? Revert the
> changes from this bug? It seems Git really wants the changes staged
> after the conflict resolution.

The first order of business is to learn how mergy operations in Git
is designed to work, and if they are in the business of building a
tool around Git to make the life of users better, avoid going against
the designed workflow.

If this "Bad news, everyone!" is why vc-git-resolve-conflicts was
added and defaults to true, I can feel safe in toggling it off
forever in my ~/.emacs, knowing that it is a totally broken option
that came from a desire to fix a problem that does not exist.

Re: [PATCH] reset: --unmerge

[Andreas Schwab]

On Okt 25 2016, Junio C Hamano <gitster@pobox.com> wrote:

> Somebody with a bright idea decided that vc-git-resolve-conflicts
> variable should be on by default in Emacs 25.1 X-<

This is consistent with the behaviour of the other VC backends, where it
isn't even customizable.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] reset: --unmerge

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Okt 25 2016, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Somebody with a bright idea decided that vc-git-resolve-conflicts
>> variable should be on by default in Emacs 25.1 X-<
>
> This is consistent with the behaviour of the other VC backends, where it
> isn't even customizable.

The problem I had was "M-x save-buffer" (after resolving the
conflicts in it manually) running "git add" on the resulting file,
robbing from "git diff" an opportunity to help the user to see how
the result looks relative to both branches.

Do you mean that VC mode broke the same feature equally other SCMs,
too?  Do other SCM supported by VC backends take advantage of
unmerged stages in the index (until you say "$scm add") by allowing
you to verify the combined diff with "$scm diff"?

Re: [PATCH] reset: --unmerge

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> The procedure to resolve a merge conflict typically goes like this:
>
>  - first open the file in the editor, and with the help of conflict
>    markers come up with a resolution.
>
>  - save the file.
>
>  - look at the output from "git diff" to see the combined diff to
>    double check if the resolution makes sense.
>
>  - perform other tests, like trying to build the result with "make".
>
>  - finally "git add file" to mark that you are done.
>
> and repeating the above until you are done with all the conflicted
> paths.  If you, for whatever reason, accidentally "git add file" by
> mistake until you are convinced that you resolved it correctly (e.g.
> doing "git add file" immediately after saving, without a chance to
> peruse the output from "git diff"), there is no good way to recover.

"git reset --unmerge file"

to undo accidental

"git add file"

during conflict resolution?

I'm afraid "unmerge" sounds like revert of "merge", rather than revert
of "resolve". I'd rather prefer to see something like:

git add --undo file

git merge --unresolve file

git reset --unresolve file

in that order, to deal with the issue.

-- Sergey