git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] commit: fix advice for empty commits during rebases
@ 2019-10-22 23:30 Johannes Schindelin via GitGitGadget
  2019-10-22 23:30 ` [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit` Johannes Schindelin via GitGitGadget
                   ` (4 more replies)
  0 siblings, 5 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-22 23:30 UTC (permalink / raw)
  To: git; +Cc: Rohit Ashiwal, Johannes Schindelin, Junio C Hamano

In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we
introduced a helpful message that suggests to run git cherry-pick --skip 
(instead of the previous message that talked about git reset) when a
cherry-pick failed due to an empty patch.

However, the same message is displayed during a rebase, when the patch
to-be-committed is empty. In this case, git reset would also have worked,
but git cherry-pick --skip does not work. This is a regression introduced in
this cycle.

Let's be more careful here.

Johannes Schindelin (3):
  cherry-pick: add test for `--skip` advice in `git commit`
  sequencer: export the function to get the path of `.git/rebase-merge/`
  commit: give correct advice for empty commit during a rebase

 builtin/commit.c                | 33 ++++++++++++++++++++++++---------
 sequencer.c                     |  4 ++--
 sequencer.h                     |  1 +
 t/t3403-rebase-skip.sh          |  9 +++++++++
 t/t3510-cherry-pick-sequence.sh |  3 ++-
 5 files changed, 38 insertions(+), 12 deletions(-)


base-commit: d966095db01190a2196e31195ea6fa0c722aa732
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-417%2Fdscho%2Ffix-commit-advice-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-417/dscho/fix-commit-advice-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/417
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit`
  2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
@ 2019-10-22 23:30 ` Johannes Schindelin via GitGitGadget
  2019-10-22 23:30 ` [PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/` Johannes Schindelin via GitGitGadget
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-22 23:30 UTC (permalink / raw)
  To: git
  Cc: Rohit Ashiwal, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
`git commit` learned to suggest to run `git cherry-pick --skip` when
trying to cherry-pick an empty patch, but that was never tested for.

Here is a test that verifies that a message is given to the user that
contains the correct invocation.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3510-cherry-pick-sequence.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 793bcc7fe3..5b94fdaa67 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -123,7 +123,8 @@ test_expect_success 'revert --skip to skip commit' '
 test_expect_success 'skip "empty" commit' '
 	pristine_detach picked &&
 	test_commit dummy foo d &&
-	test_must_fail git cherry-pick anotherpick &&
+	test_must_fail git cherry-pick anotherpick 2>err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
 	git cherry-pick --skip &&
 	test_cmp_rev dummy HEAD
 '
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/`
  2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
  2019-10-22 23:30 ` [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit` Johannes Schindelin via GitGitGadget
@ 2019-10-22 23:30 ` Johannes Schindelin via GitGitGadget
  2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-22 23:30 UTC (permalink / raw)
  To: git
  Cc: Rohit Ashiwal, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The presence of this path will be used in the next commit to fix an
incorrect piece of advice in `git commit` when in the middle of a
rebase.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 sequencer.c | 4 ++--
 sequencer.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9d5964fd81..5bd7e9d05a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,7 +47,7 @@ static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
 static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
 
-static GIT_PATH_FUNC(rebase_path, "rebase-merge")
+GIT_PATH_FUNC(git_path_rebase_merge_dir, "rebase-merge")
 /*
  * The file containing rebase commands, comments, and empty lines.
  * This file is created by "git rebase -i" then edited by the user. As
@@ -218,7 +218,7 @@ static inline int is_rebase_i(const struct replay_opts *opts)
 static const char *get_dir(const struct replay_opts *opts)
 {
 	if (is_rebase_i(opts))
-		return rebase_path();
+		return git_path_rebase_merge_dir();
 	return git_path_seq_dir();
 }
 
diff --git a/sequencer.h b/sequencer.h
index 574260f621..505852d7d1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -9,6 +9,7 @@ struct repository;
 
 const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
+const char *git_path_rebase_merge_dir(void);
 const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
  2019-10-22 23:30 ` [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit` Johannes Schindelin via GitGitGadget
  2019-10-22 23:30 ` [PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/` Johannes Schindelin via GitGitGadget
@ 2019-10-22 23:30 ` Johannes Schindelin via GitGitGadget
  2019-10-23  2:45   ` Junio C Hamano
  2019-10-24 10:15   ` Phillip Wood
  2019-10-23  3:02 ` [PATCH 0/3] commit: fix advice for empty commits during rebases Junio C Hamano
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
  4 siblings, 2 replies; 31+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-22 23:30 UTC (permalink / raw)
  To: git
  Cc: Rohit Ashiwal, Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
`git commit` learned to suggest to run `git cherry-pick --skip` when
trying to cherry-pick an empty patch.

However, it was overlooked that there are more conditions than just a
`git cherry-pick` when this advice is printed (which originally
suggested the neutral `git reset`): the same can happen during a rebase.

Let's suggest the correct command, even during a rebase.

While at it, we adjust more places in `builtin/commit.c` that
incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
surely this must be a `cherry-pick` in progress.

Note: we take pains to handle the situation when a user runs a `git
cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
line in an interactive rebase). On the other hand, it is not possible to
run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
`sequencer/` exist, we still want to advise to use `git cherry-pick
--skip`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/commit.c       | 33 ++++++++++++++++++++++++---------
 t/t3403-rebase-skip.sh |  9 +++++++++
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e588bc6ad3..2beae13620 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
 "    git commit --allow-empty\n"
 "\n");
 
+static const char empty_rebase_advice[] =
+N_("Otherwise, please use 'git rebase --skip'\n");
+
 static const char empty_cherry_pick_advice_single[] =
 N_("Otherwise, please use 'git cherry-pick --skip'\n");
 
@@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
 static const char *cleanup_arg;
 
 static enum commit_whence whence;
-static int sequencer_in_use;
+static int sequencer_in_use, rebase_in_progress;
 static int use_editor = 1, include_status = 1;
 static int have_option_m;
 static struct strbuf message = STRBUF_INIT;
@@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
 		whence = FROM_CHERRY_PICK;
 		if (file_exists(git_path_seq_dir()))
 			sequencer_in_use = 1;
+		if (file_exists(git_path_rebase_merge_dir()))
+			rebase_in_progress = 1;
 	}
 	else
 		whence = FROM_COMMIT;
@@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	if (whence != FROM_COMMIT) {
 		if (whence == FROM_MERGE)
 			die(_("cannot do a partial commit during a merge."));
-		else if (whence == FROM_CHERRY_PICK)
+		else if (whence == FROM_CHERRY_PICK) {
+			if (rebase_in_progress && !sequencer_in_use)
+				die(_("cannot do a partial commit during a rebase."));
 			die(_("cannot do a partial commit during a cherry-pick."));
+		}
 	}
 
 	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
@@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			fputs(_(empty_amend_advice), stderr);
 		else if (whence == FROM_CHERRY_PICK) {
 			fputs(_(empty_cherry_pick_advice), stderr);
-			if (!sequencer_in_use)
-				fputs(_(empty_cherry_pick_advice_single), stderr);
-			else
+			if (sequencer_in_use)
 				fputs(_(empty_cherry_pick_advice_multi), stderr);
+			else if (rebase_in_progress)
+				fputs(_(empty_rebase_advice), stderr);
+			else
+				fputs(_(empty_cherry_pick_advice_single), stderr);
 		}
 		return 0;
 	}
@@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (amend && whence != FROM_COMMIT) {
 		if (whence == FROM_MERGE)
 			die(_("You are in the middle of a merge -- cannot amend."));
-		else if (whence == FROM_CHERRY_PICK)
+		else if (whence == FROM_CHERRY_PICK) {
+			if (rebase_in_progress && !sequencer_in_use)
+				die(_("You are in the middle of a rebase -- cannot amend."));
 			die(_("You are in the middle of a cherry-pick -- cannot amend."));
+		}
 	}
 	if (fixup_message && squash_message)
 		die(_("Options --squash and --fixup cannot be used together"));
@@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reduce_heads_replace(&parents);
 	} else {
 		if (!reflog_msg)
-			reflog_msg = (whence == FROM_CHERRY_PICK)
-					? "commit (cherry-pick)"
-					: "commit";
+			reflog_msg = (whence != FROM_CHERRY_PICK)
+					? "commit"
+					: rebase_in_progress && !sequencer_in_use
+					? "commit (rebase)"
+					: "commit (cherry-pick)";
 		commit_list_insert(current_head, &parents);
 	}
 
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index 1f5122b632..77b03ac49f 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
+test_expect_success 'correct advice upon empty commit' '
+	git checkout -b rebase-skip &&
+	test_commit a1 &&
+	test_tick &&
+	git commit --amend -m amended --no-edit &&
+	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
+	test_i18ngrep "git rebase --skip" err
+'
+
 test_done
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
@ 2019-10-23  2:45   ` Junio C Hamano
  2019-10-24 10:15   ` Phillip Wood
  1 sibling, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2019-10-23  2:45 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Rohit Ashiwal, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> Note: we take pains to handle the situation when a user runs a `git
> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
> line in an interactive rebase). On the other hand, it is not possible to
> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
> `sequencer/` exist, we still want to advise to use `git cherry-pick
> --skip`.

Good description of why the code does what it does, that future
readers will surely appreciate.  Nice.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/3] commit: fix advice for empty commits during rebases
  2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
                   ` (2 preceding siblings ...)
  2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
@ 2019-10-23  3:02 ` Junio C Hamano
  2019-10-25 12:11   ` Johannes Schindelin
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
  4 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2019-10-23  3:02 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Rohit Ashiwal, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we
> introduced a helpful message that suggests to run git cherry-pick --skip 
> (instead of the previous message that talked about git reset) when a
> cherry-pick failed due to an empty patch.
>
> However, the same message is displayed during a rebase, when the patch
> to-be-committed is empty. In this case, git reset would also have worked,
> but git cherry-pick --skip does not work. This is a regression introduced in
> this cycle.
>
> Let's be more careful here.
>
> Johannes Schindelin (3):
>   cherry-pick: add test for `--skip` advice in `git commit`
>   sequencer: export the function to get the path of `.git/rebase-merge/`
>   commit: give correct advice for empty commit during a rebase

Overall they looked nicely done.  The last one may have started its
life as "let's fix advice for empty", but no longer is.

The old code used the sequencer_in_use boolean to tell between two
states ("are we doing a single pick, or a range pick?"), but now we
have another boolean rebase_in_progress, and depending on the shape
of the if statements existing codepaths happen to have, these two
are combined in different ways to deal with new states.  E.g. some
places say

	if (rebase_in_progress && !sequencer_in_use)
		we are doing a rebase;
	else
		we are doing a cherry-pick;

and some others say

	if (sequencer_in_use)
		we are doing a multi pick;
	else if (rebase_in_progress)
		we are doing a rebase;
	else
		we are doing a single pick;

I wonder if it makes the resulting logic simpler to reason about if
these combination of two variables are rewritten to use a single
variable that enumerates three (or is it four, counting "we are
doing neither a cherry-pick or a rebase"?) possible state.

Other than that, looked sensible.  Will queue.

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
  2019-10-23  2:45   ` Junio C Hamano
@ 2019-10-24 10:15   ` Phillip Wood
  2019-10-25 11:48     ` Johannes Schindelin
  1 sibling, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-10-24 10:15 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget, git
  Cc: Rohit Ashiwal, Johannes Schindelin, Junio C Hamano

Hi Dscho

On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
> `git commit` learned to suggest to run `git cherry-pick --skip` when
> trying to cherry-pick an empty patch.
> 
> However, it was overlooked that there are more conditions than just a
> `git cherry-pick` when this advice is printed (which originally
> suggested the neutral `git reset`): the same can happen during a rebase.
> 
> Let's suggest the correct command, even during a rebase.
> 
> While at it, we adjust more places in `builtin/commit.c` that
> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
> surely this must be a `cherry-pick` in progress.
> 
> Note: we take pains to handle the situation when a user runs a `git
> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
> line in an interactive rebase). On the other hand, it is not possible to
> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
> `sequencer/` exist, we still want to advise to use `git cherry-pick
> --skip`.

Thanks for working on this. It's unfortunate that rebase does not remove
CHERRY_PICK_HEAD for empty commits as it does if the commit is not
empty. I think this is because 'rebase --continue' will skip an empty
commit so the user _has_ to run 'git commit' manually to keep it. If it
had been designed so that 'rebase --continue' kept the empty commit and
'rebase --skip' skipped it then we would not have this problem but it's
a bit late to worry about that now.

I don't this patch can distinguish between an empty cherry-pick
performed by the user while a rebase is in progress and an empty pick
performed by rebase as both create CHERRY_PICK_HEAD while
.git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
created by rebase and prints advise based on that which may or may not
be the correct. I think we could distinguish the two by checking if
CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.

Best Wishes


Phillip

> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/commit.c       | 33 ++++++++++++++++++++++++---------
>  t/t3403-rebase-skip.sh |  9 +++++++++
>  2 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index e588bc6ad3..2beae13620 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
>  "    git commit --allow-empty\n"
>  "\n");
>  
> +static const char empty_rebase_advice[] =
> +N_("Otherwise, please use 'git rebase --skip'\n");
> +
>  static const char empty_cherry_pick_advice_single[] =
>  N_("Otherwise, please use 'git cherry-pick --skip'\n");
>  
> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
>  static const char *cleanup_arg;
>  
>  static enum commit_whence whence;
> -static int sequencer_in_use;
> +static int sequencer_in_use, rebase_in_progress;
>  static int use_editor = 1, include_status = 1;
>  static int have_option_m;
>  static struct strbuf message = STRBUF_INIT;
> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
>  		whence = FROM_CHERRY_PICK;
>  		if (file_exists(git_path_seq_dir()))
>  			sequencer_in_use = 1;
> +		if (file_exists(git_path_rebase_merge_dir()))
> +			rebase_in_progress = 1;
>  	}
>  	else
>  		whence = FROM_COMMIT;
> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>  	if (whence != FROM_COMMIT) {
>  		if (whence == FROM_MERGE)
>  			die(_("cannot do a partial commit during a merge."));
> -		else if (whence == FROM_CHERRY_PICK)
> +		else if (whence == FROM_CHERRY_PICK) {
> +			if (rebase_in_progress && !sequencer_in_use)
> +				die(_("cannot do a partial commit during a rebase."));
>  			die(_("cannot do a partial commit during a cherry-pick."));
> +		}
>  	}
>  
>  	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  			fputs(_(empty_amend_advice), stderr);
>  		else if (whence == FROM_CHERRY_PICK) {
>  			fputs(_(empty_cherry_pick_advice), stderr);
> -			if (!sequencer_in_use)
> -				fputs(_(empty_cherry_pick_advice_single), stderr);
> -			else
> +			if (sequencer_in_use)
>  				fputs(_(empty_cherry_pick_advice_multi), stderr);
> +			else if (rebase_in_progress)
> +				fputs(_(empty_rebase_advice), stderr);
> +			else
> +				fputs(_(empty_cherry_pick_advice_single), stderr);
>  		}
>  		return 0;
>  	}
> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  	if (amend && whence != FROM_COMMIT) {
>  		if (whence == FROM_MERGE)
>  			die(_("You are in the middle of a merge -- cannot amend."));
> -		else if (whence == FROM_CHERRY_PICK)
> +		else if (whence == FROM_CHERRY_PICK) {
> +			if (rebase_in_progress && !sequencer_in_use)
> +				die(_("You are in the middle of a rebase -- cannot amend."));
>  			die(_("You are in the middle of a cherry-pick -- cannot amend."));
> +		}
>  	}
>  	if (fixup_message && squash_message)
>  		die(_("Options --squash and --fixup cannot be used together"));
> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  			reduce_heads_replace(&parents);
>  	} else {
>  		if (!reflog_msg)
> -			reflog_msg = (whence == FROM_CHERRY_PICK)
> -					? "commit (cherry-pick)"
> -					: "commit";
> +			reflog_msg = (whence != FROM_CHERRY_PICK)
> +					? "commit"
> +					: rebase_in_progress && !sequencer_in_use
> +					? "commit (rebase)"
> +					: "commit (cherry-pick)";
>  		commit_list_insert(current_head, &parents);
>  	}
>  
> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> index 1f5122b632..77b03ac49f 100755
> --- a/t/t3403-rebase-skip.sh
> +++ b/t/t3403-rebase-skip.sh
> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
>  
>  test_debug 'gitk --all & sleep 1'
>  
> +test_expect_success 'correct advice upon empty commit' '
> +	git checkout -b rebase-skip &&
> +	test_commit a1 &&
> +	test_tick &&
> +	git commit --amend -m amended --no-edit &&
> +	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
> +	test_i18ngrep "git rebase --skip" err
> +'
> +
>  test_done
> 


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-10-24 10:15   ` Phillip Wood
@ 2019-10-25 11:48     ` Johannes Schindelin
  2019-10-25 14:01       ` Phillip Wood
  0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2019-10-25 11:48 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal, Junio C Hamano

Hi Phillip,

On Thu, 24 Oct 2019, Phillip Wood wrote:

> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
> > `git commit` learned to suggest to run `git cherry-pick --skip` when
> > trying to cherry-pick an empty patch.
> >
> > However, it was overlooked that there are more conditions than just a
> > `git cherry-pick` when this advice is printed (which originally
> > suggested the neutral `git reset`): the same can happen during a rebase.
> >
> > Let's suggest the correct command, even during a rebase.
> >
> > While at it, we adjust more places in `builtin/commit.c` that
> > incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
> > surely this must be a `cherry-pick` in progress.
> >
> > Note: we take pains to handle the situation when a user runs a `git
> > cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
> > line in an interactive rebase). On the other hand, it is not possible to
> > run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
> > `sequencer/` exist, we still want to advise to use `git cherry-pick
> > --skip`.
>
> Thanks for working on this. It's unfortunate that rebase does not remove
> CHERRY_PICK_HEAD for empty commits as it does if the commit is not
> empty.

