Skip to content

Fix worktree iteration when repository has no common directory - #5943

Merged
ethomson merged 1 commit into
libgit2:mainfrom
kcsaul:fix/5851
Aug 31, 2021
Merged

Fix worktree iteration when repository has no common directory#5943
ethomson merged 1 commit into
libgit2:mainfrom
kcsaul:fix/5851

Conversation

@kcsaul

@kcsaul kcsaul commented Jul 11, 2021

Copy link
Copy Markdown
Contributor

This is intended to fix #5851, which highlights an assertion within git_path_prettify that occurs during git_repository_foreach_worktree when it attempts to open the repository because repo->commondir is null. This occurs in contexts where the repository has been created explicitly in conjunction with a custom odb/refdb backend.

git_repository_foreach_worktree has been changed to check when commondir is null, and in that case apply the callback operation to the repository supplied only, as it would appear there are no linked worktrees to iterate.

When applying an operation to a repository created without a common directory,
which is known to occur in scenarios where custom odb/refdb backends are used,
git_repository_foreach_worktree currently asserts while attempting to open the
repository located in the common directory.

Fix this issue by applying operation to repository supplied when there are no
linked worktrees to iterate (implied by common directory being empty).
@kcsaul

kcsaul commented Jul 11, 2021

Copy link
Copy Markdown
Contributor Author

Based on a prior comment, I'm under the impression there's insufficient unit tests for custom backend implementations which may have exposed this issue. It would seem there's still an outstanding question as to how such unit tests might be implemented (unless some have been implemented since that comment was made?). With that in mind, I'm assuming this fix could be merged without any additional unit tests being written.

@kcsaul
kcsaul marked this pull request as ready for review July 11, 2021 10:37
@ethomson

Copy link
Copy Markdown
Member

Based on a prior comment, I'm under the impression there's insufficient unit tests for custom backend implementations which may have exposed this issue. It would seem there's still an outstanding question as to how such unit tests might be implemented (unless some have been implemented since that comment was made?). With that in mind, I'm assuming this fix could be merged without any additional unit tests being written.

I agree that we don't necessarily need tests. But this is the second or third such issue uncovered, which is disappointing. I want to take a little look and see if we can't do something simple with tests here. There's at least one relatively major consumer of libgit2 (aside from you) that's using custom db's entirely and would be facing such problems.

@kcsaul

kcsaul commented Aug 8, 2021

Copy link
Copy Markdown
Contributor Author

@ethomson - I wonder if such problems could be avoided in future by moving functionality which interacts with each working directory behind a worktree backend interface. This would enable libgit2 consumers using custom db's entirely to provide a custom worktree iterator and implement hooks for functionality that needs to interact with the working directory (i.e. checkout, diff).

Appreciate this suggestion will be perceived as a very big change in approach, but if feasible, it'd mitigate risk of such problems arising again, reduce the need for defining tests (for which you'd have to anticipate how a custom db may work), and it'd allow us to reuse worktree related functions in scenarios where the 'working directory' has been virtualized.

@ethomson
ethomson merged commit 3addb79 into libgit2:main Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Assertion failed path && path_out" in src/path.c while trying to rename reference on custom refdb repo without commonpath set

2 participants