threads / patch / 51020

patch, 2 partsAdd "git merge --quit"

Subject: [PATCH 0/2] Add "git merge --quit"

## tl;dr

20 messages between May 1, 2019 and May 20, 2019. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Nguyễn Thái Ngọc Duy· May 1, 2019, 13:11 UTC · lore

nd/switch-and-restore suggests 'git merge --quit' to get out of a merge even though this option is not implemented [1]. It's a soft dependency, no actual functionality is broken by the lack of --quit, so I'm sending it separately.

[1] https://public-inbox.org/git/78c7c281-82ec-2ba9-a607-dd2ecba54945@gmail.com/
Nguyễn Thái Ngọc Duy (2):
  merge: remove drop_save() in favor of remove_merge_branch_state()
  merge: add --quit
 Documentation/git-merge.txt |  4 ++++
 branch.c                    | 11 ++++++++---
 branch.h                    |  6 ++++++
 builtin/merge.c             | 30 ++++++++++++++++++------------
 t/t7600-merge.sh            | 14 ++++++++++++++
 5 files changed, 50 insertions(+), 15 deletions(-)
-- 
2.21.0.1110.g9614c01b33
Nguyễn Thái Ngọc Duy· May 1, 2019, 13:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 1/2] merge: remove drop_save() in favor of remove_merge_branch_state()

Both remove_branch_state() and drop_save() delete almost the same set of files about the current merge state. The only difference is MERGE_RR but it should also be cleaned up after a successful merge, which is what drop_save() is for.

Make a new function that deletes all merge-related state files and use it instead of drop_save(). This function will also be used in the next patch that introduces --quit.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c        | 11 ++++++++---
 branch.h        |  6 ++++++
 builtin/merge.c | 17 +++++------------
 3 files changed, 19 insertions(+), 15 deletions(-)
Show changes to 3 files +19 −15

branch.c, branch.h, builtin/merge.c

diff --git a/branch.c b/branch.c
index 28b81a7e02..1db0601a11 100644
--- a/branch.c
+++ b/branch.c
@@ -337,15 +337,20 @@ void create_branch(struct repository *r,
 	free(real_ref);
 }
 
-void remove_branch_state(struct repository *r)
+void remove_merge_branch_state(struct repository *r)
 {
-	unlink(git_path_cherry_pick_head(r));
-	unlink(git_path_revert_head(r));
 	unlink(git_path_merge_head(r));
 	unlink(git_path_merge_rr(r));
 	unlink(git_path_merge_msg(r));
 	unlink(git_path_merge_mode(r));
+}
+
+void remove_branch_state(struct repository *r)
+{
+	unlink(git_path_cherry_pick_head(r));
+	unlink(git_path_revert_head(r));
 	unlink(git_path_squash_msg(r));
+	remove_merge_branch_state(r);
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
diff --git a/branch.h b/branch.h
index 29c1afa4d0..c90ba9d7bf 100644
--- a/branch.h
+++ b/branch.h
@@ -60,6 +60,12 @@ extern int validate_branchname(const char *name, struct strbuf *ref);
  */
 extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
 
+/*
+ * Remove information about the merge state on the current
+ * branch. (E.g., MERGE_HEAD)
+ */
+void remove_merge_branch_state(struct repository *r);
+
 /*
  * Remove information about the state of working on the current
  * branch. (E.g., MERGE_HEAD)
diff --git a/builtin/merge.c b/builtin/merge.c
index 5ce8946d39..0fd448b403 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -37,6 +37,7 @@
 #include "packfile.h"
 #include "tag.h"
 #include "alias.h"
+#include "branch.h"
 #include "commit-reach.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
@@ -282,14 +283,6 @@ static struct option builtin_merge_options[] = {
 	OPT_END()
 };
 
-/* Cleans up metadata that is uninteresting after a succeeded merge. */
-static void drop_save(void)
-{
-	unlink(git_path_merge_head(the_repository));
-	unlink(git_path_merge_msg(the_repository));
-	unlink(git_path_merge_mode(the_repository));
-}
-
 static int save_state(struct object_id *stash)
 {
 	int len;
@@ -383,7 +376,7 @@ static void finish_up_to_date(const char *msg)
 {
 	if (verbosity >= 0)
 		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 }
 
 static void squash_message(struct commit *commit, struct commit_list *remoteheads)
@@ -861,7 +854,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 			&result_commit, NULL, sign_commit))
 		die(_("failed to write commit object"));
 	finish(head, remoteheads, &result_commit, "In-index merge");
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -888,7 +881,7 @@ static int finish_automerge(struct commit *head,
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(head, remoteheads, &result_commit, buf.buf);
 	strbuf_release(&buf);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -1466,7 +1459,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 
 		finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
-		drop_save();
+		remove_merge_branch_state(the_repository);
 		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
-- 
2.21.0.1110.g9614c01b33
Nguyễn Thái Ngọc Duy· May 1, 2019, 13:11 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH 2/2] merge: add --quit

This allows to cancel the current merge without reseting worktree/index, which is what --abort is for. Like other --quit(s), this is often used when you forgot that you're in the middle of a merge and already switched away, doing different things. By the time you're realize, you can't even continue the merge anymore.

This also makes all in-progress commands, am, merge, rebase, revert and cherry-pick, take all three --abort, --continue and --quit (bisect has a different UI).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-merge.txt |  4 ++++
 builtin/merge.c             | 13 +++++++++++++
 t/t7600-merge.sh            | 14 ++++++++++++++
 3 files changed, 31 insertions(+)