Oh, so that's what it is that bites me all the time? I frequently run
interactive rebases and sometimes mess up authorship (taking authorship
of patches that I did not, in fact, author). I guess what happens is
that I `git commit` in the middle of a rebase that was interrupted by
merge conflicts.

So I would actually like to see the _exact opposite_ of what you want to
see: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case.

> I think this is because 'rebase --continue' will skip an empty commit
> so the user _has_ to run 'git commit' manually to keep it. If it had
> been designed so that 'rebase --continue' kept the empty commit and
> 'rebase --skip' skipped it then we would not have this problem but
> it's a bit late to worry about that now.

True.

> I don't this patch can distinguish between an empty cherry-pick
> performed by the user while a rebase is in progress and an empty pick
> performed by rebase as both create CHERRY_PICK_HEAD while
> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
> created by rebase and prints advise based on that which may or may not
> be the correct. I think we could distinguish the two by checking if
> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.

I guess we could, but then, I would rather worry about that in the next
cycle. In this cycle, I would rather fix the common case, which is that
a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
of `git rebase --skip`.

And even if I performed a `git cherry-pick` during a `git rebase` and
the result would be an empty commit, I'd rather be told to `git rebase
--skip` to continue...

But if you feel strongly that this should be fixed differently, I'll
gladly leave it to you ;-)

Ciao,
Dscho

>
> Best Wishes
>
>
> Phillip
>
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/commit.c       | 33 ++++++++++++++++++++++++---------
> >  t/t3403-rebase-skip.sh |  9 +++++++++
> >  2 files changed, 33 insertions(+), 9 deletions(-)
> >
> > diff --git a/builtin/commit.c b/builtin/commit.c
> > index e588bc6ad3..2beae13620 100644
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
> >  "    git commit --allow-empty\n"
> >  "\n");
> >
> > +static const char empty_rebase_advice[] =
> > +N_("Otherwise, please use 'git rebase --skip'\n");
> > +
> >  static const char empty_cherry_pick_advice_single[] =
> >  N_("Otherwise, please use 'git cherry-pick --skip'\n");
> >
> > @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
> >  static const char *cleanup_arg;
> >
> >  static enum commit_whence whence;
> > -static int sequencer_in_use;
> > +static int sequencer_in_use, rebase_in_progress;
> >  static int use_editor = 1, include_status = 1;
> >  static int have_option_m;
> >  static struct strbuf message = STRBUF_INIT;
> > @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
> >  		whence = FROM_CHERRY_PICK;
> >  		if (file_exists(git_path_seq_dir()))
> >  			sequencer_in_use = 1;
> > +		if (file_exists(git_path_rebase_merge_dir()))
> > +			rebase_in_progress = 1;
> >  	}
> >  	else
> >  		whence = FROM_COMMIT;
> > @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
> >  	if (whence != FROM_COMMIT) {
> >  		if (whence == FROM_MERGE)
> >  			die(_("cannot do a partial commit during a merge."));
> > -		else if (whence == FROM_CHERRY_PICK)
> > +		else if (whence == FROM_CHERRY_PICK) {
> > +			if (rebase_in_progress && !sequencer_in_use)
> > +				die(_("cannot do a partial commit during a rebase."));
> >  			die(_("cannot do a partial commit during a cherry-pick."));
> > +		}
> >  	}
> >
> >  	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
> > @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
> >  			fputs(_(empty_amend_advice), stderr);
> >  		else if (whence == FROM_CHERRY_PICK) {
> >  			fputs(_(empty_cherry_pick_advice), stderr);
> > -			if (!sequencer_in_use)
> > -				fputs(_(empty_cherry_pick_advice_single), stderr);
> > -			else
> > +			if (sequencer_in_use)
> >  				fputs(_(empty_cherry_pick_advice_multi), stderr);
> > +			else if (rebase_in_progress)
> > +				fputs(_(empty_rebase_advice), stderr);
> > +			else
> > +				fputs(_(empty_cherry_pick_advice_single), stderr);
> >  		}
> >  		return 0;
> >  	}
> > @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
> >  	if (amend && whence != FROM_COMMIT) {
> >  		if (whence == FROM_MERGE)
> >  			die(_("You are in the middle of a merge -- cannot amend."));
> > -		else if (whence == FROM_CHERRY_PICK)
> > +		else if (whence == FROM_CHERRY_PICK) {
> > +			if (rebase_in_progress && !sequencer_in_use)
> > +				die(_("You are in the middle of a rebase -- cannot amend."));
> >  			die(_("You are in the middle of a cherry-pick -- cannot amend."));
> > +		}
> >  	}
> >  	if (fixup_message && squash_message)
> >  		die(_("Options --squash and --fixup cannot be used together"));
> > @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> >  			reduce_heads_replace(&parents);
> >  	} else {
> >  		if (!reflog_msg)
> > -			reflog_msg = (whence == FROM_CHERRY_PICK)
> > -					? "commit (cherry-pick)"
> > -					: "commit";
> > +			reflog_msg = (whence != FROM_CHERRY_PICK)
> > +					? "commit"
> > +					: rebase_in_progress && !sequencer_in_use
> > +					? "commit (rebase)"
> > +					: "commit (cherry-pick)";
> >  		commit_list_insert(current_head, &parents);
> >  	}
> >
> > diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> > index 1f5122b632..77b03ac49f 100755
> > --- a/t/t3403-rebase-skip.sh
> > +++ b/t/t3403-rebase-skip.sh
> > @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
> >
> >  test_debug 'gitk --all & sleep 1'
> >
> > +test_expect_success 'correct advice upon empty commit' '
> > +	git checkout -b rebase-skip &&
> > +	test_commit a1 &&
> > +	test_tick &&
> > +	git commit --amend -m amended --no-edit &&
> > +	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
> > +	test_i18ngrep "git rebase --skip" err
> > +'
> > +
> >  test_done
> >
>
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/3] commit: fix advice for empty commits during rebases
  2019-10-23  3:02 ` [PATCH 0/3] commit: fix advice for empty commits during rebases Junio C Hamano
@ 2019-10-25 12:11   ` Johannes Schindelin
  2019-10-29  2:05     ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2019-10-25 12:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal

Hi Junio,

On Wed, 23 Oct 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02), we
> > introduced a helpful message that suggests to run git cherry-pick --skip
> > (instead of the previous message that talked about git reset) when a
> > cherry-pick failed due to an empty patch.
> >
> > However, the same message is displayed during a rebase, when the patch
> > to-be-committed is empty. In this case, git reset would also have worked,
> > but git cherry-pick --skip does not work. This is a regression introduced in
> > this cycle.
> >
> > Let's be more careful here.
> >
> > Johannes Schindelin (3):
> >   cherry-pick: add test for `--skip` advice in `git commit`
> >   sequencer: export the function to get the path of `.git/rebase-merge/`
> >   commit: give correct advice for empty commit during a rebase
>
> Overall they looked nicely done.

Thank you!

> The last one may have started its life as "let's fix advice for
> empty", but no longer is.

Indeed. Sorry, I should have said so in the commit message...

> The old code used the sequencer_in_use boolean to tell between two
> states ("are we doing a single pick, or a range pick?"), but now we
> have another boolean rebase_in_progress, and depending on the shape
> of the if statements existing codepaths happen to have, these two
> are combined in different ways to deal with new states.  E.g. some
> places say
>
> 	if (rebase_in_progress && !sequencer_in_use)
> 		we are doing a rebase;
> 	else
> 		we are doing a cherry-pick;
>
> and some others say
>
> 	if (sequencer_in_use)
> 		we are doing a multi pick;
> 	else if (rebase_in_progress)
> 		we are doing a rebase;
> 	else
> 		we are doing a single pick;
>
> I wonder if it makes the resulting logic simpler to reason about if
> these combination of two variables are rewritten to use a single
> variable that enumerates three (or is it four, counting "we are
> doing neither a cherry-pick or a rebase"?) possible state.

That's a good idea! I'd like to postpone that until after the -rc
period, as it is not strictly necessary to fix the bug.

As the bug was introduced in this cycle, I would like to see the bug fix
in v2.24.0, though; I frequently rebase interactively, usually Git for
Windows' patch thicket on top of one of git.git's branches, which often
results in now-empty patches, and I'd like to see the correct advice ;-)

So as not to forget about introducing that `enum`, I opened a ticket at
https://github.com/gitgitgadget/git/issues/426.

Thanks,
Dscho

>
> Other than that, looked sensible.  Will queue.
>
> Thanks.
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-10-25 11:48     ` Johannes Schindelin
@ 2019-10-25 14:01       ` Phillip Wood
  2019-11-08  5:28         ` Junio C Hamano
  0 siblings, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-10-25 14:01 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal, Junio C Hamano

Hi Dscho

On 25/10/2019 12:48, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Thu, 24 Oct 2019, Phillip Wood wrote:
> 
>> On 23/10/2019 00:30, Johannes Schindelin via GitGitGadget wrote:
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
>>> `git commit` learned to suggest to run `git cherry-pick --skip` when
>>> trying to cherry-pick an empty patch.
>>>
>>> However, it was overlooked that there are more conditions than just a
>>> `git cherry-pick` when this advice is printed (which originally
>>> suggested the neutral `git reset`): the same can happen during a rebase.
>>>
>>> Let's suggest the correct command, even during a rebase.
>>>
>>> While at it, we adjust more places in `builtin/commit.c` that
>>> incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
>>> surely this must be a `cherry-pick` in progress.
>>>
>>> Note: we take pains to handle the situation when a user runs a `git
>>> cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
>>> line in an interactive rebase). On the other hand, it is not possible to
>>> run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
>>> `sequencer/` exist, we still want to advise to use `git cherry-pick
>>> --skip`.
>>
>> Thanks for working on this. It's unfortunate that rebase does not remove
>> CHERRY_PICK_HEAD for empty commits as it does if the commit is not
>> empty.
> 
> Oh, so that's what it is that bites me all the time? I frequently run
> interactive rebases and sometimes mess up authorship (taking authorship
> of patches that I did not, in fact, author). I guess what happens is
> that I `git commit` in the middle of a rebase that was interrupted by
> merge conflicts.

I used to do that a lot, I eventually trained myself not to commit after 
conflicts during a rebase and leave it to `rebase --continue` but it is 
annoying that it does not work

> So I would actually like to see the _exact opposite_ of what you want to
> see: I want to keep `CHERRY_PICK_HEAD` even in the non-empty case.

I don't necessarily object - having it consistent one way or the other 
would be a distinct improvement, it has been removed when there are 
conflicts since it's inception [1] (That's v2 of the series which 
started removing CHERRY_PICK_HEAD in the case of rebase conflicts. I 
couldn't find any discussion of v1 that recommended that change though). 
There is also another thread [2].

[1] 
https://public-inbox.org/git/1297876835-70613-1-git-send-email-jaysoffian@gmail.com/
[2] 
https://public-inbox.org/git/CAMP44s1EAwHjQ7S2ArLvhNg5qkR05DRJ70tQmP8sXYdOP=i_zQ@mail.gmail.com/ 



>> I think this is because 'rebase --continue' will skip an empty commit
>> so the user _has_ to run 'git commit' manually to keep it. If it had
>> been designed so that 'rebase --continue' kept the empty commit and
>> 'rebase --skip' skipped it then we would not have this problem but
>> it's a bit late to worry about that now.
> 
> True.
> 
>> I don't this patch can distinguish between an empty cherry-pick
>> performed by the user while a rebase is in progress and an empty pick
>> performed by rebase as both create CHERRY_PICK_HEAD while
>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
>> created by rebase and prints advise based on that which may or may not
>> be the correct. I think we could distinguish the two by checking if
>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
> 
> I guess we could, but then, I would rather worry about that in the next
> cycle. In this cycle, I would rather fix the common case, which is that
> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
> of `git rebase --skip`.
> 
> And even if I performed a `git cherry-pick` during a `git rebase` and
> the result would be an empty commit, I'd rather be told to `git rebase
> --skip` to continue...
> 
> But if you feel strongly that this should be fixed differently, I'll
> gladly leave it to you ;-)

I'm happy to wait until the next cycle once we've decided what to do 
about CHERRY_PICK_HEAD during rebases.

Best Wishes

Phillip

> Ciao,
> Dscho
> 
>>
>> Best Wishes
>>
>>
>> Phillip
>>
>>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>>   builtin/commit.c       | 33 ++++++++++++++++++++++++---------
>>>   t/t3403-rebase-skip.sh |  9 +++++++++
>>>   2 files changed, 33 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/builtin/commit.c b/builtin/commit.c
>>> index e588bc6ad3..2beae13620 100644
>>> --- a/builtin/commit.c
>>> +++ b/builtin/commit.c
>>> @@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
>>>   "    git commit --allow-empty\n"
>>>   "\n");
>>>
>>> +static const char empty_rebase_advice[] =
>>> +N_("Otherwise, please use 'git rebase --skip'\n");
>>> +
>>>   static const char empty_cherry_pick_advice_single[] =
>>>   N_("Otherwise, please use 'git cherry-pick --skip'\n");
>>>
>>> @@ -122,7 +125,7 @@ static enum commit_msg_cleanup_mode cleanup_mode;
>>>   static const char *cleanup_arg;
>>>
>>>   static enum commit_whence whence;
>>> -static int sequencer_in_use;
>>> +static int sequencer_in_use, rebase_in_progress;
>>>   static int use_editor = 1, include_status = 1;
>>>   static int have_option_m;
>>>   static struct strbuf message = STRBUF_INIT;
>>> @@ -183,6 +186,8 @@ static void determine_whence(struct wt_status *s)
>>>   		whence = FROM_CHERRY_PICK;
>>>   		if (file_exists(git_path_seq_dir()))
>>>   			sequencer_in_use = 1;
>>> +		if (file_exists(git_path_rebase_merge_dir()))
>>> +			rebase_in_progress = 1;
>>>   	}
>>>   	else
>>>   		whence = FROM_COMMIT;
>>> @@ -453,8 +458,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>>>   	if (whence != FROM_COMMIT) {
>>>   		if (whence == FROM_MERGE)
>>>   			die(_("cannot do a partial commit during a merge."));
>>> -		else if (whence == FROM_CHERRY_PICK)
>>> +		else if (whence == FROM_CHERRY_PICK) {
>>> +			if (rebase_in_progress && !sequencer_in_use)
>>> +				die(_("cannot do a partial commit during a rebase."));
>>>   			die(_("cannot do a partial commit during a cherry-pick."));
>>> +		}
>>>   	}
>>>
>>>   	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
>>> @@ -950,10 +958,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>>>   			fputs(_(empty_amend_advice), stderr);
>>>   		else if (whence == FROM_CHERRY_PICK) {
>>>   			fputs(_(empty_cherry_pick_advice), stderr);
>>> -			if (!sequencer_in_use)
>>> -				fputs(_(empty_cherry_pick_advice_single), stderr);
>>> -			else
>>> +			if (sequencer_in_use)
>>>   				fputs(_(empty_cherry_pick_advice_multi), stderr);
>>> +			else if (rebase_in_progress)
>>> +				fputs(_(empty_rebase_advice), stderr);
>>> +			else
>>> +				fputs(_(empty_cherry_pick_advice_single), stderr);
>>>   		}
>>>   		return 0;
>>>   	}
>>> @@ -1156,8 +1166,11 @@ static int parse_and_validate_options(int argc, const char *argv[],
>>>   	if (amend && whence != FROM_COMMIT) {
>>>   		if (whence == FROM_MERGE)
>>>   			die(_("You are in the middle of a merge -- cannot amend."));
>>> -		else if (whence == FROM_CHERRY_PICK)
>>> +		else if (whence == FROM_CHERRY_PICK) {
>>> +			if (rebase_in_progress && !sequencer_in_use)
>>> +				die(_("You are in the middle of a rebase -- cannot amend."));
>>>   			die(_("You are in the middle of a cherry-pick -- cannot amend."));
>>> +		}
>>>   	}
>>>   	if (fixup_message && squash_message)
>>>   		die(_("Options --squash and --fixup cannot be used together"));
>>> @@ -1628,9 +1641,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>>   			reduce_heads_replace(&parents);
>>>   	} else {
>>>   		if (!reflog_msg)
>>> -			reflog_msg = (whence == FROM_CHERRY_PICK)
>>> -					? "commit (cherry-pick)"
>>> -					: "commit";
>>> +			reflog_msg = (whence != FROM_CHERRY_PICK)
>>> +					? "commit"
>>> +					: rebase_in_progress && !sequencer_in_use
>>> +					? "commit (rebase)"
>>> +					: "commit (cherry-pick)";
>>>   		commit_list_insert(current_head, &parents);
>>>   	}
>>>
>>> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
>>> index 1f5122b632..77b03ac49f 100755
>>> --- a/t/t3403-rebase-skip.sh
>>> +++ b/t/t3403-rebase-skip.sh
>>> @@ -76,4 +76,13 @@ test_expect_success 'moved back to branch correctly' '
>>>
>>>   test_debug 'gitk --all & sleep 1'
>>>
>>> +test_expect_success 'correct advice upon empty commit' '
>>> +	git checkout -b rebase-skip &&
>>> +	test_commit a1 &&
>>> +	test_tick &&
>>> +	git commit --amend -m amended --no-edit &&
>>> +	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
>>> +	test_i18ngrep "git rebase --skip" err
>>> +'
>>> +
>>>   test_done
>>>
>>
>>

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/3] commit: fix advice for empty commits during rebases
  2019-10-25 12:11   ` Johannes Schindelin
