# BUG: "cherry-pick A..B || git reset --hard OTHER" 15 messages from 2016-12-06 to 2016-12-10. Participants: Junio C Hamano, Christian Couder, Stephan Beyer, Duy Nguyen, Nguyễn Thái Ngọc Duy. Thread: https://gitlist.dev/t/44631 ## Junio C Hamano, 2016-12-06 18:58 Subject: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/xmqqlgvs28bh.fsf%40gitster.mtv.corp.google.com ``` 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, 2016-12-07 14:31 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/CAP8UFD32PcT4XgnRFaP-St3i1D-6XG1wY6qKn3bgWPKn1Y4Qwg%40mail.gmail.com In-Reply-To: ``` On Tue, Dec 6, 2016 at 7: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. 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. ``` ## Stephan Beyer, 2016-12-07 18:36 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: <6facca6e-622a-ea8f-89d8-a18b7faee3cc@gmx.net> URL: https://gitlist.dev/e/6facca6e-622a-ea8f-89d8-a18b7faee3cc%40gmx.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2016-12-07 20:04 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/xmqq8trry08k.fsf%40gitster.mtv.corp.google.com In-Reply-To: <6facca6e-622a-ea8f-89d8-a18b7faee3cc@gmx.net> ``` Stephan Beyer 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, 2016-12-07 20:35 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: <8d32c645-e850-5e7c-b01c-6e4d81e2d672@gmx.net> URL: https://gitlist.dev/e/8d32c645-e850-5e7c-b01c-6e4d81e2d672%40gmx.net In-Reply-To: ``` Hi, On 12/07/2016 09:04 PM, Junio C Hamano wrote: > Stephan Beyer 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, 2016-12-07 23:21 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/CACsJy8AP-N4BDyaXszvbh2k8fr7jO%2BYhVj%3D_0o0x1ooWrqkR4A%40mail.gmail.com In-Reply-To: <8d32c645-e850-5e7c-b01c-6e4d81e2d672@gmx.net> ``` On Thu, Dec 8, 2016 at 3:35 AM, Stephan Beyer wrote: > Hi, > > On 12/07/2016 09:04 PM, Junio C Hamano wrote: >> Stephan Beyer 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 ``` ## Christian Couder, 2016-12-08 07:50 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/CAP8UFD0amsoo%3DQSZyc8rKcJqjO-o7P4TNDQjReXQK%2Bm4hJCB6A%40mail.gmail.com In-Reply-To: <6facca6e-622a-ea8f-89d8-a18b7faee3cc@gmx.net> ``` Hi, On Wed, Dec 7, 2016 at 7:36 PM, Stephan Beyer 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. ``` ## Duy Nguyen, 2016-12-09 11:33 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/CACsJy8CX0HO%3DLxcEK3K%2BpCecgFY%3D40R%2BgpFoy7CGeN5zEJFJVQ%40mail.gmail.com In-Reply-To: ``` On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano wrote: > Stephan Beyer 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, 2016-12-09 11:34 Subject: [PATCH] rebase: rename --forget to be consistent with sequencer Message-ID: <20161209113427.6039-1-pclouds@gmail.com> URL: https://gitlist.dev/e/20161209113427.6039-1-pclouds%40gmail.com In-Reply-To: ``` Signed-off-by: Nguyễn Thái Ngọc Duy --- 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 [ []] 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ] --root [] -'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, 2016-12-09 11:34 Subject: [PATCH] revert, cherry-pick: rename --quit to be consistent with rebase Message-ID: <20161209113427.6039-2-pclouds@gmail.com> URL: https://gitlist.dev/e/20161209113427.6039-2-pclouds%40gmail.com In-Reply-To: <20161209113427.6039-1-pclouds@gmail.com> ``` The old --quit remains supported, just hidden away. Signed-off-by: Nguyễn Thái Ngọc Duy --- 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[]] ... '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[]] ... '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, 2016-12-09 18:07 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/xmqq8trprn7f.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Duy Nguyen writes: > On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano wrote: >> Stephan Beyer 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, 2016-12-09 19:24 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/e0780f7c-ccb4-29fe-3d72-80e45a202f65%40gmx.net In-Reply-To: ``` Hi Junio, On 12/09/2016 07:07 PM, Junio C Hamano wrote: > Duy Nguyen 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, 2016-12-09 19:28 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: <6ee77e7d-b93e-107b-c3fb-519790453b59@gmx.net> URL: https://gitlist.dev/e/6ee77e7d-b93e-107b-c3fb-519790453b59%40gmx.net In-Reply-To: ``` 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, 2016-12-10 11:00 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/CACsJy8Div%3DBaenn7c-1wxgvrOh5PG%3DnaeDrEYC8gs%2BAvJE7wZA%40mail.gmail.com In-Reply-To: ``` On Sat, Dec 10, 2016 at 2:24 AM, Stephan Beyer wrote: > Hi Junio, > > On 12/09/2016 07:07 PM, Junio C Hamano wrote: >> Duy Nguyen 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, 2016-12-10 21:23 Subject: Re: BUG: "cherry-pick A..B || git reset --hard OTHER" Message-ID: URL: https://gitlist.dev/e/xmqqfulvpjgh.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Duy Nguyen 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. ```