threads / bug / 44631

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

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

## tl;dr

15 messages between Dec 6, 2016 and Dec 10, 2016.

replies: 14people: 4as markdown or json

Junio C Hamano· Dec 6, 2016, 18:58 UTC · lore

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.
Christian Couder· Dec 7, 2016, 14:31 UTC · re: Junio C Hamano · lore

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

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.
Show 47 quoted lines
> 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.
Show 6 quoted lines
>  (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.

Stephan Beyer· Dec 7, 2016, 18:36 UTC · re: Junio C Hamano · lore

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

Hi,
On 12/06/2016 07:58 PM, Junio C Hamano wrote:
Show 28 quoted lines
> 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.

Show 21 quoted lines
>  (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
Junio C Hamano· Dec 7, 2016, 20:04 UTC · re: Stephan Beyer · lore

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

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?

Stephan Beyer· Dec 7, 2016, 20:35 UTC · re: Junio C Hamano · lore

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

Hi,
On 12/07/2016 09:04 PM, Junio C Hamano wrote:
Show 7 quoted lines
> 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
Duy Nguyen· Dec 7, 2016, 23:21 UTC · re: Stephan Beyer · lore

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

On Thu, Dec 8, 2016 at 3:35 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
Show 10 quoted lines
> 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.

Show 5 quoted lines
> 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
Duy Nguyen· Dec 9, 2016, 11:33 UTC · re: Junio C Hamano · lore

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

On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> 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
Nguyễn Thái Ngọc Duy· Dec 9, 2016, 11:34 UTC · re: Duy Nguyen · lore

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

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
Nguyễn Thái Ngọc Duy· Dec 9, 2016, 11:34 UTC · re: Nguyễn Thái Ngọc Duy · lore

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

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
Junio C Hamano· Dec 9, 2016, 18:07 UTC · re: Duy Nguyen · lore

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

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

Stephan Beyer· Dec 9, 2016, 19:24 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On 12/09/2016 07:07 PM, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Stephan Beyer· Dec 9, 2016, 19:28 UTC · re: Stephan Beyer · lore

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

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
Duy Nguyen· Dec 10, 2016, 11:00 UTC · re: Stephan Beyer · lore

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

On Sat, Dec 10, 2016 at 2:24 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
Show 19 quoted lines
> 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
Junio C Hamano· Dec 10, 2016, 21:23 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 11 quoted lines
> 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.
Christian Couder· Dec 8, 2016, 07:50 UTC · re: Stephan Beyer · lore

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

Hi,
On Wed, Dec 7, 2016 at 7:36 PM, Stephan Beyer <s-beyer@gmx.net> wrote:
Show 26 quoted lines
> 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.

← back to recent threads