@ 2019-10-29  2:05     ` Junio C Hamano
  2019-10-29 13:00       ` Johannes Schindelin
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2019-10-29  2:05 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal

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

> As the bug was introduced in this cycle, I would like to see the bug fix
> in v2.24.0, though;...

Do you mean that before the change you blame for the "bug" things
worked better than what we have at the tip of 'master', or do you
mean that the change you blame for the "bug" changed the behaviour
relative to the previous released version but the updated behavior
is not what you consider correct?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 0/3] commit: fix advice for empty commits during rebases
  2019-10-29  2:05     ` Junio C Hamano
@ 2019-10-29 13:00       ` Johannes Schindelin
  0 siblings, 0 replies; 31+ messages in thread
From: Johannes Schindelin @ 2019-10-29 13:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal

Hi Junio,

On Tue, 29 Oct 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > As the bug was introduced in this cycle, I would like to see the bug fix
> > in v2.24.0, though;...
>
> Do you mean that before the change you blame for the "bug" things
> worked better than what we have at the tip of 'master', or do you
> mean that the change you blame for the "bug" changed the behaviour
> relative to the previous released version but the updated behavior
> is not what you consider correct?

What I mean is this: previously, when an interactive rebase interrupted
due to a now-empty patch, the advice given was correct, of clunky. Now
it is incorrect.

(Previously, the advice was to `git reset`, which was not completely
helpful, but now it suggests to `git cherry-pick --continue`, which is
incorrect.)

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-10-25 14:01       ` Phillip Wood
@ 2019-11-08  5:28         ` Junio C Hamano
  2019-11-08 14:09           ` Johannes Schindelin
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2019-11-08  5:28 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git,
	Rohit Ashiwal

Phillip Wood <phillip.wood123@gmail.com> writes:

>>> I don't this patch can distinguish between an empty cherry-pick
>>> performed by the user while a rebase is in progress and an empty pick
>>> performed by rebase as both create CHERRY_PICK_HEAD while
>>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
>>> created by rebase and prints advise based on that which may or may not
>>> be the correct. I think we could distinguish the two by checking if
>>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
>>
>> I guess we could, but then, I would rather worry about that in the next
>> cycle. In this cycle, I would rather fix the common case, which is that
>> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
>> of `git rebase --skip`.
>>
>> And even if I performed a `git cherry-pick` during a `git rebase` and
>> the result would be an empty commit, I'd rather be told to `git rebase
>> --skip` to continue...
>>
>> But if you feel strongly that this should be fixed differently, I'll
>> gladly leave it to you ;-)
>
> I'm happy to wait until the next cycle once we've decided what to do
> about CHERRY_PICK_HEAD during rebases.

So, is that agreed between the two?  

Should I eject js/advise-rebase-skip topic out of my tree and wait
for the decision wrt CHERRY_PICK_HEAD?

Thanks.

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-11-08  5:28         ` Junio C Hamano
@ 2019-11-08 14:09           ` Johannes Schindelin
  2019-11-11 16:13             ` Phillip Wood
  0 siblings, 1 reply; 31+ messages in thread
From: Johannes Schindelin @ 2019-11-08 14:09 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Phillip Wood, Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal

Hi,

On Fri, 8 Nov 2019, Junio C Hamano wrote:

> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >>> I don't this patch can distinguish between an empty cherry-pick
> >>> performed by the user while a rebase is in progress and an empty pick
> >>> performed by rebase as both create CHERRY_PICK_HEAD while
> >>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
> >>> created by rebase and prints advise based on that which may or may not
> >>> be the correct. I think we could distinguish the two by checking if
> >>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
> >>
> >> I guess we could, but then, I would rather worry about that in the next
> >> cycle. In this cycle, I would rather fix the common case, which is that
> >> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
> >> of `git rebase --skip`.
> >>
> >> And even if I performed a `git cherry-pick` during a `git rebase` and
> >> the result would be an empty commit, I'd rather be told to `git rebase
> >> --skip` to continue...
> >>
> >> But if you feel strongly that this should be fixed differently, I'll
> >> gladly leave it to you ;-)
> >
> > I'm happy to wait until the next cycle once we've decided what to do
> > about CHERRY_PICK_HEAD during rebases.
>
> So, is that agreed between the two?
>
> Should I eject js/advise-rebase-skip topic out of my tree and wait
> for the decision wrt CHERRY_PICK_HEAD?

Phillip, if you have some time to spend on that, I'd be very grateful, I
am a bit under the weather and in dear need for an offline weekend.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH 3/3] commit: give correct advice for empty commit during a rebase
  2019-11-08 14:09           ` Johannes Schindelin
@ 2019-11-11 16:13             ` Phillip Wood
  0 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-11-11 16:13 UTC (permalink / raw)
  To: Johannes Schindelin, Junio C Hamano
  Cc: Johannes Schindelin via GitGitGadget, git, Rohit Ashiwal

Hi Dscho & Junio

On 08/11/2019 14:09, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 8 Nov 2019, Junio C Hamano wrote:
> 
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>
>>>>> I don't this patch can distinguish between an empty cherry-pick
>>>>> performed by the user while a rebase is in progress and an empty pick
>>>>> performed by rebase as both create CHERRY_PICK_HEAD while
>>>>> .git/rebase-merge exists. It seems to assume that CHERRY_PICK_HEAD was
>>>>> created by rebase and prints advise based on that which may or may not
>>>>> be the correct. I think we could distinguish the two by checking if
>>>>> CHERRY_PICK_HEAD matches .git/rebase-merge/stopped-sha or REBASE_HEAD.
>>>>
>>>> I guess we could, but then, I would rather worry about that in the next
>>>> cycle. In this cycle, I would rather fix the common case, which is that
>>>> a `git rebase -i` fails and tells me to `git cherry-pick --skip` instead
>>>> of `git rebase --skip`.
>>>>
>>>> And even if I performed a `git cherry-pick` during a `git rebase` and
>>>> the result would be an empty commit, I'd rather be told to `git rebase
>>>> --skip` to continue...

It depends if you want to continue immediately or do something else in 
which case running reset is probably better advice. I'm not sure that 
there's an easy solution for all possible scenarios though.

>>>>
>>>> But if you feel strongly that this should be fixed differently, I'll
>>>> gladly leave it to you ;-)
>>>
>>> I'm happy to wait until the next cycle once we've decided what to do
>>> about CHERRY_PICK_HEAD during rebases.
>>
>> So, is that agreed between the two?
>>
>> Should I eject js/advise-rebase-skip topic out of my tree and wait
>> for the decision wrt CHERRY_PICK_HEAD?
> 
> Phillip, if you have some time to spend on that, I'd be very grateful, I
> am a bit under the weather and in dear need for an offline weekend.

I'm happy to have a look at it but it probably wont be for a couple of 
weeks. I've been thinking about keeping CHERRY_PICK_HEAD and it's not 
totally straight forward. CHERRY_PICK_HEAD needs to be removed when 
committing fixups to keep the author data from the commit that's being 
amended (also I think `git commit --amend` errors out if 
CHERRY_PICK_HEAD is present). It's further complicated by --reset-date 
and --committer-date-is-author-date as we want commit to respect those 
when they've been passed to the rebase command.

Junio, if you drop this series for now I'll rework it with an enum as 
discussed elsewhere in this thread.

Best Wishes

Phillip

> Thanks,
> Dscho
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 0/9] commit: fix advice for empty commits during rebases
  2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
                   ` (3 preceding siblings ...)
  2019-10-23  3:02 ` [PATCH 0/3] commit: fix advice for empty commits during rebases Junio C Hamano
@ 2019-12-06 16:06 ` Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
                     ` (8 more replies)
  4 siblings, 9 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

I've added some more commits to improve the test coverage and handle
edit & reword as well as pick commands. I've also changed the advice
printed by an empty cherry-pick performed during a rebase to be the
same whether it is part of a sequence of cherry-picks or just a single
pick.

There are a couple of RFC patches at the end that change the advice
for fixups that empty HEAD and change the handling of CHERRY_PICK_HEAD
so if a user commits a conflict resolution the authorship is
preserved. They can be split into a separate series if necessary to
avoid holding up the earlier patches.

The first patch is a general cleanup, not really related to the rest
of the series

These patches are based on a merge of ra/cherry-pick-revert-skip and
pw/sequencer-compare-with-right-parent-to-check-empty-commits


Johannes Schindelin (1):
  cherry-pick: add test for `--skip` advice in `git commit`

Phillip Wood (8):
  t3404: use test_cmp_rev
  cherry-pick: check commit error messages
  sequencer: write CHERRY_PICK_HEAD for reword and edit
  commit: use enum value for multiple cherry-picks
  commit: encapsulate determine_whence() for sequencer
  commit: give correct advice for empty commit during a rebase
  [RFC] rebase: fix advice when a fixup creates an empty commit
  [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts

 builtin/commit.c                |  68 +++++++++----
 sequencer.c                     |  93 +++++++++++++++---
 sequencer.h                     |   4 +-
 t/t3403-rebase-skip.sh          |  97 ++++++++++++++++--
 t/t3404-rebase-interactive.sh   | 168 +++++++++++++++++++++++---------
 t/t3507-cherry-pick-conflict.sh |  23 +++++
 t/t3510-cherry-pick-sequence.sh |   3 +-
 t/t7512-status-help.sh          |   2 -
 wt-status.h                     |  16 ++-
 9 files changed, 387 insertions(+), 87 deletions(-)

Range-diff to what is in pu at the moment

 -:  ---------- >  1:  a4ad3e798c t3404: use test_cmp_rev
 1:  b9f97083f1 =  2:  1e9ea48348 cherry-pick: add test for `--skip` advice in `git commit`
 2:  8eff6be234 <  -:  ---------- sequencer: export the function to get the path of `.git/rebase-merge/`
 -:  ---------- >  3:  f9be4dc3ae cherry-pick: check commit error messages
 -:  ---------- >  4:  fe15c61f1e sequencer: write CHERRY_PICK_HEAD for reword and edit
 -:  ---------- >  5:  d2cc4a59f1 commit: use enum value for multiple cherry-picks
 -:  ---------- >  6:  06ab99b367 commit: encapsulate determine_whence() for sequencer
 3:  116a408b6f !  7:  637f17212b commit: give correct advice for empty commit during a rebase
    @@
      ## Metadata ##
    -Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
    +Author: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## Commit message ##
         commit: give correct advice for empty commit during a rebase
    @@ Commit message
         cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
         line in an interactive rebase). On the other hand, it is not possible to
         run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
    -    `sequencer/` exist, we still want to advise to use `git cherry-pick
    -    --skip`.
    +    `sequencer/` exist or CHERRY_PICK_HEAD and REBASE_HEAD point to the same
    +    commit , we still want to advise to use `git cherry-pick --skip`.
     
    -    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    +    Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## builtin/commit.c ##
     @@ builtin/commit.c: N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
      "    git commit --allow-empty\n"
      "\n");
      
    -+static const char empty_rebase_advice[] =
    ++static const char empty_rebase_pick_advice[] =
     +N_("Otherwise, please use 'git rebase --skip'\n");
     +
      static const char empty_cherry_pick_advice_single[] =
      N_("Otherwise, please use 'git cherry-pick --skip'\n");
      
    -@@ builtin/commit.c: static enum commit_msg_cleanup_mode cleanup_mode;
    - static const char *cleanup_arg;
    - 
    - static enum commit_whence whence;
    --static int sequencer_in_use;
    -+static int sequencer_in_use, rebase_in_progress;
    - static int use_editor = 1, include_status = 1;
    - static int have_option_m;
    - static struct strbuf message = STRBUF_INIT;
    -@@ builtin/commit.c: static void determine_whence(struct wt_status *s)
    - 		whence = FROM_CHERRY_PICK;
    - 		if (file_exists(git_path_seq_dir()))
    - 			sequencer_in_use = 1;
    -+		if (file_exists(git_path_rebase_merge_dir()))
    -+			rebase_in_progress = 1;
    - 	}
    - 	else
    - 		whence = FROM_COMMIT;
     @@ builtin/commit.c: static const char *prepare_index(int argc, const char **argv, const char *prefix
    - 	if (whence != FROM_COMMIT) {
    - 		if (whence == FROM_MERGE)
      			die(_("cannot do a partial commit during a merge."));
    --		else if (whence == FROM_CHERRY_PICK)
    -+		else if (whence == FROM_CHERRY_PICK) {
    -+			if (rebase_in_progress && !sequencer_in_use)
    -+				die(_("cannot do a partial commit during a rebase."));
    + 		else if (is_from_cherry_pick(whence))
      			die(_("cannot do a partial commit during a cherry-pick."));
    -+		}
    ++		else if (is_from_rebase(whence))
    ++			die(_("cannot do a partial commit during a rebase."));
      	}
      
      	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
     @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
    + 	 */
    + 	else if (whence == FROM_MERGE)
    + 		hook_arg1 = "merge";
    +-	else if (is_from_cherry_pick(whence)) {
    ++	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {
    + 		hook_arg1 = "commit";
    + 		hook_arg2 = "CHERRY_PICK_HEAD";
    + 	}
    +@@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const char *prefix,
    + 		run_status(stdout, index_file, prefix, 0, s);
    + 		if (amend)
      			fputs(_(empty_amend_advice), stderr);
    - 		else if (whence == FROM_CHERRY_PICK) {
    +-		else if (is_from_cherry_pick(whence)) {
    ++		else if (is_from_cherry_pick(whence) ||
    ++			 whence == FROM_REBASE_PICK) {
      			fputs(_(empty_cherry_pick_advice), stderr);
    --			if (!sequencer_in_use)
    --				fputs(_(empty_cherry_pick_advice_single), stderr);
    + 			if (whence == FROM_CHERRY_PICK_SINGLE)
    + 				fputs(_(empty_cherry_pick_advice_single), stderr);
     -			else
    -+			if (sequencer_in_use)
    ++			else if (whence == FROM_CHERRY_PICK_MULTI)
      				fputs(_(empty_cherry_pick_advice_multi), stderr);
    -+			else if (rebase_in_progress)
    -+				fputs(_(empty_rebase_advice), stderr);
     +			else
    -+				fputs(_(empty_cherry_pick_advice_single), stderr);
    ++				fputs(_(empty_rebase_pick_advice), stderr);
      		}
      		return 0;
      	}
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
    - 	if (amend && whence != FROM_COMMIT) {
    - 		if (whence == FROM_MERGE)
      			die(_("You are in the middle of a merge -- cannot amend."));
    --		else if (whence == FROM_CHERRY_PICK)
    -+		else if (whence == FROM_CHERRY_PICK) {
    -+			if (rebase_in_progress && !sequencer_in_use)
    -+				die(_("You are in the middle of a rebase -- cannot amend."));
    + 		else if (is_from_cherry_pick(whence))
      			die(_("You are in the middle of a cherry-pick -- cannot amend."));
    -+		}
    ++		else if (whence == FROM_REBASE_PICK)
    ++			die(_("You are in the middle of a rebase -- cannot amend."));
      	}
      	if (fixup_message && squash_message)
      		die(_("Options --squash and --fixup cannot be used together"));
    +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
    + 		use_message = edit_message;
    + 	if (amend && !use_message && !fixup_message)
    + 		use_message = "HEAD";
    +-	if (!use_message && !is_from_cherry_pick(whence) && renew_authorship)
    ++	if (!use_message && !is_from_cherry_pick(whence) &&
    ++	    !is_from_rebase(whence) && renew_authorship)
    + 		die(_("--reset-author can be used only with -C, -c or --amend."));
    + 	if (use_message) {
    + 		use_message_buffer = read_commit_message(use_message);
    +@@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
    + 			author_message_buffer = use_message_buffer;
    + 		}
    + 	}
    +-	if (is_from_cherry_pick(whence) && !renew_authorship) {
    ++	if ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) &&
    ++	    !renew_authorship) {
    + 		author_message = "CHERRY_PICK_HEAD";
    + 		author_message_buffer = read_commit_message(author_message);
    + 	}
     @@ builtin/commit.c: int cmd_commit(int argc, const char **argv, const char *prefix)
    - 			reduce_heads_replace(&parents);
    - 	} else {
      		if (!reflog_msg)
    --			reflog_msg = (whence == FROM_CHERRY_PICK)
    --					? "commit (cherry-pick)"
    --					: "commit";
    -+			reflog_msg = (whence != FROM_CHERRY_PICK)
    -+					? "commit"
    -+					: rebase_in_progress && !sequencer_in_use
    + 			reflog_msg = is_from_cherry_pick(whence)
    + 					? "commit (cherry-pick)"
    ++					: is_from_rebase(whence)
     +					? "commit (rebase)"
    -+					: "commit (cherry-pick)";
    + 					: "commit";
      		commit_list_insert(current_head, &parents);
      	}
    +
    + ## sequencer.c ##
    +@@ sequencer.c: static int try_to_commit(struct repository *r,
    + 	return res;
    + }
    + 
    ++static int write_rebase_head(struct object_id *oid)
    ++{
    ++	if (update_ref("rebase", "REBASE_HEAD", oid,
    ++		       NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
    ++		return error(_("could not update %s"), "REBASE_HEAD");
    ++
    ++	return 0;
    ++}
    ++
    + static int do_commit(struct repository *r,
    + 		     const char *msg_file, const char *author,
    +-		     struct replay_opts *opts, unsigned int flags)
    ++		     struct replay_opts *opts, unsigned int flags,
    ++		     struct object_id *oid)
    + {
    + 	int res = 1;
    + 
    +@@ sequencer.c: static int do_commit(struct repository *r,
    + 			return res;
    + 		}
    + 	}
    +-	if (res == 1)
    ++	if (res == 1) {
    ++		if (is_rebase_i(opts) && oid)
    ++			if (write_rebase_head(oid))
    ++			    return -1;
    + 		return run_git_commit(r, msg_file, opts, flags);
    ++	}
    + 
    + 	return res;
    + }
    +@@ sequencer.c: static int do_pick_commit(struct repository *r,
    + 		flags |= ALLOW_EMPTY;
    + 	if (!opts->no_commit) {
    + 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
    +-			res = do_commit(r, msg_file, author, opts, flags);
    ++			res = do_commit(r, msg_file, author, opts, flags,
    ++					commit? &commit->object.oid : NULL);
    + 		else
    + 			res = error(_("unable to parse commit author"));
    + 		*check_todo = !!(flags & EDIT_MSG);
    +@@ sequencer.c: static int make_patch(struct repository *r,
    + 	p = short_commit_name(commit);
    + 	if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
    + 		return -1;
    +-	if (update_ref("rebase", "REBASE_HEAD", &commit->object.oid,
    +-		       NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
    +-		res |= error(_("could not update %s"), "REBASE_HEAD");
    ++	res |= write_rebase_head(&commit->object.oid);
    + 
    + 	strbuf_addf(&buf, "%s/patch", get_dir(opts));
    + 	memset(&log_tree_opt, 0, sizeof(log_tree_opt));
    +@@ sequencer.c: int todo_list_rearrange_squash(struct todo_list *todo_list)
    + int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
    + {
    + 	if (file_exists(git_path_cherry_pick_head(r))) {
    +-		*whence = file_exists(git_path_seq_dir()) ?
    +-			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
    ++		struct object_id cherry_pick_head, rebase_head;
    ++
    ++		if (file_exists(git_path_seq_dir()))
    ++			*whence = FROM_CHERRY_PICK_MULTI;
    ++		if (file_exists(rebase_path()) &&
    ++		    !get_oid("REBASE_HEAD", &rebase_head) &&
    ++		    !get_oid("CHERRY_PICK_HEAD", &cherry_pick_head) &&
    ++		    oideq(&rebase_head, &cherry_pick_head))
    ++			*whence = FROM_REBASE_PICK;
    ++		else
    ++			*whence = FROM_CHERRY_PICK_SINGLE;
    ++
    + 		return 1;
    + 	}
      
     
      ## t/t3403-rebase-skip.sh ##
    -@@ t/t3403-rebase-skip.sh: test_expect_success 'moved back to branch correctly' '
    +@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon picking empty commit' '
    + 	test_must_fail git rebase -i --onto goodbye \
    + 		amended-goodbye^ amended-goodbye 2>err &&
    + 	test_i18ngrep "previous cherry-pick is now empty" err &&
    +-	test_i18ngrep "git cherry-pick --skip" err &&
    ++	test_i18ngrep "git rebase --skip" err &&
    + 	test_must_fail git commit &&
    +-	test_i18ngrep "git cherry-pick --skip" err
    ++	test_i18ngrep "git rebase --skip" err
    + '
      
    - test_debug 'gitk --all & sleep 1'
    + test_expect_success 'correct authorship when committing empty pick' '
    +@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon rewording empty commit' '
    + 			--onto goodbye amended-goodbye^ amended-goodbye 2>err
    + 	) &&
    + 	test_i18ngrep "previous cherry-pick is now empty" err &&
    +-	test_i18ngrep "git cherry-pick --skip" err &&
    ++	test_i18ngrep "git rebase --skip" err &&
    + 	test_must_fail git commit &&
    +-	test_i18ngrep "git cherry-pick --skip" err
    ++	test_i18ngrep "git rebase --skip" err
    + '
      
    -+test_expect_success 'correct advice upon empty commit' '
    -+	git checkout -b rebase-skip &&
    -+	test_commit a1 &&
    -+	test_tick &&
    -+	git commit --amend -m amended --no-edit &&
    -+	test_must_fail git rebase -m --onto a1 HEAD^ 2>err &&
    + test_expect_success 'correct advice upon editing empty commit' '
    +@@ t/t3403-rebase-skip.sh: test_expect_success 'correct advice upon editing empty commit' '
    + 			--onto goodbye amended-goodbye^ amended-goodbye 2>err
    + 	) &&
    + 	test_i18ngrep "previous cherry-pick is now empty" err &&
    +-	test_i18ngrep "git cherry-pick --skip" err &&
    ++	test_i18ngrep "git rebase --skip" err &&
    + 	test_must_fail git commit &&
     +	test_i18ngrep "git rebase --skip" err
     +'
     +
    - test_done
    ++test_expect_success 'correct advice upon cherry-picking an empty commit during a rebase' '
    ++	test_when_finished "git rebase --abort" &&
    ++	(
    ++		set_fake_editor &&
    ++		test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_amended-goodbye" \
    ++			git rebase -i goodbye^ goodbye 2>err
    ++	) &&
    ++	test_i18ngrep "previous cherry-pick is now empty" err &&
    ++	test_i18ngrep "git cherry-pick --skip" err &&
    ++	test_must_fail git commit 2>err &&
    ++	test_i18ngrep "git cherry-pick --skip" err
    ++'
    ++
    ++test_expect_success 'correct advice upon multi cherry-pick picking an empty commit during a rebase' '
    ++	test_when_finished "git rebase --abort" &&
    ++	(
    ++		set_fake_editor &&
    ++		test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_goodbye_amended-goodbye" \
    ++			git rebase -i goodbye^^ goodbye 2>err
    ++	) &&
    ++	test_i18ngrep "previous cherry-pick is now empty" err &&
    ++	test_i18ngrep "git cherry-pick --skip" err &&
    ++	test_must_fail git commit 2>err &&
    + 	test_i18ngrep "git cherry-pick --skip" err
    + '
    + 
    +
    + ## t/t3404-rebase-interactive.sh ##
    +@@ t/t3404-rebase-interactive.sh: test_expect_success 'post-commit hook is called' '
    + 	test_cmp expect actual
    + '
    + 
    ++test_expect_success 'correct error message for partial commit after empty pick' '
    ++	test_when_finished "git rebase --abort" &&
    ++	(
    ++		set_fake_editor &&
    ++		FAKE_LINES="2 1 1" &&
    ++		export FAKE_LINES &&
    ++		test_must_fail git rebase -i A D
    ++	) &&
    ++	echo x >file1 &&
    ++	test_must_fail git commit file1 2>err &&
    ++	test_i18ngrep "cannot do a partial commit during a rebase." err
    ++'
    ++
    ++test_expect_success 'correct error message for commit --amend after empty pick' '
    ++	test_when_finished "git rebase --abort" &&
    ++	(
    ++		set_fake_editor &&
    ++		FAKE_LINES="1 1" &&
    ++		export FAKE_LINES &&
    ++		test_must_fail git rebase -i A D
    ++	) &&
    ++	echo x>file1 &&
    ++	test_must_fail git commit -a --amend 2>err &&
    ++	test_i18ngrep "middle of a rebase -- cannot amend." err
    ++'
    ++
    + # This must be the last test in this file
    + test_expect_success '$EDITOR and friends are unchanged' '
    + 	test_editor_unchanged
    +
    + ## wt-status.h ##
    +@@ wt-status.h: enum commit_whence {
    + 	FROM_COMMIT,     /* normal */
    + 	FROM_MERGE,      /* commit came from merge */
    + 	FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
    +-	FROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */
    ++	FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */
    ++	FROM_REBASE_PICK /* commit came from a pick/reword/edit */
    + };
    + 
    + static inline int is_from_cherry_pick(enum commit_whence whence)
    +@@ wt-status.h: static inline int is_from_cherry_pick(enum commit_whence whence)
    + 		whence == FROM_CHERRY_PICK_MULTI;
    + }
    + 
    ++static inline int is_from_rebase(enum commit_whence whence)
    ++{
    ++	return whence == FROM_REBASE_PICK;
    ++}
    ++
    + struct wt_status_change_data {
    + 	int worktree_status;
    + 	int index_status;
 -:  ---------- >  8:  6a00263595 [RFC] rebase: fix advice when a fixup creates an empty commit
 -:  ---------- >  9:  8b5ca6b60b [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts

-- 
2.24.0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v2 1/9] t3404: use test_cmp_rev
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 17:39     ` Junio C Hamano
  2019-12-06 16:06   ` [PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit` Phillip Wood
                     ` (7 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

There are a number of places where we compare two revisions with
    test $(git rev-parse rev1) = $(git rev-parse rev2)
when these fail there's no indication what has gone wrong and you need
to be running with `-x` to see where the test has failed. Lets use
test_cmp_rev instead.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3404-rebase-interactive.sh | 38 +++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c573c99069..d8a05fd439 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -191,7 +191,7 @@ test_expect_success 'no changes are a nop' '
 	git checkout branch2 &&
 	git rebase -i F &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
-	test $(git rev-parse I) = $(git rev-parse HEAD)
+	test_cmp_rev I HEAD
 '
 
 test_expect_success 'test the [branch] option' '
@@ -200,16 +200,16 @@ test_expect_success 'test the [branch] option' '
 	git commit -m "stop here" &&
 	git rebase -i F branch2 &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
-	test $(git rev-parse I) = $(git rev-parse branch2) &&
-	test $(git rev-parse I) = $(git rev-parse HEAD)
+	test_cmp_rev I branch2 &&
+	test_cmp_rev I HEAD
 '
 
 test_expect_success 'test --onto <branch>' '
 	git checkout -b test-onto branch2 &&
 	git rebase -i --onto branch1 F &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/test-onto" &&
-	test $(git rev-parse HEAD^) = $(git rev-parse branch1) &&
-	test $(git rev-parse I) = $(git rev-parse branch2)
+	test_cmp_rev HEAD^ branch1 &&
+	test_cmp_rev I branch2
 '
 
 test_expect_success 'rebase on top of a non-conflicting commit' '
@@ -218,12 +218,12 @@ test_expect_success 'rebase on top of a non-conflicting commit' '
 	git rebase -i branch2 &&
 	test file6 = $(git diff --name-only original-branch1) &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
-	test $(git rev-parse I) = $(git rev-parse branch2) &&
-	test $(git rev-parse I) = $(git rev-parse HEAD~2)
+	test_cmp_rev I branch2 &&
+	test_cmp_rev I HEAD~2
 '
 
 test_expect_success 'reflog for the branch shows state before rebase' '
-	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)
+	test_cmp_rev branch1@{1} original-branch1
 '
 
 test_expect_success 'reflog for the branch shows correct finish message' '
@@ -280,7 +280,7 @@ test_expect_success 'show conflicted patch' '
 
 test_expect_success 'abort' '
 	git rebase --abort &&
-	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
+	test_cmp_rev new-branch1 HEAD &&
 	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
 	test_path_is_missing .git/rebase-merge
 '
@@ -323,7 +323,7 @@ test_expect_success 'retain authorship w/ conflicts' '
 	echo resolved >conflict &&
 	git add conflict &&
 	git rebase --continue &&
-	test $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&
+	test_cmp_rev conflict-a^0 HEAD^ &&
 	git show >out &&
 	grep AttributeMe out
 '
@@ -340,7 +340,7 @@ test_expect_success 'squash' '
 			git rebase -i --onto master HEAD~2
 	) &&
 	test B = $(cat file7) &&
-	test $(git rev-parse HEAD^) = $(git rev-parse master)
+	test_cmp_rev HEAD^ master
 '
 
 test_expect_success 'retain authorship when squashing' '
@@ -399,9 +399,9 @@ test_expect_success REBASE_P 'preserve merges with -p' '
 	git update-index --refresh &&
 	git diff-files --quiet &&
 	git diff-index --quiet --cached HEAD -- &&
-	test $(git rev-parse HEAD~6) = $(git rev-parse branch1) &&
-	test $(git rev-parse HEAD~4^2) = $(git rev-parse to-be-preserved) &&
-	test $(git rev-parse HEAD^^2^) = $(git rev-parse HEAD^^^) &&
+	test_cmp_rev HEAD~6 branch1 &&
+	test_cmp_rev HEAD~4^2 to-be-preserved &&
+	test_cmp_rev HEAD^^2^ HEAD^^^ &&
 	test $(git show HEAD~5:file1) = B &&
 	test $(git show HEAD~3:file1) = C &&
 	test $(git show HEAD:file1) = E &&
@@ -433,7 +433,7 @@ test_expect_success '--continue tries to commit' '
 		git add file1 &&
 		FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
 	) &&
-	test $(git rev-parse HEAD^) = $(git rev-parse new-branch1) &&
+	test_cmp_rev HEAD^ new-branch1 &&
 	git show HEAD | grep chouette
 '
 
@@ -740,7 +740,7 @@ test_expect_success 'do "noop" when there is nothing to cherry-pick' '
 		--author="Somebody else <somebody@else.com>" &&
 	test $(git rev-parse branch3) != $(git rev-parse branch4) &&
 	git rebase -i branch3 &&
-	test $(git rev-parse branch3) = $(git rev-parse branch4)
+	test_cmp_rev branch3 branch4
 
 '
 
@@ -799,7 +799,7 @@ test_expect_success 'rebase -i continue with unstaged submodule' '
 	test_must_fail git rebase -i submodule-base &&
 	git reset &&
 	git rebase --continue &&
-	test $(git rev-parse submodule-base) = $(git rev-parse HEAD)
+	test_cmp_rev submodule-base HEAD
 '
 
 test_expect_success 'avoid unnecessary reset' '
@@ -822,7 +822,7 @@ test_expect_success 'reword' '
 			git rebase -i A &&
 		git show HEAD | grep "E changed" &&
 		test $(git rev-parse master) != $(git rev-parse HEAD) &&
-		test $(git rev-parse master^) = $(git rev-parse HEAD^) &&
+		test_cmp_rev master^ HEAD^ &&
 		FAKE_LINES="1 2 reword 3 4" FAKE_COMMIT_MESSAGE="D changed" \
 			git rebase -i A &&
 		git show HEAD^ | grep "D changed" &&
@@ -886,7 +886,7 @@ test_expect_success 'always cherry-pick with --no-ff' '
 		git diff HEAD~$p original-no-ff-branch~$p > out &&
 		test_must_be_empty out
 	done &&
-	test $(git rev-parse HEAD~3) = $(git rev-parse original-no-ff-branch~3) &&
+	test_cmp_rev HEAD~3 original-no-ff-branch~3 &&
 	git diff HEAD~3 original-no-ff-branch~3 > out &&
 	test_must_be_empty out
 '
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit`
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 3/9] cherry-pick: check commit error messages Phillip Wood
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
`git commit` learned to suggest to run `git cherry-pick --skip` when
trying to cherry-pick an empty patch, but that was never tested for.

Here is a test that verifies that a message is given to the user that
contains the correct invocation.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3510-cherry-pick-sequence.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 793bcc7fe3..5b94fdaa67 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -123,7 +123,8 @@ test_expect_success 'revert --skip to skip commit' '
 test_expect_success 'skip "empty" commit' '
 	pristine_detach picked &&
 	test_commit dummy foo d &&
-	test_must_fail git cherry-pick anotherpick &&
+	test_must_fail git cherry-pick anotherpick 2>err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
 	git cherry-pick --skip &&
 	test_cmp_rev dummy HEAD
 '
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 3/9] cherry-pick: check commit error messages
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit` Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit Phillip Wood
                     ` (5 subsequent siblings)
  8 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

We disallow partial commits and amending when CHERRY_PICK_HEAD
exists. Add a couple of tests to check the error message printed in each
case before we refactor the code responsible for this.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3507-cherry-pick-conflict.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh
index 9b9b4ca8d4..c9715bf674 100755
--- a/t/t3507-cherry-pick-conflict.sh
+++ b/t/t3507-cherry-pick-conflict.sh
@@ -161,6 +161,29 @@ test_expect_success 'successful commit clears CHERRY_PICK_HEAD' '
 
 	test_must_fail git rev-parse --verify CHERRY_PICK_HEAD
 '