Show changes to 3 files +31 −0

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 6294dbc09d..c01cfa6595 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -100,6 +100,10 @@ commit or stash your changes before running 'git merge'.
 'git merge --abort' is equivalent to 'git reset --merge' when
 `MERGE_HEAD` is present.
 
+--quit::
+	Forget about the current merge in progress. Leave the index
+	and the working tree as-is.
+
 --continue::
 	After a 'git merge' stops due to conflicts you can conclude the
 	merge by running 'git merge --continue' (see "HOW TO RESOLVE
diff --git a/builtin/merge.c b/builtin/merge.c
index 0fd448b403..13392ba1cf 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
+static int quit_current_merge;
 static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
@@ -270,6 +271,8 @@ static struct option builtin_merge_options[] = {
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),
+	OPT_BOOL(0, "quit", &quit_current_merge,
+		N_("--abort but leave index and working tree alone")),
 	OPT_BOOL(0, "continue", &continue_current_merge,
 		N_("continue the current in-progress merge")),
 	OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
@@ -1255,6 +1258,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		goto done;
 	}
 
+	if (quit_current_merge) {
+		if (orig_argc != 2)
+			usage_msg_opt(_("--quit expects no arguments"),
+				      builtin_merge_usage,
+				      builtin_merge_options);
+
+		remove_merge_branch_state(the_repository);
+		goto done;
+	}
+
 	if (continue_current_merge) {
 		int nargc = 1;
 		const char *nargv[] = {"commit", NULL};
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 106148254d..ea82cb744b 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -822,4 +822,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
 	verify_parents $c0 $c1
 '
 
+test_expect_success 'merge --quit' '
+	git reset --hard side &&
+	test_must_fail git -c rerere.enabled=true merge master &&
+	test_path_is_file .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_MODE &&
+	test_path_is_file .git/MERGE_MSG &&
+	test_path_is_file .git/MERGE_RR &&
+	git merge --quit &&
+	test_path_is_missing .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_MODE &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/MERGE_RR
+'
+
 test_done
-- 
2.21.0.1110.g9614c01b33
Emily Shaffer· May 2, 2019, 21:49 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 2/2] merge: add --quit

They both look fine to me, besides a couple typos in the commit message in this one.

On Wed, May 01, 2019 at 08:11:52PM +0700, Nguyễn Thái Ngọc Duy wrote:
> This allows to cancel the current merge without reseting worktree/index,
"resetting".
> which is what --abort is for. Like other --quit(s), this is often used
> when you forgot that you're in the middle of a merge and already
> switched away, doing different things. By the time you're realize, you
"By the time you've realized" ?
> can't even continue the merge anymore.

Thanks, Emily

Phillip Wood· May 2, 2019, 10:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH 0/2] Add "git merge --quit"

Hi Duy
On 01/05/2019 14:11, Nguyễn Thái Ngọc Duy wrote:
> nd/switch-and-restore suggests 'git merge --quit' to get out of a merge
> even though this option is not implemented [1]. It's a soft dependency, no
> actual functionality is broken by the lack of --quit, so I'm sending
> it separately.
Both patches look good to me
Best Wishes
Phillip
Show 14 quoted lines
> 
> [1] https://public-inbox.org/git/78c7c281-82ec-2ba9-a607-dd2ecba54945@gmail.com/
> 
> Nguyễn Thái Ngọc Duy (2):
>   merge: remove drop_save() in favor of remove_merge_branch_state()
>   merge: add --quit
> 
>  Documentation/git-merge.txt |  4 ++++
>  branch.c                    | 11 ++++++++---
>  branch.h                    |  6 ++++++
>  builtin/merge.c             | 30 ++++++++++++++++++------------
>  t/t7600-merge.sh            | 14 ++++++++++++++
>  5 files changed, 50 insertions(+), 15 deletions(-)
> 
Nguyễn Thái Ngọc Duy· May 9, 2019, 10:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 0/2] nd/merge-quit update

A couple typos in the commit message. No code change.
Nguyễn Thái Ngọc Duy (2):
  merge: remove drop_save() in favor of remove_merge_branch_state()
  merge: add --quit
 Documentation/git-merge.txt |  4 ++++
 branch.c                    | 11 ++++++++---
 branch.h                    |  6 ++++++
 builtin/merge.c             | 30 ++++++++++++++++++------------
 t/t7600-merge.sh            | 14 ++++++++++++++
 5 files changed, 50 insertions(+), 15 deletions(-)
Range-diff dựa trên v1:
1:  a87e56a43a ! 1:  51710c4c6c merge: add --quit
    @@ -2,10 +2,10 @@
     
         merge: add --quit
     
    -    This allows to cancel the current merge without reseting worktree/index,
    +    This allows to cancel the current merge without resetting worktree/index,
         which is what --abort is for. Like other --quit(s), this is often used
         when you forgot that you're in the middle of a merge and already
    -    switched away, doing different things. By the time you're realize, you
    +    switched away, doing different things. By the time you've realized, you
         can't even continue the merge anymore.
     
         This also makes all in-progress commands, am, merge, rebase, revert and
-- 
2.21.0.1141.gd54ac2cb17
Nguyễn Thái Ngọc Duy· May 9, 2019, 10:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 1/2] merge: remove drop_save() in favor of remove_merge_branch_state()

Both remove_branch_state() and drop_save() delete almost the same set of files about the current merge state. The only difference is MERGE_RR but it should also be cleaned up after a successful merge, which is what drop_save() is for.

