threads / patch / 44368

patchreset: --unmerge

Subject: [PATCH] reset: --unmerge

## tl;dr

9 messages between Oct 24, 2016 and Oct 27, 2016. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Junio C Hamano· Oct 24, 2016, 23:10 UTC · lore
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(-)
Show changes to 2 files +43 −1

builtin/reset.c, t/t7107-reset-unmerged.sh

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
Duy Nguyen· Oct 25, 2016, 11:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] reset: --unmerge

On Tue, Oct 25, 2016 at 6:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> 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.

Show 5 quoted lines
> 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
Duy Nguyen· Oct 25, 2016, 11:11 UTC · re: Duy Nguyen · lore

Re: [PATCH] reset: --unmerge

On Tue, Oct 25, 2016 at 6:06 PM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 14 quoted lines
> 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
Junio C Hamano· Oct 25, 2016, 23:28 UTC · re: Duy Nguyen · lore

Re: [PATCH] reset: --unmerge

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.

Duy Nguyen· Oct 26, 2016, 09:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] reset: --unmerge

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.
Show 5 quoted lines
>  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
Junio C Hamano· Oct 26, 2016, 17:06 UTC · re: Duy Nguyen · lore

Re: [PATCH] reset: --unmerge

Duy Nguyen <pclouds@gmail.com> writes:
Show 12 quoted lines
> 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.

Show 5 quoted lines
> 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.

Andreas Schwab· Oct 27, 2016, 16:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] reset: --unmerge

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."
Junio C Hamano· Oct 27, 2016, 16:23 UTC · re: Andreas Schwab · lore

Re: [PATCH] reset: --unmerge

Andreas Schwab <schwab@linux-m68k.org> writes:
Show 7 quoted lines
> 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"?

Sergey Organov· Oct 25, 2016, 17:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] reset: --unmerge

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> 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

← back to recent threads