+
+test_expect_success 'partial commit of cherry-pick fails' '
+	pristine_detach initial &&
+
+	test_must_fail git cherry-pick picked &&
+	echo resolved >foo &&
+	git add foo &&
+	test_must_fail git commit foo 2>err &&
+
+	test_i18ngrep "cannot do a partial commit during a cherry-pick." err
+'
+
+test_expect_success 'commit --amend of cherry-pick fails' '
+	pristine_detach initial &&
+
+	test_must_fail git cherry-pick picked &&
+	echo resolved >foo &&
+	git add foo &&
+	test_must_fail git commit --amend 2>err &&
+
+	test_i18ngrep "in the middle of a cherry-pick -- cannot amend." err
+'
+
 test_expect_success 'successful final commit clears cherry-pick state' '
 	pristine_detach initial &&
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
                     ` (2 preceding siblings ...)
  2019-12-06 16:06   ` [PATCH v2 3/9] cherry-pick: check commit error messages Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 5/9] commit: use enum value for multiple cherry-picks Phillip Wood
                     ` (4 subsequent siblings)
  8 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

`git commit` relies on the presence of CHERRY_PICK_HEAD to show the
correct error message in the case of an empty pick.  This fixes a
regression introduced by the conversion from shell to C. In the shell
version everything was a cherry-pick as far as the sequencer code was
concerned so it always wrote CHERRY_PICK_HEAD. The conversion to C
forgot to update the code that creates CHERRY_PICK_HEAD. We do not want
to create CHERRY_PICK_HEAD for fixup and squash commands as that would
prevent `git commit --amend` from running.

Note that the error message shown by `git commit` for an empty pick
during a rebase is currently wrong as it talks about running `git
cherry-pick --skip` rather than `git rebase --skip`. This will be fixed
in a future commit which is why the tests are in t3403-rebase-skip.sh.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c            |  4 +++-
 t/t3403-rebase-skip.sh | 53 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 408b7885c7..4e0370277b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1916,7 +1916,9 @@ static int do_pick_commit(struct repository *r,
 	 * However, if the merge did not even start, then we don't want to
 	 * write it at all.
 	 */
-	if (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) &&
+	if ((command == TODO_PICK || command == TODO_REWORD ||
+	     command == TODO_EDIT) && !opts->no_commit &&
+	    (res == 0 || res == 1) &&
 	    update_ref(NULL, "CHERRY_PICK_HEAD", &commit->object.oid, NULL,
 		       REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
 		res = -1;
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index ee8a8dba52..db7e917248 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -29,6 +29,13 @@ test_expect_success setup '
 	test_tick &&
 	git commit -m reverted-goodbye &&
 	git tag reverted-goodbye &&
+	git checkout goodbye &&
+	test_tick &&
+	GIT_AUTHOR_NAME="Another Author" \
+		GIT_AUTHOR_EMAIL="another.author@example.com" \
+		git commit --amend --no-edit -m amended-goodbye &&
+	test_tick &&
+	git tag amended-goodbye &&
 
 	git checkout -f skip-reference &&
 	echo moo > hello &&
@@ -85,6 +92,52 @@ test_expect_success 'moved back to branch correctly' '
 
 test_debug 'gitk --all & sleep 1'
 
+test_expect_success 'correct advice upon picking empty commit' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -i --onto goodbye \
+		amended-goodbye^ amended-goodbye 2>err &&
+	test_i18ngrep "previous cherry-pick is now empty" err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
+	test_must_fail git commit &&
+	test_i18ngrep "git cherry-pick --skip" err
+'
+
+test_expect_success 'correct authorship when committing empty pick' '
+	test_when_finished "git rebase --abort" &&
+	test_must_fail git rebase -i --onto goodbye \
+		amended-goodbye^ amended-goodbye &&
+	git commit --allow-empty &&
+	git log --pretty=format:"%an <%ae>%n%ad%B" -1 amended-goodbye >expect &&
+	git log --pretty=format:"%an <%ae>%n%ad%B" -1 HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'correct advice upon rewording empty commit' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="reword 1" git rebase -i \
+			--onto goodbye amended-goodbye^ amended-goodbye 2>err
+	) &&
+	test_i18ngrep "previous cherry-pick is now empty" err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
+	test_must_fail git commit &&
+	test_i18ngrep "git cherry-pick --skip" err
+'
+
+test_expect_success 'correct advice upon editing empty commit' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="edit 1" git rebase -i \
+			--onto goodbye amended-goodbye^ amended-goodbye 2>err
+	) &&
+	test_i18ngrep "previous cherry-pick is now empty" err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
+	test_must_fail git commit &&
+	test_i18ngrep "git cherry-pick --skip" err
+'
+
 test_expect_success 'fixup that empties commit fails' '
 	test_when_finished "git rebase --abort" &&
 	(
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 5/9] commit: use enum value for multiple cherry-picks
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
                     ` (3 preceding siblings ...)
  2019-12-06 16:06   ` [PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 18:13     ` Junio C Hamano
  2019-12-06 16:06   ` [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer Phillip Wood
                     ` (3 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add FROM_CHERRY_PICK_MULTI for a sequence of cherry-picks rather than
using a separate variable.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c | 25 +++++++++++--------------
 wt-status.h      |  9 ++++++++-
 2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d85e0ad560..3b463522be 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -122,7 +122,6 @@ static enum commit_msg_cleanup_mode cleanup_mode;
 static const char *cleanup_arg;
 
 static enum commit_whence whence;
-static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int have_option_m;
 static struct strbuf message = STRBUF_INIT;
@@ -179,11 +178,9 @@ static void determine_whence(struct wt_status *s)
 {
 	if (file_exists(git_path_merge_head(the_repository)))
 		whence = FROM_MERGE;
-	else if (file_exists(git_path_cherry_pick_head(the_repository))) {
-		whence = FROM_CHERRY_PICK;
-		if (file_exists(git_path_seq_dir()))
-			sequencer_in_use = 1;
-	}
+	else if (file_exists(git_path_cherry_pick_head(the_repository)))
+		whence = file_exists(git_path_seq_dir()) ?
+			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
 	else
 		whence = FROM_COMMIT;
 	if (s)
@@ -453,7 +450,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 	if (whence != FROM_COMMIT) {
 		if (whence == FROM_MERGE)
 			die(_("cannot do a partial commit during a merge."));
-		else if (whence == FROM_CHERRY_PICK)
+		else if (is_from_cherry_pick(whence))
 			die(_("cannot do a partial commit during a cherry-pick."));
 	}
 
@@ -771,7 +768,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	else if (whence == FROM_MERGE)
 		hook_arg1 = "merge";
-	else if (whence == FROM_CHERRY_PICK) {
+	else if (is_from_cherry_pick(whence)) {
 		hook_arg1 = "commit";
 		hook_arg2 = "CHERRY_PICK_HEAD";
 	}
@@ -948,9 +945,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
-		else if (whence == FROM_CHERRY_PICK) {
+		else if (is_from_cherry_pick(whence)) {
 			fputs(_(empty_cherry_pick_advice), stderr);
-			if (!sequencer_in_use)
+			if (whence == FROM_CHERRY_PICK_SINGLE)
 				fputs(_(empty_cherry_pick_advice_single), stderr);
 			else
 				fputs(_(empty_cherry_pick_advice_multi), stderr);
@@ -1143,7 +1140,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (amend && whence != FROM_COMMIT) {
 		if (whence == FROM_MERGE)
 			die(_("You are in the middle of a merge -- cannot amend."));
-		else if (whence == FROM_CHERRY_PICK)
+		else if (is_from_cherry_pick(whence))
 			die(_("You are in the middle of a cherry-pick -- cannot amend."));
 	}
 	if (fixup_message && squash_message)
@@ -1166,7 +1163,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_message = edit_message;
 	if (amend && !use_message && !fixup_message)
 		use_message = "HEAD";
-	if (!use_message && whence != FROM_CHERRY_PICK && renew_authorship)
+	if (!use_message && !is_from_cherry_pick(whence) && renew_authorship)
 		die(_("--reset-author can be used only with -C, -c or --amend."));
 	if (use_message) {
 		use_message_buffer = read_commit_message(use_message);
@@ -1175,7 +1172,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 			author_message_buffer = use_message_buffer;
 		}
 	}
-	if (whence == FROM_CHERRY_PICK && !renew_authorship) {
+	if (is_from_cherry_pick(whence) && !renew_authorship) {
 		author_message = "CHERRY_PICK_HEAD";
 		author_message_buffer = read_commit_message(author_message);
 	}
@@ -1589,7 +1586,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 			reduce_heads_replace(&parents);
 	} else {
 		if (!reflog_msg)
-			reflog_msg = (whence == FROM_CHERRY_PICK)
+			reflog_msg = is_from_cherry_pick(whence)
 					? "commit (cherry-pick)"
 					: "commit";
 		commit_list_insert(current_head, &parents);
diff --git a/wt-status.h b/wt-status.h
index 64f1ddc9fd..0098fdb0b5 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -39,9 +39,16 @@ enum show_ignored_type {
 enum commit_whence {
 	FROM_COMMIT,     /* normal */
 	FROM_MERGE,      /* commit came from merge */
-	FROM_CHERRY_PICK /* commit came from cherry-pick */
+	FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
+	FROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */
 };
 
+static inline int is_from_cherry_pick(enum commit_whence whence)
+{
+	return whence == FROM_CHERRY_PICK_SINGLE ||
+		whence == FROM_CHERRY_PICK_MULTI;
+}
+
 struct wt_status_change_data {
 	int worktree_status;
 	int index_status;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
                     ` (4 preceding siblings ...)
  2019-12-06 16:06   ` [PATCH v2 5/9] commit: use enum value for multiple cherry-picks Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 18:24     ` Junio C Hamano
  2019-12-06 16:06   ` [PATCH v2 7/9] commit: give correct advice for empty commit during a rebase Phillip Wood
                     ` (2 subsequent siblings)
  8 siblings, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Working out which command wants to create a commit requires detailed
knowledge of the sequencer internals and that knowledge is going to
increase in subsequent commits. With that in mind lets encapsulate that
knowledge in sequencer.c rather than spreading it into builtin/commit.c.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c |  5 +----
 sequencer.c      | 13 ++++++++++++-
 sequencer.h      |  3 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 3b463522be..d8d4c8e419 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)
 {
 	if (file_exists(git_path_merge_head(the_repository)))
 		whence = FROM_MERGE;
-	else if (file_exists(git_path_cherry_pick_head(the_repository)))
-		whence = file_exists(git_path_seq_dir()) ?
-			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
-	else
+	else if (!sequencer_determine_whence(the_repository, &whence))
 		whence = FROM_COMMIT;
 	if (s)
 		s->whence = whence;
diff --git a/sequencer.c b/sequencer.c
index 4e0370277b..98e007556c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
 
-GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
+static GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
 
 static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
@@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 
 	return 0;
 }
+
+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
+{
+	if (file_exists(git_path_cherry_pick_head(r))) {
+		*whence = file_exists(git_path_seq_dir()) ?
+			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
+		return 1;
+	}
+
+	return 0;
+}
diff --git a/sequencer.h b/sequencer.h
index 56881a6baa..8d451dbfcb 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -3,12 +3,12 @@
 
 #include "cache.h"
 #include "strbuf.h"
+#include "wt-status.h"
 
 struct commit;
 struct repository;
 
 const char *git_path_commit_editmsg(void);
-const char *git_path_seq_dir(void);
 const char *rebase_path_todo(void);
 const char *rebase_path_todo_backup(void);
 
@@ -202,4 +202,5 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 void sequencer_post_commit_cleanup(struct repository *r);
 int sequencer_get_last_command(struct repository* r,
 			       enum replay_action *action);
+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
 #endif /* SEQUENCER_H */
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 7/9] commit: give correct advice for empty commit during a rebase
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
                     ` (5 preceding siblings ...)
  2019-12-06 16:06   ` [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit Phillip Wood
  2019-12-06 16:06   ` [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts Phillip Wood
  8 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood, Johannes Schindelin

From: Phillip Wood <phillip.wood@dunelm.org.uk>

In dcb500dc16c (cherry-pick/revert: advise using --skip, 2019-07-02),
`git commit` learned to suggest to run `git cherry-pick --skip` when
trying to cherry-pick an empty patch.

However, it was overlooked that there are more conditions than just a
`git cherry-pick` when this advice is printed (which originally
suggested the neutral `git reset`): the same can happen during a rebase.

Let's suggest the correct command, even during a rebase.

While at it, we adjust more places in `builtin/commit.c` that
incorrectly assumed that the presence of a `CHERRY_PICK_HEAD` meant that
surely this must be a `cherry-pick` in progress.

Note: we take pains to handle the situation when a user runs a `git
cherry-pick` _during_ a rebase. This is quite valid (e.g. in an `exec`
line in an interactive rebase). On the other hand, it is not possible to
run a rebase during a cherry-pick, meaning: if both `rebase-merge/` and
`sequencer/` exist or CHERRY_PICK_HEAD and REBASE_HEAD point to the same
commit , we still want to advise to use `git cherry-pick --skip`.

Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c              | 24 ++++++++++++++++-----
 sequencer.c                   | 39 ++++++++++++++++++++++++++++-------
 t/t3403-rebase-skip.sh        | 36 +++++++++++++++++++++++++++-----
 t/t3404-rebase-interactive.sh | 26 +++++++++++++++++++++++
 wt-status.h                   |  8 ++++++-
 5 files changed, 114 insertions(+), 19 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d8d4c8e419..c3b6b8a6bd 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -59,6 +59,9 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
 "    git commit --allow-empty\n"
 "\n");
 
+static const char empty_rebase_pick_advice[] =
+N_("Otherwise, please use 'git rebase --skip'\n");
+
 static const char empty_cherry_pick_advice_single[] =
 N_("Otherwise, please use 'git cherry-pick --skip'\n");
 
@@ -449,6 +452,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 			die(_("cannot do a partial commit during a merge."));
 		else if (is_from_cherry_pick(whence))
 			die(_("cannot do a partial commit during a cherry-pick."));
+		else if (is_from_rebase(whence))
+			die(_("cannot do a partial commit during a rebase."));
 	}
 
 	if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec))
@@ -765,7 +770,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	else if (whence == FROM_MERGE)
 		hook_arg1 = "merge";
-	else if (is_from_cherry_pick(whence)) {
+	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {
 		hook_arg1 = "commit";
 		hook_arg2 = "CHERRY_PICK_HEAD";
 	}
@@ -942,12 +947,15 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		run_status(stdout, index_file, prefix, 0, s);
 		if (amend)
 			fputs(_(empty_amend_advice), stderr);
-		else if (is_from_cherry_pick(whence)) {
+		else if (is_from_cherry_pick(whence) ||
+			 whence == FROM_REBASE_PICK) {
 			fputs(_(empty_cherry_pick_advice), stderr);
 			if (whence == FROM_CHERRY_PICK_SINGLE)
 				fputs(_(empty_cherry_pick_advice_single), stderr);
-			else
+			else if (whence == FROM_CHERRY_PICK_MULTI)
 				fputs(_(empty_cherry_pick_advice_multi), stderr);
+			else
+				fputs(_(empty_rebase_pick_advice), stderr);
 		}
 		return 0;
 	}
@@ -1139,6 +1147,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 			die(_("You are in the middle of a merge -- cannot amend."));
 		else if (is_from_cherry_pick(whence))
 			die(_("You are in the middle of a cherry-pick -- cannot amend."));
+		else if (whence == FROM_REBASE_PICK)
+			die(_("You are in the middle of a rebase -- cannot amend."));
 	}
 	if (fixup_message && squash_message)
 		die(_("Options --squash and --fixup cannot be used together"));
@@ -1160,7 +1170,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		use_message = edit_message;
 	if (amend && !use_message && !fixup_message)
 		use_message = "HEAD";
-	if (!use_message && !is_from_cherry_pick(whence) && renew_authorship)
+	if (!use_message && !is_from_cherry_pick(whence) &&
+	    !is_from_rebase(whence) && renew_authorship)
 		die(_("--reset-author can be used only with -C, -c or --amend."));
 	if (use_message) {
 		use_message_buffer = read_commit_message(use_message);
@@ -1169,7 +1180,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 			author_message_buffer = use_message_buffer;
 		}
 	}
-	if (is_from_cherry_pick(whence) && !renew_authorship) {
+	if ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) &&
+	    !renew_authorship) {
 		author_message = "CHERRY_PICK_HEAD";
 		author_message_buffer = read_commit_message(author_message);
 	}
@@ -1585,6 +1597,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (!reflog_msg)
 			reflog_msg = is_from_cherry_pick(whence)
 					? "commit (cherry-pick)"
+					: is_from_rebase(whence)
+					? "commit (rebase)"
 					: "commit";
 		commit_list_insert(current_head, &parents);
 	}
diff --git a/sequencer.c b/sequencer.c
index 98e007556c..6e153fea76 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1430,9 +1430,19 @@ static int try_to_commit(struct repository *r,
 	return res;
 }
 
+static int write_rebase_head(struct object_id *oid)
+{
+	if (update_ref("rebase", "REBASE_HEAD", oid,
+		       NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
+		return error(_("could not update %s"), "REBASE_HEAD");
+
+	return 0;
+}
+
 static int do_commit(struct repository *r,
 		     const char *msg_file, const char *author,
-		     struct replay_opts *opts, unsigned int flags)
+		     struct replay_opts *opts, unsigned int flags,
+		     struct object_id *oid)
 {
 	int res = 1;
 
@@ -1457,8 +1467,12 @@ static int do_commit(struct repository *r,
 			return res;
 		}
 	}
-	if (res == 1)
+	if (res == 1) {
+		if (is_rebase_i(opts) && oid)
+			if (write_rebase_head(oid))
+			    return -1;
 		return run_git_commit(r, msg_file, opts, flags);
+	}
 
 	return res;
 }
@@ -1945,7 +1959,8 @@ static int do_pick_commit(struct repository *r,
 		flags |= ALLOW_EMPTY;
 	if (!opts->no_commit) {
 		if (author || command == TODO_REVERT || (flags & AMEND_MSG))
-			res = do_commit(r, msg_file, author, opts, flags);
+			res = do_commit(r, msg_file, author, opts, flags,
+					commit? &commit->object.oid : NULL);
 		else
 			res = error(_("unable to parse commit author"));
 		*check_todo = !!(flags & EDIT_MSG);
@@ -2960,9 +2975,7 @@ static int make_patch(struct repository *r,
 	p = short_commit_name(commit);
 	if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
 		return -1;
-	if (update_ref("rebase", "REBASE_HEAD", &commit->object.oid,
-		       NULL, REF_NO_DEREF, UPDATE_REFS_MSG_ON_ERR))
-		res |= error(_("could not update %s"), "REBASE_HEAD");
+	res |= write_rebase_head(&commit->object.oid);
 
 	strbuf_addf(&buf, "%s/patch", get_dir(opts));
 	memset(&log_tree_opt, 0, sizeof(log_tree_opt));
@@ -5260,8 +5273,18 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
 {
 	if (file_exists(git_path_cherry_pick_head(r))) {
-		*whence = file_exists(git_path_seq_dir()) ?
-			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
+		struct object_id cherry_pick_head, rebase_head;
+
+		if (file_exists(git_path_seq_dir()))
+			*whence = FROM_CHERRY_PICK_MULTI;
+		if (file_exists(rebase_path()) &&
+		    !get_oid("REBASE_HEAD", &rebase_head) &&
+		    !get_oid("CHERRY_PICK_HEAD", &cherry_pick_head) &&
+		    oideq(&rebase_head, &cherry_pick_head))
+			*whence = FROM_REBASE_PICK;
+		else
+			*whence = FROM_CHERRY_PICK_SINGLE;
+
 		return 1;
 	}
 
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index db7e917248..a927774910 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -97,9 +97,9 @@ test_expect_success 'correct advice upon picking empty commit' '
 	test_must_fail git rebase -i --onto goodbye \
 		amended-goodbye^ amended-goodbye 2>err &&
 	test_i18ngrep "previous cherry-pick is now empty" err &&
-	test_i18ngrep "git cherry-pick --skip" err &&
+	test_i18ngrep "git rebase --skip" err &&
 	test_must_fail git commit &&
-	test_i18ngrep "git cherry-pick --skip" err
+	test_i18ngrep "git rebase --skip" err
 '
 
 test_expect_success 'correct authorship when committing empty pick' '
@@ -120,9 +120,9 @@ test_expect_success 'correct advice upon rewording empty commit' '
 			--onto goodbye amended-goodbye^ amended-goodbye 2>err
 	) &&
 	test_i18ngrep "previous cherry-pick is now empty" err &&
-	test_i18ngrep "git cherry-pick --skip" err &&
+	test_i18ngrep "git rebase --skip" err &&
 	test_must_fail git commit &&
-	test_i18ngrep "git cherry-pick --skip" err
+	test_i18ngrep "git rebase --skip" err
 '
 
 test_expect_success 'correct advice upon editing empty commit' '
@@ -133,8 +133,34 @@ test_expect_success 'correct advice upon editing empty commit' '
 			--onto goodbye amended-goodbye^ amended-goodbye 2>err
 	) &&
 	test_i18ngrep "previous cherry-pick is now empty" err &&
-	test_i18ngrep "git cherry-pick --skip" err &&
+	test_i18ngrep "git rebase --skip" err &&
 	test_must_fail git commit &&
+	test_i18ngrep "git rebase --skip" err
+'
+
+test_expect_success 'correct advice upon cherry-picking an empty commit during a rebase' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_amended-goodbye" \
+			git rebase -i goodbye^ goodbye 2>err
+	) &&
+	test_i18ngrep "previous cherry-pick is now empty" err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
+	test_must_fail git commit 2>err &&
+	test_i18ngrep "git cherry-pick --skip" err
+'
+
+test_expect_success 'correct advice upon multi cherry-pick picking an empty commit during a rebase' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		test_must_fail env FAKE_LINES="1 exec_git_cherry-pick_goodbye_amended-goodbye" \
+			git rebase -i goodbye^^ goodbye 2>err
+	) &&
+	test_i18ngrep "previous cherry-pick is now empty" err &&
+	test_i18ngrep "git cherry-pick --skip" err &&
+	test_must_fail git commit 2>err &&
 	test_i18ngrep "git cherry-pick --skip" err
 '
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index d8a05fd439..5afa6f28cd 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1602,6 +1602,32 @@ test_expect_success 'post-commit hook is called' '
 	test_cmp expect actual
 '
 
+test_expect_success 'correct error message for partial commit after empty pick' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="2 1 1" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A D
+	) &&
+	echo x >file1 &&
+	test_must_fail git commit file1 2>err &&
+	test_i18ngrep "cannot do a partial commit during a rebase." err
+'
+
+test_expect_success 'correct error message for commit --amend after empty pick' '
+	test_when_finished "git rebase --abort" &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="1 1" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A D
+	) &&
+	echo x>file1 &&
+	test_must_fail git commit -a --amend 2>err &&
+	test_i18ngrep "middle of a rebase -- cannot amend." err
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged
diff --git a/wt-status.h b/wt-status.h
index 0098fdb0b5..5f81bf7507 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -40,7 +40,8 @@ enum commit_whence {
 	FROM_COMMIT,     /* normal */
 	FROM_MERGE,      /* commit came from merge */
 	FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
-	FROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */
+	FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */
+	FROM_REBASE_PICK /* commit came from a pick/reword/edit */
 };
 
 static inline int is_from_cherry_pick(enum commit_whence whence)
