fix: Reverse commit range before yanking

[Esgariot]

Produce <oldest>^..<newest> when yanking consecutive range.
Now, given consecutive marked selection, gitui's selection matches
git log's output in commit range.

This Pull Request fixes/closes #2246.

It changes the following:

Produce <oldest>^..<newest> when yanking consecutive range.
Now, given consecutive marked selection, gitui's selection matches
git log's output in commit range.

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

[Esgariot]

output from

 make check

cargo fmt -- --check
cargo clippy --workspace --all-features
warning: the following explicit lifetimes could be elided: 'a
  --> scopetime/src/lib.rs:35:6
   |
35 | impl<'a> Drop for ScopeTimeLog<'a> {
   |      ^^                        ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
   = note: `#[warn(clippy::needless_lifetimes)]` on by default
help: elide the lifetimes
   |
35 - impl<'a> Drop for ScopeTimeLog<'a> {
35 + impl Drop for ScopeTimeLog<'_> {
   |

warning: `scopetime` (lib) generated 1 warning (run `cargo clippy --fix --lib -p scopetime` to apply 1 suggestion)
warning: [email protected]: buildname 'nightly 2024-12-03 (3a861dc)'
    Checking asyncgit v0.26.3 (/home/esgariot/Development/gitui/asyncgit)
    Checking filetreelist v0.5.2 (/home/esgariot/Development/gitui/filetreelist)
error: it is more idiomatic to use `Option<&T>` instead of `&Option<T>`
   --> filetreelist/src/filetreeitems.rs:276:2
    |
276 | /     fn update_visibility(
277 | |         &mut self,
278 | |         prefix: &Option<PathBuf>,
    | |                 ---------------- help: change this to: `Option<&PathBuf>`
279 | |         start_idx: usize,
...   |
325 | |         }
326 | |     }
    | |_____^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ref_option
note: the lint level is defined here
   --> filetreelist/src/lib.rs:10:53
    |
10  | #![deny(clippy::all, clippy::perf, clippy::nursery, clippy::pedantic)]
    |                                                     ^^^^^^^^^^^^^^^^
    = note: `#[deny(clippy::ref_option)]` implied by `#[deny(clippy::pedantic)]`

error: could not compile `filetreelist` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: empty line after doc comment
  --> asyncgit/src/sync/config.rs:73:1
   |
73 | / /// get string from config
74 | |
   | |_
75 | / pub fn get_config_string(
76 | |     repo_path: &RepoPath,
77 | |     key: &str,
78 | | ) -> Result<Option<String>> {
   | |___________________________- the comment documents this function
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#empty_line_after_doc_comments
note: the lint level is defined here
  --> asyncgit/src/lib.rs:11:9
   |
11 | #![deny(clippy::all, clippy::perf, clippy::nursery, clippy::pedantic)]
   |         ^^^^^^^^^^^
   = note: `#[deny(clippy::empty_line_after_doc_comments)]` implied by `#[deny(clippy::all)]`
   = help: if the empty line is unintentional remove it

error: the following explicit lifetimes could be elided: 'a
  --> asyncgit/src/sync/logwalker.rs:13:6
   |
13 | impl<'a> Eq for TimeOrderedCommit<'a> {}
   |      ^^                           ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
   = note: `#[deny(clippy::needless_lifetimes)]` implied by `#[deny(clippy::all)]`
help: elide the lifetimes
   |
13 - impl<'a> Eq for TimeOrderedCommit<'a> {}
13 + impl Eq for TimeOrderedCommit<'_> {}
   |

error: the following explicit lifetimes could be elided: 'a
  --> asyncgit/src/sync/logwalker.rs:15:6
   |
15 | impl<'a> PartialEq for TimeOrderedCommit<'a> {
   |      ^^                                  ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
   |
15 - impl<'a> PartialEq for TimeOrderedCommit<'a> {
15 + impl PartialEq for TimeOrderedCommit<'_> {
   |

error: the following explicit lifetimes could be elided: 'a
  --> asyncgit/src/sync/logwalker.rs:21:6
   |
21 | impl<'a> PartialOrd for TimeOrderedCommit<'a> {
   |      ^^                                   ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
   |
21 - impl<'a> PartialOrd for TimeOrderedCommit<'a> {
21 + impl PartialOrd for TimeOrderedCommit<'_> {
   |

error: the following explicit lifetimes could be elided: 'a
  --> asyncgit/src/sync/logwalker.rs:27:6
   |
27 | impl<'a> Ord for TimeOrderedCommit<'a> {
   |      ^^                            ^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
   |
27 - impl<'a> Ord for TimeOrderedCommit<'a> {
27 + impl Ord for TimeOrderedCommit<'_> {
   |

error: could not compile `asyncgit` (lib) due to 5 previous errors
make: *** [Makefile:92: clippy] Błąd 101

lists errors unrelated to changes this pull request introduced

[naseschwarz]

Hey @Esgariot

I've been looking at this and am having somewhat of a hard time - not so much with your patch, but how the other order could ever have made any sense:

• The dotted range A..B is equivalent to ^A B, i.e. include B, hide A. A is always after B if the history is linear and If A is after B, this set is always empty for a linear history.
• That being said, range notation also doesn't work for nonlinear history (Copying multiple hashes yields incorrect dotted range on nonlinear histories and consecutive selections #2576), which is why I suggest to remove it altogether via Disable dotted range commit yanking #2577 for the time being.

Any insight from your side is appreciated.

Still, I see the reasonableness of reversing the order anyhow, even if we only copy individual commit hashes, and want to help your patch along. If you're still interested, would you please rebase onto or merge master in order to resolve conflicts, then rerun make check and fix the remaining issue in order to get this to a point where make check passes.

[naseschwarz]

This is not entirely correct - right now (or in case #2577 gets merged), ordering is reversed for all lists of commits, even if they're lists of individual hashes.

[Esgariot]

Thanks for the input, @naseschwarz!
I'll try to update my pull request in the next couple of days. I'm not yet sure how your #2577 change affects my pull request, or if I have made assumptions about the range received from marked that would invalidate the whole PR.

It feels like the good outcome would be to only return .. range notation in valid cases (maybe by following parent commits, if they're accessible, or checking parent count) so that there is a case that mimics git log's output that then can be cherry-picked (as described in the linked issue).

[naseschwarz]

Thanks for your response. Glad you're still interested in this. :)

It feels like the good outcome would be to only return .. range notation in valid cases (maybe by following parent commits, if they're accessible, or checking parent count) so that there is a case that mimics git log's output that then can be cherry-picked (as described in the linked issue).

Yeah. If you're interested in addressing this, I suggest the correct way to check whether A^..B is a valid representation for some set of selected commits C is to do a Revwalk with push(B) and hide(A's first parent) and check whether this revwalk walks all and only the commits in C.

Writing this, I notice that this seems to be another issue with the logic that's currently implemented - a second parent of A would also break the current approach. Doing a revwalk to cross-check would also detect this case, though, so my suggestion would be safe.

However, this means that this revwalk may be very large and should probably be moved to asyncgit.

[Esgariot]

Hey @naseschwarz ,

I've done my first iteration of changes for this PR, using Revwalk as suggested. Feel free to take a look if you’re interested! Ran the the code against some repos and it produces the ranges that look like they could be correct 🤞

Review-wise, I'd like to ask about couple of things:

1. revwalk.rs constructs git2::Repository ad-hoc - other git apis in asyncgit do that, so it's probably fine here too,
2. revwalk accepts commit bounds and closure that exposes the revwalk iterator.
   • pros:
     • not exposing Revwalk
     • not collecting iterator
   • cons:
     • probably needlessly overcomplicated

Still on the TODO list:

• tests
• untangle marked vs marked.rev() vs Sort::REVERSED
• adjust the changelog
• resolve conflicts

[Esgariot]

I'm having trouble testing my is_topo_consecutive - my code that is outside of asyncgit crate needs repo_init to construct repository. repo_init is behind #[cfg(test)], so I can't import it cross crate.

I find it hard to believe that my tests would be the first use case of needing to import the test utils cross-crate, so I think I'm doing something wrong achitecture-wise.

To test concat_selected_commit_ids, I can either:

• wrap Repository in some newtype for which i define a trait, so I can mock it in test
• copy-paste test utils to components crate
• change CommitList so it receives a revwalk or is_topo_consecutive closure, thanks to which I could provide my alternative implementation just for tests,
• remove the CommitList tests
• modify Cargo.toml such that test utils are a dev dependency and can be sourced by my commitlist.rs's tests
• move selection logic to asyncgit crate and factor out the "UI-like" stuff from "checking topo order for subset of date sorted commits-like" stuff

none of the workarounds above look satisfying to me :/

[extrawurst]

I fail to even find the mentioned function is_topo_consecutive in the diff of this PR

[Esgariot]

Sorry about that. I'm mid rebase and didn't include the function extracted from this diff range
https://github.com/gitui-org/gitui/pull/2441/files#diff-70d86fadb5c35cfef60d8d53c6b37ad6aa47ff93da839edaf7fa66665c6f0c15R148-R168

	fn is_topo_consecutive<'a>(
		&self,
		latest: &(usize, CommitId),
		earliest: &(usize, CommitId),
		marked_rev: impl Iterator<Item = &'a (usize, CommitId)>,
	) -> Result<bool> {
		Ok(revwalk(
			&self.repo.borrow(),
			Bound::Excluded(&earliest.1),
			Bound::Included(&latest.1),
			Sort::TOPOLOGICAL | Sort::REVERSE,
			|revwalk| {
				revwalk.zip(marked_rev).try_fold(
					true,
					|acc, (r, m)| {
						let revwalked = CommitId::new(r?);
						let marked = m.1;
						Ok(acc && (revwalked == marked))
					},
				)
			},
		)?)
	}

After milling over it since yesterday I think what I should do is:

separate the the "producing range or enumerated commits" from "preparing formatted string to yank", meaning splitting UI part from logic part.
By that I mean to create some kind of "range" helper in asyncgit such that commitlist.rs doesn't consume revwalk directly.

//revwalk.rs or range_translator.rs or something else

pub enum CommitRange {
  Disjoint([<list of commitIds>]),
  Consecutive(start: CommitId, end: CommitId),
  // (maybe a ctor for multiple consecutive ranges)
}

pub fn translate_into_range(input: &[CommitId]) -> CommitRange {
// calls revwalk here
}

This way I can write tests for commit ranges in asyncgit, and use translate_into_range in commitlist.rs.

[extrawurst]

This way I can write tests for commit ranges in asyncgit, and use translate_into_range in commitlist.rs.

that sounds like a solid plan!