BUG: "cherry-pick A..B || git reset --hard OTHER"

BUG: "cherry-pick A..B || git reset --hard OTHER"

[Junio C Hamano]

I was burned a few times with this in the past few years, but it did
not irritate me often enough that I didn't write it down.  But I
think this is serious enough that deserves attention from those who
were involved.

A short reproduction recipe, using objects from git.git that are
publicly available and stable, shows how bad it is.

    $ git checkout v2.9.3^0

    $ git cherry-pick 0582a34f52..a94bb68397
    ... see conflict, decide to give up backporting to
    ... such an old fork point.
    ... the git-prompt gives "|CHERRY-PICKING" correctly.

    $ git reset --hard v2.10.2^0
    ... the git-prompt no longer says "|CHERRY-PICKING"

    $ edit && git commit -m "prelim work for backporting"
    [detached HEAD cc5a6a9219] prelim work for backporting

    $ git cherry-pick 0582a34f52..a94bb68397
    error: a cherry-pick or revert is already in progress
    hint: try "git cherry-pick (--continue | --quit | --abort)"
    fatal: cherry-pick failed

    $ git cherry-pick --abort
    ... we come back to v2.9.3^0, losing the new commit!

The above shows two bugs.

 (1) The third invocation of "cherry-pick" with "--abort" to get rid
     of the state from the unfinished cherry-pick we did previously
     is necessary, but the command does not notice that we resetted
     to a new branch AND we even did some other work there.  This
     loses end-user's work.  

     "git cherry-pick --abort" should learn from "git am --abort"
     that has an extra safety to deal with the above workflow.  The
     state from the unfinished "am" is removed, but the head is not
     rewound to avoid losing end-user's work.

     You can try by replacing two instances of

	$ git cherry-pick 0582a34f52..a94bb68397

     with

	$ git format-patch --stdout 0582a34f52..a94bb68397 | git am

     in the above sequence, and conclude with "git am--abort" to see
     how much more pleasant and safe "git am --abort" is.

 (2) The bug in "cherry-pick --abort" is made worse because the
     git-completion script seems to be unaware of $GIT_DIR/sequencer
     and stops saying "|CHERRY-PICKING" after the step to switch to
     a different state with "git reset --hard v2.10.2^0".  If the
     prompt showed it after "git reset", the end user would have
     thought twice before starting the "prelim work".

Thanks.

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Christian Couder]

On Tue, Dec 6, 2016 at 7:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I was burned a few times with this in the past few years, but it did
> not irritate me often enough that I didn't write it down.  But I
> think this is serious enough that deserves attention from those who
> were involved.

Yeah, I agree that we should do something about it.

> A short reproduction recipe, using objects from git.git that are
> publicly available and stable, shows how bad it is.
>
>     $ git checkout v2.9.3^0
>
>     $ git cherry-pick 0582a34f52..a94bb68397
>     ... see conflict, decide to give up backporting to
>     ... such an old fork point.
>     ... the git-prompt gives "|CHERRY-PICKING" correctly.
>
>     $ git reset --hard v2.10.2^0
>     ... the git-prompt no longer says "|CHERRY-PICKING"
>
>     $ edit && git commit -m "prelim work for backporting"
>     [detached HEAD cc5a6a9219] prelim work for backporting
>
>     $ git cherry-pick 0582a34f52..a94bb68397
>     error: a cherry-pick or revert is already in progress
>     hint: try "git cherry-pick (--continue | --quit | --abort)"
>     fatal: cherry-pick failed
>
>     $ git cherry-pick --abort
>     ... we come back to v2.9.3^0, losing the new commit!
>
> The above shows two bugs.
>
>  (1) The third invocation of "cherry-pick" with "--abort" to get rid
>      of the state from the unfinished cherry-pick we did previously
>      is necessary, but the command does not notice that we resetted
>      to a new branch AND we even did some other work there.  This
>      loses end-user's work.
>
>      "git cherry-pick --abort" should learn from "git am --abort"
>      that has an extra safety to deal with the above workflow.  The
>      state from the unfinished "am" is removed, but the head is not
>      rewound to avoid losing end-user's work.
>
>      You can try by replacing two instances of
>
>         $ git cherry-pick 0582a34f52..a94bb68397
>
>      with
>
>         $ git format-patch --stdout 0582a34f52..a94bb68397 | git am
>
>      in the above sequence, and conclude with "git am--abort" to see
>      how much more pleasant and safe "git am --abort" is.

Ok I will try to take a look at that next week.

>  (2) The bug in "cherry-pick --abort" is made worse because the
>      git-completion script seems to be unaware of $GIT_DIR/sequencer
>      and stops saying "|CHERRY-PICKING" after the step to switch to
>      a different state with "git reset --hard v2.10.2^0".  If the
>      prompt showed it after "git reset", the end user would have
>      thought twice before starting the "prelim work".

I am not used to hacking the prompt or completion scripts, so I would
appreciate if someone else could do it.

Thanks,
Christian.

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

Hi,

On 12/06/2016 07:58 PM, Junio C Hamano wrote:
> I was burned a few times with this in the past few years, but it did
> not irritate me often enough that I didn't write it down.  But I
> think this is serious enough that deserves attention from those who
> were involved.
> 
> A short reproduction recipe, using objects from git.git that are
> publicly available and stable, shows how bad it is.
> 
>     $ git checkout v2.9.3^0
> 
>     $ git cherry-pick 0582a34f52..a94bb68397
>     ... see conflict, decide to give up backporting to
>     ... such an old fork point.
>     ... the git-prompt gives "|CHERRY-PICKING" correctly.
> 
>     $ git reset --hard v2.10.2^0
>     ... the git-prompt no longer says "|CHERRY-PICKING"
> 
>     $ edit && git commit -m "prelim work for backporting"
>     [detached HEAD cc5a6a9219] prelim work for backporting
> 
>     $ git cherry-pick 0582a34f52..a94bb68397
>     error: a cherry-pick or revert is already in progress
>     hint: try "git cherry-pick (--continue | --quit | --abort)"
>     fatal: cherry-pick failed
> 
>     $ git cherry-pick --abort
>     ... we come back to v2.9.3^0, losing the new commit!

Apart from the git-prompt bug: isn't this a user error? I think "git
cherry-pick --quit"[1] would be the right thing to do, not --abort.

