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

Introduce Refdb type #982

Merged
merged 14 commits into from
Apr 2, 2020
Merged

Introduce Refdb type #982

merged 14 commits into from
Apr 2, 2020

Conversation

ddevault
Copy link
Contributor

@ddevault ddevault commented Feb 28, 2020

TODO:

  • Wrap git_refdb in Refdb type
  • Add Reference constructor(s)
  • Create RefdbBackend
  • Create repositories with custom refdb/odb
  • Create Refdb from scratch
  • Tests
  • Update docs/backends.rst

Future work:

  • Iterating over a refdb_backend from Python
  • First-class reflog support
  • Transactional refdb updates

src/refdb.c Outdated Show resolved Hide resolved
src/refdb.c Outdated Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
Copy link
Member

@jdavid jdavid left a comment

Choose a reason for hiding this comment

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

Thanks for your work! It's quite a lot.

src/refdb_backend.c Outdated Show resolved Hide resolved
src/refdb_backend.c Show resolved Hide resolved
src/refdb_backend.c Outdated Show resolved Hide resolved
@jdavid
Copy link
Member

jdavid commented Mar 7, 2020

Looking at the API level.

I see there're a number of ways to get a backend:

refdb = repo.refdb       # wraps git_repository_refdb
refdb = Refdb(repo)      # wraps git_refdb_new
refdb = Refdb.open(repo) # wraps git_refdb_open

For consistency and to be explicit please change Refdb(repo) to Refdb.new(repo)

@ddevault ddevault force-pushed the refdb branch 2 times, most recently from 4169c01 to a4cbb58 Compare March 9, 2020 15:26
@ddevault
Copy link
Contributor Author

ddevault commented Mar 9, 2020

For consistency and to be explicit please change Refdb(repo) to Refdb.new(repo)

Done.

@ddevault
Copy link
Contributor Author

ddevault commented Mar 9, 2020

Related libgit2 bug:

libgit2/libgit2#5449

This prevents us from being able to create a RefdbFsBackend and use it to create a Refdb from scratch. Other than that, the functionality of this patch is complete. I'll add tests in a day or two.

Copy link
Member

@jdavid jdavid left a comment

Choose a reason for hiding this comment

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

Only a couple of comments for the 1st commit. Thanks!

src/odb_backend.c Outdated Show resolved Hide resolved
src/odb_backend.c Outdated Show resolved Hide resolved
src/odb_backend.c Outdated Show resolved Hide resolved
@ddevault
Copy link
Contributor Author

Addressed comments on odb fixes. Still need to write tests and verify that everything works. I ran into some issues in a shallow test the other day.

@ddevault
Copy link
Contributor Author

Note: Windows CI is failing due to lack of fnmatch support, which I used to implement globbing.

Options:

  • Drop in musl libc's implementation, ~300 LoC, MIT licensed
  • Add a code path for Windows which doesn't support globbing
  • Implement globbing some other way

@ddevault
Copy link
Contributor Author

ddevault commented Mar 19, 2020

✓ Tests

Code-wise, this should be ready for review. The question of what to do about fnmatch on Windows remains, and I need to write up some docs and rebase everything.

@ddevault
Copy link
Contributor Author

Oh, note that this requires the following libgit2 change before it'll work: libgit2/libgit2#5456

@jdavid
Copy link
Member

jdavid commented Mar 21, 2020

Regarding fnmatch does libgit2 provide an implementation for this? Apparently they did, but replaced it by wildmatch, see libgit2/libgit2#5110 So I think we should use the same as libgit2.

@ddevault
Copy link
Contributor Author

libgit2/libgit2#5458

I'll just vendor it in the meanwhile (on Monday).

@ddevault
Copy link
Contributor Author

✓ fnmatch -> wildmatch

✓ Questions answered from libgit2 about the semantics of refdb_backend.write

Still needs docs but otherwise I think this is good to go.

@ddevault ddevault changed the title [WIP] Introduce Refdb type Introduce Refdb type Mar 23, 2020
@jdavid
Copy link
Member

jdavid commented Mar 24, 2020

The test failures are because of libgit2/libgit2#5456 ?
I see this has been merged to master but not to maint/v0.99, then please update the CI files to use master, so the tests pass.

@ddevault ddevault force-pushed the refdb branch 3 times, most recently from fdf0244 to 2406bf4 Compare March 24, 2020 13:54
@ddevault
Copy link
Contributor Author

Got all of the CI pointing at libgit2 master and the tests are passing.

@ddevault
Copy link
Contributor Author

I have rigged up the documentation as well.

@jdavid
Copy link
Member

jdavid commented Mar 26, 2020

Cool, now we have to wait for a libgit2 release with libgit2/libgit2#5456
Hopefully it won't take long.

Prevents freeing an uninitialized pointer later on.
Custom refdb backends will need to be able to construct References, and
this change adds the necessary constructors. However, the resulting
Reference objects are fragile, and should not be used for much else.
This is noted in the docstring.
Currently this allows you to manipulate refdbs in a vacuum. A later
commit will introduce RefdbBackend, then allow you to create a
Repository given a refdb backend and an odb backend.

The refdb is more or less a write-only interface from libgit2, so it's
not really possible to verify that the functions added here have
affected libgit2's state in the desired way. For this reason, tests are
not included for this type.
This creates a git_repository with no odb and no refdb.
This is imported from libgit2
@ddevault
Copy link
Contributor Author

I pushed a little bit of clean up in the course of implementing this:

https://git.sr.ht/~sircmpwn/git.sr.ht/commit/a3fe57b6844709eac64fdca60370e5c227979fd3

It works 😁

@@ -198,7 +198,6 @@ pygit2_refdb_backend_lookup(git_reference **out,

*out = result->reference;
out:
Py_DECREF(result);
Copy link
Member

Choose a reason for hiding this comment

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

Why this? Doesn't lookup(..) return a new reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently not. This was causing double frees.

@jdavid jdavid merged commit e012bb5 into libgit2:master Apr 2, 2020
@ddevault
Copy link
Contributor Author

ddevault commented Apr 2, 2020

Thanks!

@jdavid
Copy link
Member

jdavid commented Apr 3, 2020

Thank you!

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.

2 participants