@@ -49,6 +50,11 @@ static inline int is_from_cherry_pick(enum commit_whence whence)
 		whence == FROM_CHERRY_PICK_MULTI;
 }
 
+static inline int is_from_rebase(enum commit_whence whence)
+{
+	return whence == FROM_REBASE_PICK;
+}
+
 struct wt_status_change_data {
 	int worktree_status;
 	int index_status;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
                     ` (6 preceding siblings ...)
  2019-12-06 16:06   ` [PATCH v2 7/9] commit: give correct advice for empty commit during a rebase Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2020-02-26 19:45     ` Elijah Newren
  2019-12-06 16:06   ` [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts Phillip Wood
  8 siblings, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a specific message to advise the user what to do when a fixup or
squash command would create an empty commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    I'm slightly nervous of moving the call to determine_whence() to
    finalized_deferred_config(). I think it is ok but need to do a more
    thorough audit of the code to be sure.

 builtin/commit.c       | 32 ++++++++++++++++++++++++++++----
 sequencer.c            | 31 ++++++++++++++++++++++++++++++-
 sequencer.h            |  3 ++-
 t/t3403-rebase-skip.sh | 18 ++++++++++++------
 wt-status.h            |  5 +++--
 5 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index c3b6b8a6bd..4ae984c470 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -52,6 +52,20 @@ N_("You asked to amend the most recent commit, but doing so would make\n"
 "it empty. You can repeat your command with --allow-empty, or you can\n"
 "remove the commit entirely with \"git reset HEAD^\".\n");
 
+static const char empty_rebase_fixup_advice[] =
+N_("The fixup would make the commit empty\n"
+"If you wish to commit it anyway use:\n"
+"\n"
+"    git commit --amend --allow-empty\n"
+"    git rebase --continue\n"
+"\n"
+"To remove the commit entirely use:\n"
+"\n"
+"    git reset HEAD^\n"
+"    git rebase --continue\n"
+"\n"
+"Otherwise, please use 'git rebase --skip' to skip it\n");
+
 static const char empty_cherry_pick_advice[] =
 N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n"
 "If you wish to commit it anyway, use:\n"
@@ -181,8 +195,14 @@ static void determine_whence(struct wt_status *s)
 {
 	if (file_exists(git_path_merge_head(the_repository)))
 		whence = FROM_MERGE;
-	else if (!sequencer_determine_whence(the_repository, &whence))
-		whence = FROM_COMMIT;
+	else {
+		int res = sequencer_determine_whence(the_repository, &whence,
+						     amend);
+		if (res < 0)
+			die(_("could not read sequencer state"));
+		else if (!res)
+			whence = FROM_COMMIT;
+	}
 	if (s)
 		s->whence = whence;
 }
@@ -192,7 +212,6 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
 	wt_status_prepare(the_repository, s);
 	init_diff_ui_defaults();
 	git_config(fn, s);
-	determine_whence(s);
 	s->hints = advice_status_hints; /* must come after git_config() */
 }
 
@@ -943,9 +962,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 */
 	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
+		fprintf(stderr, "\nwhence = %d\n", whence);
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
-		if (amend)
+		if (whence == FROM_REBASE_FIXUP)
+			fputs(_(empty_rebase_fixup_advice), stderr);
+		else if (amend)
 			fputs(_(empty_amend_advice), stderr);
 		else if (is_from_cherry_pick(whence) ||
 			 whence == FROM_REBASE_PICK) {
@@ -1114,6 +1136,8 @@ static void finalize_deferred_config(struct wt_status *s)
 
 	if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
+
+	determine_whence(s);
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
diff --git a/sequencer.c b/sequencer.c
index 6e153fea76..64242f4ce7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5270,7 +5270,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
 	return 0;
 }
 
-int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,
+			       int amending)
 {
 	if (file_exists(git_path_cherry_pick_head(r))) {
 		struct object_id cherry_pick_head, rebase_head;
@@ -5286,6 +5287,34 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
 			*whence = FROM_CHERRY_PICK_SINGLE;
 
 		return 1;
+	} else if (amending && file_exists(rebase_path_current_fixups()) &&
+		   (file_exists(git_path_squash_msg(r)) ||
+		    file_exists(git_path_merge_msg(r)))) {
+		/*
+		 * If rebase_path_amend() exists the user is running `git
+		 * commit`, if not we're committing a fixup/squash directly from
+		 * the sequencer
+		 */
+		if (file_exists(rebase_path_amend())) {
+			struct strbuf rev = STRBUF_INIT;
+			struct object_id to_amend, head;
+
+			if (get_oid("HEAD", &head))
+				return error(_("amending invalid head")); /* let commit deal with error */
+			if (!read_oneliner(&rev, rebase_path_amend(), 0))
+				return error(_("invalid file: '%s'"),
+					     rebase_path_amend());
+			if (get_oid_hex(rev.buf, &to_amend))
+				return error(_("invalid contents: '%s'"),
+					     rebase_path_amend());
+			if (oideq(&head, &to_amend)) {
+				*whence = FROM_REBASE_FIXUP;
+				return 1;
+			}
+		} else {
+			*whence = FROM_REBASE_FIXUP;
+			return 1;
+		}
 	}
 
 	return 0;
diff --git a/sequencer.h b/sequencer.h
index 8d451dbfcb..4e631900b4 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -202,5 +202,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 void sequencer_post_commit_cleanup(struct repository *r);
 int sequencer_get_last_command(struct repository* r,
 			       enum replay_action *action);
-int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
+int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,
+			       int amending);
 #endif /* SEQUENCER_H */
diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
index a927774910..bc110b66b3 100755
--- a/t/t3403-rebase-skip.sh
+++ b/t/t3403-rebase-skip.sh
@@ -164,22 +164,28 @@ test_expect_success 'correct advice upon multi cherry-pick picking an empty comm
 	test_i18ngrep "git cherry-pick --skip" err
 '
 
-test_expect_success 'fixup that empties commit fails' '
+test_expect_success 'correct advice when fixup empties commit' '
 	test_when_finished "git rebase --abort" &&
 	(
 		set_fake_editor &&
 		test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \
-			goodbye^ reverted-goodbye
-	)
+			goodbye^ reverted-goodbye 2>err
+	) &&
+	test_i18ngrep "git rebase --skip" err &&
+	test_must_fail git commit --amend --no-edit 2>err &&
+	test_i18ngrep "git rebase --skip" err
 '
 
-test_expect_success 'squash that empties commit fails' '
+test_expect_success 'correct advice when squash empties commit' '
 	test_when_finished "git rebase --abort" &&
 	(
 		set_fake_editor &&
 		test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \
-			goodbye^ reverted-goodbye
-	)
+			goodbye^ reverted-goodbye 2>err
+	) &&
+	test_i18ngrep "git rebase --skip" err &&
+	test_must_fail git commit --amend --no-edit 2>err &&
+	test_i18ngrep "git rebase --skip" err
 '
 
 # Must be the last test in this file
diff --git a/wt-status.h b/wt-status.h
index 5f81bf7507..a4b7fe6de9 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -41,7 +41,8 @@ enum commit_whence {
 	FROM_MERGE,      /* commit came from merge */
 	FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
 	FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */
-	FROM_REBASE_PICK /* commit came from a pick/reword/edit */
+	FROM_REBASE_PICK, /* commit came from a pick/reword/edit */
+	FROM_REBASE_FIXUP /* commit came from fixup/squash */
 };
 
 static inline int is_from_cherry_pick(enum commit_whence whence)