On the other hand, one (as a user) could also expect that "git reset
--hard" also resets sequencer-related states (and that is what the
git-prompt suggests), but that would probably break a lot of scripts ;)

[1] By the way: git cherry-pick --quit, git rebase --forget ...
different wording for the same thing makes things unintuitive.

>  (1) The third invocation of "cherry-pick" with "--abort" to get rid
>      of the state from the unfinished cherry-pick we did previously
>      is necessary, but the command does not notice that we resetted
>      to a new branch AND we even did some other work there.  This
>      loses end-user's work.  
> 
>      "git cherry-pick --abort" should learn from "git am --abort"
>      that has an extra safety to deal with the above workflow.  The
>      state from the unfinished "am" is removed, but the head is not
>      rewound to avoid losing end-user's work.
> 
>      You can try by replacing two instances of
> 
> 	$ git cherry-pick 0582a34f52..a94bb68397
> 
>      with
> 
> 	$ git format-patch --stdout 0582a34f52..a94bb68397 | git am
> 
>      in the above sequence, and conclude with "git am--abort" to see
>      how much more pleasant and safe "git am --abort" is.
Definitely. I'd volunteer to add that safety guard. (But (2) remains.)

~Stephan

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Junio C Hamano]

Stephan Beyer <s-beyer@gmx.net> writes:

> [1] By the way: git cherry-pick --quit, git rebase --forget ...
> different wording for the same thing makes things unintuitive.

It is not too late to STOP "--forget" from getting added to "rebase"
and give it a better name.

Having said that, I have a feeling that these options do not have to
exist; isn't their presence just a symptom that the "--abort" for
the command misbehaves?  Isn't the reason why there is no need for
"am --quit" because its "--abort" behaves more sensibly?

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

Hi,

On 12/07/2016 09:04 PM, Junio C Hamano wrote:
> Stephan Beyer <s-beyer@gmx.net> writes:
> 
>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>> different wording for the same thing makes things unintuitive.
> 
> It is not too late to STOP "--forget" from getting added to "rebase"
> and give it a better name.

Oh. ;) I am not sure. I personally think that --forget is a better name
than --quit because when I hear --quit I tend to look into the manual
page first to check if there are weird side effects (and then the manual
page says that it "forgets" ;D).
So I'd rather favor adding --forget to cherry-pick/revert instead... or
this:

> Having said that, I have a feeling that these options do not have to
> exist; isn't their presence just a symptom that the "--abort" for
> the command misbehaves?  Isn't the reason why there is no need for
> "am --quit" because its "--abort" behaves more sensibly?
You're probably right. I have no other use-case in mind than "oh I
forgot that I was rebasing... now just abort that and don't bother me
further (i.e. please don't bring me back)"

~Stephan

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Duy Nguyen]

On Thu, Dec 8, 2016 at 3:35 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 12/07/2016 09:04 PM, Junio C Hamano wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.

Sorry I didn't know about --quit (and it has been there since 2011, I
guess I'm just not big sequencer user).

> Oh. ;) I am not sure. I personally think that --forget is a better name

Yeah, I was stuck with the name --destroy for many months and was very
happy the day I found --forget, which does not imply any destructive
side effects and is distinct enough from --abort to not confuse
people.

> than --quit because when I hear --quit I tend to look into the manual
> page first to check if there are weird side effects (and then the manual
> page says that it "forgets" ;D).
> So I'd rather favor adding --forget to cherry-pick/revert instead... or
> this:
-- 
Duy

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Duy Nguyen]

On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stephan Beyer <s-beyer@gmx.net> writes:
>
>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>> different wording for the same thing makes things unintuitive.
>
> It is not too late to STOP "--forget" from getting added to "rebase"
> and give it a better name.

Having the same operation with different names only increases git
reputation of bad/inconsistent UI. Either forget is renamed to quit,
or vice versa. I prefer forget, but the decision is yours and the
community's. So I'm sending two patches to rename in either direction.
You can pick one.
-- 
Duy

[PATCH] rebase: rename --forget to be consistent with sequencer

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-rebase.txt           | 4 ++--
 contrib/completion/git-completion.bash | 4 ++--
 git-rebase.sh                          | 6 +++---
 t/t3407-rebase-abort.sh                | 8 ++++----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e01d78e..f892458 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -12,7 +12,7 @@ SYNOPSIS
 	[<upstream> [<branch>]]
 'git rebase' [-i | --interactive] [options] [--exec <cmd>] [--onto <newbase>]
 	--root [<branch>]
-'git rebase' --continue | --skip | --abort | --forget | --edit-todo
+'git rebase' --continue | --skip | --abort | --quit | --edit-todo
 
 DESCRIPTION
 -----------
@@ -252,7 +252,7 @@ leave out at most one of A and B, in which case it defaults to HEAD.
 	will be reset to where it was when the rebase operation was
 	started.
 
---forget::
+--quit::
 	Abort the rebase operation but HEAD is not reset back to the
 	original branch. The index and working tree are also left
 	unchanged as a result.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8159f28..2c134f8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1670,10 +1670,10 @@ _git_rebase ()
 {
 	local dir="$(__gitdir)"
 	if [ -f "$dir"/rebase-merge/interactive ]; then
-		__gitcomp "--continue --skip --abort --forget --edit-todo"
+		__gitcomp "--continue --skip --abort --quit --edit-todo"
 		return
 	elif [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
-		__gitcomp "--continue --skip --abort --forget"
+		__gitcomp "--continue --skip --abort --quit"
 		return
 	fi
 	__git_complete_strategy && return
diff --git a/git-rebase.sh b/git-rebase.sh
index f0de633..c62b178 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,7 +43,7 @@ continue!          continue
 abort!             abort and check out the original branch
 skip!              skip current patch and continue
 edit-todo!         edit the todo list during an interactive rebase
-forget!            abort but keep HEAD where it is
+quit!              abort but keep HEAD where it is
 "
 . git-sh-setup
 . git-sh-i18n
@@ -240,7 +240,7 @@ do
 	--verify)
 		ok_to_skip_pre_rebase=
 		;;
-	--continue|--skip|--abort|--forget|--edit-todo)
+	--continue|--skip|--abort|--quit|--edit-todo)
 		test $total_argc -eq 2 || usage
 		action=${1##--}
 		;;
@@ -403,7 +403,7 @@ abort)
 	finish_rebase
 	exit
 	;;
