Add a revision option to gix clone#2563
Conversation
Co-authored-by: gemini-cli <218195315+gemini-cli@users.noreply.github.com> Signed-off-by: Theo Paris <theo@theoparis.com>
Co-authored-by: gemini-cli <218195315+gemini-cli@users.noreply.github.com> Signed-off-by: Theo Paris <theo@theoparis.com>
Co-authored-by: gemini-cli <218195315+gemini-cli@users.noreply.github.com> Signed-off-by: Theo Paris <theo@theoparis.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f29944f6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| if let Some(rev) = &self.revision { | ||
| let id = repo.rev_parse_single(rev.as_bstr())?; | ||
| repo.edit_reference(gix_ref::transaction::RefEdit { |
There was a problem hiding this comment.
When with_revision() is used together with with_ref_name() (or CLI --revision with --ref) in a non-bare clone, this block detaches HEAD to the revision, but fetch_then_checkout() still passes the original ref_name into PrepareCheckout, and main_worktree_inner() prioritizes ref_name over HEAD. The result is a worktree checked out at the ref tip while HEAD points to a different commit, producing an inconsistent clone state (typically an immediate dirty tree).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds a “revision after clone” capability to the gix clone plumbing command by plumbing a new --revision/--rev option through gitoxide-core into gix::clone::PrepareFetch, and adjusts clone ref resolution to better handle object-id-based selection.
Changes:
- Add
--revision(--rev) option to thegix cloneplumbing CLI and pass it intogitoxide-coreclone execution. - Extend
gix::clone::PrepareFetchwith arevisionfield andwith_revision()builder method, and resolve it post-fetch by updatingHEAD. - Adjust clone custom ref-name resolution to handle object-id refspec matches, plus add a regression test around cloning by object id.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/plumbing/options/mod.rs |
Adds --revision/--rev CLI flag for gix clone. |
src/plumbing/main.rs |
Wires the new revision CLI option into core clone options. |
gitoxide-core/src/repository/clone.rs |
Passes revision into the gix clone builder chain. |
gix/src/clone/mod.rs |
Adds revision field to PrepareFetch and initializes it. |
gix/src/clone/access.rs |
Adds PrepareFetch::with_revision() builder API. |
gix/src/clone/fetch/mod.rs |
Resolves revision via rev_parse_single() and updates HEAD after fetch. |
gix/src/clone/fetch/util.rs |
Extends custom ref matching to handle object-id sources. |
gix/tests/gix/clone.rs |
Adds a test for cloning by object id using with_ref_name(). |
| (None, gix_refspec::match_group::SourceRef::ObjectId(id)) => { | ||
| let target = ref_map | ||
| .mappings | ||
| .iter() | ||
| .find_map(|m| m.remote.as_id().filter(|remote_id| *remote_id == *id)) | ||
| .expect("if it matched, it must be in the mappings"); | ||
| Ok((Some(target), None)) |
There was a problem hiding this comment.
find_custom_refname() will panic when the user-provided refspec is an object id that is not the target of any advertised remote ref (the .expect("if it matched…") will fail). Fetch-by-oid is valid even when no ref points at that oid, so this should return the requested id without searching ref_map.mappings and without panicking (e.g., return an owned id or adjust the function to carry the oid separately from the borrowed ref-map data).
| #[cfg(feature = "revision")] | ||
| if let Some(rev) = &self.revision { | ||
| let id = repo.rev_parse_single(rev.as_bstr())?; | ||
| repo.edit_reference(gix_ref::transaction::RefEdit { | ||
| change: gix_ref::transaction::Change::Update { | ||
| log: gix_ref::transaction::LogChange { | ||
| mode: gix_ref::transaction::RefLog::AndReference, | ||
| force_create_reflog: false, | ||
| message: reflog_message.clone(), | ||
| }, | ||
| expected: gix_ref::transaction::PreviousValue::Any, | ||
| new: gix_ref::Target::Object(id.detach()), | ||
| }, | ||
| name: "HEAD".try_into().expect("valid"), | ||
| deref: false, | ||
| })?; | ||
| } |
There was a problem hiding this comment.
When revision is set, fetch_only() updates HEAD, but fetch_then_checkout() still passes self.ref_name into PrepareCheckout, and main_worktree() prefers ref_name over HEAD. As a result, using both ref_name and revision will ignore revision for non-bare clones. Consider making these options mutually exclusive at the API/CLI level, or ensure revision takes precedence by clearing/ignoring ref_name for checkout when revision is present.
| #[cfg(feature = "revision")] | ||
| if let Some(rev) = &self.revision { | ||
| let id = repo.rev_parse_single(rev.as_bstr())?; | ||
| repo.edit_reference(gix_ref::transaction::RefEdit { | ||
| change: gix_ref::transaction::Change::Update { | ||
| log: gix_ref::transaction::LogChange { | ||
| mode: gix_ref::transaction::RefLog::AndReference, | ||
| force_create_reflog: false, | ||
| message: reflog_message.clone(), | ||
| }, | ||
| expected: gix_ref::transaction::PreviousValue::Any, | ||
| new: gix_ref::Target::Object(id.detach()), | ||
| }, | ||
| name: "HEAD".try_into().expect("valid"), | ||
| deref: false, | ||
| })?; | ||
| } |
There was a problem hiding this comment.
The new revision behavior in fetch_only() isn’t covered by tests (e.g. verifying that with_revision(Some("<spec>")) results in a detached HEAD at the resolved id, and that it interacts correctly with shallow clones and/or ref_name). Adding a dedicated clone test would help prevent regressions in revspec resolution and HEAD updates.
| /// Set the revision to check out after fetching. | ||
| /// | ||
| /// If `None`, the `HEAD` will be used, which is the default. | ||
| pub fn with_revision<T>(mut self, revision: Option<T>) -> Self | ||
| where | ||
| T: Into<BString>, | ||
| { | ||
| self.revision = revision.map(Into::into); | ||
| self |
There was a problem hiding this comment.
with_revision() is always available, but the actual resolution logic is #[cfg(feature = "revision")] in the fetch implementation. In builds with default-features = false where revision is disabled, this setter becomes a silent no-op. Consider gating the method/field behind the same feature, or documenting/returning an error when the feature isn’t enabled to avoid surprising API behavior.
| /// The revision to check out after cloning. | ||
| /// | ||
| /// This is useful if you want to clone a specific commit that is not a branch tip. | ||
| /// It will fetch the default branch and then attempt to check out this revision. |
There was a problem hiding this comment.
The help text says cloning with --revision will “fetch the default branch”, but the underlying clone implementation may fetch more than the default branch (e.g. wildcard refspecs in non-shallow clones). Consider rewording to something like “perform a normal clone/fetch and then attempt to check out this revision” to match actual behavior.
| /// It will fetch the default branch and then attempt to check out this revision. | |
| /// It will perform a normal clone/fetch and then attempt to check out this revision. |
| /// Set the revision to check out after fetching. | ||
| /// | ||
| /// If `None`, the `HEAD` will be used, which is the default. | ||
| pub fn with_revision<T>(mut self, revision: Option<T>) -> Self | ||
| where | ||
| T: Into<BString>, | ||
| { | ||
| self.revision = revision.map(Into::into); | ||
| self | ||
| } | ||
| } |
There was a problem hiding this comment.
The new clone_by_object_id test uses with_ref_name(Some(<hex oid>)), but the API docs for with_ref_name() currently state that passing an object-id-as-hex causes subsequent clone operations to panic. Either update that doc comment to reflect the now-supported behavior (and any remaining limitations), or adjust the implementation so the documented panic can’t occur.
|
I'm setting this to draft until the auto review comments have been addressed, by rejecting or accepting them. I also want to remind everyone that the |
I implemented this with the help of gemini cli. I'm not sure if this is the best approach (since gix still has a
--refflag which might be confusing), but it does seem to work with something like:./target/debug/gix clone --revision 894a8ced1b09ff22ea0b8826fcccfbb73834896f https://github.com/zlib-ng/zlib-ng ~/zlib-ngwhich should result in
Bump Google Benchmark to v1.9.5being the latest commit in git log.