Make a new function that deletes all merge-related state files and use it instead of drop_save(). This function will also be used in the next patch that introduces --quit.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c        | 11 ++++++++---
 branch.h        |  6 ++++++
 builtin/merge.c | 17 +++++------------
 3 files changed, 19 insertions(+), 15 deletions(-)
Show changes to 3 files +19 −15

branch.c, branch.h, builtin/merge.c

diff --git a/branch.c b/branch.c
index 28b81a7e02..1db0601a11 100644
--- a/branch.c
+++ b/branch.c
@@ -337,15 +337,20 @@ void create_branch(struct repository *r,
 	free(real_ref);
 }
 
-void remove_branch_state(struct repository *r)
+void remove_merge_branch_state(struct repository *r)
 {
-	unlink(git_path_cherry_pick_head(r));
-	unlink(git_path_revert_head(r));
 	unlink(git_path_merge_head(r));
 	unlink(git_path_merge_rr(r));
 	unlink(git_path_merge_msg(r));
 	unlink(git_path_merge_mode(r));
+}
+
+void remove_branch_state(struct repository *r)
+{
+	unlink(git_path_cherry_pick_head(r));
+	unlink(git_path_revert_head(r));
 	unlink(git_path_squash_msg(r));
+	remove_merge_branch_state(r);
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
diff --git a/branch.h b/branch.h
index 29c1afa4d0..c90ba9d7bf 100644
--- a/branch.h
+++ b/branch.h
@@ -60,6 +60,12 @@ extern int validate_branchname(const char *name, struct strbuf *ref);
  */
 extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
 
+/*
+ * Remove information about the merge state on the current
+ * branch. (E.g., MERGE_HEAD)
+ */
+void remove_merge_branch_state(struct repository *r);
+
 /*
  * Remove information about the state of working on the current
  * branch. (E.g., MERGE_HEAD)
diff --git a/builtin/merge.c b/builtin/merge.c
index e47d77baee..e9663f027a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -37,6 +37,7 @@
 #include "packfile.h"
 #include "tag.h"
 #include "alias.h"
+#include "branch.h"
 #include "commit-reach.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
@@ -279,14 +280,6 @@ static struct option builtin_merge_options[] = {
 	OPT_END()
 };
 
-/* Cleans up metadata that is uninteresting after a succeeded merge. */
-static void drop_save(void)
-{
-	unlink(git_path_merge_head(the_repository));
-	unlink(git_path_merge_msg(the_repository));
-	unlink(git_path_merge_mode(the_repository));
-}
-
 static int save_state(struct object_id *stash)
 {
 	int len;
@@ -380,7 +373,7 @@ static void finish_up_to_date(const char *msg)
 {
 	if (verbosity >= 0)
 		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 }
 
 static void squash_message(struct commit *commit, struct commit_list *remoteheads)
@@ -858,7 +851,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 			&result_commit, NULL, sign_commit))
 		die(_("failed to write commit object"));
 	finish(head, remoteheads, &result_commit, "In-index merge");
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -885,7 +878,7 @@ static int finish_automerge(struct commit *head,
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(head, remoteheads, &result_commit, buf.buf);
 	strbuf_release(&buf);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -1463,7 +1456,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 
 		finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
-		drop_save();
+		remove_merge_branch_state(the_repository);
 		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
-- 
2.21.0.1141.gd54ac2cb17
Nguyễn Thái Ngọc Duy· May 9, 2019, 10:10 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v2 2/2] merge: add --quit

This allows to cancel the current merge without resetting worktree/index, which is what --abort is for. Like other --quit(s), this is often used when you forgot that you're in the middle of a merge and already switched away, doing different things. By the time you've realized, you can't even continue the merge anymore.

This also makes all in-progress commands, am, merge, rebase, revert and cherry-pick, take all three --abort, --continue and --quit (bisect has a different UI).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-merge.txt |  4 ++++
 builtin/merge.c             | 13 +++++++++++++
 t/t7600-merge.sh            | 14 ++++++++++++++
 3 files changed, 31 insertions(+)
Show changes to 3 files +31 −0

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4cc86469f3..b7d581fc76 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -99,6 +99,10 @@ commit or stash your changes before running 'git merge'.
 'git merge --abort' is equivalent to 'git reset --merge' when
 `MERGE_HEAD` is present.
 
+--quit::
+	Forget about the current merge in progress. Leave the index
+	and the working tree as-is.
+
 --continue::
 	After a 'git merge' stops due to conflicts you can conclude the
 	merge by running 'git merge --continue' (see "HOW TO RESOLVE
diff --git a/builtin/merge.c b/builtin/merge.c
index e9663f027a..598d56edfe 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
+static int quit_current_merge;
 static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
@@ -267,6 +268,8 @@ static struct option builtin_merge_options[] = {
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),
+	OPT_BOOL(0, "quit", &quit_current_merge,
+		N_("--abort but leave index and working tree alone")),
 	OPT_BOOL(0, "continue", &continue_current_merge,
 		N_("continue the current in-progress merge")),
 	OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
@@ -1252,6 +1255,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		goto done;
 	}
 
+	if (quit_current_merge) {
+		if (orig_argc != 2)
+			usage_msg_opt(_("--quit expects no arguments"),
+				      builtin_merge_usage,
+				      builtin_merge_options);
+
+		remove_merge_branch_state(the_repository);
+		goto done;
+	}
+
 	if (continue_current_merge) {
 		int nargc = 1;
 		const char *nargv[] = {"commit", NULL};
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 106148254d..ea82cb744b 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -822,4 +822,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
 	verify_parents $c0 $c1
 '
 
+test_expect_success 'merge --quit' '
+	git reset --hard side &&
+	test_must_fail git -c rerere.enabled=true merge master &&
+	test_path_is_file .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_MODE &&
+	test_path_is_file .git/MERGE_MSG &&
+	test_path_is_file .git/MERGE_RR &&
+	git merge --quit &&
+	test_path_is_missing .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_MODE &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/MERGE_RR
+'
+
 test_done
-- 
2.21.0.1141.gd54ac2cb17
Nguyễn Thái Ngọc Duy· May 14, 2019, 09:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 0/2] nd/merge-quit updates