-forget)
+quit)
 	exec rm -rf "$state_dir"
 	;;
 edit-todo)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 6bc5e71..910f218 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -99,26 +99,26 @@ testrebase() {
 testrebase "" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
-test_expect_success 'rebase --forget' '
+test_expect_success 'rebase --quit' '
 	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase master &&
 	test_path_is_dir .git/rebase-apply &&
 	head_before=$(git rev-parse HEAD) &&
-	git rebase --forget &&
+	git rebase --quit &&
 	test $(git rev-parse HEAD) = $head_before &&
 	test ! -d .git/rebase-apply
 '
 
-test_expect_success 'rebase --merge --forget' '
+test_expect_success 'rebase --merge --quit' '
 	cd "$work_dir" &&
 	# Clean up the state from the previous one
 	git reset --hard pre-rebase &&
 	test_must_fail git rebase --merge master &&
 	test_path_is_dir .git/rebase-merge &&
 	head_before=$(git rev-parse HEAD) &&
-	git rebase --forget &&
+	git rebase --quit &&
 	test $(git rev-parse HEAD) = $head_before &&
 	test ! -d .git/rebase-merge
 '
-- 
2.8.2.524.g6ff3d78

[PATCH] revert, cherry-pick: rename --quit to be consistent with rebase

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

The old --quit remains supported, just hidden away.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-cherry-pick.txt      |  2 +-
 Documentation/git-revert.txt           |  2 +-
 Documentation/sequencer.txt            |  2 +-
 builtin/revert.c                       |  7 +++++--
 contrib/completion/git-completion.bash |  4 ++--
 sequencer.c                            |  2 +-
 t/t3510-cherry-pick-sequence.sh        | 14 +++++++-------
 t/t3511-cherry-pick-x.sh               |  2 +-
 8 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 6154e57..de878ff 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff]
 		  [-S[<keyid>]] <commit>...
 'git cherry-pick' --continue
-'git cherry-pick' --quit
+'git cherry-pick' --forget
 'git cherry-pick' --abort
 
 DESCRIPTION
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 573616a..c21a43b 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[<keyid>]] <commit>...
 'git revert' --continue
-'git revert' --quit
+'git revert' --forget
 'git revert' --abort
 
 DESCRIPTION
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 5747f44..ddfaad6 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -3,7 +3,7 @@
 	'.git/sequencer'.  Can be used to continue after resolving
 	conflicts in a failed cherry-pick or revert.
 
---quit::
+--forget::
 	Forget about the current operation in progress.  Can be used
 	to clear the sequencer state after a failed cherry-pick or
 	revert.
diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..663eaf7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -77,7 +77,10 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	const char *me = action_name(opts);
 	int cmd = 0;
 	struct option options[] = {
-		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
+		OPT_CMDMODE(0, "forget", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
+		{ OPTION_CMDMODE, 0, "quit", &cmd, NULL,
+		  N_("end revert or cherry-pick sequence"),
+		  PARSE_OPT_NOARG|PARSE_OPT_NONEG|PARSE_OPT_HIDDEN, NULL, 'q' },
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
 		OPT_CMDMODE(0, "abort", &cmd, N_("cancel revert or cherry-pick sequence"), 'a'),
 		OPT_BOOL('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
@@ -134,7 +137,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->subcommand != REPLAY_NONE) {
 		char *this_operation;
 		if (opts->subcommand == REPLAY_REMOVE_STATE)
-			this_operation = "--quit";
+			this_operation = "--forget";
 		else if (opts->subcommand == REPLAY_CONTINUE)
 			this_operation = "--continue";
 		else {
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8159f28..d5c74e7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1047,7 +1047,7 @@ _git_cherry_pick ()
 {
 	local dir="$(__gitdir)"
 	if [ -f "$dir"/CHERRY_PICK_HEAD ]; then
-		__gitcomp "--continue --quit --abort"
+		__gitcomp "--continue --forget --abort"
 		return
 	fi
 	case "$cur" in
@@ -2303,7 +2303,7 @@ _git_revert ()
 {
 	local dir="$(__gitdir)"
 	if [ -f "$dir"/REVERT_HEAD ]; then
-		__gitcomp "--continue --quit --abort"
+		__gitcomp "--continue --forget --abort"
 		return
 	fi
 	case "$cur" in
diff --git a/sequencer.c b/sequencer.c
index e66f2fe..12d10d0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -812,7 +812,7 @@ static int create_seq_dir(void)
 {
 	if (file_exists(git_path_seq_dir())) {
 		error(_("a cherry-pick or revert is already in progress"));
-		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
+		advise(_("try \"git cherry-pick (--continue | --forget | --abort)\""));
 		return -1;
 	}
 	else if (mkdir(git_path_seq_dir(), 0777) < 0)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89d..d84fafa 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -18,7 +18,7 @@ test_description='Test cherry-pick continuation features
 _r10='\1\1\1\1\1\1\1\1\1\1'
 
 pristine_detach () {
-	git cherry-pick --quit &&
+	git cherry-pick --forget &&
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
 	git clean -d -f -f -q -x
@@ -89,9 +89,9 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' '
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success '--quit does not complain when no cherry-pick is in progress' '
+test_expect_success '--forget does not complain when no cherry-pick is in progress' '
 	pristine_detach initial &&
-	git cherry-pick --quit
+	git cherry-pick --forget
 '
 
 test_expect_success '--abort requires cherry-pick in progress' '
@@ -99,14 +99,14 @@ test_expect_success '--abort requires cherry-pick in progress' '
 	test_must_fail git cherry-pick --abort
 '
 
-test_expect_success '--quit cleans up sequencer state' '
+test_expect_success '--forget cleans up sequencer state' '
 	pristine_detach initial &&
 	test_expect_code 1 git cherry-pick base..picked &&
-	git cherry-pick --quit &&
+	git cherry-pick --forget &&
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success '--quit keeps HEAD and conflicted index intact' '
+test_expect_success '--forget keeps HEAD and conflicted index intact' '
 	pristine_detach initial &&
 	cat >expect <<-\EOF &&
 	OBJID
@@ -116,7 +116,7 @@ test_expect_success '--quit keeps HEAD and conflicted index intact' '
 	:000000 100644 OBJID OBJID A	unrelated
 	EOF
 	test_expect_code 1 git cherry-pick base..picked &&
-	git cherry-pick --quit &&
+	git cherry-pick --forget &&
 	test_path_is_missing .git/sequencer &&
 	test_must_fail git update-index --refresh &&
 	{
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..a56d48e 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -5,7 +5,7 @@ test_description='Test cherry-pick -x and -s'
 . ./test-lib.sh
 
 pristine_detach () {
-	git cherry-pick --quit &&
+	git cherry-pick --forget &&
 	git checkout -f "$1^0" &&
 	git read-tree -u --reset HEAD &&
 	git clean -d -f -f -q -x
-- 
2.8.2.524.g6ff3d78

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.
>
> Having the same operation with different names only increases git
> reputation of bad/inconsistent UI. Either forget is renamed to quit,
> or vice versa. I prefer forget, but the decision is yours and the
> community's. So I'm sending two patches to rename in either direction.
> You can pick one.

I actually was advocating to remove both by making --abort saner.
With an updated --abort that behaves saner, is "rebase --forget"
still necessary?

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

Hi Junio,

On 12/09/2016 07:07 PM, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>> Having the same operation with different names only increases git
>> reputation of bad/inconsistent UI. Either forget is renamed to quit,
>> or vice versa. I prefer forget, but the decision is yours and the
>> community's. So I'm sending two patches to rename in either direction.
>> You can pick one.
> 
> I actually was advocating to remove both by making --abort saner.
> With an updated --abort that behaves saner, is "rebase --forget"
> still necessary?

A quick change in t3407 of the "rebase --forget" test to use "rebase
--abort" failed.  That's because it checks the use-case of
forgetting/aborting without changing the HEAD.  So --abort makes a
rollback, --forget just keeps the current head.  I am not sure if that
tested use-case is a real use-case though.

A quick change in the pristine_detach function in t3510 and t3511 from
"cherry-pick --quit" to "cherry-pick --abort" works when one ignores the
return value of "cherry-pick --abort". The "--quit" is used here to
ensure a clean cherry-pick state, and --quit always succeeds, even if no
cherry-pick is in progress.  That may be a real use-case somehow that
could also be used for "rebase --forget"

t3510 also shows another use-case for --quit: the title says it all:
"cherry-pick --quit" to "cherry-pick --abort"

With this additional information, I'd vote to keep --quit/--forget and
just make it consistent.

~Stephan

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

On 12/09/2016 08:24 PM, Stephan Beyer wrote:
> t3510 also shows another use-case for --quit: the title says it all:
> "cherry-pick --quit" to "cherry-pick --abort"

I should've read what I actually pasted.
I wanted to paste: '--quit keeps HEAD and conflicted index intact'

Sorry for making no sense ;)

> With this additional information, I'd vote to keep --quit/--forget and
> just make it consistent.

Now!

~Stephan

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Duy Nguyen]

On Sat, Dec 10, 2016 at 2:24 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi Junio,
>
> On 12/09/2016 07:07 PM, Junio C Hamano wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>> Having the same operation with different names only increases git
>>> reputation of bad/inconsistent UI. Either forget is renamed to quit,
>>> or vice versa. I prefer forget, but the decision is yours and the
>>> community's. So I'm sending two patches to rename in either direction.
>>> You can pick one.
>>
>> I actually was advocating to remove both by making --abort saner.
>> With an updated --abort that behaves saner, is "rebase --forget"
>> still necessary?
>
> A quick change in t3407 of the "rebase --forget" test to use "rebase
> --abort" failed.  That's because it checks the use-case of
> forgetting/aborting without changing the HEAD.  So --abort makes a
> rollback, --forget just keeps the current head.  I am not sure if that
> tested use-case is a real use-case though.

It is. I wanted something like this for years but "rm -rf
/path/to/.git/rebase*" was not as bad when there were no linked
worktrees.

rebase and cherry-pick/revert are not exactly in the same situation.
When cherry-pick/revert in "continue/abort" mode, there's usually some
conflicted files and it's easy to notice.

But an interactive rebase could stop at some commit with clean
worktree (the 'edit' command). Then I could even add some more commits
on top. I don't see how 'rebase --abort' can know my intention in this
case, whether I tried (with some new commits) and failed, and want to
revert/abort the whole thing, moving HEAD back to the original; or
whether I forgot I was in the middle of rebase and started to do
something else, and --abort needs to keep HEAD where it is.
-- 
Duy

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> rebase and cherry-pick/revert are not exactly in the same situation.
> When cherry-pick/revert in "continue/abort" mode, there's usually some
> conflicted files and it's easy to notice.
>
> But an interactive rebase could stop at some commit with clean
> worktree (the 'edit' command). Then I could even add some more commits
> on top. I don't see how 'rebase --abort' can know my intention in this
> case, whether I tried (with some new commits) and failed, and want to
> revert/abort the whole thing, moving HEAD back to the original; or
> whether I forgot I was in the middle of rebase and started to do
> something else, and --abort needs to keep HEAD where it is.

OK.

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Christian Couder]

Hi,

On Wed, Dec 7, 2016 at 7:36 PM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 12/06/2016 07:58 PM, Junio C Hamano wrote:
>
>>  (1) The third invocation of "cherry-pick" with "--abort" to get rid
>>      of the state from the unfinished cherry-pick we did previously
>>      is necessary, but the command does not notice that we resetted
>>      to a new branch AND we even did some other work there.  This
>>      loses end-user's work.
>>
>>      "git cherry-pick --abort" should learn from "git am --abort"
>>      that has an extra safety to deal with the above workflow.  The
>>      state from the unfinished "am" is removed, but the head is not
>>      rewound to avoid losing end-user's work.
>>
>>      You can try by replacing two instances of
>>
>>       $ git cherry-pick 0582a34f52..a94bb68397
>>
>>      with
>>
>>       $ git format-patch --stdout 0582a34f52..a94bb68397 | git am
>>
>>      in the above sequence, and conclude with "git am--abort" to see
>>      how much more pleasant and safe "git am --abort" is.
> Definitely. I'd volunteer to add that safety guard. (But (2) remains.)

That would be great if you could take care of that!

Thanks,
Christian.

[PATCH 1/5] am: Fix filename in safe_to_abort() error message

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 Okay let's give it a try. Some minor things that I found
 are also in this patchset (patch 01, 02 and 05).
 The branch can also be found on
   https://github.com/sbeyer/git/commits/sequencer-abort-safety

 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce..7cf40e6f2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
 
 	if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
 		if (get_oid_hex(sb.buf, &abort_safety))
-			die(_("could not parse %s"), am_path(state, "abort_safety"));
+			die(_("could not parse %s"), am_path(state, "abort-safety"));
 	} else
 		oidclr(&abort_safety);
 
-- 
2.11.0.27.g4eed97c

Re: [PATCH 1/5] am: Fix filename in safe_to_abort() error message

[Paul Tan]

Hi Stephan,

On Thu, Dec 8, 2016 at 5:51 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> diff --git a/builtin/am.c b/builtin/am.c
> index 6981f42ce..7cf40e6f2 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
>
>         if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
>                 if (get_oid_hex(sb.buf, &abort_safety))
> -                       die(_("could not parse %s"), am_path(state, "abort_safety"));
> +                       die(_("could not parse %s"), am_path(state, "abort-safety"));

Ah, this is obviously correct. Sorry for the oversight.

>         } else
>                 oidclr(&abort_safety);
>
> --
> 2.11.0.27.g4eed97c

Thanks,
Paul

[PATCH 2/5] am: Change safe_to_abort()'s not rewinding error into a warning

[Stephan Beyer]

The error message tells the user that something went terribly wrong
and the --abort could not be performed. But the --abort is performed,
only without rewinding. By simply changing the error into a warning,
we indicate the user that she must not try something like
"git am --abort --force", instead she just has to check the HEAD.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 7cf40e6f2..826f18ba1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)
 	if (!oidcmp(&head, &abort_safety))
 		return 1;
 
-	error(_("You seem to have moved HEAD since the last 'am' failure.\n"
+	warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
 		"Not rewinding to ORIG_HEAD"));
 
 	return 0;
-- 
2.11.0.27.g4eed97c

[PATCH 3/5] Add test that cherry-pick --abort does not unsafely change HEAD

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 t/t3510-cherry-pick-sequence.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89dbd..372307c21 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' '
 	git diff-index --exit-code HEAD
 '
 
+test_expect_success '--abort does not unsafely change HEAD' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick picked anotherpick &&
+	git reset --hard base &&
+	test_must_fail git cherry-pick picked anotherpick &&
+	git cherry-pick --abort 2>actual &&
+	test_i18ngrep "You seem to have moved HEAD" actual &&
+	test_cmp_rev base HEAD
+'
+
 test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 	pristine_detach anotherpick &&
 	test_expect_code 1 git revert base..picked &&
-- 
2.11.0.27.g4eed97c

[PATCH 4/5] Make sequencer abort safer

[Stephan Beyer]

In contrast to "git am --abort", a sequencer abort did not check
whether the current HEAD is the one that is expected. This can
lead to loss of work (when not spotted and resolved using reflog
before the garbage collector chimes in).

This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD
is not the expected HEAD.

A new file "sequencer/current" is added to save the expected HEAD.

The new behavior is only active when --abort is invoked on multiple
picks. The problem does not occur for the single-pick case because
it is handled differently.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 sequencer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 30b10ba14..c9b560ac1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,7 @@ 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")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)
 	return -1;
 }
 
+static void update_curr_file()
+{
+	struct object_id head;
+
+	/* Do nothing on a single-pick */
+	if (!file_exists(git_path_seq_dir()))
+		return;
+
+	if (!get_oid("HEAD", &head))
+		write_file(git_path_curr_file(), "%s", oid_to_hex(&head));
+	else
+		write_file(git_path_curr_file(), "%s", "");
+}
+
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 			int unborn, struct replay_opts *opts)
 {
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	strbuf_release(&sb);
 	strbuf_release(&err);
 	ref_transaction_free(transaction);
+	update_curr_file();
 	return 0;
 }
 
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 
 leave:
 	free_message(commit, &msg);
+	update_curr_file();
 
 	return res;
 }
@@ -1132,9 +1149,34 @@ static int save_head(const char *head)
 	return 0;
 }
 
