Skip to content

git_refdb API fixes #5106

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.

Sign up for GitHub

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

Merged
pks-t merged 8 commits into from
Sep 27, 2019
Merged

git_refdb API fixes #5106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8db9fd3
refdb: documentation
tiennou Feb 2, 2019
c2cf984
refdb: check the version of the backend we're about to set
tiennou Feb 2, 2019
baf411e
refdb: ensure all mandatory functions are provided at setup time
tiennou Feb 2, 2019
0a88c83
refdb: make low-level deletion helpers explicit
tiennou Feb 2, 2019
9b25cf1
refdb: fix packed_delete clobbering some errors
tiennou Feb 2, 2019
8fd855f
refdb: reorder parameters for consistency
tiennou Feb 2, 2019
171116e
refdb: repurpose filesystem prune function
tiennou Jun 14, 2019
8c14224
refdb: make sure to remove packed refs first
tiennou Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 57 additions & 21 deletions include/git2/sys/refdb_backend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,22 @@ struct git_reference_iterator {

/** An instance for a custom backend */
struct git_refdb_backend {
unsigned int version;
unsigned int version; /**< The backend API version */

/**
* Queries the refdb backend to determine if the given ref_name
* exists. A refdb implementation must provide this function.
* Queries the refdb backend for the existence of a reference.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(exists)(
int *exists,
git_refdb_backend *backend,
const char *ref_name);

/**
* Queries the refdb backend for a given reference. A refdb
* implementation must provide this function.
* Queries the refdb backend for a given reference.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(lookup)(
git_reference **out,
Expand All @@ -88,82 +90,116 @@ struct git_refdb_backend {
struct git_refdb_backend *backend,
const char *glob);

/*
* Writes the given reference to the refdb. A refdb implementation
* must provide this function.
/**
* Writes the given reference to the refdb.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(write)(git_refdb_backend *backend,
const git_reference *ref, int force,
const git_signature *who, const char *message,
const git_oid *old, const char *old_target);

/**
* Rename a reference in the refdb.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(rename)(
git_reference **out, git_refdb_backend *backend,
const char *old_name, const char *new_name, int force,
const git_signature *who, const char *message);

/**
* Deletes the given reference (and if necessary its reflog)
* from the refdb. A refdb implementation must provide this
* function.
* Deletes the given reference from the refdb.
*
* If it exists, its reflog should be deleted as well.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(del)(git_refdb_backend *backend, const char *ref_name, const git_oid *old_id, const char *old_target);

/**
* Suggests that the given refdb compress or optimize its references.
* This mechanism is implementation specific. (For on-disk reference
* databases, this may pack all loose references.) A refdb
* implementation may provide this function; if it is not provided,
* nothing will be done.
*
* This mechanism is implementation specific. For on-disk reference
* databases, this may pack all loose references.
*
* A refdb implementation may provide this function; if it is not
* provided, nothing will be done.
*/
int GIT_CALLBACK(compress)(git_refdb_backend *backend);

/**
* Query whether a particular reference has a log (may be empty)
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(has_log)(git_refdb_backend *backend, const char *refname);

/**
* Make sure a particular reference will have a reflog which
* will be appended to on writes.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(ensure_log)(git_refdb_backend *backend, const char *refname);

/**
* Frees any resources held by the refdb (including the `git_refdb_backend`
* itself). A refdb backend implementation must provide this function.
* itself).
*
* A refdb backend implementation must provide this function.
*/
void GIT_CALLBACK(free)(git_refdb_backend *backend);

/**
* Read the reflog for the given reference name.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(reflog_read)(git_reflog **out, git_refdb_backend *backend, const char *name);

/**
* Write a reflog to disk.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(reflog_write)(git_refdb_backend *backend, git_reflog *reflog);

/**
* Rename a reflog
* Rename a reflog.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(reflog_rename)(git_refdb_backend *_backend, const char *old_name, const char *new_name);

/**
* Remove a reflog.
*
* A refdb implementation must provide this function.
*/
int GIT_CALLBACK(reflog_delete)(git_refdb_backend *backend, const char *name);

/**
* Lock a reference. The opaque parameter will be passed to the unlock function
* Lock a reference.
*
* The opaque parameter will be passed to the unlock function.
*
* A refdb implementation may provide this function; if it is not
* provided, the transaction API will fail to work.
*/
int GIT_CALLBACK(lock)(void **payload_out, git_refdb_backend *backend, const char *refname);

/**
* Unlock a reference. Only one of target or symbolic_target
* will be set. success indicates whether to update the
* reference or discard the lock (if it's false)
* Unlock a reference.
*
* Only one of target or symbolic_target will be set.
* `success` will be true if the reference should be update, false if
* the lock must be discarded.
*
* A refdb implementation must provide this function if a `lock`
* implementation is provided.
*/
int GIT_CALLBACK(unlock)(git_refdb_backend *backend, void *payload, int success, int update_reflog,
const git_reference *ref, const git_signature *sig, const char *message);
Expand Down
12 changes: 12 additions & 0 deletions src/refdb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,18 @@ static void refdb_free_backend(git_refdb *db)

int git_refdb_set_backend(git_refdb *db, git_refdb_backend *backend)
{
GIT_ERROR_CHECK_VERSION(backend, GIT_REFDB_BACKEND_VERSION, "git_refdb_backend");

if (!backend->exists || !backend->lookup || !backend->iterator ||
!backend->write || !backend->rename || !backend->del ||
!backend->has_log || !backend->ensure_log || !backend->free ||
!backend->reflog_read || !backend->reflog_write ||
!backend->reflog_rename || !backend->reflog_delete ||
(backend->lock && !backend->unlock)) {
git_error_set(GIT_ERROR_REFERENCE, "incomplete refdb backend implementation");
return GIT_EINVALID;

@ethomson ethomson Jun 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would have made this an assertion instead of a soft error, personally. There's no recovery for end-users here.

@tiennou tiennou Jun 10, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point. I didn't want to fail too hard, since we were missing the check in the first place, but maybe an assert is warranted.

@pks-t pks-t Jun 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But if we used an assert, then we wouldn't error at all on non-debug builds

@ethomson ethomson Jun 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah but this is a truism throughout our codebase. We generally assert in places that are only a result of the consumer of libgit2 missing the preconditions for our api or writing bad or incomplete code.

@tiennou tiennou Jun 13, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I prefer runtime checks/error return to static asserts for things like this, as I'd be raising exceptions (NSInternalInconsistencyException FTW) if I had those. But asserts don't really work that way. I just had a feeling that the user might not be in control of that code as well (think copy-pasta from libgit2-backends).

As another data point, struct version checks are currently runtime errors, not asserts.

@ethomson ethomson Jun 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think moving asserts to something else is a reasonable concern. But I think it should be a separate PR instead of trying to figure out what that next thing is in this one, and I think we should move the entire code base en masse.

So I think that we should assert for now so that we're consistent with the rest of the code base and so that it's easier to mechanically replace when we decide on what we do want to do.

@tiennou tiennou Jun 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If that's possible, I'd like to keep it as it: as this is kinda a backward incompatible change about 1) something we forgot to enforce and 2) it's only to be more helpful, as right now "bad" users would eat a segfault anyway, I feel like it would be more helpful for unsuspecting end-users to have a normal "error" (even if it's not quite expected).

To be clear, I'm trying to fixing the few bugs in the refdb layer for which I have an almost 2-years old patch set, so if it's really that contentious, I'll open the issue about the general cleanup and drop it from here — with the missing version check is in, at least this struct is safe for 1.0.

}

refdb_free_backend(db);
db->backend = backend;

Expand Down
Loading