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

Allow copying multiple commits #1288

Merged
merged 7 commits into from Sep 19, 2022
Merged

Conversation

remique
Copy link
Contributor

@remique remique commented Aug 19, 2022

This Pull Request fixes #1244.

It changes the following:

  • allows copying multiple commits by marking them

As of now, the functionality is as follows:

  • No commits marked: copies selected line with short commit id
  • One commit marked: copies long short commit id
  • More than one commit marked consecutively: copies c1^..c8 (short hashes)
  • More than one commit marked randomly: copies c1 c2 c4 c5 c8 (short hashes)

I followed the checklist:

  • I added unittests
  • I ran make check without errors
  • I tested the overall application
  • I added an appropriate item to the changelog

@remique
Copy link
Contributor Author

remique commented Aug 22, 2022

I have noticed that sometimes the app crashes after I yank the commits. I reckon that it is caused by sorting the indexes and I am looking into it.

@remique
Copy link
Contributor Author

remique commented Aug 22, 2022

Seems to be working now. I'm not sure if it is the greatest design, but works and does not crash.

EDIT: Also, I updated the original comment, as it is now copying short hashes only.

@extrawurst
Copy link
Owner

You seem to be saving indices. Did you check if that works still in big repos with the log being loaded in batches?

@remique
Copy link
Contributor Author

remique commented Aug 23, 2022

I'm only saving indices that have been marked. I tested it with 25k commit repo and it works fine.

@extrawurst
Copy link
Owner

can you make the required addition to the changelog?

@remique
Copy link
Contributor Author

remique commented Aug 26, 2022

Done!

@extrawurst extrawurst added this to the v0.22 milestone Aug 27, 2022
@extrawurst
Copy link
Owner

putting in draft until the required changes are made

@extrawurst extrawurst marked this pull request as draft September 18, 2022 13:04
@extrawurst extrawurst removed this from the v0.22 milestone Sep 18, 2022
@remique
Copy link
Contributor Author

remique commented Sep 18, 2022

Hey, I updated the changes and everything seems to be working fine on a big repo. I also ran make check locally with no errors. Failed build seems to be a clippy bug though?

error[E0277]: expected a `FnOnce<()>` closure, found `std::string::String`
   --> src/components/commitlist.rs:195:20
    |
195 |                         .map_or_else(String::new(), |le| {
    |                          ----------- ^^^^^^^^^^^^^ expected an `FnOnce<()>` closure, found `std::string::String`
    |                          |
    |                          required by a bound introduced by this call
    |
    = help: the trait `FnOnce<()>` is not implemented for `std::string::String`
    = note: wrap the `std::string::String` in a closure with no arguments: `|| { /* code */ }`

Copy link
Owner

@extrawurst extrawurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix clippy nightly. see inline comment

@extrawurst extrawurst marked this pull request as ready for review September 19, 2022 07:37
@extrawurst extrawurst merged commit e0fa63c into extrawurst:master Sep 19, 2022
@extrawurst
Copy link
Owner

Thank you ❤️

heiskane pushed a commit to heiskane/gitui that referenced this pull request Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ranged copy hash
2 participants