+static int rollback_is_safe()
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct object_id expected_head, actual_head;
+
+	if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {
+		strbuf_trim(&sb);
+		if (get_oid_hex(sb.buf, &expected_head)) {
+			strbuf_release(&sb);
+			die(_("could not parse %s"), git_path_curr_file());
+		}
+		strbuf_release(&sb);
+	}
+	else if (errno == ENOENT)
+		oidclr(&expected_head);
+	else
+		die_errno(_("could not read '%s'"), git_path_curr_file());
+
+	if (get_oid("HEAD", &actual_head))
+		oidclr(&actual_head);
+
+	return !oidcmp(&actual_head, &expected_head);
+}
+
 static int reset_for_rollback(const unsigned char *sha1)
 {
 	const char *argv[4];	/* reset --merge <arg> + NULL */
+
 	argv[0] = "reset";
 	argv[1] = "--merge";
 	argv[2] = sha1_to_hex(sha1);
@@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts)
 		error(_("cannot abort from a branch yet to be born"));
 		goto fail;
 	}
+
+	if (!rollback_is_safe()) {
+		/* Do not error, just do not rollback */
+		warning(_("You seem to have moved HEAD. "
+			  "Not rewinding, check your HEAD!"));
+	} else
 	if (reset_for_rollback(sha1))
 		goto fail;
 	strbuf_release(&buf);
@@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (save_opts(opts))
 		return -1;
+	update_curr_file();
 	res = pick_commits(&todo_list, opts);
 	todo_list_release(&todo_list);
 	return res;
-- 
2.11.0.27.g4eed97c

Re: [PATCH 4/5] Make sequencer abort safer

[Johannes Schindelin]

Hi,

On Wed, 7 Dec 2016, Stephan Beyer wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 30b10ba14..c9b560ac1 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -27,6 +27,7 @@ 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")
>  static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")

Is it required by law to have a four-letter infix, or can we have a nicer
variable name (e.g. git_path_current_file)?

Ciao,
Dscho

Re: [PATCH 4/5] Make sequencer abort safer

[Junio C Hamano]

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

> On Wed, 7 Dec 2016, Stephan Beyer wrote:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 30b10ba14..c9b560ac1 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -27,6 +27,7 @@ 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")
>>  static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
>
> Is it required by law to have a four-letter infix, or can we have a nicer
> variable name (e.g. git_path_current_file)?

I agree with you that, as other git_path_*_file variables match the
actual name on the filesystem, this one should too, together with
the update_curr_file() function.

By the way, this step seems to be a fix to an existing problem, and
the new test added in 3/5 seems to be a demonstration of the issue.
If that is the case, shouldn't the new test initially expect failure
and updated by this step to expect success?

I'll queue this on top of step 4/5 as "SQUASH???" as usual.  The
other SQUASH??? that must come after 3/5 for t3510 should be trivial
(the reverse of what appears here).

Thanks.


 sequencer.c                     | 22 +++++++++++-----------
 t/t3510-cherry-pick-sequence.sh |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c9b560ac15..ce04377f8e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ 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")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -311,7 +311,7 @@ static int error_dirty_index(struct replay_opts *opts)
 	return -1;
 }
 
-static void update_curr_file()
+static void update_current_file(void)
 {
 	struct object_id head;
 
@@ -320,9 +320,9 @@ static void update_curr_file()
 		return;
 
 	if (!get_oid("HEAD", &head))
-		write_file(git_path_curr_file(), "%s", oid_to_hex(&head));
+		write_file(git_path_current_file(), "%s", oid_to_hex(&head));
 	else
-		write_file(git_path_curr_file(), "%s", "");
+		write_file(git_path_current_file(), "%s", "");
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -354,7 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	strbuf_release(&sb);
 	strbuf_release(&err);
 	ref_transaction_free(transaction);
-	update_curr_file();
+	update_current_file();
 	return 0;
 }
 
@@ -829,7 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 
 leave:
 	free_message(commit, &msg);
-	update_curr_file();
+	update_current_file();
 
 	return res;
 }
@@ -1149,23 +1149,23 @@ static int save_head(const char *head)
 	return 0;
 }
 
-static int rollback_is_safe()
+static int rollback_is_safe(void)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id expected_head, actual_head;
 
-	if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {
+	if (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {
 		strbuf_trim(&sb);
 		if (get_oid_hex(sb.buf, &expected_head)) {
 			strbuf_release(&sb);
-			die(_("could not parse %s"), git_path_curr_file());
+			die(_("could not parse %s"), git_path_current_file());
 		}
 		strbuf_release(&sb);
 	}
 	else if (errno == ENOENT)
 		oidclr(&expected_head);
 	else
-		die_errno(_("could not read '%s'"), git_path_curr_file());
+		die_errno(_("could not read '%s'"), git_path_current_file());
 
 	if (get_oid("HEAD", &actual_head))
 		oidclr(&actual_head);
@@ -1441,7 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (save_opts(opts))
 		return -1;
-	update_curr_file();
+	update_current_file();
 	res = pick_commits(&todo_list, opts);
 	todo_list_release(&todo_list);
 	return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc485..372307c21b 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
 	git diff-index --exit-code HEAD
 '
 
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick picked anotherpick &&
 	git reset --hard base &&

Re: [PATCH 4/5] Make sequencer abort safer

[Stephan Beyer]

Hi,

I'm a little afraid of feeding Parkinson's law of triviality here, but... ;)

On 12/08/2016 06:27 PM, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> On Wed, 7 Dec 2016, Stephan Beyer wrote:
>>
>>> diff --git a/sequencer.c b/sequencer.c
>>> index 30b10ba14..c9b560ac1 100644
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -27,6 +27,7 @@ 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")
>>>  static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
>>> +static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
>>
>> Is it required by law to have a four-letter infix, or can we have a nicer
>> variable name (e.g. git_path_current_file)?
> 
> I agree with you that, as other git_path_*_file variables match the
> actual name on the filesystem, this one should too, together with
> the update_curr_file() function.

I totally agree with that (and I don't know why I used "curr", probably
just because it looked consistent and good...).

However:

> -static void update_curr_file()
> +static void update_current_file(void)

This function name could lead to the impression that there is some
current file (defined by a global state or whatever) that is updated.

So I'd rather rename the *file* to one of

 * sequencer/abort-safety (consistent to am, describes its purpose)
 * sequencer/safety (shorter, still describes the purpose)
 * sequencer/current-head (describes what it contains)
 * sequencer/last (a four-letter word, not totally unambiguous though)

> By the way, this step seems to be a fix to an existing problem, and
> the new test added in 3/5 seems to be a demonstration of the issue.
> If that is the case, shouldn't the new test initially expect failure
> and updated by this step to expect success?

That's usually a matter of taste that I sometimes also discuss with
colleagues in other projects... However, for the git test suite with its
"known breakage" behavior, your recommendation is surely the best way to
do it (aside from introducing the test and the fix in one commit... but
that does not show in the history that there actually was that breakage)

~Stephan

Re: [PATCH 4/5] Make sequencer abort safer

[Junio C Hamano]

Stephan Beyer <s-beyer@gmx.net> writes:

> However:
>
>> -static void update_curr_file()
>> +static void update_current_file(void)
>
> This function name could lead to the impression that there is some
> current file (defined by a global state or whatever) that is updated.
>
> So I'd rather rename the *file* to one of
>
>  * sequencer/abort-safety (consistent to am, describes its purpose)
>  * sequencer/safety (shorter, still describes the purpose)
>  * sequencer/current-head (describes what it contains)
>  * sequencer/last (a four-letter word, not totally unambiguous though)

OK, so here is a patch that needs to be squashed further on top of
4/5.  I just picked the first one on your list ;-)

Thanks.

 sequencer.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 874aaa4cd4..3ac4cb8d3b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ 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")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -306,7 +306,7 @@ static int error_dirty_index(struct replay_opts *opts)
 	return -1;
 }
 