v3 fixes the test breakage when GPG tests are skipped ('side' branch is affected by these skipped tests)

Nguyễn Thái Ngọc Duy (2):
  merge: remove drop_save() in favor of remove_merge_branch_state()
  merge: add --quit
 Documentation/git-merge.txt |  4 ++++
 branch.c                    | 11 ++++++++---
 branch.h                    |  6 ++++++
 builtin/merge.c             | 30 ++++++++++++++++++------------
 t/t7600-merge.sh            | 14 ++++++++++++++
 5 files changed, 50 insertions(+), 15 deletions(-)
Range-diff dựa trên v2:
1:  324d237f0c ! 1:  86dd0fd99c merge: add --quit
    @@ -13,7 +13,6 @@
         different UI).
     
         Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
    -    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
      --- a/Documentation/git-merge.txt
    @@ -76,7 +75,7 @@
      '
      
     +test_expect_success 'merge --quit' '
    -+	git reset --hard side &&
    ++	git reset --hard c2 &&
     +	test_must_fail git -c rerere.enabled=true merge master &&
     +	test_path_is_file .git/MERGE_HEAD &&
     +	test_path_is_file .git/MERGE_MODE &&
-- 
2.21.0.1141.gd54ac2cb17
Nguyễn Thái Ngọc Duy· May 14, 2019, 09:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 1/2] merge: remove drop_save() in favor of remove_merge_branch_state()

Both remove_branch_state() and drop_save() delete almost the same set of files about the current merge state. The only difference is MERGE_RR but it should also be cleaned up after a successful merge, which is what drop_save() is for.

Make a new function that deletes all merge-related state files and use it instead of drop_save(). This function will also be used in the next patch that introduces --quit.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c        | 11 ++++++++---
 branch.h        |  6 ++++++
 builtin/merge.c | 17 +++++------------
 3 files changed, 19 insertions(+), 15 deletions(-)
Show changes to 3 files +19 −15

branch.c, branch.h, builtin/merge.c

diff --git a/branch.c b/branch.c
index 28b81a7e02..1db0601a11 100644
--- a/branch.c
+++ b/branch.c
@@ -337,15 +337,20 @@ void create_branch(struct repository *r,
 	free(real_ref);
 }
 
-void remove_branch_state(struct repository *r)
+void remove_merge_branch_state(struct repository *r)
 {
-	unlink(git_path_cherry_pick_head(r));
-	unlink(git_path_revert_head(r));
 	unlink(git_path_merge_head(r));
 	unlink(git_path_merge_rr(r));
 	unlink(git_path_merge_msg(r));
 	unlink(git_path_merge_mode(r));
+}
+
+void remove_branch_state(struct repository *r)
+{
+	unlink(git_path_cherry_pick_head(r));
+	unlink(git_path_revert_head(r));
 	unlink(git_path_squash_msg(r));
+	remove_merge_branch_state(r);
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
diff --git a/branch.h b/branch.h
index 29c1afa4d0..c90ba9d7bf 100644
--- a/branch.h
+++ b/branch.h
@@ -60,6 +60,12 @@ extern int validate_branchname(const char *name, struct strbuf *ref);
  */
 extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
 
+/*
+ * Remove information about the merge state on the current
+ * branch. (E.g., MERGE_HEAD)
+ */
+void remove_merge_branch_state(struct repository *r);
+
 /*
  * Remove information about the state of working on the current
  * branch. (E.g., MERGE_HEAD)
diff --git a/builtin/merge.c b/builtin/merge.c
index e47d77baee..e9663f027a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -37,6 +37,7 @@
 #include "packfile.h"
 #include "tag.h"
 #include "alias.h"
+#include "branch.h"
 #include "commit-reach.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
@@ -279,14 +280,6 @@ static struct option builtin_merge_options[] = {
 	OPT_END()
 };
 
-/* Cleans up metadata that is uninteresting after a succeeded merge. */
-static void drop_save(void)
-{
-	unlink(git_path_merge_head(the_repository));
-	unlink(git_path_merge_msg(the_repository));
-	unlink(git_path_merge_mode(the_repository));
-}
-
 static int save_state(struct object_id *stash)
 {
 	int len;
@@ -380,7 +373,7 @@ static void finish_up_to_date(const char *msg)
 {
 	if (verbosity >= 0)
 		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 }
 
 static void squash_message(struct commit *commit, struct commit_list *remoteheads)
@@ -858,7 +851,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 			&result_commit, NULL, sign_commit))
 		die(_("failed to write commit object"));
 	finish(head, remoteheads, &result_commit, "In-index merge");
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -885,7 +878,7 @@ static int finish_automerge(struct commit *head,
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(head, remoteheads, &result_commit, buf.buf);
 	strbuf_release(&buf);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -1463,7 +1456,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 
 		finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
-		drop_save();
+		remove_merge_branch_state(the_repository);
 		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
-- 
2.21.0.1141.gd54ac2cb17
Nguyễn Thái Ngọc Duy· May 14, 2019, 09:13 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v3 2/2] merge: add --quit

This allows to cancel the current merge without resetting worktree/index, which is what --abort is for. Like other --quit(s), this is often used when you forgot that you're in the middle of a merge and already switched away, doing different things. By the time you've realized, you can't even continue the merge anymore.

This also makes all in-progress commands, am, merge, rebase, revert and cherry-pick, take all three --abort, --continue and --quit (bisect has a different UI).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-merge.txt |  4 ++++
 builtin/merge.c             | 13 +++++++++++++
 t/t7600-merge.sh            | 14 ++++++++++++++
 3 files changed, 31 insertions(+)
Show changes to 3 files +31 −0

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4cc86469f3..b7d581fc76 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -99,6 +99,10 @@ commit or stash your changes before running 'git merge'.
 'git merge --abort' is equivalent to 'git reset --merge' when
 `MERGE_HEAD` is present.
 
+--quit::
+	Forget about the current merge in progress. Leave the index
+	and the working tree as-is.
+
 --continue::
 	After a 'git merge' stops due to conflicts you can conclude the
 	merge by running 'git merge --continue' (see "HOW TO RESOLVE
diff --git a/builtin/merge.c b/builtin/merge.c
index e9663f027a..598d56edfe 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
+static int quit_current_merge;
 static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
@@ -267,6 +268,8 @@ static struct option builtin_merge_options[] = {
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),
+	OPT_BOOL(0, "quit", &quit_current_merge,
+		N_("--abort but leave index and working tree alone")),
 	OPT_BOOL(0, "continue", &continue_current_merge,
 		N_("continue the current in-progress merge")),
 	OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
@@ -1252,6 +1255,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		goto done;
 	}
 
+	if (quit_current_merge) {
+		if (orig_argc != 2)
+			usage_msg_opt(_("--quit expects no arguments"),
+				      builtin_merge_usage,
+				      builtin_merge_options);
+
+		remove_merge_branch_state(the_repository);
+		goto done;
+	}
+
 	if (continue_current_merge) {
 		int nargc = 1;
 		const char *nargv[] = {"commit", NULL};
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 106148254d..d453710ef6 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -822,4 +822,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
 	verify_parents $c0 $c1
 '
 
+test_expect_success 'merge --quit' '
+	git reset --hard c2 &&
+	test_must_fail git -c rerere.enabled=true merge master &&
+	test_path_is_file .git/MERGE_HEAD &&
+	test_path_is_file .git/MERGE_MODE &&
+	test_path_is_file .git/MERGE_MSG &&
+	test_path_is_file .git/MERGE_RR &&
+	git merge --quit &&
+	test_path_is_missing .git/MERGE_HEAD &&
+	test_path_is_missing .git/MERGE_MODE &&
+	test_path_is_missing .git/MERGE_MSG &&
+	test_path_is_missing .git/MERGE_RR
+'
+
 test_done
-- 
2.21.0.1141.gd54ac2cb17
Johannes Schindelin· May 14, 2019, 13:44 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v3 2/2] merge: add --quit

Hi Duy,
On Tue, 14 May 2019, Nguyễn Thái Ngọc Duy wrote:
Show 11 quoted lines
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 106148254d..d453710ef6 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -822,4 +822,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
>  	verify_parents $c0 $c1
>  '
>
> +test_expect_success 'merge --quit' '
> +	git reset --hard c2 &&
> +	test_must_fail git -c rerere.enabled=true merge master &&

This makes me really worried. It is the same `master` (i.e. *not* a tag) that broke this test case in the previous round.

> +	test_path_is_file .git/MERGE_HEAD &&
> +	test_path_is_file .git/MERGE_MODE &&
> +	test_path_is_file .git/MERGE_MSG &&
> +	test_path_is_file .git/MERGE_RR &&

Isn't this a clear implementation details of `git rerere` that you just taught `git merge`'s regression test?

That's *prone* to become a test failure without a bug.

It would probably make a ton more sense to look at the output of `git rerere status` instead.

Ciao, Johannes

Show 12 quoted lines
> +	git merge --quit &&
> +	test_path_is_missing .git/MERGE_HEAD &&
> +	test_path_is_missing .git/MERGE_MODE &&
> +	test_path_is_missing .git/MERGE_MSG &&
> +	test_path_is_missing .git/MERGE_RR
> +'
> +
>  test_done
> --
> 2.21.0.1141.gd54ac2cb17
>
>
Junio C Hamano· May 15, 2019, 02:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3 2/2] merge: add --quit

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
>> +test_expect_success 'merge --quit' '
>> +	git reset --hard c2 &&
>> +	test_must_fail git -c rerere.enabled=true merge master &&
>
> This makes me really worried. It is the same `master` (i.e. *not* a tag)
> that broke this test case in the previous round.
I'll let you two figure this out, but I tend to agree.
Show 10 quoted lines
>> +	test_path_is_file .git/MERGE_HEAD &&
>> +	test_path_is_file .git/MERGE_MODE &&
>> +	test_path_is_file .git/MERGE_MSG &&
>> +	test_path_is_file .git/MERGE_RR &&
>
> Isn't this a clear implementation details of `git rerere` that you just
> taught `git merge`'s regression test?
> ...
> It would probably make a ton more sense to look at the output of `git
> rerere status` instead.

While I understand your concern, it is not the business of this test to detect a bug in "git rerere status", either. The safest thing to do would be to test both ;-)

t4151 that tests "am --abort" already looks at MERGE_RR for the same "we want to make sure that the rerere state is cleared" purpose, so I'd not be worried too much about this particular test.

Thanks.
Johannes Schindelin· May 15, 2019, 15:00 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/2] merge: add --quit

Hi Junio,
On Wed, 15 May 2019, Junio C Hamano wrote:
Show 20 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> +	test_path_is_file .git/MERGE_HEAD &&
> >> +	test_path_is_file .git/MERGE_MODE &&
> >> +	test_path_is_file .git/MERGE_MSG &&
> >> +	test_path_is_file .git/MERGE_RR &&
> >
> > Isn't this a clear implementation details of `git rerere` that you just
> > taught `git merge`'s regression test?
> > ...
> > It would probably make a ton more sense to look at the output of `git
> > rerere status` instead.
>
> While I understand your concern, it is not the business of this test
> to detect a bug in "git rerere status", either.  The safest thing to
> do would be to test both ;-)
>
> t4151 that tests "am --abort" already looks at MERGE_RR for the same
> "we want to make sure that the rerere state is cleared" purpose, so
> I'd not be worried too much about this particular test.

I spend *way* too much time chasing regression test failures that turn out not to show any bugs in the code they want to safeguard, but instead the bugs are found in the *assumptions* of the regression tests. So much time, in fact, that I have to disagree with you here. t4151 is just as wrong.

Ciao, Dscho

Junio C Hamano· May 15, 2019, 02:52 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v3 2/2] merge: add --quit

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
Show 21 quoted lines
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 106148254d..d453710ef6 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -822,4 +822,18 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
>  	verify_parents $c0 $c1
>  '
>  
> +test_expect_success 'merge --quit' '
> +	git reset --hard c2 &&
> +	test_must_fail git -c rerere.enabled=true merge master &&
> +	test_path_is_file .git/MERGE_HEAD &&
> +	test_path_is_file .git/MERGE_MODE &&
> +	test_path_is_file .git/MERGE_MSG &&
> +	test_path_is_file .git/MERGE_RR &&
> +	git merge --quit &&
> +	test_path_is_missing .git/MERGE_HEAD &&
> +	test_path_is_missing .git/MERGE_MODE &&
> +	test_path_is_missing .git/MERGE_MSG &&
> +	test_path_is_missing .git/MERGE_RR
> +'

I'd appreciate to see test_when_finished used in this test so that later tests, possibly added by another topic in parallel, would start in a more-or-less reasonable state.

Nguyễn Thái Ngọc Duy· May 18, 2019, 11:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 0/2] nd/merge-quit updates

Another round because apparently the test case is not perfect.
Nguyễn Thái Ngọc Duy (2):
  merge: remove drop_save() in favor of remove_merge_branch_state()
  merge: add --quit
 Documentation/git-merge.txt |  4 ++++
 branch.c                    | 11 ++++++++---
 branch.h                    |  6 ++++++
 builtin/merge.c             | 30 ++++++++++++++++++------------
 t/t7600-merge.sh            | 26 ++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 15 deletions(-)
Interdiff dựa trên v3:
Show changes to t/t7600-merge.sh +23 −11
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index d453710ef6..625a24a980 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -823,17 +823,29 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
 '
 
 test_expect_success 'merge --quit' '
-	git reset --hard c2 &&
-	test_must_fail git -c rerere.enabled=true merge master &&
-	test_path_is_file .git/MERGE_HEAD &&
-	test_path_is_file .git/MERGE_MODE &&
-	test_path_is_file .git/MERGE_MSG &&
-	test_path_is_file .git/MERGE_RR &&
-	git merge --quit &&
-	test_path_is_missing .git/MERGE_HEAD &&
-	test_path_is_missing .git/MERGE_MODE &&
-	test_path_is_missing .git/MERGE_MSG &&
-	test_path_is_missing .git/MERGE_RR
+	git init merge-quit &&
+	(
+		cd merge-quit &&
+		test_commit base &&
+		echo one >>base.t &&
+		git commit -am one &&
+		git branch one &&
+		git checkout base &&
+		echo two >>base.t &&
+		git commit -am two &&
+		test_must_fail git -c rerere.enabled=true merge one &&
+		test_path_is_file .git/MERGE_HEAD &&
+		test_path_is_file .git/MERGE_MODE &&
+		test_path_is_file .git/MERGE_MSG &&
+		git rerere status >rerere.before &&
+		git merge --quit &&
+		test_path_is_missing .git/MERGE_HEAD &&
+		test_path_is_missing .git/MERGE_MODE &&
+		test_path_is_missing .git/MERGE_MSG &&
+		git rerere status >rerere.after &&
+		test_must_be_empty rerere.after &&
+		! test_cmp rerere.after rerere.before
+	)
 '
 
 test_done
-- 
2.22.0.rc0.322.g2b0371e29a
Nguyễn Thái Ngọc Duy· May 18, 2019, 11:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 1/2] merge: remove drop_save() in favor of remove_merge_branch_state()

Both remove_branch_state() and drop_save() delete almost the same set of files about the current merge state. The only difference is MERGE_RR but it should also be cleaned up after a successful merge, which is what drop_save() is for.

Make a new function that deletes all merge-related state files and use it instead of drop_save(). This function will also be used in the next patch that introduces --quit.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 branch.c        | 11 ++++++++---
 branch.h        |  6 ++++++
 builtin/merge.c | 17 +++++------------
 3 files changed, 19 insertions(+), 15 deletions(-)
Show changes to 3 files +19 −15

branch.c, branch.h, builtin/merge.c

diff --git a/branch.c b/branch.c
index 28b81a7e02..1db0601a11 100644
--- a/branch.c
+++ b/branch.c
@@ -337,15 +337,20 @@ void create_branch(struct repository *r,
 	free(real_ref);
 }
 
