Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear copied commits selection after pasting (cherry-pick) #3198

Closed
jesseduffield opened this issue Jan 5, 2024 · 7 comments
Closed

Clear copied commits selection after pasting (cherry-pick) #3198

jesseduffield opened this issue Jan 5, 2024 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@jesseduffield
Copy link
Owner

Is your feature request related to a problem? Please describe.

It's annoying that each time I cherry pick commits I need to manually clear the selection by pressing escape in order for lazygit to stop telling me that I've copied commits.

There are two ways to solve this problem:

  1. clear the selection upon paste
  2. don't clear the selection, but stop telling the user that there is a selection i.e. hide but don't clear

The first option is obviously the easiest to implement, but the reason I initially required the selection to be manually cleared is that some users may need to paste a commit to multiple destination branches.

Now, if such a user has pasted a bespoke commit (e.g. that adds a new file) and the selection is automatically cleared, they can just copy the newly created commit and then go paste that in the other destination. But if it's a commit whose patch changes based due to the new base (or if it's a set of commits, one of which ends up being a no-op in the destination branch due to existing changes), they'll need to navigate back to the original branch to re-copy the original commits.

So the question is: how much do we care about that situation? I very rarely encounter it myself, but it is a real thing.

If we do want to support this use case, we could make it so that upon pasting, we hide the seletion without clearing it. So that means the commits will no longer be highlighted and the bottom right of the screen no longer mentions copied commits. We could tell the user when they confirm the cherry-pick 'Lazygit will remember the copied commits so you can still paste them again in another destination'. This honestly doesn't even sound that hard to implement: you'd just need a new boolean flag like 'isPasted' which, if true, hides all the UI stuff around cherry picking.

Putting aside the question of hide vs clear, we need to consider another scenario: merge conflicts, If we encounter conflicts when cherry-picking, do we:

  1. clear the selection anyway
  2. wait until the rebase has successfully completed before clearing the selection
  3. leave it to the user to clear the selection

1 is easy to implement but is jarring for users who want to retry the cherry pick after aborting. As for 2, it's not clear to me how to do this in a clean way: do you just say that any time any rebase finishes, we clear the selection? And how do you even know if a rebase was successful or not, e.g. if it happened outside of git (users often resolve merge conflicts outside of lazygit)? Option 3 is straightforward but inconsistent with the default behaviour of automatically clearing. Nonetheless, if conflicts happen rarely enough, that inconsistency could be tolerable.

Alternatives considered

We could add a config option for whether you clear on paste, but I don't like the idea because whether you want it cleared on paste is situation dependent, and you can't know ahead of time.

Open questions

  • Do we want a concept of a 'hidden' selection for those who want to paste to multiple destinations?
  • What do we do with the selection when we encounter conflicts?

What do people think?

@jesseduffield jesseduffield added the enhancement New feature or request label Jan 5, 2024
@stefanhaller
Copy link
Collaborator

This also bothers me every time I paste commits. I have never had the need to cherry-pick commits to more than one destination, so for me it would be fine to auto-clear on paste. Hiding the selection seems messy to me (both conceptually, and in the code).

As for conflicts, I don't care too much, I could live with either 1. or 3. I'd probably vote for 1. as it seems like the most simple one.

@mark2185
Copy link
Collaborator

mark2185 commented Jan 7, 2024

There are two ways to solve this problem:

1. clear the selection upon paste

2. don't clear the selection, but stop telling the user that there is a selection i.e. hide but don't clear

I'd like to add a third option:

  • clear the selection upon paste, but provide the option to reselect the previous commits

It does mean people who want to paste to multiple destinations will still have to press an extra key or two, but it doesn't hide state.

@jesseduffield
Copy link
Owner Author

It sounds like the simplest approach is to just auto-clear on paste no matter what. I like the idea of being able to reselect previous commits, but I'm happy to wait and see if anybody actually needs it before implementing it

@jesseduffield jesseduffield added the good first issue Good for newcomers label Jan 11, 2024
stefanhaller added a commit that referenced this issue Jan 30, 2024
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.
@Vanav
Copy link

Vanav commented Oct 11, 2024

I often need to cherry-pick same commits in multiple branches, e.g. backport patch to multiple major versions or multiple environments. Can you, please, add a way to reselect commits or to paste same commits again?

@jesseduffield
Copy link
Owner Author

We could do a thing where we keep track of the copied commits and if you go to re-paste again we can show a confirmation modal saying 'paste previously copied commits?'

@stefanhaller
Copy link
Collaborator

We could do a thing where we keep track of the copied commits and if you go to re-paste again we can show a confirmation modal saying 'paste previously copied commits?'

This is very similar to your suggestion 2. above ("don't clear the selection, but stop telling the user that there is a selection i.e. hide but don't clear"). This seems a little clearer to me, the effect should almost be the same. Here's a PR that does this: #3983.

The subtle difference to your suggestion is that we get the same confirmation message again when pasting a second time. If you prefer to have the "paste previously copied commits?" one instead, this would be very easy to do of course.

And just for completeness, two other options we could consider:

  • add a config clearCopiedCommitsAfterPasting, defaulting to true
  • turn the current confirmation ("Are you sure you want to cherry-pick the copied commits onto this branch?") into a menu with the options "Paste and clear", "Paste and keep", "Cancel".

stefanhaller added a commit that referenced this issue Oct 13, 2024
- **PR Description**

After pasting commits, hide the cherry-pick status (i.e. remove the "x
commits copied" status in the lower right corner, and hide the blue
selection of the copied commits). However, keep the copied commits
around so that it's possible to paste them again. This can be useful
e.g. to backport a bugfix to multiple major version release branches.

Discussed in #3198.

- **Please check if the PR fulfills these requirements**

* [x] Cheatsheets are up-to-date (run `go generate ./...`)
* [x] Code has been formatted (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting))
* [x] Tests have been added/updated (see
[here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md)
for the integration test guide)
* [ ] Text is internationalised (see
[here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation))
* [ ] If a new UserConfig entry was added, make sure it can be
hot-reloaded (see
[here](https://github.com/jesseduffield/lazygit/blob/master/docs/dev/Codebase_Guide.md#using-userconfig))
* [ ] Docs have been updated if necessary
* [x] You've read through your own file changes for silly mistakes etc
@stefanhaller
Copy link
Collaborator

This was implemented in #3240, and improved in #3983.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants