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.

Sign up for GitHub

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

Jump to bottom

Disable dotted range commit yanking #2577

Merged
merged 1 commit into from
Apr 4, 2025
Merged

Disable dotted range commit yanking #2577

merged 1 commit into from
Apr 4, 2025

Conversation

naseschwarz
Copy link
Contributor

@naseschwarz naseschwarz commented Mar 20, 2025 •
edited
Loading

The algorithm for computing marked_consecutive assumes that commits are consecutive in the commit graph if they are consecutive in the linearized log used in commitlist.rs. That is not universally correct, as siblings may also be displayed consecutively in this list.

For now, this just removes generating commit lists in dotted range notation.

This Pull Request fixes/closes #2576

It changes the following:

  • Removes dotted range notation commit yanking

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

@naseschwarz naseschwarz marked this pull request as draft March 20, 2025 20:46
@naseschwarz naseschwarz marked this pull request as ready for review March 20, 2025 21:07
This was referenced Mar 20, 2025
Closed
Open
@extrawurst
Copy link
Collaborator

extrawurst commented Mar 22, 2025 •
edited
Loading

@naseschwarz this could be unittested, right?

@extrawurst extrawurst requested a review from vlad-anger March 22, 2025 10:44
@naseschwarz
Copy link
Contributor Author

@vlad-anger: I'll add some test to this and give you a heads up. No need to look at this right now.

@naseschwarz naseschwarz marked this pull request as draft March 22, 2025 11:59
@naseschwarz naseschwarz force-pushed the fix_marked_consecutive branch 3 times, most recently from d17d5fe to 83603ad Compare March 23, 2025 00:13
@naseschwarz
Copy link
Contributor Author

@vlad-anger: Here we go. :)

@naseschwarz naseschwarz marked this pull request as ready for review March 23, 2025 00:19
vlad-anger
vlad-anger reviewed Mar 23, 2025
View reviewed changes
Copy link
Contributor

@vlad-anger vlad-anger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've checked PR & see why prevsly implemented dotted yank was not sufficient.
Should we add some notes/todos for future implementors?
Also my 2 review cents:

Disable dotted range commit yanking
7f65df4 
The algorithm for computing marked_consecutive assumes that commits are
consecutive in the commit graph if they are consecutive in the
linearized log used in` commitlist.rs`. That is not universally correct,
as siblings may also be displayed consecutively in this list.

For now, this just removes generating commit lists in dotted range
notation.
@naseschwarz naseschwarz force-pushed the fix_marked_consecutive branch from 35265f8 to 7f65df4 Compare March 25, 2025 07:01
@naseschwarz
Copy link
Contributor Author

Thank you, @vlad-anger. I've applied all suggestions.

@extrawurst extrawurst merged commit 89f73d2 into gitui-org:master Apr 4, 2025
21 checks passed
@extrawurst
Copy link
Collaborator

Awesome !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vlad-anger vlad-anger vlad-anger left review comments

Assignees
No one assigned
Labels
merge_ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Copying multiple hashes yields incorrect dotted range on nonlinear histories and consecutive selections
3 participants
@naseschwarz @extrawurst @vlad-anger