Clear cherry-picked commits after pasting

[molejnik88]

• PR Description

It can be tedious after each cherry-pick opearation to clear the selection by pressing escape in order for lazygit to stop displaying info about copied commits. Also, it seems to be a rare case to cherry-pick commits to more than one destination.

The simplest solution to address this issue is to clear the selection upon paste, including merge conflict scenario.
Previously discussed in #3198.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[stefanhaller]

Looks good, thanks. One suggestions about error handling below, apart from that it's great. The adaptions of the integration tests are well done.

I have to say that last week I did run into a situation where it was convenient to have the copied commits still around; the situation was that pasting failed with a conflict, so I aborted the rebase, and realized that pasting into the middle of my branch would avoid the conflicts, so I pressed 'e' somewhere in the middle and pasted again there. It would have been a minor inconvenience to have to copy the commits again.

However, such a situation is probably rare enough that we don't have to worry about it; I just wanted to mention it.

[stefanhaller]

The lack of error handling makes me slightly nervous. If a function can return an error, it should be handled; if the function never returns an error, or it's not important to handle, then it shouldn't have a return type of "error".

One way to solve this could be to remove the Reset from here, turn the two returns below into err = ... (the second would have to be in an else block then), and at the very end of the function do

if err == nil {
  err = Reset()
}
return err

[stefanhaller]

OK, that suggestion doesn't work, because of the WithWaitingStatus in the else branch, which is asynchronous.

But we can still do the Reset after carrying out the operation, and only if it didn't return an error, we just have to do it in both places. I pushed a fixup to do that (ea90359); and I also included an additional check to only clear the commits if we didn't get conflicts, to address the concern I brought up above. I feel this is a good middle ground.

Let me know what you think!

[molejnik88]

Thank you @stefanhaller for your feedback and your fix.

I tested the proposed solution in the sandbox and it makes a lot of sense to me too.
Currently there is a slight inconsistency in the behavior (after having solved merge conflicts user still needs to manually press Esc to clear the copied commits), but I think it's an acceptable trade-off.

[stefanhaller]

I agree that the trade-off is acceptable. Just waiting for @jesseduffield's ok before I merge.

[jesseduffield]

Looking good, left some feedback.

Also, can we add an assertion to pkg/integration/tests/cherry_pick/cherry_pick_conflicts.go that the selection is retained when there's conflicts?

[jesseduffield]

More idiomatic way of writing the same thing:

if err != nil {
	return err
}
return self.Reset()

[jesseduffield]

Same as above:

err = self.rebaseHelper.CheckMergeOrRebase(err)
if err != nil {
	return err
}

// If we're in an interactive rebase at this point, it must
// be because there were conflicts. Don't clear the copied
// commits in this case, since we might want to abort and
// try pasting them again.
isInRebase, err = self.c.Git().Status.IsInInteractiveRebase()
if err != nil {
	return err
}

if !isInRebase {
	return self.Reset()
}

return nil

I know it's more verbose to do it this way but it's less nested and I find it easier to reason about go code when I can skim over if err != nil blocks compared to if err == nil

[stefanhaller]

Makes sense. @molejnik88, are you up for taking over again?

[molejnik88]

@stefanhaller, yes, I can take care of it.

[molejnik88]

@jesseduffield @stefanhaller I can see some checks are awaiting for their status to be reported. Do I need to do anything to fix it?

[stefanhaller]

I hadn't realized that you were done addressing the feedback; it's best to ping people again in that case, the notification that new commits were pushed is a bit too subtle for that (in my opinion).

It would be good if you could squash the three commits into one, and rebase onto master. Please don't merge master into feature branches; rebase on master instead, it results in a cleaner git history.

[molejnik88]

@stefanhaller @jesseduffield I squashed the commits and added suggested changes. Can you please take a look at it?

[jesseduffield]

Just one more comment, otherwise LGTM

[jesseduffield]

Worth adding a comment explaining why we expect to see this i.e.:
"cherry pick selection is not cleared when there are conflicts, so that the user is able to abort and try again without having to re-copy the commits"

[molejnik88]

Good point. Thank you for the feedback.

[molejnik88]

@jesseduffield I amended and force-pushed the suggested comment.

[stefanhaller]

Nice. Great work @molejnik88, and thanks for bearing with us!

[afhoffman]

Hey y'all I was just running the tests today and noticed that pkg/integration/tests/demo/cherry_pick.go needs a tiny update after these changes. Just figured I'd share since I managed to catch it by random chance: afhoffman@fdc54b7

[stefanhaller]

Good find, thanks. I usually don't run the demos locally, and they don't run on CI (maybe they should?), so that's why it slipped through. Do you feel like making a PR with this commit?

(Edit: found this comment...)

[afhoffman]

Maybe it would be good to run the demos in CI? But that comment you found is spot on, of course. Would have to strip out the pauses. Doesn't seem like it would add a lot to the CI runs, but it could also be pretty rare that one of the demos failing in CI catches something.

Sure thing, I'll put in a PR.

[stefanhaller]

Maybe it would be good to run the demos in CI? But that comment you found is spot on, of course. Would have to strip out the pauses.

I'm not sure it's such a big deal, actually. We're running the tests on multiple cores, so while one test is pausing we can saturate the CPU running other tests meanwhile.

I took some timings, and on my machine the tests take 25s to run without the demos, 32s with the demos with pauses, and 29s with demos but with the Sleep in TestDriver.Wait commented out. From this, I'd say we can totally afford running them, and it isn't necessarily worth doing something about the delays.

[afhoffman]

Oh! Yeah that makes sense if they're multi-threaded. I just gave it a try too and saw similar deltas: 37s w/o demos, 46s w/ demos, and 45s w/ demos & that Sleep commented out. I think it's definitely not worth worrying about the delays if you include the demos in CI.