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

[Junio C Hamano]

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"

[Duy Nguyen]

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"

[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"

[Stephan Beyer]

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"

[Junio C Hamano]

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"

[Duy Nguyen]

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"

[Christian Couder]

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"

[Duy Nguyen]

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"

[Stephan Beyer]

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"

[Junio C Hamano]

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"

[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"

[Christian Couder]

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.

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.