You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We have just finished a DfsRepository backend over S3-compatible object storage for Skills Gateway, an open-source gateway that serves git repositories over smart-HTTP. Refs and the live pack set live in a small JSON manifest; every transition is a conditional PutObject guarded by If-Match on that manifest's ETag, so uncoordinated replicas serialise on the store itself rather than on a lock service.
This is offered as field notes rather than a proposal. #251 discusses a Hibernate-backed DFS implementation and lands on maintaining it out of tree; ours is out of tree too, and will stay there. What follows is what cost us time, in the hope it is useful to whoever writes the next one.
The thing we most wish had existed: a DFS conformance kit
JGit ships InMemoryRepository as the only DfsRepository implementation and no way for an implementer to check their own backend behaves like RefDirectory where the wire protocol can tell the difference.
Every substantive defect we found was in exactly that space, and none of them were caught by a compiler or by ordinary functional tests:
performsAtomicTransactions() silently changes an advertised capability.RefDatabase returns false; RefDirectory and DfsReftableDatabase return true; ReceivePack advertises CAPABILITY_ATOMIC only when it is true. A new backend inherits the default and quietly stops advertising atomic. Ordinary pushes never notice — git push --atomic fails client-side with "the receiving end does not support --atomic". Nothing warns you at any point.
Symbolic refs in a copy loop.getRefs() reports a symbolic ref with the id it resolves to, so the obvious migration loop — read refs, write each through updateRef(name) — dereferences HEAD and writes the destination's branch. That is correct for every repository whose head is main and silent corruption for every one that is not. Our tests passed until we deliberately deleted the HEAD copy and watched them stay green.
Discarded RefUpdate results.forceUpdate() and delete() return a Result and do not throw on refusal. A backend, or a caller above it, that ignores the return value turns a correctly-detected conflict into a silent success. We found this in our own filesystem code before the object-store backend existed.
Sohn's remark in #251 that a persistent implementation would "validate the DFS API" is, we think, the right instinct pointed at the wrong artefact: the value is in an executable conformance suite, not in a second reference implementation that then needs maintaining. A kit costs nothing at runtime, has no dependencies to license, and does not need a committer to adopt a storage engine.
We wrote one for our two backends, parameterised so a new backend joins by adding a line. The gateway-specific half is ours; the JGit-facing half — capability parity, symbolic-ref round trips, result codes on refusal, HEAD on a fresh repository — generalises, and we would be glad to contribute it if there is appetite. It would also be the natural place to assert the SHA-256 readiness raised in #251, mechanically, for every implementer.
compareAndPut's precondition granularity
DfsRefDatabase's three abstract methods are a compare-and-swap interface, and that is a genuinely good fit — the mapping onto a conditional PUT is direct.
The mismatch is granularity. compareAndPut(oldRef, newRef) expresses a per-ref precondition; a manifest ETag is a per-repository one. Two writers updating disjoint refs both hold valid ref-level preconditions, yet one loses the ETag race. Returning false there is wrong: DfsRefUpdate.doUpdate converts it to LOCK_FAILURE, JGit does not retry, and the caller sees a spurious failure for an update that should have succeeded. A correct coarse-grained backend must therefore re-read, re-check the ref-level precondition and re-PUT insidecompareAndPut, returning false only when the ref itself moved.
That is fine once you know it. Nothing says it. The cheapest fix is a javadoc sentence: false means the reference moved, not the store was busy, and a store whose precondition is coarser than one ref must absorb its own contention.
Two smaller surprises
ReceivePack.setAtomic(true) is overwritten.service() calls setAtomic(isCapabilityEnabled(CAPABILITY_ATOMIC)), so the setter looks authoritative but is superseded by what the client negotiated. We had a line of configuration that did nothing, and only found out by reading the source.
Ranged reads.ReadableChannel supports positioned reads and DfsBlockCache is there, but there is no worked example of driving it from HTTP range requests. We fetch whole packs into a bounded local cache on first open — defensible, and it narrows the window where GC deletes a pack mid-upload-pack, but we chose it partly for want of a reference.
The internal package
The DFS extension points live in org.eclipse.jgit.internal.storage.dfs and carry no compatibility promise, while being the only supported way to write a storage backend. We have configured our dependency automation to raise JGit upgrades on their own rather than grouped with unrelated bumps, precisely so a minor release that moves this surface is reviewed deliberately. We are not asking for a frozen API — a stated stability policy, or promotion of the SPI to a supported package, would let implementers plan.
What we are not asking for
We are not proposing our backend for inclusion. Object storage brings its own operational obligations — pack compaction, tombstones and grace periods, a cross-replica freshness bound — and those belong with the people who run it. #251's conclusion looks right to us for the same reasons.
Happy to open a Gerrit change for the conformance kit if that would be welcome, or to write any of the above up as individual issues if that is the more useful shape.
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
Uh oh!
There was an error while loading. Please reload this page.
We have just finished a
DfsRepositorybackend over S3-compatible object storage for Skills Gateway, an open-source gateway that serves git repositories over smart-HTTP. Refs and the live pack set live in a small JSON manifest; every transition is a conditionalPutObjectguarded byIf-Matchon that manifest's ETag, so uncoordinated replicas serialise on the store itself rather than on a lock service.This is offered as field notes rather than a proposal. #251 discusses a Hibernate-backed DFS implementation and lands on maintaining it out of tree; ours is out of tree too, and will stay there. What follows is what cost us time, in the hope it is useful to whoever writes the next one.
The thing we most wish had existed: a DFS conformance kit
JGit ships
InMemoryRepositoryas the onlyDfsRepositoryimplementation and no way for an implementer to check their own backend behaves likeRefDirectorywhere the wire protocol can tell the difference.Every substantive defect we found was in exactly that space, and none of them were caught by a compiler or by ordinary functional tests:
performsAtomicTransactions()silently changes an advertised capability.RefDatabasereturnsfalse;RefDirectoryandDfsReftableDatabasereturntrue;ReceivePackadvertisesCAPABILITY_ATOMIConly when it istrue. A new backend inherits the default and quietly stops advertisingatomic. Ordinary pushes never notice —git push --atomicfails client-side with "the receiving end does not support --atomic". Nothing warns you at any point.getRefs()reports a symbolic ref with the id it resolves to, so the obvious migration loop — read refs, write each throughupdateRef(name)— dereferencesHEADand writes the destination's branch. That is correct for every repository whose head ismainand silent corruption for every one that is not. Our tests passed until we deliberately deleted theHEADcopy and watched them stay green.RefUpdateresults.forceUpdate()anddelete()return aResultand do not throw on refusal. A backend, or a caller above it, that ignores the return value turns a correctly-detected conflict into a silent success. We found this in our own filesystem code before the object-store backend existed.Sohn's remark in #251 that a persistent implementation would "validate the DFS API" is, we think, the right instinct pointed at the wrong artefact: the value is in an executable conformance suite, not in a second reference implementation that then needs maintaining. A kit costs nothing at runtime, has no dependencies to license, and does not need a committer to adopt a storage engine.
We wrote one for our two backends, parameterised so a new backend joins by adding a line. The gateway-specific half is ours; the JGit-facing half — capability parity, symbolic-ref round trips, result codes on refusal,
HEADon a fresh repository — generalises, and we would be glad to contribute it if there is appetite. It would also be the natural place to assert the SHA-256 readiness raised in #251, mechanically, for every implementer.compareAndPut's precondition granularityDfsRefDatabase's three abstract methods are a compare-and-swap interface, and that is a genuinely good fit — the mapping onto a conditional PUT is direct.The mismatch is granularity.
compareAndPut(oldRef, newRef)expresses a per-ref precondition; a manifest ETag is a per-repository one. Two writers updating disjoint refs both hold valid ref-level preconditions, yet one loses the ETag race. Returningfalsethere is wrong:DfsRefUpdate.doUpdateconverts it toLOCK_FAILURE, JGit does not retry, and the caller sees a spurious failure for an update that should have succeeded. A correct coarse-grained backend must therefore re-read, re-check the ref-level precondition and re-PUT insidecompareAndPut, returningfalseonly when the ref itself moved.That is fine once you know it. Nothing says it. The cheapest fix is a javadoc sentence:
falsemeans the reference moved, not the store was busy, and a store whose precondition is coarser than one ref must absorb its own contention.Two smaller surprises
ReceivePack.setAtomic(true)is overwritten.service()callssetAtomic(isCapabilityEnabled(CAPABILITY_ATOMIC)), so the setter looks authoritative but is superseded by what the client negotiated. We had a line of configuration that did nothing, and only found out by reading the source.Ranged reads.
ReadableChannelsupports positioned reads andDfsBlockCacheis there, but there is no worked example of driving it from HTTP range requests. We fetch whole packs into a bounded local cache on first open — defensible, and it narrows the window where GC deletes a pack mid-upload-pack, but we chose it partly for want of a reference.The
internalpackageThe DFS extension points live in
org.eclipse.jgit.internal.storage.dfsand carry no compatibility promise, while being the only supported way to write a storage backend. We have configured our dependency automation to raise JGit upgrades on their own rather than grouped with unrelated bumps, precisely so a minor release that moves this surface is reviewed deliberately. We are not asking for a frozen API — a stated stability policy, or promotion of the SPI to a supported package, would let implementers plan.What we are not asking for
We are not proposing our backend for inclusion. Object storage brings its own operational obligations — pack compaction, tombstones and grace periods, a cross-replica freshness bound — and those belong with the people who run it. #251's conclusion looks right to us for the same reasons.
Happy to open a Gerrit change for the conformance kit if that would be welcome, or to write any of the above up as individual issues if that is the more useful shape.
All reactions