-static void update_current_file(void)
+static void update_abort_safety_file(void)
 {
 	struct object_id head;
 
@@ -315,9 +315,9 @@ static void update_current_file(void)
 		return;
 
 	if (!get_oid("HEAD", &head))
-		write_file(git_path_current_file(), "%s", oid_to_hex(&head));
+		write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
 	else
-		write_file(git_path_current_file(), "%s", "");
+		write_file(git_path_abort_safety_file(), "%s", "");
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -349,7 +349,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	strbuf_release(&sb);
 	strbuf_release(&err);
 	ref_transaction_free(transaction);
-	update_current_file();
+	update_abort_safety_file();
 	return 0;
 }
 
@@ -824,7 +824,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 
 leave:
 	free_message(commit, &msg);
-	update_current_file();
+	update_abort_safety_file();
 
 	return res;
 }
@@ -1149,18 +1149,18 @@ static int rollback_is_safe(void)
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id expected_head, actual_head;
 
-	if (strbuf_read_file(&sb, git_path_current_file(), 0) >= 0) {
+	if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
 		strbuf_trim(&sb);
 		if (get_oid_hex(sb.buf, &expected_head)) {
 			strbuf_release(&sb);
-			die(_("could not parse %s"), git_path_current_file());
+			die(_("could not parse %s"), git_path_abort_safety_file());
 		}
 		strbuf_release(&sb);
 	}
 	else if (errno == ENOENT)
 		oidclr(&expected_head);
 	else
-		die_errno(_("could not read '%s'"), git_path_current_file());
+		die_errno(_("could not read '%s'"), git_path_abort_safety_file());
 
 	if (get_oid("HEAD", &actual_head))
 		oidclr(&actual_head);
@@ -1436,7 +1436,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (save_opts(opts))
 		return -1;
-	update_current_file();
+	update_abort_safety_file();
 	res = pick_commits(&todo_list, opts);
 	todo_list_release(&todo_list);
 	return res;

[PATCH v2 1/5] am: Fix filename in safe_to_abort() error message

[Stephan Beyer]

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce..7cf40e6f2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
 
 	if (read_state_file(&sb, state, "abort-safety", 1) > 0) {
 		if (get_oid_hex(sb.buf, &abort_safety))
-			die(_("could not parse %s"), am_path(state, "abort_safety"));
+			die(_("could not parse %s"), am_path(state, "abort-safety"));
 	} else
 		oidclr(&abort_safety);
 
-- 
2.11.0.27.g74d6bea

[PATCH v2 2/5] am: Change safe_to_abort()'s not rewinding error into a warning

[Stephan Beyer]

The error message tells the user that something went terribly wrong
and the --abort could not be performed. But the --abort is performed,
only without rewinding. By simply changing the error into a warning,
we indicate the user that she must not try something like
"git am --abort --force", instead she just has to check the HEAD.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 7cf40e6f2..826f18ba1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)
 	if (!oidcmp(&head, &abort_safety))
 		return 1;
 
-	error(_("You seem to have moved HEAD since the last 'am' failure.\n"
+	warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
 		"Not rewinding to ORIG_HEAD"));
 
 	return 0;
-- 
2.11.0.27.g74d6bea

[PATCH v2 3/5] Add test that cherry-pick --abort does not unsafely change HEAD

[Stephan Beyer]

The test expects failure because it is a current breakage
reported by Junio C Hamano.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 t/t3510-cherry-pick-sequence.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89dbd..efcd4fc48 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' '
 	git diff-index --exit-code HEAD
 '
 
+test_expect_failure '--abort does not unsafely change HEAD' '
+	pristine_detach initial &&
+	test_must_fail git cherry-pick picked anotherpick &&
+	git reset --hard base &&
+	test_must_fail git cherry-pick picked anotherpick &&
+	git cherry-pick --abort 2>actual &&
+	test_i18ngrep "You seem to have moved HEAD" actual &&
+	test_cmp_rev base HEAD
+'
+
 test_expect_success 'cherry-pick --abort to cancel multiple revert' '
 	pristine_detach anotherpick &&
 	test_expect_code 1 git revert base..picked &&
-- 
2.11.0.27.g74d6bea

[PATCH v2 4/5] Make sequencer abort safer

[Stephan Beyer]

In contrast to "git am --abort", a sequencer abort did not check
whether the current HEAD is the one that is expected. This can
lead to loss of work (when not spotted and resolved using reflog
before the garbage collector chimes in).

This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD
is not the expected HEAD.

A new file "sequencer/current" is added to save the expected HEAD.

The new behavior is only active when --abort is invoked on multiple
picks. The problem does not occur for the single-pick case because
it is handled differently.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 sequencer.c                     | 48 +++++++++++++++++++++++++++++++++++++++++
 t/t3510-cherry-pick-sequence.sh |  2 +-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 30b10ba14..35c158471 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,7 @@ 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")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)
 	return -1;
 }
 
+static void update_abort_safety_file(void)
+{
+	struct object_id head;
+
+	/* Do nothing on a single-pick */
+	if (!file_exists(git_path_seq_dir()))
+		return;
+
+	if (!get_oid("HEAD", &head))
+		write_file(git_path_abort_safety_file(), "%s", oid_to_hex(&head));
+	else
+		write_file(git_path_abort_safety_file(), "%s", "");
+}
+
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 			int unborn, struct replay_opts *opts)
 {
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
 	strbuf_release(&sb);
 	strbuf_release(&err);
 	ref_transaction_free(transaction);
+	update_abort_safety_file();
 	return 0;
 }
 
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 
 leave:
 	free_message(commit, &msg);
+	update_abort_safety_file();
 
 	return res;
 }
@@ -1132,6 +1149,30 @@ static int save_head(const char *head)
 	return 0;
 }
 
+static int rollback_is_safe(void)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct object_id expected_head, actual_head;
+
+	if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
+		strbuf_trim(&sb);
+		if (get_oid_hex(sb.buf, &expected_head)) {
+			strbuf_release(&sb);
+			die(_("could not parse %s"), git_path_abort_safety_file());
+		}
+		strbuf_release(&sb);
+	}
+	else if (errno == ENOENT)
+		oidclr(&expected_head);
+	else
+		die_errno(_("could not read '%s'"), git_path_abort_safety_file());
+
+	if (get_oid("HEAD", &actual_head))
+		oidclr(&actual_head);
+
+	return !oidcmp(&actual_head, &expected_head);
+}
+
 static int reset_for_rollback(const unsigned char *sha1)
 {
 	const char *argv[4];	/* reset --merge <arg> + NULL */
@@ -1189,6 +1230,12 @@ int sequencer_rollback(struct replay_opts *opts)
 		error(_("cannot abort from a branch yet to be born"));
 		goto fail;
 	}
