Unambiguous uncommitted files CLI ID and cleanup #11463

jonathantanmy2 · 2025-12-05T04:31:39Z

Next one in the series. The next step for me is either

look at but-hunk-assignment, but it itself uses but-hunk-dependency, so investigation on that might need to wait until we figure out how to move forward with Fix line_shift bug in but-hunk-dependency #11368 , or
treat the output of the hunk assignments calculation as a black box and refactor IdMap to only require RefInfo and the hunk assignments

My inclination is to do the latter. Something needs to know how to generate the RefInfo and the hunk assignments it needs, though - unfortunately I think that will have to be Context.

As for this commit chain itself, note that there is an error message change in "id: unambiguous uncommitted file CLI IDs", as described in its commit message. Take a look and see if that's reasonable. Also, how do you want it tested? I saw crates/but/src/legacy/id/tests.rs but I don't know how to add uncommitted files (both in the 00 area and in a stack) without using but commands (which would require an integration test). Or, we can wait until I've refactored IdMap to take in the hunk assignments directly, and only test this part then.

vercel · 2025-12-05T04:31:44Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Preview	Comments	Updated (UTC)
gitbutler-web	Ignored	Preview		Dec 5, 2025 9:58am

Copilot

Pull request overview

This PR refactors the CLI ID system by renaming IdDb to IdMap and implementing unambiguous uncommitted file CLI IDs. The key change is that uncommitted files now have their CLI IDs pre-computed and stored in the IdMap rather than being dynamically generated from file paths and assignments. This makes the IDs deterministic and unambiguous.

Renamed IdDb to IdMap throughout the codebase
Added id: String field to CliId::UncommittedFile and changed path from String to BString
Removed file_from_assignment(), matches(), and matches_prefix() methods from CliId
Changed parse_str() to no longer require a mutable Context parameter

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
crates/but/src/legacy/id/mod.rs	Core refactoring: renamed `IdDb` to `IdMap`, added `UncommittedFile` struct for pre-computed IDs, modified `parse_str()` to use exact file ID lookups, added `uncommitted_file()` method
crates/but/src/legacy/id/tests.rs	Updated test to use new `IdMap` name and simplified API without `ctx` parameter
crates/but/tests/but/id.rs	Updated TODO comment from `IdDb` to `IdMap`
crates/but/tests/but/command/status.rs	Updated expected CLI ID from "xe" to "g0" reflecting new ID generation algorithm
crates/but/tests/but/journey.rs	Improved error message consistency for missing default target
crates/but/src/command/legacy/status/mod.rs	Updated to use `IdMap`, removed `all_files()` and `all_branches()` helper functions, pass `id_map` to assignment constructors
crates/but/src/command/legacy/status/json.rs	Updated all references from `id_db` to `id_map`
crates/but/src/command/legacy/status/assignment.rs	Changed `From` impl to `from_assignment()` method that takes `IdMap` parameter, use `uncommitted_file()` method for ID generation
crates/but/src/command/legacy/rub/mod.rs	Updated to use `IdMap` and `BString` for paths, modified `parse_list()` to not require `ctx`
crates/but/src/command/legacy/rub/assign.rs	Changed `path` parameter type from `&str` to `&BStr`
crates/but/src/command/legacy/rub/amend.rs	Changed `path` parameter type from `&str` to `&BStr`
crates/but/src/command/legacy/push.rs	Updated to use `IdMap`
crates/but/src/command/legacy/mark.rs	Updated to use `IdMap`
crates/but/src/command/legacy/forge/review.rs	Updated to use `IdMap`
crates/but/src/command/legacy/describe.rs	Updated to use `IdMap`
crates/but/src/command/legacy/commit.rs	Updated to use `IdMap`, simplified function signatures
crates/but/src/command/legacy/branch/mod.rs	Updated to use `IdMap`
crates/but/src/command/legacy/absorb.rs	Updated to use `IdMap` and `BStr` type
crates/gitbutler-branch-actions/src/base.rs	Renamed variable (appears to be unrelated to main PR focus)

crates/gitbutler-branch-actions/src/base.rs

crates/but/src/legacy/id/mod.rs

In preparation for a future commit which needs a mut Context to construct the IdDb, take a mut Context instead of a non-mut one. While we're at it, since many lines will need to be changed anyway, rename IdDb to a more natural name, IdMap.

They also do not collide with committed file IDs or branch IDs. Sample output: ``` ╭┄000 [Unassigned Changes] ┊ g0 A d ┊ ┊╭┄ux [b00] (no commits) ├╯ ┊ ┊╭┄wo [work] ┊│ h0 A e ┊● 89f03ba test commit ┊│ j0 A a ┊│ k0 A b ┊│ l0 A c ├╯ ┊ ┊╭┄i0 [i0] (no commits) ├╯ ┊ ┴ dba337d (common base) [origin/main] 2025-11-11 Merge pull request #2254 from ralphmodales/credent ``` I had to do some contortions in `context_info()` to ensure that `repo`, and hence the indirect borrowing of `ctx`, was dropped before `but_hunk_assignment::assignments_with_fallback()`, which needs to borrow `ctx` mutably, was called. This will be fixed once `context_info()` does not take `ctx`, but only what it needs (most likely, just the `RefInfo` and the hunk assignments). There is also a change to an error message in a test (`crates/but/tests/but/journey.rs`), likely due to uncommitted files being read at `IdMap` construction time, instead of when it is used to parse a string. TODO need to write tests

In `parse_str`, despite the comment near `self.find_branches_by_name`, `self.find_branches_by_name` actually handles partial branch name lookup already, which means that the other branch searches are redundant. Remove them, which also allows a lot of other code cleanup. In particular, `Context` is no longer needed when parsing strings - only when generating the `IdMap`. This makes it clear that the only inputs are the `RefInfo` and the hunk assignments.

jonathantanmy2 · 2025-12-05T06:43:45Z

Updated following Copilot's review and also rebased on latest master.

Byron

Thanks again, and I agree that making IdMap solid is something that can be done while but-hunk-dependency is pending.

I love where this is going, and thought it was interesting that the &mut Context requirement immediately signals database access (i.e. assignments) just because all DB access needs mutability right now. It's a major issue, and I want to address it till Monday, to let read-only remain read-only.

Regarding testing, there is but_hunk_assignment::assign(ctx, reqs, None)? to make assignments, and unit-tests can be made to assign after the initial fixture was created. It's writable, after all, and it should work.

Let me try to add an example test before merging.

Byron · 2025-12-05T06:23:58Z

crates/but/src/legacy/id/mod.rs

+#[derive(PartialEq, Eq, PartialOrd, Ord)]
+struct UncommittedFile {
+    assignment_path: (Option<StackId>, BString),
+    id: String,
+}
+impl Borrow<(Option<StackId>, BString)> for UncommittedFile {
+    fn borrow(&self) -> &(Option<StackId>, BString) {
+        &self.assignment_path
+    }
+}
+impl Borrow<str> for UncommittedFile {
+    fn borrow(&self) -> &str {
+        &self.id
+    }
+}


I really like these Borrow implementations by the way and I wasn't aware of how this can be used to use sets a little like maps!
It's great.

CC @krlvi

Haha I think it's a bit of a hack (I think that there should be a new specific trait PartialKey for this purpose, instead of reusing Borrow) but it works.

crates/gitbutler-branch-actions/src/base.rs

crates/but/src/legacy/id/mod.rs

Byron · 2025-12-05T06:39:37Z

crates/but/tests/but/journey.rs

+Error: errors.projects.default_target.not_found
+
+Caused by:
+    there is no default target


You probably mentioned it, but this feels like a regression.
However, it was also coincidentally 'good' before, and I think this can be ignored until single-branch mode is properly supported.
Now the ID map seems to trigger another related error early, somewhere in old code probably.

- make clear that `ContextInfo` is an intermediary, a utility by immediately destructuring it. - address copilot comments

Copilot

Pull request overview

Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.

crates/but/src/legacy/id/mod.rs

crates/but/tests/but/utils.rs

This also makes the Sandbox more generally available, in support of a more generalised testing style of somewhat higher-level functions that use the context.

Copilot

Pull request overview

Copilot reviewed 41 out of 42 changed files in this pull request and generated 3 comments.

crates/but-meta/src/legacy.rs

crates/but/src/legacy/id/mod.rs

Copilot · 2025-12-05T09:52:18Z

crates/but-testsupport/Cargo.toml

 publish = false
 rust-version = "1.89"

 [lib]


