Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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.
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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
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
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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?
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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.
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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?
Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
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"
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.