+
+	if (!rollback_is_safe()) {
+		/* Do not error, just do not rollback */
+		warning(_("You seem to have moved HEAD. "
+			  "Not rewinding, check your HEAD!"));
+	} else
 	if (reset_for_rollback(sha1))
 		goto fail;
 	strbuf_release(&buf);
@@ -1393,6 +1440,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return -1;
 	if (save_opts(opts))
 		return -1;
+	update_abort_safety_file();
 	res = pick_commits(&todo_list, opts);
 	todo_list_release(&todo_list);
 	return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc48..372307c21 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
 	git diff-index --exit-code HEAD
 '
 
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick picked anotherpick &&
 	git reset --hard base &&
-- 
2.11.0.27.g74d6bea

Re: [PATCH v2 4/5] Make sequencer abort safer

[Christian Couder]

On Fri, Dec 9, 2016 at 8:01 PM, Stephan Beyer <s-beyer@gmx.net> wrote:

[...]

> +static int rollback_is_safe(void)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       struct object_id expected_head, actual_head;
> +
> +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
> +               strbuf_trim(&sb);
> +               if (get_oid_hex(sb.buf, &expected_head)) {
> +                       strbuf_release(&sb);
> +                       die(_("could not parse %s"), git_path_abort_safety_file());
> +               }
> +               strbuf_release(&sb);
> +       }

Maybe the following is a bit simpler:

       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
               int res;
               strbuf_trim(&sb);
               res = get_oid_hex(sb.buf, &expected_head);
               strbuf_release(&sb);
               if (res)
                   die(_("could not parse %s"), git_path_abort_safety_file());
       }

Thanks,
Christian.

Re: [PATCH v2 4/5] Make sequencer abort safer

[Jeff King]

On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:

> > +static int rollback_is_safe(void)
> > +{
> > +       struct strbuf sb = STRBUF_INIT;
> > +       struct object_id expected_head, actual_head;
> > +
> > +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
> > +               strbuf_trim(&sb);
> > +               if (get_oid_hex(sb.buf, &expected_head)) {
> > +                       strbuf_release(&sb);
> > +                       die(_("could not parse %s"), git_path_abort_safety_file());
> > +               }
> > +               strbuf_release(&sb);
> > +       }
> 
> Maybe the following is a bit simpler:
> 
>        if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>                int res;
>                strbuf_trim(&sb);
>                res = get_oid_hex(sb.buf, &expected_head);
>                strbuf_release(&sb);
>                if (res)
>                    die(_("could not parse %s"), git_path_abort_safety_file());
>        }

Is there any point in calling strbuf_release() if we're about to die
anyway? I could see it if it were "return error()", but it's normal in
our code base for die() to be abrupt.

-Peff

Re: [PATCH v2 4/5] Make sequencer abort safer

[Stephan Beyer]

On 12/10/2016 09:04 PM, Jeff King wrote:
> On Sat, Dec 10, 2016 at 08:56:26PM +0100, Christian Couder wrote:
> 
>>> +static int rollback_is_safe(void)
>>> +{
>>> +       struct strbuf sb = STRBUF_INIT;
>>> +       struct object_id expected_head, actual_head;
>>> +
>>> +       if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>>> +               strbuf_trim(&sb);
>>> +               if (get_oid_hex(sb.buf, &expected_head)) {
>>> +                       strbuf_release(&sb);
>>> +                       die(_("could not parse %s"), git_path_abort_safety_file());
>>> +               }
>>> +               strbuf_release(&sb);
>>> +       }
>>
>> Maybe the following is a bit simpler:
>>
>>        if (strbuf_read_file(&sb, git_path_abort_safety_file(), 0) >= 0) {
>>                int res;
>>                strbuf_trim(&sb);
>>                res = get_oid_hex(sb.buf, &expected_head);
>>                strbuf_release(&sb);
>>                if (res)
>>                    die(_("could not parse %s"), git_path_abort_safety_file());
>>        }
> 
> Is there any point in calling strbuf_release() if we're about to die
> anyway? I could see it if it were "return error()", but it's normal in
> our code base for die() to be abrupt.

The point is that someone "libifies" the function some day; then "die()"
becomes "return error()" almost automatically. Chances are high that the
resulting memory leak is forgotten. That's one of the reasons why I like
being strict about memory leaks.

However, I cannot tell if mine or Christian's variant is really
"simpler" (with whatever measure) and I also don't care much.

~Stephan

[PATCH v2 5/5] sequencer: Remove useless get_dir() function

[Stephan Beyer]

This function is used only once, for the removal of the
directory. It is not used for the creation of the directory
nor anywhere else.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 sequencer.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 35c158471..aba096a0a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)
 	return 0;
 }
 
-static const char *get_dir(const struct replay_opts *opts)
-{
-	return git_path_seq_dir();
-}
-
 static const char *get_todo_path(const struct replay_opts *opts)
 {
 	return git_path_todo_file();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 		free(opts->xopts[i]);
 	free(opts->xopts);
 
-	strbuf_addf(&dir, "%s", get_dir(opts));
+	strbuf_addf(&dir, "%s", git_path_seq_dir());
 	remove_dir_recursively(&dir, 0);
 	strbuf_release(&dir);
 
-- 
2.11.0.27.g74d6bea

[PATCH 5/5] sequencer: Remove useless get_dir() function

[Stephan Beyer]

This function is used only once, for the removal of the
directory. It is not used for the creation of the directory
nor anywhere else.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 sequencer.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c9b560ac1..689cfa5f1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)
 	return 0;
 }
 
-static const char *get_dir(const struct replay_opts *opts)
-{
-	return git_path_seq_dir();
-}
-
 static const char *get_todo_path(const struct replay_opts *opts)
 {
 	return git_path_todo_file();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)
 		free(opts->xopts[i]);
 	free(opts->xopts);
 
-	strbuf_addf(&dir, "%s", get_dir(opts));
+	strbuf_addf(&dir, "%s", git_path_seq_dir());
 	remove_dir_recursively(&dir, 0);
 	strbuf_release(&dir);
 
-- 
2.11.0.27.g4eed97c