fix: Ensure topological continuity when yanking commit range as `<oldest>^..<newest>`

[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:

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

[Esgariot]

output from

 make check

cargo fmt -- --check
cargo clippy --workspace --all-features
cargo test --workspace

running 165 tests
test progress::tests::test_progress_rounding ... ok
test progress::tests::test_progress_zero_all ... ok
test progress::tests::test_progress_zero_total ... ok
test sync::blame::tests::test_blame_windows_path_dividers ... ok
test revlog::tests::test_smoke_in_subdir ... ok
test sync::blame::tests::test_blame ... ok
test asyncjob::test::test_cancel ... ok
test revlog::tests::test_env_variables ... ok
test sync::branch::rename::test::test_rename_branch ... ok
test sync::branch::test_delete_branch::test_delete_branch ... ok
test asyncjob::test::test_overwrite ... ok
test sync::branch::merge_commit::test::test_merge_normal ... ok
test sync::branch::merge_commit::test::test_merge_normal_non_ff ... ok
test sync::branch::tests_branch_name::test_empty_repo ... ok
test sync::branch::tests_branch_compare::test_smoke ... ok
test sync::branch::merge_rebase::test::test_merge_conflict ... ok
test sync::branch::tests_branch_name::test_smoke ... ok
test sync::branch::tests_branches::test_branch_no_upstream_merge_config ... ok
test sync::branch::merge_ff::test::test_merge_fastforward ... ok
test sync::branch::tests_branches::test_branch_remote_no_branch ... ok
test sync::branch::tests_branches::test_branch_remote_no_upstream ... ok
test sync::branch::test_remote_branches::test_checkout_remote_branch ... ok
test sync::branch::tests_branches::test_branch_with_upstream_merge_config ... ok
test sync::branch::merge_rebase::test::test_merge_normal ... ok
test sync::branch::merge_rebase::test::test_merge_multiple ... ok
test sync::branch::tests_branches::test_multiple ... ok
test sync::branch::tests_branches::test_smoke ... ok
test sync::branch::test_remote_branches::test_has_tracking ... ok
test sync::branch::test_remote_branches::test_remote_branches ... ok
test sync::branch::test_remote_branches::test_checkout_remote_branch_hirachical ... ok
test sync::branch::tests_checkout::test_smoke ... ok
test sync::branch::tests_checkout::test_branch_with_slash_in_name ... ok
test sync::branch::tests_create_branch::test_smoke ... ok
test sync::branch::tests_checkout::test_staged_new_file ... ok
test sync::branch::tests_checkout::test_multiple ... ok
test sync::commit::tests::test_empty_comment_char ... ok
test sync::branch::tests_checkout_commit::test_smoke ... ok
test sync::commit_details::tests::test_commit_message_combine ... ok
test sync::commit::tests::test_commit_in_empty_repo ... ok
test sync::commit_details::tests::test_msg_linefeeds ... ok
test sync::commit::tests::test_amend ... ok
test sync::commit::tests::test_empty_email ... ok
test sync::commit::tests::test_commit ... ok
test sync::commit::tests::test_with_comment_char ... ok
test sync::commit::tests::test_tag ... ok
test sync::commit::tests::test_empty_name ... ok
test sync::commit::tests::test_tag_with_message ... ok
test sync::commit_details::tests::test_msg_invalid_utf8 ... ok
test sync::cred::tests::test_credential_complete ... ok
test sync::cred::tests::test_credential_not_complete ... ok
test sync::commit_files::tests::test_smoke ... ok
test sync::commits_info::tests::test_invalid_utf8 ... ok
test sync::commits_info::tests::test_get_commit_from_revision ... ok
test sync::commit_files::tests::test_stashed_untracked ... ok
test sync::cred::tests::test_extract_nothing_from_url ... ok
test sync::config::tests::test_get_config ... ok
test sync::cred::tests::test_extract_username_from_url ... ok
test sync::commits_info::tests::test_log_first_msg_line ... ok
test sync::cred::tests::test_extract_username_password_from_url ... ok
test sync::commits_info::tests::test_log ... ok
test sync::cred::tests::test_dont_need_username_password_if_pushurl_ssh ... ok
test sync::commit_files::tests::test_stashed_untracked_and_modified ... ok
test sync::diff::tests::test_binary_diff_delta_size_untracked ... ok
test sync::cred::tests::test_dont_need_username_password_if_ssh ... ok
test sync::diff::tests::test_diff_delta_size ... ok
test sync::diff::tests::test_diff_newfile_in_sub_dir_current_dir ... ok
test sync::diff::tests::test_diff_delta_size_commit ... ok
test sync::diff::tests::test_empty_repo ... ok
test sync::cred::tests::test_error_if_no_remote_when_trying_to_extract_username_password - should panic ... ok
test sync::diff::tests::test_untracked_subfolder ... ok
test sync::diff::tests::test_hunks ... ok
test sync::cred::tests::test_error_if_no_remote_when_trying_to_retrieve_if_need_username_password - should panic ... ok
test sync::hunks::tests::reset_untracked_file_which_will_not_find_hunk ... ok
test sync::ignore::tests::test_append ... ok
test sync::ignore::tests::test_append_no_newline_at_end ... ok
test sync::cred::tests::test_extract_username_from_repo ... ok
test sync::ignore::tests::test_ignore_ignore ... ok
test sync::ignore::tests::test_empty ... ok
test sync::cred::tests::test_extract_username_password_from_repo ... ok
test sync::logwalker::tests::test_limit ... ok
test sync::logwalker::tests::test_logwalker ... ok
test sync::cred::tests::test_need_username_password_if_https ... ok
test sync::logwalker::tests::test_logwalker_with_filter_search ... ok
test sync::logwalker::tests::test_logwalker_with_filter ... ok
test sync::logwalker::tests::test_logwalker_without_filter ... ok
test sync::hooks::tests::test_hooks_commit_msg_reject_in_subfolder ... ok
test sync::merge::tests::test_smoke ... ok
test sync::branch::tests_branches::test_remotes_of_branches ... ok
test sync::hooks::tests::test_hooks_commit_msg_reject_in_hooks_folder_githooks_moved_absolute ... ok
test sync::rebase::test_conflict_free_rebase::test_conflict ... ok
test sync::rebase::test_conflict_free_rebase::test_smoke ... ok
test sync::hooks::tests::test_pre_commit_workdir ... ok
test sync::rebase::test_rebase::test_conflicted_abort ... ok
test sync::remotes::push::tests::test_delete_remote_branch ... ok
test sync::remotes::push::tests::test_force_push ... ok
test sync::hooks::tests::test_post_commit_hook_reject_in_subfolder ... ok
test sync::remotes::push::tests::test_force_push_rewrites_history ... ok
test sync::remotes::tags::tests::test_get_remote_tags ... ok
test sync::remotes::tests::test_default_remote ... ok
test sync::remotes::tags::tests::test_tags_missing_remote ... ok
test sync::remotes::tags::tests::test_push_pull_tags ... ok
test sync::remotes::tags::tests::test_tags_delete_remote ... ok
test sync::remotes::tags::tests::test_tags_fetch ... ok
test sync::remotes::tests::test_default_remote_for_fetch ... ok
test sync::remotes::tags::tests::test_tags_fetch_all ... ok
test sync::remotes::tests::test_default_remote_for_push ... ok
test sync::reset::tests::test_reset_folder ... ok
test sync::remotes::tests::test_smoke ... ok
test sync::reset::tests::unstage_in_empty_repo ... ok
test sync::remotes::tests::test_default_remote_inconclusive ... ok
test sync::remotes::tests::test_default_remote_out_of_order ... ok
test sync::reset::tests::test_reset_untracked_in_subdir ... ok
test sync::revwalk::tests_is_continuous::test_linear_commits_are_continuous ... ok
test sync::reset::tests::test_reset_untracked_in_subdir_with_cwd_in_subdir ... ok
test sync::sign::tests::test_gpg_program_configs ... ok
test sync::sign::tests::test_invalid_signing_format ... ok
test sync::reset::tests::test_reset_untracked_subdir ... ok
test sync::sign::tests::test_program_and_signing_key_defaults ... ok
test sync::revwalk::tests_is_continuous::test_merge_commits_break_continuity ... ok
test sync::reset::tests::test_reset_only_unstaged ... ok
test sync::sign::tests::test_ssh_program_configs ... ok
test sync::revwalk::tests_is_continuous::test_non_continuous_commits ... ok
test sync::sign::tests::test_user_signingkey ... ok
test sync::reset::tests::test_reset_untracked_in_subdir_and_index ... ok
test sync::reword::tests::test_reword ... ok
test sync::staging::discard_tracked::test::test_discard3 ... ok
test sync::staging::discard_tracked::test::test_discard2 ... ok
test sync::staging::discard_tracked::test::test_discard4 ... ok
test sync::staging::discard_tracked::test::test_discard ... ok
test sync::staging::discard_tracked::test::test_discard5 ... ok
test sync::staging::discard_tracked::test::test_discard_deletions_filestart_breaking_with_zero_context ... ok
test sync::staging::discard_tracked::test::test_discard_if_first_selected_line_is_not_in_any_hunk ... ok
test sync::staging::stage_tracked::test::test_panic_stage_no_newline ... ok
test sync::staging::stage_tracked::test::test_stage ... ok
test sync::stash::tests::test_smoke ... ok
test sync::stash::tests::test_stash_nothing_untracked ... ok
test sync::stash::tests::test_stash_apply_conflict ... ok
test sync::staging::stage_tracked::test::test_unstage ... ok
test sync::stash::tests::test_stash_pop_conflict ... ok
test sync::stash::tests::test_stash_apply_conflict2 ... ok
test sync::tree::tests::test_path_cmp ... ok
test sync::tree::tests::test_path_file_cmp ... ok
test sync::stash::tests::test_stash_apply_creating_conflict ... ok
test sync::tree::tests::test_sorting ... ok
test sync::tree::tests::test_sorting_folders ... ok
test sync::tree::tests::test_sorting_folders2 ... ok
test sync::stash::tests::test_stash_pop_conflict_after_commit ... ok
test sync::stash::tests::test_stash_pop_no_conflict ... ok
test sync::stash::tests::test_stashes ... ok
test sync::tags::tests::test_smoke ... ok
test sync::stash::tests::test_stashing ... ok
test sync::utils::tests::test_head_empty ... ok
test sync::tags::tests::test_multitags ... ok
test sync::utils::tests::test_head ... ok
test sync::utils::tests::test_stage_add_smoke ... ok
test sync::stash::tests::test_stash_without_second_parent ... ok
test sync::utils::tests::test_not_staging_untracked_folder ... ok
test sync::tree::tests::test_smoke ... ok
test sync::utils::tests::test_staging_folder ... ok
test sync::utils::tests::test_staging_one_file ... ok
test sync::utils::tests::test_undo_commit_empty_repo ... ok
test sync::utils::tests::test_staging_deleted_file ... ok
test sync::utils::tests::test_undo_commit ... ok
test sync::utils::tests::test_staging_sub_git_folder ... ok
test sync::submodules::tests::test_smoke ... ok

test result: ok. 165 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 3.15s


running 31 tests
test filetree::test::test_selection_left_parent ... ok
test filetree::test::test_selection ... ok
test filetree::test::test_selection_page_updown ... ok
test filetree::test::test_selection_top ... ok
test filetree::test::test_selection_skips_collapsed ... ok
test filetree::test::test_selection_left_collapse ... ok
test filetree::test::test_selection_right_expand ... ok
test filetree::test::test_visible_selection ... ok
test filetreeitems::test_merging::test_merge_nothing ... ok
test filetreeitems::test_merging::test_merge_indent ... ok
test filetreeitems::test_merging::test_merge_simple ... ok
test filetreeitems::test_merging::test_merge_simple2 ... ok
test filetreeitems::test_merging::test_merge_single_paths ... ok
test filetreeitems::tests::test_collapse ... ok
test filetreeitems::tests::test_collapse_too_much ... ok
test filetreeitems::tests::test_expand ... ok
test filetreeitems::tests::test_expand_bug ... ok
test filetreeitems::tests::test_expand_with_collapsed_sub_parts ... ok
test filetreeitems::tests::test_folder ... ok
test filetreeitems::tests::test_folder_dup ... ok
test filetreeitems::tests::test_indent ... ok
test filetreeitems::tests::test_indent_folder_file_name ... ok
test filetreeitems::tests::test_iterate_collapsed ... ok
test filetreeitems::tests::test_push_path ... ok
test filetreeitems::tests::test_push_path2 ... ok
test filetreeitems::tests::test_show_element ... ok
test filetreeitems::tests::test_show_element_downward_parent ... ok
test filetreeitems::tests::test_show_element_expand_visible_parent ... ok
test filetreeitems::tests::test_show_element_later_elements ... ok
test filetreeitems::tests::test_simple ... ok
test item::tests::test_smoke ... ok

test result: ok. 31 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


running 22 tests
test hookspath::test::test_hookspath_absolute ... ok
test hookspath::test::test_hookspath_relative ... ok
test tests::test_hook_pwd_in_bare_without_workdir ... ok
test tests::test_hook_pwd ... ok
test tests::test_no_hook_found ... ok
test tests::test_hook_with_missing_shebang ... ok
test tests::test_hooks_commit_msg_ok ... ok
test tests::test_hooks_commit_msg_reject ... ok
test tests::test_env_containing_path ... ok
test tests::test_commit_msg_no_block_but_alter ... ok
test tests::test_hooks_commit_msg_with_shell_command_ok ... ok
test tests::test_hooks_prep_commit_msg_success ... ok
test tests::test_hooks_prep_commit_msg_reject ... ok
test tests::test_other_path ... ok
test tests::test_pre_commit_fail_bare ... ok
test tests::test_other_path_precendence ... ok
test tests::test_pre_commit_fail_hookspath ... ok
test tests::test_pre_commit_fail_py ... ok
test tests::test_pre_commit_fail_sh ... ok
test tests::test_pre_commit_py ... FAILED
test tests::test_pre_commit_sh ... ok
test tests::test_smoke ... ok

failures:

---- tests::test_pre_commit_py stdout ----
[90m[[0m2025-06-20T19:37:30Z [36mTRACE[0m git2_hooks::hookspath[90m][0m run hook '"/private/var/folders/yv/_lhj0sl11ts4ss0lt60ywc500000gn/T/.tmpvBzjCP/.git/hooks/pre-commit"' in '"/private/var/folders/yv/_lhj0sl11ts4ss0lt60ywc500000gn/T/.tmpvBzjCP/"'

thread 'tests::test_pre_commit_py' panicked at git2-hooks/src/lib.rs:510:9:
RunNotSuccessful { code: Some(127), stdout: "", stderr: "env: python: No such file or directory\n", hook: "/private/var/folders/yv/_lhj0sl11ts4ss0lt60ywc500000gn/T/.tmpvBzjCP/.git/hooks/pre-commit" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    tests::test_pre_commit_py

test result: FAILED. 21 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.32s

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 (#2576), which is why I suggest to remove it altogether via #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.

[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!

[Esgariot]

@naseschwarz Would you like to take a look again? The revwalk turned out to be the simpler part, instead I've struggled with figuring out where to place the code and where to move commit range tests..