-void remove_branch_state(struct repository *r)
+void remove_merge_branch_state(struct repository *r)
 {
-	unlink(git_path_cherry_pick_head(r));
-	unlink(git_path_revert_head(r));
 	unlink(git_path_merge_head(r));
 	unlink(git_path_merge_rr(r));
 	unlink(git_path_merge_msg(r));
 	unlink(git_path_merge_mode(r));
+}
+
+void remove_branch_state(struct repository *r)
+{
+	unlink(git_path_cherry_pick_head(r));
+	unlink(git_path_revert_head(r));
 	unlink(git_path_squash_msg(r));
+	remove_merge_branch_state(r);
 }
 
 void die_if_checked_out(const char *branch, int ignore_current_worktree)
diff --git a/branch.h b/branch.h
index 29c1afa4d0..c90ba9d7bf 100644
--- a/branch.h
+++ b/branch.h
@@ -60,6 +60,12 @@ extern int validate_branchname(const char *name, struct strbuf *ref);
  */
 extern int validate_new_branchname(const char *name, struct strbuf *ref, int force);
 
+/*
+ * Remove information about the merge state on the current
+ * branch. (E.g., MERGE_HEAD)
+ */
+void remove_merge_branch_state(struct repository *r);
+
 /*
  * Remove information about the state of working on the current
  * branch. (E.g., MERGE_HEAD)
diff --git a/builtin/merge.c b/builtin/merge.c
index e47d77baee..e9663f027a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -37,6 +37,7 @@
 #include "packfile.h"
 #include "tag.h"
 #include "alias.h"
+#include "branch.h"
 #include "commit-reach.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
@@ -279,14 +280,6 @@ static struct option builtin_merge_options[] = {
 	OPT_END()
 };
 
-/* Cleans up metadata that is uninteresting after a succeeded merge. */
-static void drop_save(void)
-{
-	unlink(git_path_merge_head(the_repository));
-	unlink(git_path_merge_msg(the_repository));
-	unlink(git_path_merge_mode(the_repository));
-}
-
 static int save_state(struct object_id *stash)
 {
 	int len;
@@ -380,7 +373,7 @@ static void finish_up_to_date(const char *msg)
 {
 	if (verbosity >= 0)
 		printf("%s%s\n", squash ? _(" (nothing to squash)") : "", msg);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 }
 
 static void squash_message(struct commit *commit, struct commit_list *remoteheads)
@@ -858,7 +851,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 			&result_commit, NULL, sign_commit))
 		die(_("failed to write commit object"));
 	finish(head, remoteheads, &result_commit, "In-index merge");
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -885,7 +878,7 @@ static int finish_automerge(struct commit *head,
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(head, remoteheads, &result_commit, buf.buf);
 	strbuf_release(&buf);
-	drop_save();
+	remove_merge_branch_state(the_repository);
 	return 0;
 }
 
@@ -1463,7 +1456,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 
 		finish(head_commit, remoteheads, &commit->object.oid, msg.buf);
-		drop_save();
+		remove_merge_branch_state(the_repository);
 		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
-- 
2.22.0.rc0.322.g2b0371e29a
Nguyễn Thái Ngọc Duy· May 18, 2019, 11:30 UTC · re: Nguyễn Thái Ngọc Duy · lore

[PATCH v4 2/2] merge: add --quit

This allows to cancel the current merge without resetting worktree/index, which is what --abort is for. Like other --quit(s), this is often used when you forgot that you're in the middle of a merge and already switched away, doing different things. By the time you've realized, you can't even continue the merge anymore.

This also makes all in-progress commands, am, merge, rebase, revert and cherry-pick, take all three --abort, --continue and --quit (bisect has a different UI).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-merge.txt |  4 ++++
 builtin/merge.c             | 13 +++++++++++++
 t/t7600-merge.sh            | 26 ++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)
Show changes to 3 files +43 −0

Documentation/git-merge.txt, builtin/merge.c, t/t7600-merge.sh

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index 4cc86469f3..b7d581fc76 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -99,6 +99,10 @@ commit or stash your changes before running 'git merge'.
 'git merge --abort' is equivalent to 'git reset --merge' when
 `MERGE_HEAD` is present.
 
