- Notifications
You must be signed in to change notification settings - Fork2.5k
Make refdb_fs (hopefully) fully aware of per worktree refs#6387
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Signed-off-by: Sven Strickroth <email@cs-ware.de>
59b4385 to21e5903CompareFixes issue isselibgit2#5492.Signed-off-by: Sven Strickroth <email@cs-ware.de>
csware commentedJul 17, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@ethomson Any chance to get a review? |
| @@ -863,6 +867,49 @@ static int iter_load_loose_paths(refdb_fs_backend *backend, refdb_fs_iter *iter) | |||
| error = git_vector_insert(&iter->loose, ref_dup); | |||
| } | |||
| if (!error && git_repository_is_worktree(backend->repo) == 1) { | |||
| git_iterator_free(fsit); | |||
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.
I think from a layering perspective, the filesystem refdb is not the right place for this — that would mean thatevery reference db would have to handle the per-corktree references themselves. I think that we should hoist the worktree logic intorefs.c instead. In theory, the reference database should be able to operate without any knowledge of a repository (indeed, should be able to operate without a repository instance at all).
Reference databases should just handle the storage of references, and thegit_reference family of functions should handle the business logic, including understanding whether there's a worktree or not.
Let me know if this makes sense — I can take a stab at an implementation on the references if you'd like.
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.
Good point.
However, here, both parts are within the filesystem.
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.
Not sure that I understand? Are you saying that since worktrees are in the filesystem and the default git-compatible ref database is also on the filesystem, that this is a reasonable separation of concerns?
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.
Er, no, you're saying that the worktree reference storage living in the commondir is actually an implementation detail of the git-compatible ref database implementation itself. 🤔
That's a bit disappointing, but I think that you may be correct given our current database architecture.
Thanks for the fix! I hope to come back and maybe re-think the abstraction around worktree refs databases, so that each refdb backend doesn't need to think about worktrees. In the meantime, this is great. 🎉 |
This makes refdb_fs fully aware of per worktree refs (fixes issue#5492).
The code has some duplication but does the trick.