Conversation
| !backend->reflog_rename || !backend->reflog_delete || | ||
| (backend->lock && !backend->unlock)) { | ||
| git_error_set(GIT_ERROR_REFERENCE, "incomplete refdb backend implementation"); | ||
| return GIT_EINVALID; |
There was a problem hiding this comment.
I would have made this an assertion instead of a soft error, personally. There's no recovery for end-users here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But if we used an assert, then we wouldn't error at all on non-debug builds
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| error = packed_write(backend); | ||
| if (found) | ||
| error = packed_write(backend); |
There was a problem hiding this comment.
I wonder if it's worth making sure that GIT_ENOTFOUND could not be propagated back from either git_sortedcache_wlock or packed_write. A quick glance suggests that those will never return those, and it's hard to imagine a world where they would be refactored to a point where GIT_ENOTFOUND would be meaningful, but it feels Not Impossible. So we might want to be strict about converting those to a -1 if we want to treat them as a true failure here. 🤔
OTOH, it will probably never matter.
There was a problem hiding this comment.
That… went over my head. What's the concern ? That a GIT_ENOTFOUND from wlock or write could be misinterpreted as meaning the ref doesn't exist, while the issue could be more severe ?
|
|
||
| git_sortedcache_wunlock(backend->refcache); | ||
|
|
||
| error = packed_write(backend); |
There was a problem hiding this comment.
It's not only about clobbering error values, is it? Previously, we would've tried to write the packed file even in cases where we didn't remove any reference at all, right?
There was a problem hiding this comment.
Yes, from a functional POV it's slightly different in that the packed file will not be rewritten. AFAIR, this was also some kind of performance improvement, in that a packed-ref lookup failure when deleting would cause a "spurious" rewrite of an innocent file (but please double-check me, I'm having a hard time with how the locking is handled). I'll move that to the commit message to clarify that it's not only about the clobbering.
|
On Thu, Jun 13, 2019 at 04:24:06AM -0700, Edward Thomson wrote:
ethomson commented on this pull request.
> @@ -68,6 +68,16 @@ 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;
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.
Fair enough. The question is whether we want to say "We do so
everywhere, so let's just introduce more places where we may run
into hard crashes in production". I know, this is kind-of framing
this in a rather biased way, but I feel like we should just use
error codes. The current mainstream opinion seems to be "never
cause segfaults in a library", from what I've read in the last
few years.
Alternatively, we could also introduce a new macro
`GIT_FATAL(GIT_EINVALID, msg)`. On debug builds this may trigger
an assert, while it may cause an error return code on release
builds. But I don't know whether that's too magical, at least I'm
not too thrilled by that.
|
I think this is reasonable but we should split this out into a separate conversation, IMO. |
|
On Thu, Jun 13, 2019 at 01:32:13PM -0700, Edward Thomson wrote:
> The current mainstream opinion seems to be "never cause
> segfaults in a library", from what I've read in the last few
> years.
I think this is reasonable but we should split this out into a
separate conversation, IMO.
Agreed, no need to keep this PR on hold for that. For the sake of
having an easier path of conversion I'd be fine with raising an
assert for now.
|
f01b370 to
240a369
Compare
In the case of a failed lookup, we'd paper over that by writing back the packed-refs successfully.
This fixes part of the issue where, given a concurrent `git pack-refs`, a ref lookup could return an old, vestigial value from the packed file, as the valid loose one would have been deleted.
240a369 to
8c14224
Compare
|
Thanks a lot, @tiennou! |
This is the preliminary part to the reflog message PR (#4316), and is limited to documentation, QOL improvements, NFC, and some bugfixes.
(Please make sure it's it in sync with #4316 on merge, since it's likely to get rebased again).