+--quit::
+	Forget about the current merge in progress. Leave the index
+	and the working tree as-is.
+
 --continue::
 	After a 'git merge' stops due to conflicts you can conclude the
 	merge by running 'git merge --continue' (see "HOW TO RESOLVE
diff --git a/builtin/merge.c b/builtin/merge.c
index e9663f027a..598d56edfe 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -73,6 +73,7 @@ static int option_renormalize;
 static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
+static int quit_current_merge;
 static int continue_current_merge;
 static int allow_unrelated_histories;
 static int show_progress = -1;
@@ -267,6 +268,8 @@ static struct option builtin_merge_options[] = {
 	OPT__VERBOSITY(&verbosity),
 	OPT_BOOL(0, "abort", &abort_current_merge,
 		N_("abort the current in-progress merge")),
+	OPT_BOOL(0, "quit", &quit_current_merge,
+		N_("--abort but leave index and working tree alone")),
 	OPT_BOOL(0, "continue", &continue_current_merge,
 		N_("continue the current in-progress merge")),
 	OPT_BOOL(0, "allow-unrelated-histories", &allow_unrelated_histories,
@@ -1252,6 +1255,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		goto done;
 	}
 
+	if (quit_current_merge) {
+		if (orig_argc != 2)
+			usage_msg_opt(_("--quit expects no arguments"),
+				      builtin_merge_usage,
+				      builtin_merge_options);
+
+		remove_merge_branch_state(the_repository);
+		goto done;
+	}
+
 	if (continue_current_merge) {
 		int nargc = 1;
 		const char *nargv[] = {"commit", NULL};
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 106148254d..625a24a980 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -822,4 +822,30 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
 	verify_parents $c0 $c1
 '
 
+test_expect_success 'merge --quit' '
+	git init merge-quit &&
+	(
+		cd merge-quit &&
+		test_commit base &&
+		echo one >>base.t &&
+		git commit -am one &&
+		git branch one &&
+		git checkout base &&
+		echo two >>base.t &&
+		git commit -am two &&
+		test_must_fail git -c rerere.enabled=true merge one &&
+		test_path_is_file .git/MERGE_HEAD &&
+		test_path_is_file .git/MERGE_MODE &&
+		test_path_is_file .git/MERGE_MSG &&
+		git rerere status >rerere.before &&
+		git merge --quit &&
+		test_path_is_missing .git/MERGE_HEAD &&
+		test_path_is_missing .git/MERGE_MODE &&
+		test_path_is_missing .git/MERGE_MSG &&
+		git rerere status >rerere.after &&
+		test_must_be_empty rerere.after &&
+		! test_cmp rerere.after rerere.before
+	)
+'
+
 test_done
-- 
2.22.0.rc0.322.g2b0371e29a
Johannes Schindelin· May 20, 2019, 18:05 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v4 2/2] merge: add --quit

Hi Duy,
On Sat, 18 May 2019, Nguyễn Thái Ngọc Duy wrote:
Show 33 quoted lines
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 106148254d..625a24a980 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -822,4 +822,30 @@ test_expect_success EXECKEEPSPID 'killed merge can be completed with --continue'
>  	verify_parents $c0 $c1
>  '
>
> +test_expect_success 'merge --quit' '
> +	git init merge-quit &&
> +	(
> +		cd merge-quit &&
> +		test_commit base &&
> +		echo one >>base.t &&
> +		git commit -am one &&
> +		git branch one &&
> +		git checkout base &&
> +		echo two >>base.t &&
> +		git commit -am two &&
> +		test_must_fail git -c rerere.enabled=true merge one &&
> +		test_path_is_file .git/MERGE_HEAD &&
> +		test_path_is_file .git/MERGE_MODE &&
> +		test_path_is_file .git/MERGE_MSG &&
> +		git rerere status >rerere.before &&
> +		git merge --quit &&
> +		test_path_is_missing .git/MERGE_HEAD &&
> +		test_path_is_missing .git/MERGE_MODE &&
> +		test_path_is_missing .git/MERGE_MSG &&
> +		git rerere status >rerere.after &&
> +		test_must_be_empty rerere.after &&
> +		! test_cmp rerere.after rerere.before
> +	)
> +'

Good test cases do not *need* to be excessively long. Something like this should be conciser, and more importantly, less inviting to typos or other bugs:

	test_commit quit-test &&
	test_commit quit-one quit-test.t one &&
	git reset --hard HEAD^ &&
	test_commit quit-two quit-test.t two &&
	test_must_fail git -c rerere.enabled=true merge one &&
	test_path_is_file .git/MERGE_HEAD &&
	git rerere status >rerere &&
	test -s rerere &&
	git merge --quit &&
	test_path_is_missing .git/MERGE_HEAD &&
	git rerere status >rerere &&
	test_must_be_empty rerere

Note that this does not do an exhaustive test for all the .git/MERGE_* files: this test regression promises to verify that `git merge --quit` works, it does not promise to verify that a failed `git merge` leaves all of those files! Using just `MERGE_HEAD` as a tell-tale for that is plenty sufficient.

Likewise, this test case does not verify that the output of `git rerere status` is different before and after the `git merge --quit`. That is not the point of the test, to make sure that they are different. The point is to make sure that it is empty afterwards, but not empty beforehand.

Technically, your version of the test case verifies the same (if a file's contents differ from an empty file's, then necessarily it is not empty, but it requires some gymnastics to come to that conclusion). There is no need for convoluted thinking in regression test cases. In fact, the easier it is to understand the *intent* of a test case, the quicker the investigation of any future bug, and consequently the faster the bug fix.

Also, the less you execute, the quicker the test case runs. That might not sound like much, but we have over 20,000 test cases in our test suite. That multiplication is really easy to compute in your head. If all of them were written succinctly, I bet you would find it much less taxing on your patience to actually run the full test suite from time to time ;-)

And this illustrates a very real cost of a slow test suite in addition to the time: developers will run it less often, causing more regressions, wasting even more time in the long run.

Ciao, Johannes

Johannes Schindelin· May 20, 2019, 17:01 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH v4 0/2] nd/merge-quit updates

Hi Duy,
On Sat, 18 May 2019, Nguyễn Thái Ngọc Duy wrote:
> Another round because apparently the test case is not perfect.

Test cases are never perfect. But at least the good ones are "actionable", i.e. when they fail, you know there is a regression, it helps you figure out easily why it broke, and it helps you fix the regression.

Many, many test cases are bad: they break for all kinds of reasons *except* for a regression. They break because an error message was changed. They break because of timing issues. They break because somebody inserted another test case. They break because a missing prereq caused a previous test case not to run, removing a side effect on which the breaking test case relied.

Those bad test cases are very frustrating for people who actually look at them. I am one of those people.

And if I did not know delightful test suites that are populated with "good" test cases, I would not point out when a "bad" one is contributed.

So: it *is* possible to have a good test suite. Let's make ours better
than it is right now.

Thanks, Johannes

← back to recent threads