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

refs: preserve the owning refdb when duping reference #4618

Merged
merged 2 commits into from Apr 17, 2018

Conversation

tiennou
Copy link
Contributor

@tiennou tiennou commented Apr 10, 2018

This fixes a segfault in git_reference_owner on references returned from git_reference__read_head and git_reference_dup ones.

This fixes a segfault in git_reference_owner on references returned from git_reference__read_head and git_reference_dup ones.
@tiennou
Copy link
Contributor Author

tiennou commented Apr 10, 2018

Note that I don't feel confident enough to change the behavior of __read_head, but maybe it's the right thing to do ?

@tiennou
Copy link
Contributor Author

tiennou commented Apr 10, 2018

This was introduced in 5b65ac2, and the fix looks easy : grab the refdb from the repo and put it in. That'll allow the comment to be dropped and preserve the "owner" semantics.

@pks-t maybe you can say if not having an owner is an oversight or a performance improvement of sorts (the commit message implies the latter) ? Mostly because right now I'm looking at the refdb ownership mechanism and worktrees, and I feel overwhelmed 😉.

@pks-t
Copy link
Member

pks-t commented Apr 12, 2018

Hm. First thing I notice is that git_reference__read_head is poorly named. It really should've been something like git_reference__from_file, as it doesn't necessarily have anything to do with HEADs at all. Furthermore, I think that your easy fix would be the right thing to do. We already associate it with the passed in repo in case it is not a symbolic reference, so we should do the same when it is a symbolic reference. It shouldn't impact performance at all to just have an additional refcount increment on the refdb.

@ethomson
Copy link
Member

Hm. First thing I notice is that git_reference__read_head is poorly named. It really should've been something like git_reference__from_file, as it doesn't necessarily have anything to do with HEADs at all.

Sure it does. HEAD, MERGE_HEAD, ORIG_HEAD, etc are the only files in a git repository that we can guarantee are in that format. Every other access of a reference should go through the refdb.

The name indicates what it does (reads a HEAD) instead of how it does it (reading from a file) but to say that this is "poor" is a judgement call that I don't agree with.

@pks-t
Copy link
Member

pks-t commented Apr 12, 2018

Umm... are they? The function should be totally safe to call on any reference, disregarding if it is a symbolic one or not. I probably shouldn't argue, because I'm the one who named that function like that :P

@ethomson
Copy link
Member

Maybe I'm not understanding - but I'm thinking that you shouldn't call this on any reference except special HEAD refs that are guaranteed to be in the git dir. This function assumes that the ref exists on the filesystem which violates the abstraction of the refdb. So it only works on loose refs, but only accidentally, and wouldn't work at all on packed refs or if you had installed your own custom refdb backend.

@tiennou
Copy link
Contributor Author

tiennou commented Apr 12, 2018

Just for clarification, it's only used to resolve worktree HEADs at the moment, and it seems to be perfectly safe to use as long as you don't really need the reference.

The only issue it has is that it doesn't setup the owning refdb pointer correctly when resolving a symbolic reference (as it just parses the file's content), so you need to make sure to resolve a second time (which git_repository_head_for_worktree does), hence the "performance improvement" argument I made.

I just hit that missing refdb case while moving unborn-ness checking in refs.c (see 37b69a5, part of this.

In all, as long as that weirdness is documented, I think it's fine to keep it that way, I just have an extra repo parameter in that resolve_unborn function because I can't depend on the ref's ownership 🤷‍♂️.

@tiennou
Copy link
Contributor Author

tiennou commented Apr 12, 2018

(Also also, that worktree head resolution function return NOTFOUND instead of UNBORN)

@ethomson
Copy link
Member

Thanks, @tiennou!

@ethomson ethomson merged commit 1fd2676 into libgit2:master Apr 17, 2018
@tiennou tiennou deleted the fix/pwned-references branch April 17, 2018 22:42
@tiennou
Copy link
Contributor Author

tiennou commented Apr 17, 2018

I'd like to know if that NOTFOUND/UNBORN nuance should be considered a bug/missing feature from the worktree support (note that I have absolutely no idea how worktrees work so this may not even be considered valid) ?

From the cursory look I gave the code, git_repository_head creates that nuance (and APIs that are downstream from it), to differentiate cases where the HEAD file is missing (NOTFOUND) from the case where the pointed-to ref is missing (UNBORN). AFAICT the refdb will NOTFOUND on resolving a missing pointed-to symbolic ref.

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.

None yet

3 participants