@@ -52,7 +53,7 @@ static inline int is_from_cherry_pick(enum commit_whence whence)
 
 static inline int is_from_rebase(enum commit_whence whence)
 {
-	return whence == FROM_REBASE_PICK;
+	return whence == FROM_REBASE_PICK || whence == FROM_REBASE_FIXUP;
 }
 
 struct wt_status_change_data {
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts
  2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
                     ` (7 preceding siblings ...)
  2019-12-06 16:06   ` [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit Phillip Wood
@ 2019-12-06 16:06   ` Phillip Wood
  2019-12-18 14:35     ` Phillip Wood
  8 siblings, 1 reply; 31+ messages in thread
From: Phillip Wood @ 2019-12-06 16:06 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Since the inception of CHERRY_PICK_HEAD in d7e5c0cbfb ("Introduce
CHERRY_PICK_HEAD", 2011-02-19) 'rebase -i' has removed it when there are
conflicts. The rationale for this was that the rebase wanted to handle
the conflicts itself. However sometimes (e.g. after an edit command) the
user wants to commit the conflict resolution before making some other
changes or running some tests. Without CHERRY_PICK_HEAD the authorship
information is lost when the user makes the commit. Fix this by leaving
CHERRY_PICK_HEAD when we're not amending.

Note that this changes the output of `git status`. The advice to run
`git reset` is not appropriate for rebase as we do not allow partial
commits.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    This has semantic conflicts with ra/rebase-i-more-options as it does not
    respect the options passed to rebase when the user commits
    
    I haven't checked how this affects the shell prompt in contrib yet, I
    suspect it may need changing to cope the presence of CHERRY_PICK_HEAD
    during a rebase.
    
    I'd like to change the existing authorship tests to rely on the "Original
    Author" changes here, but they are a web of hidden interdependencies which is
    hard to untangle.

 sequencer.c                   |  12 ++--
 t/t3404-rebase-interactive.sh | 104 +++++++++++++++++++++++++---------
 t/t7512-status-help.sh        |   2 -
 3 files changed, 85 insertions(+), 33 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 64242f4ce7..624e96c930 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -372,11 +372,15 @@ static void print_advice(struct repository *r, int show_hint,
 	if (msg) {
 		fprintf(stderr, "%s\n", msg);
 		/*
-		 * A conflict has occurred but the porcelain
-		 * (typically rebase --interactive) wants to take care
-		 * of the commit itself so remove CHERRY_PICK_HEAD
+		 * A conflict has occurred but the porcelain wants to take care
+		 * of the commit itself so remove CHERRY_PICK_HEAD. Note that we
+		 * do not do this for interactive rebases anymore in order to
+		 * preserve the author identity when the user runs 'git commit'
+		 * to commit the conflict resolution rather than relying on
+		 * 'rebase --continue' to do it for them.
 		 */
-		unlink(git_path_cherry_pick_head(r));
+		if (!is_rebase_i(opts))
+			unlink(git_path_cherry_pick_head(r));
 		return;
 	}
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5afa6f28cd..5cd7db18f8 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -33,31 +33,35 @@ Initial setup:
 # in the expect2 file for the 'stop on conflicting pick' test.
 
 test_expect_success 'setup' '
-	test_commit A file1 &&
-	test_commit B file1 &&
-	test_commit C file2 &&
-	test_commit D file1 &&
-	test_commit E file3 &&
-	git checkout -b branch1 A &&
-	test_commit F file4 &&
-	test_commit G file1 &&
-	test_commit H file5 &&
-	git checkout -b branch2 F &&
-	test_commit I file6 &&
-	git checkout -b conflict-branch A &&
-	test_commit one conflict &&
-	test_commit two conflict &&
-	test_commit three conflict &&
-	test_commit four conflict &&
-	git checkout -b no-conflict-branch A &&
-	test_commit J fileJ &&
-	test_commit K fileK &&
-	test_commit L fileL &&
-	test_commit M fileM &&
-	git checkout -b no-ff-branch A &&
-	test_commit N fileN &&
-	test_commit O fileO &&
-	test_commit P fileP
+	(
+		GIT_AUTHOR_NAME="Original Author" &&
+		GIT_AUTHOR_EMAIL="original.author@example.com" &&
+		test_commit A file1 &&
+		test_commit B file1 &&
+		test_commit C file2 &&
+		test_commit D file1 &&
+		test_commit E file3 &&
+		git checkout -b branch1 A &&
+		test_commit F file4 &&
+		test_commit G file1 &&
+		test_commit H file5 &&
+		git checkout -b branch2 F &&
+		test_commit I file6 &&
+		git checkout -b conflict-branch A &&
+		test_commit one conflict &&
+		test_commit two conflict &&
+		test_commit three conflict &&
+		test_commit four conflict &&
+		git checkout -b no-conflict-branch A &&
+		test_commit J fileJ &&
+		test_commit K fileK &&
+		test_commit L fileL &&
+		test_commit M fileM &&
+		git checkout -b no-ff-branch A &&
+		test_commit N fileN &&
+		test_commit O fileO &&
+		test_commit P fileP
+	)
 '
 
 # "exec" commands are run with the user shell by default, but this may
@@ -252,12 +256,12 @@ test_expect_success 'stop on conflicting pick' '
 	-A
 	+G
 	EOF
-	cat >expect2 <<-\EOF &&
+	cat >expect2 <<-EOF &&
 	<<<<<<< HEAD
 	D
 	=======
 	G
-	>>>>>>> 5d18e54... G
+	>>>>>>> $(git rev-parse --short HEAD)... G
 	EOF
 	git tag new-branch1 &&
 	test_must_fail git rebase -i master &&
@@ -1628,6 +1632,52 @@ test_expect_success 'correct error message for commit --amend after empty pick'
 	test_i18ngrep "middle of a rebase -- cannot amend." err
 '
 
+test_expect_success 'correct error message for partial commit after confilct' '
+	test_when_finished "git rebase --abort" &&
+	git checkout D &&
+	(
+		set_fake_editor &&
+		FAKE_LINES="2 3" &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A
+	) &&
+	echo x >file1 &&
+	echo y >file2 &&
+	git add file1 file2 &&
+	test_must_fail git commit file1 2>err &&
+	test_i18ngrep "cannot do a partial commit during a rebase." err
+'
+
+test_expect_success 'correct error message for commit --amend after conflict' '
+	test_when_finished "git rebase --abort" &&
+	git checkout D &&
+	(
+		set_fake_editor &&
+		FAKE_LINES=3 &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A
+	) &&
+	echo x>file1 &&
+	test_must_fail git commit -a --amend 2>err &&
+	test_i18ngrep "middle of a rebase -- cannot amend." err
+'
+
+test_expect_success 'correct authorship and message after conflict' '
+	git checkout D &&
+	(
+		set_fake_editor &&
+		FAKE_LINES=3 &&
+		export FAKE_LINES &&
+		test_must_fail git rebase -i A
+	) &&
+	echo x >file1 &&
+	git commit -a &&
+	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 D >expect &&
+	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 HEAD >actual &&
+	test_cmp expect actual &&
+	git rebase --continue
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index c1eb72555d..2adceb35e2 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -148,7 +148,6 @@ You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
   (use "git rebase --abort" to check out the original branch)
 
 Unmerged paths:
-  (use "git reset HEAD <file>..." to unstage)
   (use "git add <file>..." to mark resolution)
 
 	both modified:   main.txt
@@ -176,7 +175,6 @@ You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
   (all conflicts fixed: run "git rebase --continue")
 
 Changes to be committed:
-  (use "git reset HEAD <file>..." to unstage)
 
 	modified:   main.txt
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 1/9] t3404: use test_cmp_rev
  2019-12-06 16:06   ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
@ 2019-12-06 17:39     ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2019-12-06 17:39 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin, Phillip Wood

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> There are a number of places where we compare two revisions with
>     test $(git rev-parse rev1) = $(git rev-parse rev2)
> when these fail there's no indication what has gone wrong and you need
> to be running with `-x` to see where the test has failed. Lets use
> test_cmp_rev instead.

Makes sense.  Wonder if we can have a version of coccinelle for
shell scripts ;-)

>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  t/t3404-rebase-interactive.sh | 38 +++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c573c99069..d8a05fd439 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -191,7 +191,7 @@ test_expect_success 'no changes are a nop' '
>  	git checkout branch2 &&
>  	git rebase -i F &&
>  	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
> -	test $(git rev-parse I) = $(git rev-parse HEAD)
> +	test_cmp_rev I HEAD
>  '
>  
>  test_expect_success 'test the [branch] option' '
> @@ -200,16 +200,16 @@ test_expect_success 'test the [branch] option' '
>  	git commit -m "stop here" &&
>  	git rebase -i F branch2 &&
>  	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch2" &&
> -	test $(git rev-parse I) = $(git rev-parse branch2) &&
> -	test $(git rev-parse I) = $(git rev-parse HEAD)
> +	test_cmp_rev I branch2 &&
> +	test_cmp_rev I HEAD
>  '
>  
>  test_expect_success 'test --onto <branch>' '
>  	git checkout -b test-onto branch2 &&
>  	git rebase -i --onto branch1 F &&
>  	test "$(git symbolic-ref -q HEAD)" = "refs/heads/test-onto" &&
> -	test $(git rev-parse HEAD^) = $(git rev-parse branch1) &&
> -	test $(git rev-parse I) = $(git rev-parse branch2)
> +	test_cmp_rev HEAD^ branch1 &&
> +	test_cmp_rev I branch2
>  '
>  
>  test_expect_success 'rebase on top of a non-conflicting commit' '
> @@ -218,12 +218,12 @@ test_expect_success 'rebase on top of a non-conflicting commit' '
>  	git rebase -i branch2 &&
>  	test file6 = $(git diff --name-only original-branch1) &&
>  	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
> -	test $(git rev-parse I) = $(git rev-parse branch2) &&
> -	test $(git rev-parse I) = $(git rev-parse HEAD~2)
> +	test_cmp_rev I branch2 &&
> +	test_cmp_rev I HEAD~2
>  '
>  
>  test_expect_success 'reflog for the branch shows state before rebase' '
> -	test $(git rev-parse branch1@{1}) = $(git rev-parse original-branch1)
> +	test_cmp_rev branch1@{1} original-branch1
>  '
>  
>  test_expect_success 'reflog for the branch shows correct finish message' '
> @@ -280,7 +280,7 @@ test_expect_success 'show conflicted patch' '
>  
>  test_expect_success 'abort' '
>  	git rebase --abort &&
> -	test $(git rev-parse new-branch1) = $(git rev-parse HEAD) &&
> +	test_cmp_rev new-branch1 HEAD &&
>  	test "$(git symbolic-ref -q HEAD)" = "refs/heads/branch1" &&
>  	test_path_is_missing .git/rebase-merge
>  '
> @@ -323,7 +323,7 @@ test_expect_success 'retain authorship w/ conflicts' '
>  	echo resolved >conflict &&
>  	git add conflict &&
>  	git rebase --continue &&
> -	test $(git rev-parse conflict-a^0) = $(git rev-parse HEAD^) &&
> +	test_cmp_rev conflict-a^0 HEAD^ &&
>  	git show >out &&
>  	grep AttributeMe out
>  '
> @@ -340,7 +340,7 @@ test_expect_success 'squash' '
>  			git rebase -i --onto master HEAD~2
>  	) &&
>  	test B = $(cat file7) &&
> -	test $(git rev-parse HEAD^) = $(git rev-parse master)
> +	test_cmp_rev HEAD^ master
>  '
>  
>  test_expect_success 'retain authorship when squashing' '
> @@ -399,9 +399,9 @@ test_expect_success REBASE_P 'preserve merges with -p' '
>  	git update-index --refresh &&
>  	git diff-files --quiet &&
>  	git diff-index --quiet --cached HEAD -- &&
> -	test $(git rev-parse HEAD~6) = $(git rev-parse branch1) &&
> -	test $(git rev-parse HEAD~4^2) = $(git rev-parse to-be-preserved) &&
> -	test $(git rev-parse HEAD^^2^) = $(git rev-parse HEAD^^^) &&
> +	test_cmp_rev HEAD~6 branch1 &&
> +	test_cmp_rev HEAD~4^2 to-be-preserved &&
> +	test_cmp_rev HEAD^^2^ HEAD^^^ &&
>  	test $(git show HEAD~5:file1) = B &&
>  	test $(git show HEAD~3:file1) = C &&
>  	test $(git show HEAD:file1) = E &&
> @@ -433,7 +433,7 @@ test_expect_success '--continue tries to commit' '
>  		git add file1 &&
>  		FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
>  	) &&
> -	test $(git rev-parse HEAD^) = $(git rev-parse new-branch1) &&
> +	test_cmp_rev HEAD^ new-branch1 &&
>  	git show HEAD | grep chouette
>  '
>  
> @@ -740,7 +740,7 @@ test_expect_success 'do "noop" when there is nothing to cherry-pick' '
>  		--author="Somebody else <somebody@else.com>" &&
>  	test $(git rev-parse branch3) != $(git rev-parse branch4) &&
>  	git rebase -i branch3 &&
> -	test $(git rev-parse branch3) = $(git rev-parse branch4)
> +	test_cmp_rev branch3 branch4
>  
>  '
>  
> @@ -799,7 +799,7 @@ test_expect_success 'rebase -i continue with unstaged submodule' '
>  	test_must_fail git rebase -i submodule-base &&
>  	git reset &&
>  	git rebase --continue &&
> -	test $(git rev-parse submodule-base) = $(git rev-parse HEAD)
> +	test_cmp_rev submodule-base HEAD
>  '
>  
>  test_expect_success 'avoid unnecessary reset' '
> @@ -822,7 +822,7 @@ test_expect_success 'reword' '
>  			git rebase -i A &&
>  		git show HEAD | grep "E changed" &&
>  		test $(git rev-parse master) != $(git rev-parse HEAD) &&
> -		test $(git rev-parse master^) = $(git rev-parse HEAD^) &&
> +		test_cmp_rev master^ HEAD^ &&
>  		FAKE_LINES="1 2 reword 3 4" FAKE_COMMIT_MESSAGE="D changed" \
>  			git rebase -i A &&
>  		git show HEAD^ | grep "D changed" &&
> @@ -886,7 +886,7 @@ test_expect_success 'always cherry-pick with --no-ff' '
>  		git diff HEAD~$p original-no-ff-branch~$p > out &&
>  		test_must_be_empty out
>  	done &&
> -	test $(git rev-parse HEAD~3) = $(git rev-parse original-no-ff-branch~3) &&
> +	test_cmp_rev HEAD~3 original-no-ff-branch~3 &&
>  	git diff HEAD~3 original-no-ff-branch~3 > out &&
>  	test_must_be_empty out
>  '

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 5/9] commit: use enum value for multiple cherry-picks
  2019-12-06 16:06   ` [PATCH v2 5/9] commit: use enum value for multiple cherry-picks Phillip Wood
@ 2019-12-06 18:13     ` Junio C Hamano
  0 siblings, 0 replies; 31+ messages in thread
From: Junio C Hamano @ 2019-12-06 18:13 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin, Phillip Wood

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add FROM_CHERRY_PICK_MULTI for a sequence of cherry-picks rather than
> using a separate variable.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/commit.c | 25 +++++++++++--------------
>  wt-status.h      |  9 ++++++++-
>  2 files changed, 19 insertions(+), 15 deletions(-)

Makes sense.  The checks have become quite pleasant to read, thanks
to the new helper function.

