Conversation
Reviewer's GuideEnhance the Git backend’s clone operation to gracefully handle existing dangling lock files by retrying the import operation after removing the lock, and add tests to validate this behavior. Class diagram for updated GitBackend clone logicclassDiagram
class GitBackend {
+_clone(url: str, refspec: GitRefSpec, target: Path) Repo
}
class LocalRepo {
+refs: Refs
+get_worktree()
}
class Refs {
+import_refs(base, other)
}
class FileLocked {
+lockfilename: bytes
}
GitBackend --> LocalRepo : uses
LocalRepo --> Refs : has
Refs <.. FileLocked : raises
GitBackend --> FileLocked : handles exception
GitBackend --> Logger : logs debug
GitBackend --> FileSystem : removes lock file
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider catching and ignoring OSError around the Path.unlink call to avoid failures if the lock file doesn’t exist or can’t be removed.
- Rather than hardcoding two retries, consider making the retry count configurable or introducing a small backoff to handle more persistent lock file conflicts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider catching and ignoring OSError around the Path.unlink call to avoid failures if the lock file doesn’t exist or can’t be removed.
- Rather than hardcoding two retries, consider making the retry count configurable or introducing a small backoff to handle more persistent lock file conflicts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
automatically removing a lock file surely undermines the point of the lock file? |
Sure, but that is what many users (including myself) do manually if they encounter this state. Of course, it would be better if git/dulwich could handle it by itself. But if they do not, removing it may be a practical way to deal with it. I assume the lock file should not be there at this point anyway. Otherwise, we should at least give a better error message like "try again in some seconds/minutes and if you still experience this issue remove the lock file manually." Maybe, @jelmer can assess if we risk some other (more serious) issues if we remove the lock file. |
|
Agree that a better error message would be desirable. Also agree that if dulwich lock files can dangle then that suggests a bug in dulwich.
I expect that the lock file serves some purpose - and that poetry assuming that dulwich has gone wrong would defeat that purpose. A user is much better placed to decide whether they are indeed in the middle of performing two operations at once, or whatever, and to make a judgment about whether the lock file is really dangling - or is just doing its job. |
|
I've added a comment in #7832 (comment) - I think we should try to understand first why this is happening. The lock files aren't specific to Dulwich. |
|
Superseded by #10535 |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
Pull Request Check List
Resolves: #7832
Summary by Sourcery
Make Git clone more robust against stale lock files by retrying ref imports and deleting dangling locks
Bug Fixes:
Enhancements:
Tests: