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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(+)

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

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

[Phillip Wood]

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

> 
> [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(-)
> 

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

[Emily Shaffer]

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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(+)

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

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

[Nguyễn Thái Ngọc Duy]

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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(+)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Hi Duy,

On Tue, 14 May 2019, Nguyễn Thái Ngọc Duy wrote:

> 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

> +	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
>
>

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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.

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +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.

>> +	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.

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

[Johannes Schindelin]

Hi Junio,

On Wed, 15 May 2019, Junio C Hamano wrote:

> 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

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

[Nguyễn Thái Ngọc Duy]

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:
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

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

[Nguyễn Thái Ngọc Duy]

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(-)

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

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

[Nguyễn Thái Ngọc Duy]

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(+)

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3368 bytes --]

Hi Duy,

On Sat, 18 May 2019, Nguyễn Thái Ngọc Duy wrote:

> 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