Skip to content
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

fix(pkg): Only parse blob objects into files #9352

Merged
merged 1 commit into from Dec 7, 2023

Conversation

Leonidas-from-XIV
Copy link
Collaborator

While investigating #9328 it turns out that the tree has an object that's of type commit. I originally thought that this was a symlink to nowhere (since the revision it points to does not exist) but it turns out it is something different as symlinks are represented as blobs after all.

The code should discard these non-blob objects. However I don't yet know how to create a commit object for the repro case so for now it only tests broken symlinks. Any git-fu appreciated!

Fixes #9328

Re.exec_opt re line
|> Option.bind ~f:(fun m ->
match Re.Group.get m 1 with
| "blob" ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only want blobs, how about we modify the regex to accept only blobs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I did it this way is because I was thinking that maybe we want to support commit objects by e.g. looking up through the commit id and exposing the tree that that commit is using.

@rgrinberg
Copy link
Member

Could you add a test case in a separate PR so that we can record the issue existing and then being fixed?

@Leonidas-from-XIV
Copy link
Collaborator Author

@rgrinberg Sure, that actually prompted me into building a repo that has this exact issue that Ali described: #9372. I will rebase this PR on top of that when it is merged, since that repro test is much better than the one here.

@Leonidas-from-XIV
Copy link
Collaborator Author

Rebased on top of the repro case and it shows that locking works now even in the presence of a commit object.

Fixes ocaml#9328

Signed-off-by: Marek Kubica <marek@tarides.com>
@Leonidas-from-XIV Leonidas-from-XIV merged commit 2eb12b9 into ocaml:main Dec 7, 2023
25 of 27 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the ignore-non-blobs branch December 7, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pkg: rev_store can't handle symlinks
2 participants