-
-
Notifications
You must be signed in to change notification settings - Fork 354
PrepareCheckout non-head references #1403
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for giving it a shot! I strongly recommend to try and find existing tests to adapt and to test the new functionality with them. This also allows to double-check the outcome. That way, turnaround times will be much shorter. |
Hi so I attempted to add the unit test as requested, though unfortunately I'm not very familiar with the test setup and rather confused with everything. I tried using the generated repositories for the test and I apologize though I'll mention that I did allow maintainers to modify the branch in the case you'd like to do the finish touches. Thanks! git branch -a
a
b
c
d
e
f
g
h
i
j
* main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look like a struggle, and I am happy to finish the PR.
Before that, could you solve the linting failure and maybe setup tooling to auto-format code?
I formatted the files and it CI seems to be ok now (other than the broken test) Appreciate your patience |
abbcef1
to
eba1204
Compare
I have started working on this and plan to force-push a couple of times. It's probably best to not interact with this PR with more than comments until it is merged. |
7c37302
to
9d0e01d
Compare
`--ref` is similar to `--branch`, but was renamed as it also supports tags for example.
Now it's clear why it does what it does with the internal repository, even though admittedly it could also be made so that it can be called multiple times (despite making no sense).
b252b15
to
f36b9bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This certainly looks a tiny bit different than how it started out, and was quite hard even for me as I only had vague memory on the 'prior art' within the codebase. Fortunately, everything turned out to be easy once I rediscovered the facilities that were already available.
This PR adds support for checking non-head references such as commits, tags, branches etc. using
repo.find_reference()
inPrepareCheckout
.While I preferred using
Reference
as a parameter rather than&BStr
, I kept running into lifetime issues related to borrows withself.repo
in PrepareCheckout.At the time of writing this, it seems this solution is incomplete as the checkout does not set the git repo to checkout the reference. Specifically (excuse my lack of git terminology)
git status
is still set to track the HEAD reference it seems. However, the files are correctly cloned.I hope this PR isn't too messy, though feel free to ask for changes to follow the repo's code style. Thanks!
In reference to discussion #1389
Research
How does Git do it?
The ls-refs call of
git clone --branch status <gitoxide-url>
, thus--branch
definitely assures anything with that name (if the protocol supports filtering).With
--single-branch
also specified, we get:So it's the same, but what's different is the list of wants, which now is very short, matching
954bab9f16839002a02ac809c7ca3184e8588261 refs/heads/status
Lastly, without
--branch --single-branch
the ls-refs call looks like this:With a long want list.
How does Gix do it?
By default, it auto-generates the ref-prefix from refspecs, creating an ls-refs call similar to what Git produces.
Conclusion
Tasks
--ref
to thegix
CLIPostponed: single-ref support
Implementing this should be fairly simple, but also might take longer if the parts don't work as anticipated. Let's leave it for another day or to a request.
--single-ref
to thegix
CLI to make this feature more broadly available (and verifiable by hand)