Codex/reset bare path git compat#433
Merged
Merged
Conversation
Signed-off-by: Quanyi Ma <eli@patch.sh>
* test(cloud): isolate restore object tests from cwd Signed-off-by: Quanyi Ma <eli@patch.sh> * fix(reset): preserve separator for dash-prefixed paths Signed-off-by: Quanyi Ma <eli@patch.sh> * test(file): avoid sqlite3 binary in obliterate recovery Signed-off-by: Quanyi Ma <eli@patch.sh> --------- Signed-off-by: Quanyi Ma <eli@patch.sh>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves libra reset Git-compatibility around “bare” pathspec parsing and -- disambiguation, aligning behavior with Git when a token could be interpreted as either a revision or a filename. It also updates tests and documentation to lock in the new contract and improves a crash-recovery test to avoid depending on host sqlite3 binaries.
Changes:
- Teach
resetto treat a bare first positional as a pathspec (targetingHEAD) when it’s a known path and not a resolvable revision, while rejecting revision/path collisions as ambiguous (LBR-CLI-002). - Add argv rewriting in the top-level CLI to preserve the presence of
--forresetvia a hidden internal flag. - Add/adjust integration tests and docs to cover the new disambiguation behavior; remove reliance on external
sqlite3forfile obliterate --recovertests.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/command/reset_test.rs | Adds coverage for bare pathspec unstage, -- disambiguation, and ambiguous revision/path rejection; updates ResetArgs construction for new fields. |
| tests/command/file_obliterate_test.rs | Reworks crash-recovery setup to mutate repo DB via SeaORM instead of shelling out to sqlite3. |
| src/internal/ai/review/launcher.rs | Gates /proc/<pid>/stat parsing helper for Linux + tests. |
| src/command/reset.rs | Implements bare-pathspec Git-like disambiguation, adds an ambiguity error variant, and introduces hidden separator plumbing. |
| src/command/cloud.rs | Serializes + isolates a couple of tests that require an initialized Libra repo context. |
| src/command/cherry_pick.rs | Updates internal reset invocation for the new ResetArgs shape. |
| src/cli.rs | Adds argv rewrite to preserve -- semantics specifically for reset. |
| docs/development/integration/integration-scenarios/cli.restore-reset-diff.md | Updates scenario to use libra reset <path> and documents the new disambiguation rule. |
| docs/development/integration/integration-scenarios/_parameter-tables.md | Updates parameter table entries to reflect reset <path> behavior and ambiguity handling. |
| docs/development/commands/reset.md | Updates internal command development doc synopsis and behavior notes for bare pathspec compatibility. |
| docs/commands/zh-CN/reset.md | Updates user docs for bare pathspec usage, -- disambiguation, and ambiguity error behavior. |
| docs/commands/reset.md | Updates user docs for bare pathspec usage, -- disambiguation, and ambiguity error behavior. |
| COMPATIBILITY.md | Documents the new Git-like bare-pathspec disambiguation behavior for reset. |
Comment on lines
+510
to
+568
| let Some(target_arg) = args.target.as_deref() else { | ||
| return Ok(ResetRequest { target, pathspecs }); | ||
| }; | ||
| let resolves_as_revision = target_resolves_as_revision(target_arg).await?; | ||
| let matches_path = pathspec_matches_known_path(target_arg).await?; | ||
|
|
||
| match (resolves_as_revision, matches_path) { | ||
| (true, true) => Err(ResetError::AmbiguousRevisionPath(target_arg.to_string())), | ||
| (true, false) => Ok(ResetRequest { target, pathspecs }), | ||
| (false, true) => { | ||
| pathspecs.insert(0, target_arg.to_string()); | ||
| Ok(ResetRequest { | ||
| target: DEFAULT_RESET_TARGET.to_string(), | ||
| pathspecs, | ||
| }) | ||
| } | ||
| (false, false) => Ok(ResetRequest { target, pathspecs }), | ||
| } | ||
| } | ||
|
|
||
| async fn target_resolves_as_revision(target: &str) -> Result<bool, ResetError> { | ||
| match resolve_commit(target).await { | ||
| Ok(_) => Ok(true), | ||
| Err(ResetError::InvalidRevision(_)) | Err(ResetError::HeadUnborn) => Ok(false), | ||
| Err(error) => Err(error), | ||
| } | ||
| } | ||
|
|
||
| async fn pathspec_matches_known_path(pathspec: &str) -> Result<bool, ResetError> { | ||
| let absolute = util::workdir_to_absolute(PathBuf::from(pathspec)); | ||
| if absolute.exists() { | ||
| return Ok(true); | ||
| } | ||
| if !util::is_sub_path(&absolute, util::working_dir()) { | ||
| return Ok(false); | ||
| } | ||
|
|
||
| let relative_path = util::workdir_to_current(PathBuf::from(pathspec)); | ||
| let path_str = relative_path | ||
| .to_str() | ||
| .ok_or_else(|| ResetError::InvalidPathspecEncoding(relative_path.display().to_string()))?; | ||
|
|
||
| let index = Index::load(path::index()).map_err(|e| ResetError::IndexLoad(e.to_string()))?; | ||
| if index.get(path_str, 0).is_some() { | ||
| return Ok(true); | ||
| } | ||
|
|
||
| let Some(head_commit_id) = Head::current_commit_result() | ||
| .await | ||
| .map_err(map_reset_head_commit_error)? | ||
| else { | ||
| return Ok(false); | ||
| }; | ||
| let commit: Commit = load_object(&head_commit_id) | ||
| .map_err(|e| object_load_error("commit", head_commit_id.to_string(), e.to_string()))?; | ||
| let tree: Tree = load_object(&commit.tree_id) | ||
| .map_err(|e| object_load_error("tree", commit.tree_id.to_string(), e.to_string()))?; | ||
| find_tree_item(&tree, path_str).map(|item| item.is_some()) | ||
| } |
Comment on lines
+1062
to
+1069
| out.push(format!( | ||
| "--{}", | ||
| command::reset::RESET_PATHSPEC_SEPARATOR_FLAG | ||
| )); | ||
| if !has_target_before_separator && args.get(separator_index + 1).is_some() { | ||
| out.push("HEAD".to_string()); | ||
| } | ||
| out.push("--".to_string()); |
Signed-off-by: Quanyi Ma <eli@patch.sh>
Signed-off-by: Quanyi Ma <eli@patch.sh>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.