Setting test = false disables unit tests for this crate. If this is intentional to avoid circular dependencies or because all tests are integration tests, consider adding a comment explaining why. Otherwise, this prevents running tests with cargo test for this crate.

Suggested change

[lib]

[lib]

# Unit tests are disabled for this crate to avoid circular dependencies and because all tests are integration tests.

Fair - it can be confusing to write a unit test and not see it running, let's make this more prominent.

Byron · 2025-12-05T09:54:09Z

That was a quite a refactor, which ultimately led to a generalised Sandbox implementation which should be useful even in plumbing crates. It's all about isolation and the kinds of components it can produce, and in but-api mode, opted in by feature toggle, it will also isolate application data which right now is really only needed for projects.json (and of course, for the clients under test).

Right now, there are no non-but-api users of the Sandbox, but I think it's good to have the feature toggle to be forced to keep the 'plumbing-vs-api' distinction alive in code as well, and clearly separated.

github-actions bot added the rust Pull requests that update Rust code label Dec 5, 2025

Byron requested a review from Copilot December 5, 2025 06:21

Copilot started reviewing on behalf of Byron December 5, 2025 06:21 View session

Copilot finished reviewing on behalf of Byron December 5, 2025 06:24

Copilot AI reviewed Dec 5, 2025

View reviewed changes

crates/gitbutler-branch-actions/src/base.rs Outdated Show resolved Hide resolved

crates/but/src/legacy/id/mod.rs Show resolved Hide resolved

jonathantanmy2 force-pushed the jt/ucfid branch from f6584bd to 64db39d Compare December 5, 2025 06:41

jonathantanmy2 added 3 commits December 4, 2025 22:42

id: rename IdDb to IdMap, needs mut Context

97b163a

In preparation for a future commit which needs a mut Context to construct the IdDb, take a mut Context instead of a non-mut one. While we're at it, since many lines will need to be changed anyway, rename IdDb to a more natural name, IdMap.

jonathantanmy2 force-pushed the jt/ucfid branch from 64db39d to 56f88ef Compare December 5, 2025 06:42

Byron approved these changes Dec 5, 2025

View reviewed changes

refactor

4a29a19

- make clear that `ContextInfo` is an intermediary, a utility by immediately destructuring it. - address copilot comments

Byron force-pushed the jt/ucfid branch from 68eb9e2 to 4a29a19 Compare December 5, 2025 07:24

Byron requested review from Byron and Copilot December 5, 2025 09:12

Copilot started reviewing on behalf of Byron December 5, 2025 09:13 View session

Copilot finished reviewing on behalf of Byron December 5, 2025 09:15

Copilot AI reviewed Dec 5, 2025

View reviewed changes

crates/but/src/legacy/id/mod.rs Outdated Show resolved Hide resolved

crates/but/tests/but/utils.rs Outdated Show resolved Hide resolved

Byron force-pushed the jt/ucfid branch from 765553d to 33cfbb9 Compare December 5, 2025 09:31

IdMap unit'y test with assignments (and add but-testsupport::Sandbox)

bf8b9a0

This also makes the Sandbox more generally available, in support of a more generalised testing style of somewhat higher-level functions that use the context.

Byron force-pushed the jt/ucfid branch from 33cfbb9 to bf8b9a0 Compare December 5, 2025 09:46

Byron requested a review from Copilot December 5, 2025 09:46

Copilot started reviewing on behalf of Byron December 5, 2025 09:47 View session

Copilot finished reviewing on behalf of Byron December 5, 2025 09:49

Copilot AI reviewed Dec 5, 2025

View reviewed changes

Address Copilot review notes

61ffd17

Byron enabled auto-merge December 5, 2025 09:58

Byron merged commit 96a182f into master Dec 5, 2025
22 checks passed

Byron deleted the jt/ucfid branch December 5, 2025 10:17

	[lib]
	[lib]
	# Unit tests are disabled for this crate to avoid circular dependencies and because all tests are integration tests.

Unambiguous uncommitted files CLI ID and cleanup #11463

Unambiguous uncommitted files CLI ID and cleanup #11463

Conversation

jonathantanmy2 commented Dec 5, 2025

Uh oh!

vercel bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

jonathantanmy2 commented Dec 5, 2025

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Byron Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

jonathantanmy2 Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Byron Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Byron Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Byron commented Dec 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel bot commented Dec 5, 2025 •

edited

Loading