> ...
> -	if (whence == FROM_CHERRY_PICK && !renew_authorship) {
> +	if (is_from_cherry_pick(whence) && !renew_authorship) {
>  		author_message = "CHERRY_PICK_HEAD";
>  		author_message_buffer = read_commit_message(author_message);
>  	}
> diff --git a/wt-status.h b/wt-status.h
> index 64f1ddc9fd..0098fdb0b5 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -39,9 +39,16 @@ enum show_ignored_type {
>  enum commit_whence {
>  	FROM_COMMIT,     /* normal */
>  	FROM_MERGE,      /* commit came from merge */
> -	FROM_CHERRY_PICK /* commit came from cherry-pick */
> +	FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
> +	FROM_CHERRY_PICK_MULTI /* commit came from a sequence of cherry-picks */
>  };

It might be worth to end PICK_MULTI with a trailing comma to
future-proof, but not worth a reroll for this alone.


^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer
  2019-12-06 16:06   ` [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer Phillip Wood
@ 2019-12-06 18:24     ` Junio C Hamano
  2019-12-18 14:26       ` Phillip Wood
  0 siblings, 1 reply; 31+ messages in thread
From: Junio C Hamano @ 2019-12-06 18:24 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin, Phillip Wood

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Working out which command wants to create a commit requires detailed
> knowledge of the sequencer internals and that knowledge is going to
> increase in subsequent commits. With that in mind lets encapsulate that
> knowledge in sequencer.c rather than spreading it into builtin/commit.c.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/commit.c |  5 +----
>  sequencer.c      | 13 ++++++++++++-
>  sequencer.h      |  3 ++-
>  3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 3b463522be..d8d4c8e419 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)
>  {
>  	if (file_exists(git_path_merge_head(the_repository)))
>  		whence = FROM_MERGE;
> -	else if (file_exists(git_path_cherry_pick_head(the_repository)))
> -		whence = file_exists(git_path_seq_dir()) ?
> -			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
> -	else
> +	else if (!sequencer_determine_whence(the_repository, &whence))
>  		whence = FROM_COMMIT;
>  	if (s)
>  		s->whence = whence;
> diff --git a/sequencer.c b/sequencer.c
> index 4e0370277b..98e007556c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>  
>  GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>  
> -GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
> +static GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>  
>  static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
>  static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
> @@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>  
>  	return 0;
>  }
> +
> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
> +{
> +	if (file_exists(git_path_cherry_pick_head(r))) {
> +		*whence = file_exists(git_path_seq_dir()) ?
> +			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
> +		return 1;
> +	}
> +
> +	return 0;
> +}

I am not sure if this is a good move---determine_whence() that can
tell not just we are in the middle of cherry-pick (either a single
or multi) but also during a merge may be at the right abstraction
level.  Why would we want to invent a separate function that says "I
dunno" during a merge, instead of moving the logic for merge to the
new helper as well?  The original determine_whence that takes
wt_status and populates it still has to call the new helper either
way.  Also for the matter FROM_COMMIT may also want to be part of
the helper.  This all depends on the new callers you plan to invent,
of course.

Not part of this topic, but the call to file_exists() may want to
become a call to dir_exists() as git-path-seq-dir is clearly a
directory and cannot be a file, right?

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer
  2019-12-06 18:24     ` Junio C Hamano
@ 2019-12-18 14:26       ` Phillip Wood
  0 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-18 14:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Johannes Schindelin, Phillip Wood

On 06/12/2019 18:24, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Working out which command wants to create a commit requires detailed
>> knowledge of the sequencer internals and that knowledge is going to
>> increase in subsequent commits. With that in mind lets encapsulate that
>> knowledge in sequencer.c rather than spreading it into builtin/commit.c.
>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   builtin/commit.c |  5 +----
>>   sequencer.c      | 13 ++++++++++++-
>>   sequencer.h      |  3 ++-
>>   3 files changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 3b463522be..d8d4c8e419 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -178,10 +178,7 @@ static void determine_whence(struct wt_status *s)
>>   {
>>   	if (file_exists(git_path_merge_head(the_repository)))
>>   		whence = FROM_MERGE;
>> -	else if (file_exists(git_path_cherry_pick_head(the_repository)))
>> -		whence = file_exists(git_path_seq_dir()) ?
>> -			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
>> -	else
>> +	else if (!sequencer_determine_whence(the_repository, &whence))
>>   		whence = FROM_COMMIT;
>>   	if (s)
>>   		s->whence = whence;
>> diff --git a/sequencer.c b/sequencer.c
>> index 4e0370277b..98e007556c 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -40,7 +40,7 @@ static const char cherry_picked_prefix[] = "(cherry picked from commit ";
>>   
>>   GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG")
>>   
>> -GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>> +static GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
>>   
>>   static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
>>   static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
>> @@ -5256,3 +5256,14 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>>   
>>   	return 0;
>>   }
>> +
>> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
>> +{
>> +	if (file_exists(git_path_cherry_pick_head(r))) {
>> +		*whence = file_exists(git_path_seq_dir()) ?
>> +			FROM_CHERRY_PICK_MULTI : FROM_CHERRY_PICK_SINGLE;
>> +		return 1;
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I am not sure if this is a good move---determine_whence() that can
> tell not just we are in the middle of cherry-pick (either a single
> or multi) but also during a merge may be at the right abstraction
> level.  Why would we want to invent a separate function that says "I
> dunno" during a merge, instead of moving the logic for merge to the
> new helper as well?  The original determine_whence that takes
> wt_status and populates it still has to call the new helper either
> way.  Also for the matter FROM_COMMIT may also want to be part of
> the helper.  This all depends on the new callers you plan to invent,
> of course.

The idea was for determine_whence() to be able to delegate to the 
sequencer to ask if it is doing something without having to expose all 
the implementation details of that check in builtin/commit.c. It is 
simple enough at this stage but the next patches add more complexity 
which would mean exposing various sequencer state files to 
builtin/commit.c. This function is only meant to be called from 
determine_whence() - callers that want to know if any operations (merge, 
cherry-pick etc.) are in progress should be calling that not the 
function added here.

> Not part of this topic, but the call to file_exists() may want to
> become a call to dir_exists() as git-path-seq-dir is clearly a
> directory and cannot be a file, right?

Yes

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts
  2019-12-06 16:06   ` [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts Phillip Wood
@ 2019-12-18 14:35     ` Phillip Wood
  0 siblings, 0 replies; 31+ messages in thread
From: Phillip Wood @ 2019-12-18 14:35 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Johannes Schindelin, Junio C Hamano, Phillip Wood

On 06/12/2019 16:06, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> Since the inception of CHERRY_PICK_HEAD in d7e5c0cbfb ("Introduce
> CHERRY_PICK_HEAD", 2011-02-19) 'rebase -i' has removed it when there are
> conflicts. The rationale for this was that the rebase wanted to handle
> the conflicts itself. However sometimes (e.g. after an edit command) the
> user wants to commit the conflict resolution before making some other
> changes or running some tests. Without CHERRY_PICK_HEAD the authorship
> information is lost when the user makes the commit. Fix this by leaving
> CHERRY_PICK_HEAD when we're not amending.

I'm not so sure about this approach as it wont work with 'merge' 
commands when rebasing. I wonder if it would be better to add a new file 
COMMIT_AUTHOR (or maybe MERGE_AUTHOR) that can be parsed by 
split_ident() and sets the authorship for a commit. The file would 
override $GIT_AUTHOR_NAME/EMAIL/DATE but could be overridden on the 
commandline by --author/date/reset-author

Best Wishes

Phillip

> Note that this changes the output of `git status`. The advice to run
> `git reset` is not appropriate for rebase as we do not allow partial
> commits.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> 
> Notes:
>      This has semantic conflicts with ra/rebase-i-more-options as it does not
>      respect the options passed to rebase when the user commits
>      
>      I haven't checked how this affects the shell prompt in contrib yet, I
>      suspect it may need changing to cope the presence of CHERRY_PICK_HEAD
>      during a rebase.
>      
>      I'd like to change the existing authorship tests to rely on the "Original
>      Author" changes here, but they are a web of hidden interdependencies which is
>      hard to untangle.
> 
>   sequencer.c                   |  12 ++--
>   t/t3404-rebase-interactive.sh | 104 +++++++++++++++++++++++++---------
>   t/t7512-status-help.sh        |   2 -
>   3 files changed, 85 insertions(+), 33 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 64242f4ce7..624e96c930 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -372,11 +372,15 @@ static void print_advice(struct repository *r, int show_hint,
>   	if (msg) {
>   		fprintf(stderr, "%s\n", msg);
>   		/*
> -		 * A conflict has occurred but the porcelain
> -		 * (typically rebase --interactive) wants to take care
> -		 * of the commit itself so remove CHERRY_PICK_HEAD
> +		 * A conflict has occurred but the porcelain wants to take care
> +		 * of the commit itself so remove CHERRY_PICK_HEAD. Note that we
> +		 * do not do this for interactive rebases anymore in order to
> +		 * preserve the author identity when the user runs 'git commit'
> +		 * to commit the conflict resolution rather than relying on
> +		 * 'rebase --continue' to do it for them.
>   		 */
> -		unlink(git_path_cherry_pick_head(r));
> +		if (!is_rebase_i(opts))
> +			unlink(git_path_cherry_pick_head(r));
>   		return;
>   	}
>   
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 5afa6f28cd..5cd7db18f8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -33,31 +33,35 @@ Initial setup:
>   # in the expect2 file for the 'stop on conflicting pick' test.
>   
>   test_expect_success 'setup' '
> -	test_commit A file1 &&
> -	test_commit B file1 &&
> -	test_commit C file2 &&
> -	test_commit D file1 &&
> -	test_commit E file3 &&
> -	git checkout -b branch1 A &&
> -	test_commit F file4 &&
> -	test_commit G file1 &&
> -	test_commit H file5 &&
> -	git checkout -b branch2 F &&
> -	test_commit I file6 &&
> -	git checkout -b conflict-branch A &&
> -	test_commit one conflict &&
> -	test_commit two conflict &&
> -	test_commit three conflict &&
> -	test_commit four conflict &&
> -	git checkout -b no-conflict-branch A &&
> -	test_commit J fileJ &&
> -	test_commit K fileK &&
> -	test_commit L fileL &&
> -	test_commit M fileM &&
> -	git checkout -b no-ff-branch A &&
> -	test_commit N fileN &&
> -	test_commit O fileO &&
> -	test_commit P fileP
> +	(
> +		GIT_AUTHOR_NAME="Original Author" &&
> +		GIT_AUTHOR_EMAIL="original.author@example.com" &&
> +		test_commit A file1 &&
> +		test_commit B file1 &&
> +		test_commit C file2 &&
> +		test_commit D file1 &&
> +		test_commit E file3 &&
> +		git checkout -b branch1 A &&
> +		test_commit F file4 &&
> +		test_commit G file1 &&
> +		test_commit H file5 &&
> +		git checkout -b branch2 F &&
> +		test_commit I file6 &&
> +		git checkout -b conflict-branch A &&
> +		test_commit one conflict &&
> +		test_commit two conflict &&
> +		test_commit three conflict &&
> +		test_commit four conflict &&
> +		git checkout -b no-conflict-branch A &&
> +		test_commit J fileJ &&
> +		test_commit K fileK &&
> +		test_commit L fileL &&
> +		test_commit M fileM &&
> +		git checkout -b no-ff-branch A &&
> +		test_commit N fileN &&
> +		test_commit O fileO &&
> +		test_commit P fileP
> +	)
>   '
>   
>   # "exec" commands are run with the user shell by default, but this may
> @@ -252,12 +256,12 @@ test_expect_success 'stop on conflicting pick' '
>   	-A
>   	+G
>   	EOF
> -	cat >expect2 <<-\EOF &&
> +	cat >expect2 <<-EOF &&
>   	<<<<<<< HEAD
>   	D
>   	=======
>   	G
> -	>>>>>>> 5d18e54... G
> +	>>>>>>> $(git rev-parse --short HEAD)... G
>   	EOF
>   	git tag new-branch1 &&
>   	test_must_fail git rebase -i master &&
> @@ -1628,6 +1632,52 @@ test_expect_success 'correct error message for commit --amend after empty pick'
>   	test_i18ngrep "middle of a rebase -- cannot amend." err
>   '
>   
> +test_expect_success 'correct error message for partial commit after confilct' '
> +	test_when_finished "git rebase --abort" &&
> +	git checkout D &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES="2 3" &&
> +		export FAKE_LINES &&
> +		test_must_fail git rebase -i A
> +	) &&
> +	echo x >file1 &&
> +	echo y >file2 &&
> +	git add file1 file2 &&
> +	test_must_fail git commit file1 2>err &&
> +	test_i18ngrep "cannot do a partial commit during a rebase." err
> +'
> +
> +test_expect_success 'correct error message for commit --amend after conflict' '
> +	test_when_finished "git rebase --abort" &&
> +	git checkout D &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES=3 &&
> +		export FAKE_LINES &&
> +		test_must_fail git rebase -i A
> +	) &&
> +	echo x>file1 &&
> +	test_must_fail git commit -a --amend 2>err &&
> +	test_i18ngrep "middle of a rebase -- cannot amend." err
> +'
> +
> +test_expect_success 'correct authorship and message after conflict' '
> +	git checkout D &&
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES=3 &&
> +		export FAKE_LINES &&
> +		test_must_fail git rebase -i A
> +	) &&
> +	echo x >file1 &&
> +	git commit -a &&
> +	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 D >expect &&
> +	git log --pretty=format:"%an <%ae>%n%ad%n%B" -1 HEAD >actual &&
> +	test_cmp expect actual &&
> +	git rebase --continue
> +'
> +
>   # This must be the last test in this file
>   test_expect_success '$EDITOR and friends are unchanged' '
>   	test_editor_unchanged
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index c1eb72555d..2adceb35e2 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -148,7 +148,6 @@ You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
>     (use "git rebase --abort" to check out the original branch)
>   
>   Unmerged paths:
> -  (use "git reset HEAD <file>..." to unstage)
>     (use "git add <file>..." to mark resolution)
>   
>   	both modified:   main.txt
> @@ -176,7 +175,6 @@ You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO
>     (all conflicts fixed: run "git rebase --continue")
>   
>   Changes to be committed:
> -  (use "git reset HEAD <file>..." to unstage)
>   
>   	modified:   main.txt
>   
> 

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit
  2019-12-06 16:06   ` [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit Phillip Wood
@ 2020-02-26 19:45     ` Elijah Newren
  0 siblings, 0 replies; 31+ messages in thread
From: Elijah Newren @ 2020-02-26 19:45 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Johannes Schindelin, Junio C Hamano

On Fri, Dec 6, 2019 at 8:10 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add a specific message to advise the user what to do when a fixup or
> squash command would create an empty commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> Notes:
>     I'm slightly nervous of moving the call to determine_whence() to
>     finalized_deferred_config(). I think it is ok but need to do a more
>     thorough audit of the code to be sure.

Any update?

>
>  builtin/commit.c       | 32 ++++++++++++++++++++++++++++----
>  sequencer.c            | 31 ++++++++++++++++++++++++++++++-
>  sequencer.h            |  3 ++-
>  t/t3403-rebase-skip.sh | 18 ++++++++++++------
>  wt-status.h            |  5 +++--
>  5 files changed, 75 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index c3b6b8a6bd..4ae984c470 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -52,6 +52,20 @@ N_("You asked to amend the most recent commit, but doing so would make\n"
>  "it empty. You can repeat your command with --allow-empty, or you can\n"
>  "remove the commit entirely with \"git reset HEAD^\".\n");
>
> +static const char empty_rebase_fixup_advice[] =
> +N_("The fixup would make the commit empty\n"
> +"If you wish to commit it anyway use:\n"
> +"\n"
> +"    git commit --amend --allow-empty\n"
> +"    git rebase --continue\n"
> +"\n"
> +"To remove the commit entirely use:\n"
> +"\n"
> +"    git reset HEAD^\n"
> +"    git rebase --continue\n"
> +"\n"
> +"Otherwise, please use 'git rebase --skip' to skip it\n");

How does skipping differ from removing the commit entirely?  Which one
tosses just the changes from the fixup commit, and which tosses both
the fixup and the commit it is fixing?  Or do they both do the same
thing?

>  static const char empty_cherry_pick_advice[] =
>  N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n"
>  "If you wish to commit it anyway, use:\n"
> @@ -181,8 +195,14 @@ static void determine_whence(struct wt_status *s)
>  {
>         if (file_exists(git_path_merge_head(the_repository)))
>                 whence = FROM_MERGE;
> -       else if (!sequencer_determine_whence(the_repository, &whence))
> -               whence = FROM_COMMIT;
> +       else {
> +               int res = sequencer_determine_whence(the_repository, &whence,
> +                                                    amend);
> +               if (res < 0)
> +                       die(_("could not read sequencer state"));

sequencer_determine_whence() tries to determine what type of sequencer
operation is in effect, and can return a range of values in whence.
It can also fail to determine which type of sequencer operation is in
effect -- in multiple ways.  And it distinguishes those with via res <
0 vs. res == 0.  I guess it makes sense, but it seems a bit weird.

Not sure what to suggest.  One possibly bad idea just to get the
creative juices flowing: Maybe change the call signature to

enum commit_whence sequencer_determine_whence(struct repository *r,
int amending, enum commit_whence default);

with callers passing default == FROM_COMMIT, and the function can
return FROM_COMMIT if there is no sequencer state, or FROM_ERROR
(FROM_INVALID? FROM_FAILURE?) if there is sequencer state but it
cannot be read.


(And maybe do something similar with patch 6?)

> +               else if (!res)
> +                       whence = FROM_COMMIT;
> +       }
>         if (s)
>                 s->whence = whence;
>  }
> @@ -192,7 +212,6 @@ static void status_init_config(struct wt_status *s, config_fn_t fn)
>         wt_status_prepare(the_repository, s);
>         init_diff_ui_defaults();
>         git_config(fn, s);
> -       determine_whence(s);
>         s->hints = advice_status_hints; /* must come after git_config() */
>  }
>
> @@ -943,9 +962,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>          */
>         if (!committable && whence != FROM_MERGE && !allow_empty &&
>             !(amend && is_a_merge(current_head))) {
> +               fprintf(stderr, "\nwhence = %d\n", whence);

Leftover debugging?

>                 s->display_comment_prefix = old_display_comment_prefix;
>                 run_status(stdout, index_file, prefix, 0, s);
> -               if (amend)
> +               if (whence == FROM_REBASE_FIXUP)
> +                       fputs(_(empty_rebase_fixup_advice), stderr);
> +               else if (amend)
>                         fputs(_(empty_amend_advice), stderr);
>                 else if (is_from_cherry_pick(whence) ||
>                          whence == FROM_REBASE_PICK) {
> @@ -1114,6 +1136,8 @@ static void finalize_deferred_config(struct wt_status *s)
>
>         if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
>                 s->ahead_behind_flags = AHEAD_BEHIND_FULL;
> +
> +       determine_whence(s);
>  }
>
>  static int parse_and_validate_options(int argc, const char *argv[],
> diff --git a/sequencer.c b/sequencer.c
> index 6e153fea76..64242f4ce7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5270,7 +5270,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>         return 0;
>  }
>
> -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,
> +                              int amending)
>  {
>         if (file_exists(git_path_cherry_pick_head(r))) {
>                 struct object_id cherry_pick_head, rebase_head;
> @@ -5286,6 +5287,34 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
>                         *whence = FROM_CHERRY_PICK_SINGLE;
>
>                 return 1;
> +       } else if (amending && file_exists(rebase_path_current_fixups()) &&
> +                  (file_exists(git_path_squash_msg(r)) ||
> +                   file_exists(git_path_merge_msg(r)))) {
> +               /*
> +                * If rebase_path_amend() exists the user is running `git
> +                * commit`, if not we're committing a fixup/squash directly from
> +                * the sequencer
> +                */
> +               if (file_exists(rebase_path_amend())) {
> +                       struct strbuf rev = STRBUF_INIT;
> +                       struct object_id to_amend, head;
> +
> +                       if (get_oid("HEAD", &head))
> +                               return error(_("amending invalid head")); /* let commit deal with error */
> +                       if (!read_oneliner(&rev, rebase_path_amend(), 0))
> +                               return error(_("invalid file: '%s'"),
> +                                            rebase_path_amend());
> +                       if (get_oid_hex(rev.buf, &to_amend))
> +                               return error(_("invalid contents: '%s'"),
> +                                            rebase_path_amend());
> +                       if (oideq(&head, &to_amend)) {
> +                               *whence = FROM_REBASE_FIXUP;
> +                               return 1;
> +                       }
> +               } else {
> +                       *whence = FROM_REBASE_FIXUP;
> +                       return 1;
> +               }
>         }
>
>         return 0;
> diff --git a/sequencer.h b/sequencer.h
> index 8d451dbfcb..4e631900b4 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -202,5 +202,6 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
>  void sequencer_post_commit_cleanup(struct repository *r);
>  int sequencer_get_last_command(struct repository* r,
>                                enum replay_action *action);
> -int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
> +int sequencer_determine_whence(struct repository *r, enum commit_whence *whence,
> +                              int amending);
>  #endif /* SEQUENCER_H */
> diff --git a/t/t3403-rebase-skip.sh b/t/t3403-rebase-skip.sh
> index a927774910..bc110b66b3 100755
> --- a/t/t3403-rebase-skip.sh
> +++ b/t/t3403-rebase-skip.sh
> @@ -164,22 +164,28 @@ test_expect_success 'correct advice upon multi cherry-pick picking an empty comm
>         test_i18ngrep "git cherry-pick --skip" err
>  '
>
> -test_expect_success 'fixup that empties commit fails' '
> +test_expect_success 'correct advice when fixup empties commit' '
>         test_when_finished "git rebase --abort" &&
>         (
>                 set_fake_editor &&
>                 test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i \
> -                       goodbye^ reverted-goodbye
> -       )
> +                       goodbye^ reverted-goodbye 2>err
> +       ) &&
> +       test_i18ngrep "git rebase --skip" err &&
> +       test_must_fail git commit --amend --no-edit 2>err &&
> +       test_i18ngrep "git rebase --skip" err
>  '
>
> -test_expect_success 'squash that empties commit fails' '
> +test_expect_success 'correct advice when squash empties commit' '
>         test_when_finished "git rebase --abort" &&
>         (
>                 set_fake_editor &&
>                 test_must_fail env FAKE_LINES="1 squash 2" git rebase -i \
> -                       goodbye^ reverted-goodbye
> -       )
> +                       goodbye^ reverted-goodbye 2>err
> +       ) &&
> +       test_i18ngrep "git rebase --skip" err &&
> +       test_must_fail git commit --amend --no-edit 2>err &&
> +       test_i18ngrep "git rebase --skip" err
>  '
>
>  # Must be the last test in this file
> diff --git a/wt-status.h b/wt-status.h
> index 5f81bf7507..a4b7fe6de9 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -41,7 +41,8 @@ enum commit_whence {
>         FROM_MERGE,      /* commit came from merge */
>         FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
>         FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */
> -       FROM_REBASE_PICK /* commit came from a pick/reword/edit */
> +       FROM_REBASE_PICK, /* commit came from a pick/reword/edit */
> +       FROM_REBASE_FIXUP /* commit came from fixup/squash */
>  };
>
>  static inline int is_from_cherry_pick(enum commit_whence whence)
> @@ -52,7 +53,7 @@ static inline int is_from_cherry_pick(enum commit_whence whence)
>
>  static inline int is_from_rebase(enum commit_whence whence)
>  {
> -       return whence == FROM_REBASE_PICK;
> +       return whence == FROM_REBASE_PICK || whence == FROM_REBASE_FIXUP;
>  }
>
>  struct wt_status_change_data {
> --
> 2.24.0
>

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2020-02-26 19:45 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-22 23:30 [PATCH 0/3] commit: fix advice for empty commits during rebases Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 1/3] cherry-pick: add test for `--skip` advice in `git commit` Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 2/3] sequencer: export the function to get the path of `.git/rebase-merge/` Johannes Schindelin via GitGitGadget
2019-10-22 23:30 ` [PATCH 3/3] commit: give correct advice for empty commit during a rebase Johannes Schindelin via GitGitGadget
2019-10-23  2:45   ` Junio C Hamano
2019-10-24 10:15   ` Phillip Wood
2019-10-25 11:48     ` Johannes Schindelin
2019-10-25 14:01       ` Phillip Wood
2019-11-08  5:28         ` Junio C Hamano
2019-11-08 14:09           ` Johannes Schindelin
2019-11-11 16:13             ` Phillip Wood
2019-10-23  3:02 ` [PATCH 0/3] commit: fix advice for empty commits during rebases Junio C Hamano
2019-10-25 12:11   ` Johannes Schindelin
2019-10-29  2:05     ` Junio C Hamano
2019-10-29 13:00       ` Johannes Schindelin
2019-12-06 16:06 ` [PATCH v2 0/9] " Phillip Wood
2019-12-06 16:06   ` [PATCH v2 1/9] t3404: use test_cmp_rev Phillip Wood
2019-12-06 17:39     ` Junio C Hamano
2019-12-06 16:06   ` [PATCH v2 2/9] cherry-pick: add test for `--skip` advice in `git commit` Phillip Wood
2019-12-06 16:06   ` [PATCH v2 3/9] cherry-pick: check commit error messages Phillip Wood
2019-12-06 16:06   ` [PATCH v2 4/9] sequencer: write CHERRY_PICK_HEAD for reword and edit Phillip Wood
2019-12-06 16:06   ` [PATCH v2 5/9] commit: use enum value for multiple cherry-picks Phillip Wood
2019-12-06 18:13     ` Junio C Hamano
2019-12-06 16:06   ` [PATCH v2 6/9] commit: encapsulate determine_whence() for sequencer Phillip Wood
2019-12-06 18:24     ` Junio C Hamano
2019-12-18 14:26       ` Phillip Wood
2019-12-06 16:06   ` [PATCH v2 7/9] commit: give correct advice for empty commit during a rebase Phillip Wood
2019-12-06 16:06   ` [PATCH v2 8/9] [RFC] rebase: fix advice when a fixup creates an empty commit Phillip Wood
2020-02-26 19:45     ` Elijah Newren
2019-12-06 16:06   ` [PATCH v2 9/9] [RFC] rebase -i: leave CHERRY_PICK_HEAD when there are conflicts Phillip Wood
2019-12-18 14:35     ` Phillip Wood

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).