-
Notifications
You must be signed in to change notification settings - Fork 150
Fix: Clone from auto-cache fails when the repository’s remote default branch changes #895
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
base: main
Are you sure you want to change the base?
Conversation
d3048f5 to
6551422
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #895 +/- ##
=======================================
Coverage 85.95% 85.95%
=======================================
Files 11 11
Lines 3453 3447 -6
=======================================
- Hits 2968 2963 -5
+ Misses 485 484 -1
|
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.
Pull request overview
This pull request fixes a critical issue where cloning from auto-cache fails when a repository's default branch is renamed on the remote. The fix ensures the cached repository's HEAD is synchronized with the remote HEAD before updating, preventing checkout failures.
Key Changes
- Adds logic to update the auto-cache's symbolic HEAD reference before syncing with remote to handle default branch changes
- Implements an optimization to skip remote updates when the requested revision is already present in the auto-cache
- Updates documentation to clarify auto-cache behavior regarding on-demand synchronization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/west/app/project.py | Implements HEAD synchronization logic before remote updates and adds optimization to skip unnecessary remote fetches; updates help text and documentation for auto-cache behavior; extends _rev_type function with cwd parameter |
| tests/test_project_caching.py | Adds comprehensive test for auto-cache remote update optimization, including scenarios with offline remotes, new commits, and branch revisions; imports chdir helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6551422 to
e473e19
Compare
|
UPDATE: |
e473e19 to
0e2c9ec
Compare
0e2c9ec to
f86db73
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f86db73 to
486468a
Compare
marc-hb
left a comment
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.
So I personally prefer a solution that fixes the auto-cache, instead of just fixing the clone in the workspace.
Why not fix both?
With newest git version 2.52.0 (as it is used within west CI), the issue is resolved.
What changed in git 2.52 and which part does it fix?
west must stay compatible with a large range of git versions so I'm not sure what you're trying to say here.
Therefore I will change to the simple solution which just fixes the clone.
BTW prefer "workspace" than "clone" because everything is a clone.
src/west/app/project.py
Outdated
| ) | ||
| # Reset the remote's URL to the project's fetch URL. | ||
| project.git(['remote', 'set-url', project.remote_name, project.url]) | ||
| # Make sure a valid revision is checked out |
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 comment is too short, it relates to neither the issue nor to #895 that describes it.
src/west/app/project.py
Outdated
| # Reset the remote's URL to the project's fetch URL. | ||
| project.git(['remote', 'set-url', project.remote_name, project.url]) | ||
| # Make sure a valid revision is checked out | ||
| project.git(['checkout', '--quiet', project.revision]) |
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.
Rather than 2 git checkout ... commands, would this single checkout achieve the same result?
And then --detach is probably redundant.
| project.git(['checkout', '--quiet', project.revision]) | |
| project.git(['checkout', '--quiet', project.revision + "~0"]) |
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.
You are right, it really seems to work without the project.git('checkout --quiet --detach HEAD'). So only this line is enough
project.git(['checkout', '--quiet', project.revision])
With your suggested ~0 it does not work anymore.
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.
Nevertheless the issue occurs in code which seems to have no effect. At least all tests still pass even when returning after project.git(['remote', 'set-url', project.remote_name, project.url]).
Maybe it makes more sense to clean up (throw out this code) instead of applying fixes for obsolete code?
|
I see that the original author was @mbolivar (reference: c50d342). Do you remember the rationale for deleting all branches after the repository is cloned into the workspace from a cache? Also, do you see any potential risks in keeping the local branches? |
7dd6b8a to
229d7ae
Compare
What do you mean "does not work"; how can this fail? I've never seen appending Appending
There are always areas with little or no test coverage, "all tests pass" is great but never enough. |
This commit removes existing code, which uses HEAD right after cloning the repository into the workspace from a cache. Background: When a remote default branch is renamed and the auto-cache is updated with remote, the auto-cache becomes corrupt as HEAD points to an invalid remote HEAD. Therefore, avoid using HEAD within git operations during west update, as long as no revision is checked out.
229d7ae to
c99a3da
Compare
With this change the added test does not pass, so it "does not work". |
This comment was marked as resolved.
This comment was marked as resolved.
|
This is making less and less sense. I suggested Compare: |
Issue: Auto-cache fails when cached repository’s default branch changes
We’re encountering failures when using auto-cache in scenarios where the default branch name of a cached repository has changed. @pdgendt has been able to reproduce this issue locally as well.
During analysis, we observed that
git remote update --prunecompletes successfully. However, attempting to clone a repository from the auto-cache into the workspace subsequently results in a valid clone, but with no HEAD existing.This behavior is resolved with git (2.48.0). Without the fix from this PR, the added test fails with older git (before 2.48.0, e.g. 2.34.1 from Ubuntu 22), but it passes when newer git (>=2.48.0) is used.
Reference to git Release Notes: https://github.com/git/git/blob/master/Documentation/RelNotes/2.48.0.adoc
A typical
west updateerror looks like this:Root Cause
When a remote default branch is renamed and the auto-cache is updated with remote, the auto-cache becomes corrupt as HEAD points to an invalid remote HEAD
Git clone from such a corrupt auto-cache succeeds, but with a warning
warning: remote HEAD refers to nonexistent ref, unable to checkout.. Subsequent git commands using HEAD may fail. For example, the next git command inwest updateisgit checkout --detach HEAD, which now fails because HEAD does not exist.First Approach: Ensure HEAD exists
After cloning, run
git checkout {project.revision}to make sure that HEAD exists. Subsequentgit checkout --detach HEADwill succeed. But in this case, the auto-cache still remains corrupt. So I personally prefer a solution that fixes the auto-cache, instead of just fixing the clone in the workspace.Second Approach: Fixing the auto-cache
After running
git remote update --prune, we need to set HEAD to remote HEAD within the auto-cache.This is addressed by:
git ls-remote --symref origin HEADgit symbolic-ref HEAD <remote-HEAD>This ensures the cache stays in sync with the upstream repository’s default branch and prevents the checkout errors described above. This solution helps to ensure that HEAD will always exist after cloning from the cache.
Third Approach: Avoid running into the issue by not using HEAD at all
Avoid using
HEADat all duringwest updateafter cloning a repository from a cache into the workspace.