Skip to contentSkip to conversation

refs: introduce reftable backend

PATCH Superseded Patrick Steinhardt 4 participants 23 messages Jan 30, 2024 – Feb 15, 2024

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover refs: introduce reftable backend

Hi,

all prequisites for the reftable backend have now been merged or are about to be merged. This patch series thus introduces the reftable backend itself.

The patch series is built on top of c5b454771e (The eleventh batch, 2024-01-29). In addition it depends on ps/tests-with-ref-files-backend at bbd6106967 (t: mark tests regarding git-pack-refs(1) to be backend specific, 2024-01-29), which is about to be merged to "next".

The patch series is impacted by three in-flight patch series:

- ps/reftable-compacted-tables-permission-fix, which fixes permissions on "tables.list" when compacting so that it correctly honors the "core.sharedRepository" setting.

- jc/reftable-core-fsync, which starts to fsync reftable data to disk.

- A follow-up to that patch series [1], which starts to fsync "tables.list" to disk during compaction.

The patch series does _not_ depend on those series. Instead, I have added a couple of tests marked with test_expect_failure to t0610. These tests will start to pass once those topics land. As I don't expect this patch series to land on its first iteration I very much assume that the in-flight patch series will land before the reftable backend does.

Patrick

[1]: <7bdafc9bd7f53f38a24d69a563615b6ad484e1ba.1706592127.git.ps@pks.im>

Patrick Steinhardt (2):
refs: introduce reftable backend
ci: add jobs to test with the reftable backend

 .github/workflows/main.yml                    |    9 +
 .gitlab-ci.yml                                |    9 +
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 ci/lib.sh                                     |    2 +-
 ci/run-build-and-tests.sh                     |    3 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2286 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 17 files changed, 3237 insertions(+), 7 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh
Signature · 1 line
2.43.GIT
#1 lore raw

PATCH 1/2 refs: introduce reftable backend

Due to scalability issues, Shawn Pearce has originally proposed a new "reftable" format more than six years ago [1]. Initially, this new format was implemented in JGit with promising results. Around two years ago, we have then added the "reftable" library to the Git codebase via a4bbd13be3 (Merge branch 'hn/reftable', 2021-12-15). With this we have landed all the low-level code to read and write reftables. Notably missing though was the integration of this low-level code into the Git code base in the form of a new ref backend that ties all of this together.

This gap is now finally closed by introducing a new "reftable" backend into the Git codebase. This new backend promises to bring some notable improvements to Git repositories:

- It becomes possible to do truly atomic writes where either all refs are committed to disk or none are. This was not possible with the "files" backend because ref updates were split across multiple loose files.

- The disk space required to store many refs is reduced, both compared to loose refs and packed-refs. This is enabled both by the reftable format being a binary format, which is more compact, and by prefix compression.

- We can ignore filesystem-specific behaviour as ref names are not encoded via paths anymore. This means there is no need to handle case sensitivity on Windows systems or Unicode precomposition on macOS.

- There is no need to rewrite the complete refdb anymore every time a ref is being deleted like it was the case for packed-refs. This means that ref deletions are now constant time instead of scaling linearly with the number of refs.

- We can ignore file/directory conflicts so that it becomes possible to store both "refs/heads/foo" and "refs/heads/foo/bar".

- Due to this property we can retain reflogs for deleted refs. We have previously been deleting reflogs together with their refs to avoid file/directory conflicts, which is not necessary anymore.

Not all of these improvements are realized with the current "reftable" backend implementation. At this point, the new backend is supposed to be a drop-in replacement for the "files" backend that is used by basically all Git repositories nowadays. It strives for 1:1 compatibility, which means that a user can expect the same behaviour regardless of whether they use the "reftable" backend or the "files" backend for most of the part.

Most notably, this means we artificially limit the capabilities of the "reftable" backend to match the limits of the "files" backend. It is not possible to create refs that would end up with file/directory conflicts, we do not retain reflogs, we perform stricter-than-necessary checks. This is done intentionally due to two main reasons:

- It makes it significantly easier to land the "reftable" backend as tests behave the same. It would be tough to argue for each and every single test that doesn't pass with the "reftable" backend.

- It ensures compatibility between repositories that use the "files" backend and repositories that use the "reftable" backend. Like this, hosters can migrate their repositories to use the "reftable" backend without causing issues for clients that use the "files" backend in their clones.

It is expected that these artificial limitations may eventually go away in the long term.

Performance-wise things very much depend on the actual workload. The following benchmarks compare the "files" and "reftable" backends in the current version:

- Creating N refs in separate transactions shows that the "files" backend is ~50% faster. This is not surprising given that creating a ref only requires us to create a single loose ref. The "reftable" backend will also perform auto compaction on updates. In real-world workloads we would likely also want to perform pack loose refs, which would likely change the picture.

Benchmark 1: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 2.1 ms ± 0.3 ms [User: 0.6 ms, System: 1.7 ms]
Range (min … max): 1.8 ms … 4.3 ms 133 runs

Benchmark 2: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 2.7 ms ± 0.1 ms [User: 0.6 ms, System: 2.2 ms]
Range (min … max): 2.4 ms … 2.9 ms 132 runs

Benchmark 3: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 1.975 s ± 0.006 s [User: 0.437 s, System: 1.535 s]
Range (min … max): 1.969 s … 1.980 s 3 runs

Benchmark 4: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 2.611 s ± 0.013 s [User: 0.782 s, System: 1.825 s]
Range (min … max): 2.597 s … 2.622 s 3 runs

Benchmark 5: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 198.442 s ± 0.241 s [User: 43.051 s, System: 155.250 s]
Range (min … max): 198.189 s … 198.670 s 3 runs

Benchmark 6: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 294.509 s ± 4.269 s [User: 104.046 s, System: 190.326 s]
Range (min … max): 290.223 s … 298.761 s 3 runs

- Creating N refs in a single transaction shows that the "files" backend is significantly slower once we start to write many refs. The "reftable" backend only needs to update two files, whereas the "files" backend needs to write one file per ref.

Benchmark 1: update-ref: create many refs (refformat = files, refcount = 1)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.4 ms, System: 1.4 ms]
Range (min … max): 1.8 ms … 2.6 ms 151 runs

Benchmark 2: update-ref: create many refs (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.5 ms ± 0.1 ms [User: 0.7 ms, System: 1.7 ms]
Range (min … max): 2.4 ms … 3.4 ms 148 runs

Benchmark 3: update-ref: create many refs (refformat = files, refcount = 1000)
Time (mean ± σ): 152.5 ms ± 5.2 ms [User: 19.1 ms, System: 133.1 ms]
Range (min … max): 148.5 ms … 167.8 ms 15 runs

Benchmark 4: update-ref: create many refs (refformat = reftable, refcount = 1000)
Time (mean ± σ): 58.0 ms ± 2.5 ms [User: 28.4 ms, System: 29.4 ms]
Range (min … max): 56.3 ms … 72.9 ms 40 runs

Benchmark 5: update-ref: create many refs (refformat = files, refcount = 1000000)
Time (mean ± σ): 152.752 s ± 0.710 s [User: 20.315 s, System: 131.310 s]
Range (min … max): 152.165 s … 153.542 s 3 runs

Benchmark 6: update-ref: create many refs (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 51.912 s ± 0.127 s [User: 26.483 s, System: 25.424 s]
Range (min … max): 51.769 s … 52.012 s 3 runs

- Deleting a ref in a fully-packed repository shows that the "files" backend scales with the number of refs. The "reftable" backend has constant-time deletions.

Benchmark 1: update-ref: delete ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.7 ms ± 0.1 ms [User: 0.4 ms, System: 1.2 ms]
Range (min … max): 1.6 ms … 2.1 ms 316 runs

Benchmark 2: update-ref: delete ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.8 ms ± 0.1 ms [User: 0.4 ms, System: 1.3 ms]
Range (min … max): 1.7 ms … 2.1 ms 294 runs

Benchmark 3: update-ref: delete ref (refformat = files, refcount = 1000)
Time (mean ± σ): 2.0 ms ± 0.1 ms [User: 0.5 ms, System: 1.4 ms]
Range (min … max): 1.9 ms … 2.5 ms 287 runs

Benchmark 4: update-ref: delete ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.5 ms, System: 1.3 ms]
Range (min … max): 1.8 ms … 2.1 ms 217 runs

Benchmark 5: update-ref: delete ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 229.8 ms ± 7.9 ms [User: 182.6 ms, System: 46.8 ms]
Range (min … max): 224.6 ms … 245.2 ms 6 runs

Benchmark 6: update-ref: delete ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 2.0 ms ± 0.0 ms [User: 0.6 ms, System: 1.3 ms]
Range (min … max): 2.0 ms … 2.1 ms 3 runs

- Listing all refs shows no significant advantage for either of the backends. The "files" backend is a bit faster, but not by a significant margin. When repositories are not packed the "reftable" backend outperforms the "files" backend because the "reftable" backend performs auto-compaction.

Benchmark 1: show-ref: print all refs (refformat = files, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1729 runs

Benchmark 2: show-ref: print all refs (refformat = reftable, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.8 ms 1816 runs

Benchmark 3: show-ref: print all refs (refformat = files, refcount = 1000, packed = true)
Time (mean ± σ): 4.3 ms ± 0.1 ms [User: 0.9 ms, System: 3.3 ms]
Range (min … max): 4.1 ms … 4.6 ms 645 runs

Benchmark 4: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = true)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.0 ms, System: 3.3 ms]
Range (min … max): 4.2 ms … 5.9 ms 643 runs

Benchmark 5: show-ref: print all refs (refformat = files, refcount = 1000000, packed = true)
Time (mean ± σ): 2.537 s ± 0.034 s [User: 0.488 s, System: 2.048 s]
Range (min … max): 2.511 s … 2.627 s 10 runs

Benchmark 6: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = true)
Time (mean ± σ): 2.712 s ± 0.017 s [User: 0.653 s, System: 2.059 s]
Range (min … max): 2.692 s … 2.752 s 10 runs

Benchmark 7: show-ref: print all refs (refformat = files, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.9 ms 1834 runs

Benchmark 8: show-ref: print all refs (refformat = reftable, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.0 ms 1840 runs

Benchmark 9: show-ref: print all refs (refformat = files, refcount = 1000, packed = false)
Time (mean ± σ): 13.8 ms ± 0.2 ms [User: 2.8 ms, System: 10.8 ms]
Range (min … max): 13.3 ms … 14.5 ms 208 runs

Benchmark 10: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = false)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.2 ms, System: 3.3 ms]
Range (min … max): 4.3 ms … 6.2 ms 624 runs

Benchmark 11: show-ref: print all refs (refformat = files, refcount = 1000000, packed = false)
Time (mean ± σ): 12.127 s ± 0.129 s [User: 2.675 s, System: 9.451 s]
Range (min … max): 11.965 s … 12.370 s 10 runs

Benchmark 12: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = false)
Time (mean ± σ): 2.799 s ± 0.022 s [User: 0.735 s, System: 2.063 s]
Range (min … max): 2.769 s … 2.836 s 10 runs

- Printing a single ref shows no real difference between the "files" and "reftable" backends.

Benchmark 1: show-ref: print single ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.4 ms, System: 1.0 ms]
Range (min … max): 1.4 ms … 1.8 ms 1779 runs

Benchmark 2: show-ref: print single ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.5 ms 1753 runs

Benchmark 3: show-ref: print single ref (refformat = files, refcount = 1000)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.3 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 1.9 ms 1840 runs

Benchmark 4: show-ref: print single ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1831 runs

Benchmark 5: show-ref: print single ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1848 runs

Benchmark 6: show-ref: print single ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1762 runs

So overall, performance depends on the usecases. Except for many sequential writes the "reftable" backend is roughly on par or significantly faster than the "files" backend though. Given that the "files" backend has received 18 years of optimizations by now this can be seen as a win. Furthermore, we can expect that the "reftable" backend will grow faster over time when attention turns more towards optimizations.

The complete test suite passes, except for those tests explicitly marked to require the REFFILES prerequisite. Some tests in t0610 are marked as failing because they depend on still-in-flight bug fixes. Tests can be run with the new backend by setting the GIT_TEST_DEFAULT_REF_FORMAT environment variable to "reftable".

There is a single known conceptual incompatibility with the dumb HTTP transport. As "info/refs" SHOULD NOT contain the HEAD reference, and because the "HEAD" file is not valid anymore, it is impossible for the remote client to figure out the default branch without changing the protocol. This shortcoming needs to be handled in a subsequent patch series.

As the reftable library has already been introduced a while ago, this commit message will not go into the details of how exactly the on-disk format works. Please refer to our preexisting technical documentation at Documentation/technical/reftable for this.

[1]: https://public-inbox.org/git/CAJo=hJtyof=HRy=2sLP0ng0uZ4=S-DpZ5dR1aF+VHVETKG20OQ@mail.gmail.com/

---
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2286 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 13 files changed, 3215 insertions(+), 6 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh
Documentation/ref-storage-format.txt +2 -0
index 1a65cac468..14fff8a9c6 100644
@@ -1 +1,3 @@
 * `files` for loose files with packed-refs. This is the default.
+* `reftable` for the reftable format. This format is experimental and its
+  internals are subject to change.
Documentation/technical/repository-version.txt +3 -2
index 27be3741e6..47281420fc 100644
@@ -103,5 +103,6 @@ GIT_COMMON_DIR/worktrees/<id>/config.worktree)
 
 ==== `refStorage`
 
-Specifies the file format for the ref database. The only valid value
-is `files` (loose references with a packed-refs file).
+Specifies the file format for the ref database. The valid values are
+`files` (loose references with a packed-refs file) and `reftable` (see
+Documentation/technical/reftable.txt).
Makefile +1 -0
index 0f748a52e6..b302516173 100644
@@ -1127,6 +1127,7 @@ LIB_OBJS += reflog.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
+LIB_OBJS += refs/reftable-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
contrib/workdir/git-new-workdir +1 -1
index 888c34a521..989197aace 100755
@@ -79,7 +79,7 @@ trap cleanup $siglist
 # create the links to the original repo.  explicitly exclude index, HEAD and
 # logs/HEAD from the list since they are purely related to the current working
 # directory, and should not be shared.
-for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn
+for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn reftable
 do
 	# create a containing directory if needed
 	case $x in
path.c +1 -1
index 0fb527918b..8bb223c92c 100644
@@ -871,7 +871,7 @@ const char *enter_repo(const char *path, int strict)
 	return NULL;
 }
 
-static int calc_shared_perm(int mode)
+int calc_shared_perm(int mode)
 {
 	int tweak;
 
path.h +1 -0
index b3233c51fa..e053effef2 100644
@@ -181,6 +181,7 @@ const char *git_path_shallow(struct repository *r);
 int ends_with_path_components(const char *path, const char *components);
 int validate_headref(const char *ref);
 
+int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
refs.c +1 -0
index c633abf284..fff343c256 100644
@@ -35,6 +35,7 @@
  */
 static const struct ref_storage_be *refs_backends[] = {
 	[REF_STORAGE_FORMAT_FILES] = &refs_be_files,
+	[REF_STORAGE_FORMAT_REFTABLE] = &refs_be_reftable,
 };
 
 static const struct ref_storage_be *find_ref_storage_backend(unsigned int ref_storage_format)
refs/refs-internal.h +1 -0
index 82219829b0..83e0f0bba3 100644
@@ -693,6 +693,7 @@ struct ref_storage_be {
 };
 
 extern struct ref_storage_be refs_be_files;
+extern struct ref_storage_be refs_be_reftable;
 extern struct ref_storage_be refs_be_packed;
 
 /*
refs/reftable-backend.c new file +2286 -0

2,286 lines not on this page · raw message

repository.h +3 -2
index 7a250a6605..1645cef518 100644
@@ -24,8 +24,9 @@ enum fetch_negotiation_setting {
 	FETCH_NEGOTIATION_NOOP,
 };
 
-#define REF_STORAGE_FORMAT_UNKNOWN 0
-#define REF_STORAGE_FORMAT_FILES   1
+#define REF_STORAGE_FORMAT_UNKNOWN  0
+#define REF_STORAGE_FORMAT_FILES    1
+#define REF_STORAGE_FORMAT_REFTABLE 2
 
 struct repo_settings {
 	int initialized;
t/t0610-reftable-basics.sh new file +887 -0

648 lines not on this page · raw message

t/t0611-reftable-httpd.sh new file +26 -0
new file mode 100755
index 0000000000..5e05b9c1f2

Lines of this file are not shown. View the raw message.

t/test-lib.sh +2 -0
index fc93aa57e6..0fb8757b7d 100644

Lines of this file are not shown. View the raw message.

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.43.GIT

Part of the diff is not shown here. View the raw message.

#5 lore raw

PATCH 2/2 ci: add jobs to test with the reftable backend

In reply to Patrick Steinhardt

Add CI jobs for both GitHub Workflows and GitLab CI to run Git with the new reftable backend.

---
 .github/workflows/main.yml | 9 +++++++++
 .gitlab-ci.yml             | 9 +++++++++
 ci/lib.sh                  | 2 +-
 ci/run-build-and-tests.sh  | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)
.github/workflows/main.yml +9 -0
index 4d97da57ec..1b43e49dad 100644
@@ -266,6 +266,9 @@ jobs:
           - jobname: linux-sha256
             cc: clang
             pool: ubuntu-latest
+          - jobname: linux-reftable
+            cc: clang
+            pool: ubuntu-latest
           - jobname: linux-gcc
             cc: gcc
             cc_package: gcc-8
@@ -277,6 +280,9 @@ jobs:
           - jobname: osx-clang
             cc: clang
             pool: macos-13
+          - jobname: osx-reftable
+            cc: clang
+            pool: macos-13
           - jobname: osx-gcc
             cc: gcc
             cc_package: gcc-13
@@ -287,6 +293,9 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-reftable-leaks
+            cc: gcc
+            pool: ubuntu-latest
           - jobname: linux-asan-ubsan
             cc: clang
             pool: ubuntu-latest
.gitlab-ci.yml +9 -0
index 43bfbd8834..c0fa2fe90b 100644
@@ -26,6 +26,9 @@ test:linux:
       - jobname: linux-sha256
         image: ubuntu:latest
         CC: clang
+      - jobname: linux-reftable
+        image: ubuntu:latest
+        CC: clang
       - jobname: linux-gcc
         image: ubuntu:20.04
         CC: gcc
@@ -40,6 +43,9 @@ test:linux:
       - jobname: linux-leaks
         image: ubuntu:latest
         CC: gcc
+      - jobname: linux-reftable-leaks
+        image: ubuntu:latest
+        CC: gcc
       - jobname: linux-asan-ubsan
         image: ubuntu:latest
         CC: clang
@@ -79,6 +85,9 @@ test:osx:
       - jobname: osx-clang
         image: macos-13-xcode-14
         CC: clang
+      - jobname: osx-reftable
+        image: macos-13-xcode-14
+        CC: clang
   artifacts:
     paths:
       - t/failed-test-artifacts
ci/lib.sh +1 -1
index d5dd2f2697..0a73fc7bd1 100755
@@ -367,7 +367,7 @@ linux-musl)
 	MAKEFLAGS="$MAKEFLAGS NO_REGEX=Yes ICONV_OMITS_BOM=Yes"
 	MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8"
 	;;
-linux-leaks)
+linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	export GIT_TEST_SANITIZE_LEAK_LOG=true
ci/run-build-and-tests.sh +3 -0
index 7a1466b868..c192bd613c 100755
@@ -37,6 +37,9 @@ linux-clang)
 linux-sha256)
 	export GIT_TEST_DEFAULT_HASH=sha256
 	;;
+linux-reftable|linux-reftable-leaks|osx-reftable)
+	export GIT_TEST_DEFAULT_REF_FORMAT=reftable
+	;;
 pedantic)
 	# Don't run the tests; we only care about whether Git can be
 	# built.
Signature · 1 line
2.43.GIT
#6 lore raw
In reply to Patrick Steinhardt
Quoted 3 lines · Patrick Steinhardt

Patrick Steinhardt (2):
refs: introduce reftable backend
ci: add jobs to test with the reftable backend

I haven't checked the code in refs/reftable-backend.c at all, but judging from the fallout to other files, it issurprisingly cleanly done. I somehow expected that we would need more changes like exposing the helper function to compute shared permission bits, but I see that it was the only change needed. Everything else done to other files are natural requirement to plug the new thing as a new ref backend to the system. Very nicely done.

Thanks.

Quoted 21 lines
 .github/workflows/main.yml                    |    9 +
 .gitlab-ci.yml                                |    9 +
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 ci/lib.sh                                     |    2 +-
 ci/run-build-and-tests.sh                     |    3 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2286 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 17 files changed, 3237 insertions(+), 7 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh
#2 lore raw
In reply to Patrick Steinhardt

Hello,

Quoted 20 lines · Patrick Steinhardt

- It becomes possible to do truly atomic writes where either all refs are committed to disk or none are. This was not possible with the "files" backend because ref updates were split across multiple loose files.

- The disk space required to store many refs is reduced, both compared to loose refs and packed-refs. This is enabled both by the reftable format being a binary format, which is more compact, and by prefix compression.

- We can ignore filesystem-specific behaviour as ref names are not encoded via paths anymore. This means there is no need to handle case sensitivity on Windows systems or Unicode precomposition on macOS.

- There is no need to rewrite the complete refdb anymore every time a ref is being deleted like it was the case for packed-refs. This means that ref deletions are now constant time instead of scaling linearly with the number of refs.

Quoted 7 lines

- We can ignore file/directory conflicts so that it becomes possible to store both "refs/heads/foo" and "refs/heads/foo/bar".

- Due to this property we can retain reflogs for deleted refs. We have previously been deleting reflogs together with their refs to avoid file/directory conflicts, which is not necessary anymore.

Nit: Maybe also a point about how with the current files backend doesn't have a good way to differentiate between regular files and refs. While regular refs are in the 'refs/' folder, pseudorefs have no namespace. While with the reftable implementation we know that everything within the reftable is a ref and there are no other refs we need to consider.

Quoted 3 lines

Performance-wise things very much depend on the actual workload. The following benchmarks compare the "files" and "reftable" backends in the current version:

Pretty nice to have these numbers here.

Quoted 24 lines

- Creating N refs in separate transactions shows that the "files" backend is ~50% faster. This is not surprising given that creating a ref only requires us to create a single loose ref. The "reftable" backend will also perform auto compaction on updates. In real-world workloads we would likely also want to perform pack loose refs, which would likely change the picture.

Benchmark 1: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 2.1 ms ± 0.3 ms [User: 0.6 ms, System: 1.7 ms]
Range (min … max): 1.8 ms … 4.3 ms 133 runs

Benchmark 2: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 2.7 ms ± 0.1 ms [User: 0.6 ms, System: 2.2 ms]
Range (min … max): 2.4 ms … 2.9 ms 132 runs

Benchmark 3: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 1.975 s ± 0.006 s [User: 0.437 s, System: 1.535 s]
Range (min … max): 1.969 s … 1.980 s 3 runs

Benchmark 4: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 2.611 s ± 0.013 s [User: 0.782 s, System: 1.825 s]
Range (min … max): 2.597 s … 2.622 s 3 runs

Quoted 8 lines

Benchmark 5: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 198.442 s ± 0.241 s [User: 43.051 s, System: 155.250 s]
Range (min … max): 198.189 s … 198.670 s 3 runs

Benchmark 6: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 294.509 s ± 4.269 s [User: 104.046 s, System: 190.326 s]
Range (min … max): 290.223 s … 298.761 s 3 runs

Nit: The refcount is missing in these benchmarks.

refs/reftable-backend.c line 4

Quoted 10 lines
refs/reftable-backend.c new file +4 -0
new file mode 100644
index 0000000000..895de0b273
@@ -0,0 +1,2286 @@
+#include "../git-compat-util.h"
+#include "../abspath.h"
+#include "../chdir-notify.h"
+#include "../config.h"

I believe this header can be dropped.

Not sure about removing headers which aren't needed. The coding guidelines mentions that we should include headers that declare the functions and the types used. I found some headers here which are not required considering this rule.

Quoted 4 lines
refs/reftable-backend.c new file +4 -0
+#include "../environment.h"
+#include "../gettext.h"
+#include "../hash.h"
+#include "../hex.h"

refs/reftable-backend.c line 18

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+#include "../iterator.h"
+#include "../ident.h"
+#include "../lockfile.h"
+#include "../object.h"
+#include "../path.h"
+#include "../refs.h"
+#include "../reftable/reftable-stack.h"
+#include "../reftable/reftable-record.h"
+#include "../reftable/reftable-error.h"
+#include "../reftable/reftable-blocksource.h"

This header can be dropped too.

refs/reftable-backend.c line 19

Quoted 1 line
refs/reftable-backend.c new file +1 -0
+#include "../reftable/reftable-reader.h"

This also.

refs/reftable-backend.c line 22

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+#include "../reftable/reftable-iterator.h"
+#include "../reftable/reftable-merged.h"
+#include "../reftable/reftable-generic.h"

This one as well.

refs/reftable-backend.c line 25

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+#include "../setup.h"
+#include "../strmap.h"
+#include "../worktree.h"

This one too.

refs/reftable-backend.c line 44

Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+struct reftable_ref_store {
+	struct ref_store base;
+
+	struct reftable_stack *main_stack;
+	struct reftable_stack *worktree_stack;
+	struct strmap worktree_stacks;
+	struct reftable_write_options write_options;
+
+	unsigned int store_flags;
+	int err;
+};

So, I'm assuming that main_stack here would be the primary reference db, even when inside the worktree. I'm wondering why we can't just have

    struct reftable_stack *current_stack;
    struct strmap worktree_stacks;

Reading further on, it becomes necessary to have both, because the user could
1. Request main-worktree/{ref}
2. Request worktrees/{worktree}/{ref}
So it's important that we have access to both the main worktree, the current worktree and also any other worktree.

Maybe we could just drop the struct reftable_stack *worktree_stack and rely on the map entirely, but I guess worktree_stack acts as a caching layer and avoids using the map unless necessary.

refs/reftable-backend.c line 250

Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+static struct ref_store *reftable_be_init(struct repository *repo,
+					  const char *gitdir,
+					  unsigned int store_flags)
+{
+	struct reftable_ref_store *refs = xcalloc(1, sizeof(*refs));
+	struct strbuf path = STRBUF_INIT;
+	int is_worktree;
+	mode_t mask;
+
+	mask = umask(0);
+	umask(mask);

Took me more than a glance to understand this. For other readers who didn't know: there is no single API to get the current umask. But umask(2) sets the umask and returns the prev umask. So we set a random value (here 0) to get the prev umask and reset it.

refs/reftable-backend.c line 255

Quoted 6 lines
refs/reftable-backend.c new file +5 -0
+
+	base_ref_store_init(&refs->base, repo, gitdir, &refs_be_reftable);
+	strmap_init(&refs->worktree_stacks);
+	refs->store_flags = store_flags;
+	refs->write_options.block_size = 4096;

Perhaps use DEFAULT_BLOCK_SIZE from "reftable/constants.h" here.

refs/reftable-backend.c line 267

Quoted 12 lines
refs/reftable-backend.c new file +12 -0
+	refs->write_options.hash_id = repo->hash_algo->format_id;
+	refs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);
+
+	/*
+	 * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.
+	 * This stack contains both the shared and the main worktree refs.
+	 */
+	is_worktree = get_common_dir_noenv(&path, gitdir);
+	if (!is_worktree) {
+		strbuf_reset(&path);
+		strbuf_realpath(&path, gitdir, 0);
+	}

Nit: would be nice to have a comment that for wortrees, gitdir would already be an absolute path.

Quoted 9 lines
refs/reftable-backend.c new file +9 -0
+	strbuf_addstr(&path, "/reftable");
+	refs->err = reftable_new_stack(&refs->main_stack, path.buf,
+				       refs->write_options);
+	if (refs->err)
+		goto done;
+
+	/*
+	 * If we're in a worktree we also need to set up the worktree reftable
+	 * stack that is contained in the per-worktree GIT_DIR.

refs/reftable-backend.c line 286

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+	 */
+	if (is_worktree) {
+		strbuf_reset(&path);
+		strbuf_addf(&path, "%s/reftable", gitdir);
+
+		refs->err = reftable_new_stack(&refs->worktree_stack, path.buf,
+					       refs->write_options);
+		if (refs->err)
+			goto done;
+	}

Wondering if we should also add this to the refs->worktree_stacks.

refs/reftable-backend.c line 335

Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+struct reftable_ref_iterator {
+	struct ref_iterator base;
+	struct reftable_ref_store *refs;
+	struct reftable_iterator iter;
+	struct reftable_ref_record ref;
+	struct object_id oid;
+
+	const char *prefix;
+	unsigned int flags;
+	int err;
+};

So the flags in this structure are for the iterator itself but the flags in base are flags related to the current ref stored in base.

refs/reftable-backend.c line 345

Quoted 9 lines
refs/reftable-backend.c new file +9 -0
+static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct reftable_ref_iterator *iter =
+		(struct reftable_ref_iterator *)ref_iterator;
+	struct reftable_ref_store *refs = iter->refs;
+
+	while (!iter->err) {
+		int flags = 0;
+

Nit: I think the syntax in reftable_reflog_iterator_advance is similar but nicer to read, the usage of while(1) and returning within the while loop is better than breaking here and returning outside, either ways, I think it'd be nicer to make both of them consistent.

refs/reftable-backend.c line 357

Quoted 12 lines
refs/reftable-backend.c new file +12 -0
+		iter->err = reftable_iterator_next_ref(&iter->iter, &iter->ref);
+		if (iter->err)
+			break;
+
+		/*
+		 * The files backend only lists references contained in
+		 * "refs/". We emulate the same behaviour here and thus skip
+		 * all references that don't start with this prefix.
+		 */
+		if (!starts_with(iter->ref.refname, "refs/"))
+			continue;
+

Since my patch series [1] to print all refs is now merged to next, maybe you could add this in?

    diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
    index 895de0b273..3f4f905292 100644
    --- a/refs/reftable-backend.c
    +++ b/refs/reftable-backend.c
    @@ -348,11 +348,10 @@ static int

reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
break;

/*
- * The files backend only lists references contained in
- * "refs/". We emulate the same behaviour here and thus skip
- * all references that don't start with this prefix.
+ * Unless the DO_FOR_EACH_INCLUDE_ALL_REFS flag is use, we only
+ * list references contained in "refs/" to mimic the file-backend.
*/
- if (!starts_with(iter->ref.refname, "refs/"))
+ if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
!starts_with(iter->ref.refname, "refs/"))

     			continue;

     		if (iter->prefix &&
Quoted 25 lines
refs/reftable-backend.c new file +25 -0
+static enum iterator_selection iterator_select(struct ref_iterator *iter_worktree,
+					       struct ref_iterator *iter_common,
+					       void *cb_data UNUSED)
+{
+	if (iter_worktree && !iter_common) {
+		/*
+		 * Return the worktree ref if there are no more common refs.
+		 */
+		return ITER_SELECT_0;
+	} else if (iter_common) {
+		/*
+		 * In case we have pending worktree and common refs we need to
+		 * yield them based on their lexicographical order. Worktree
+		 * refs that have the same name as common refs shadow the
+		 * latter.
+		 */
+		if (iter_worktree) {
+			int cmp = strcmp(iter_worktree->refname,
+					 iter_common->refname);
+			if (cmp < 0)
+				return ITER_SELECT_0;
+			else if (!cmp)
+				return ITER_SELECT_0_SKIP_1;
+		}
+

refs/reftable-backend.c line 525

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+		 /*
+		  * Otherwise, if we either have no worktree refs anymore or if
+		  * the common ref sorts before the next worktree ref, we need
+		  * to figure out whether the next common ref belongs to the
+		  * main worktree. In that case, it should be ignored.
+		  */
+		if (parse_worktree_ref(iter_common->refname, NULL, NULL,
+				       NULL) == REF_WORKTREE_SHARED)
+			return ITER_SELECT_1;
+

I'm not sure I understand this. When would this situation occur?

Quoted 65 lines
refs/reftable-backend.c new file +64 -0
+		return ITER_SKIP_1;
+	} else {
+		return ITER_DONE;
+	}
+}
+
+
+static int reftable_be_transaction_prepare(struct ref_store *ref_store,
+					   struct ref_transaction *transaction,
+					   struct strbuf *err)
+{
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare");
+	struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;
+	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+	struct reftable_transaction_data *tx_data = NULL;
+	struct object_id head_oid;
+	unsigned int head_type = 0;
+	size_t i;
+	int ret;
+
+	ret = refs->err;
+	if (ret < 0)
+		goto done;
+
+	tx_data = xcalloc(1, sizeof(*tx_data));
+
+	/*
+	 * Preprocess all updates. For one we check that there are no duplicate
+	 * reference updates in this transaction. Second, we lock all stacks
+	 * that will be modified during the transaction.
+	 */
+	for (i = 0; i < transaction->nr; i++) {
+		ret = prepare_transaction_update(NULL, refs, tx_data,
+						 transaction->updates[i], err);
+		if (ret)
+			goto done;
+
+		string_list_append(&affected_refnames,
+				   transaction->updates[i]->refname);
+	}
+
+	/*
+	 * Now that we have counted updates per stack we can preallocate their
+	 * arrays. This avoids having to reallocate many times.
+	 */
+	for (i = 0; i < tx_data->args_nr; i++) {
+		CALLOC_ARRAY(tx_data->args[i].updates, tx_data->args[i].updates_expected);
+		tx_data->args[i].updates_alloc = tx_data->args[i].updates_expected;
+	}
+
+	/*
+	 * Fail if a refname appears more than once in the transaction.
+	 * This code is taken from the files backend and is a good candidate to
+	 * be moved into the generic layer.
+	 */
+	string_list_sort(&affected_refnames);
+	if (ref_update_reject_duplicates(&affected_refnames, err)) {
+		ret = TRANSACTION_GENERIC_ERROR;
+		goto done;
+	}
+
+	ret = read_ref_without_reload(stack_for(refs, "HEAD", NULL), "HEAD", &head_oid,
+				      &head_referent, &head_type);

refs/reftable-backend.c line 813

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+	if (ret < 0)
+		goto done;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *u = transaction->updates[i];
+		struct object_id current_oid = {0};
+		struct reftable_stack *stack;
+		const char *rewritten_ref;
+
+		stack = stack_for(refs, u->refname, &rewritten_ref);

Wondering why we didn't just iterate over tx_data.args and args[i].updates since there we already have the required stack.

Update: we don't save rewritten_ref in tx_data, so that's one blocker for this.

refs/reftable-backend.c line 919

Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+
+		if (u->type & REF_ISSYMREF) {
+			const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0,
+								       &current_oid, NULL);
+
+			if (u->flags & REF_NO_DEREF) {
+				/*
+				 * The reftable stack is locked at this point
+				 * already, so it should be safe to call
+				 * `refs_resolve_ref_unsafe()` here.
+				 */

Shouldn't this comment be a few lines before?

Quoted 49 lines
refs/reftable-backend.c new file +49 -0
+
+static int write_transaction_table(struct reftable_writer *writer, void *cb_data)
+{
+	struct write_transaction_table_arg *arg = cb_data;
+	struct reftable_merged_table *mt =
+		reftable_stack_merged_table(arg->stack);
+	uint64_t ts = reftable_stack_next_update_index(arg->stack);
+	struct reftable_log_record *logs = NULL;
+	size_t logs_nr = 0, logs_alloc = 0, i;
+	int ret = 0;
+
+	QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);
+
+	reftable_writer_set_limits(writer, ts, ts);
+
+	for (i = 0; i < arg->updates_nr; i++) {
+		struct reftable_transaction_update *tx_update = &arg->updates[i];
+		struct ref_update *u = tx_update->update;
+
+		/*
+		 * Write a reflog entry when updating a ref to point to
+		 * something new in either of the following cases:
+		 *
+		 * - The reference is about to be deleted. We always want to
+		 *   delete the reflog in that case.
+		 * - REF_FORCE_CREATE_REFLOG is set, asking us to always create
+		 *   the reflog entry.
+		 * - `core.logAllRefUpdates` tells us to create the reflog for
+		 *   the given ref.
+		 */
+		if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) {
+			struct reftable_log_record log = {0};
+			struct reftable_iterator it = {0};
+
+			/*
+			 * When deleting refs we also delete all reflog entries
+			 * with them. While it is not strictly required to
+			 * delete reflogs together with their refs, this
+			 * matches the behaviour of the files backend.
+			 *
+			 * Unfortunately, we have no better way than to delete
+			 * all reflog entries one by one.
+			 */
+			ret = reftable_merged_table_seek_log(mt, &it, u->refname);
+			while (ret == 0) {
+				struct reftable_log_record *tombstone;
+
+				ret = reftable_iterator_next_log(&it, &log);
+				if (ret < 0)

refs/reftable-backend.c line 1107

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+					break;
+				if (ret > 0 || strcmp(log.refname, u->refname)) {
+					ret = 0;
+					break;
+				}
+
+				ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
+				tombstone = &logs[logs_nr++];
+				tombstone->refname = xstrdup(u->refname);
+				tombstone->value_type = REFTABLE_LOG_DELETION,

Why is this a comma? one other such instance in this file.

Quoted 37 lines
refs/reftable-backend.c new file +36 -0
+static int write_copy_table(struct reftable_writer *writer, void *cb_data)
+{
+	struct write_copy_arg *arg = cb_data;
+	uint64_t deletion_ts, creation_ts;
+	struct reftable_merged_table *mt = reftable_stack_merged_table(arg->stack);
+	struct reftable_ref_record old_ref = {0}, refs[2] = {0};
+	struct reftable_log_record old_log = {0}, *logs = NULL;
+	struct reftable_iterator it = {0};
+	struct string_list skip = STRING_LIST_INIT_NODUP;
+	struct strbuf errbuf = STRBUF_INIT;
+	size_t logs_nr = 0, logs_alloc = 0, i;
+	int ret;
+
+	if (reftable_stack_read_ref(arg->stack, arg->oldname, &old_ref)) {
+		ret = error(_("refname %s not found"), arg->oldname);
+		goto done;
+	}
+	if (old_ref.value_type == REFTABLE_REF_SYMREF) {
+		ret = error(_("refname %s is a symbolic ref, copying it is not supported"),
+			    arg->oldname);
+		goto done;
+	}
+
+	/*
+	 * There's nothing to do in case the old and new name are the same, so
+	 * we exit early in that case.
+	 */
+	if (!strcmp(arg->oldname, arg->newname)) {
+		ret = 0;
+		goto done;
+	}
+
+	/*
+	 * Verify that the new refname is available.
+	 */
+	string_list_insert(&skip, arg->oldname);

refs/reftable-backend.c line 1400

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+	ret = refs_verify_refname_available(&arg->refs->base, arg->newname,
+					    NULL, &skip, &errbuf);
+	if (ret < 0) {
+		error("%s", errbuf.buf);
+		goto done;
+	}
+
+	/*
+	 * When deleting the old reference we have to use two update indices:
+	 * one to delete the old ref and its reflog, and once to create the new

s/once/one

Quoted 35 lines
refs/reftable-backend.c new file +35 -0
+	 * ref and its reflog. They need to be staged with two separate indices
+	 * because the new reflog needs to encode both the deletion of the old
+	 * branch and the creation of the new branch, and we cannot do two
+	 * changes to a reflog in a single update.
+	 */
+	deletion_ts = creation_ts = reftable_stack_next_update_index(arg->stack);
+	if (arg->delete_old)
+		creation_ts++;
+	reftable_writer_set_limits(writer, deletion_ts, creation_ts);
+
+	/*
+	 * Add the new reference. If this is a rename then we also delete the
+	 * old reference.
+	 */
+	refs[0] = old_ref;
+	refs[0].refname = (char *)arg->newname;
+	refs[0].update_index = creation_ts;
+	if (arg->delete_old) {
+		refs[1].refname = (char *)arg->oldname;
+		refs[1].value_type = REFTABLE_REF_DELETION;
+		refs[1].update_index = deletion_ts;
+	}
+	ret = reftable_writer_add_refs(writer, refs, arg->delete_old ? 2 : 1);
+	if (ret < 0)
+		goto done;
+
+	/*
+	 * When deleting the old branch we need to create a reflog entry on the
+	 * new branch name that indicates that the old branch has been deleted
+	 * and then recreated. This is a tad weird, but matches what the files
+	 * backend does.
+	 */
+	if (arg->delete_old) {
+		struct strbuf head_referent = STRBUF_INIT;
+		struct object_id head_oid;

refs/reftable-backend.c line 1445

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+		int append_head_reflog;
+		unsigned head_type = 0;
+
+		ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
+		memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
+		fill_reftable_log_record(&logs[logs_nr]);
+		logs[logs_nr].refname = (char *)arg->newname;
+		logs[logs_nr].update_index = deletion_ts;
+		logs[logs_nr].value.update.message =
+			xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);

Question: here and other places in this file, shouldn't we free memory allocated by xstrndup?

Also, why is it block_size / 2 ?

Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftable_ref_store *refs,
+								  struct reftable_stack *stack)
+{
+	struct reftable_reflog_iterator *iter;
+	struct reftable_merged_table *mt;
+	int ret;
+
+	iter = xcalloc(1, sizeof(*iter));
+	base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);
+	iter->refs = refs;
+	iter->base.oid = &iter->oid;

refs/reftable-backend.c line 1718

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+
+	ret = reftable_stack_reload(refs->main_stack);
+	if (ret < 0)
+		goto done;
+
+	mt = reftable_stack_merged_table(stack);
+	ret = reftable_merged_table_seek_log(mt, &iter->iter, "");
+	if (ret < 0)
+		goto done;
+

Keeping it similar to ref_iterator_for_stack, perhaps:

    ret = refs->err;
	if (ret)
		goto done;

	ret = reftable_stack_reload(stack);
	if (ret)
		goto done;

	merged_table = reftable_stack_merged_table(stack);

	ret = reftable_merged_table_seek_ref(merged_table, &iter->iter, prefix);
	if (ret)
		goto done;
Quoted 5 lines
refs/reftable-backend.c new file +5 -0
+static int reftable_be_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+						   const char *refname,
+						   each_reflog_ent_fn fn,
+						   void *cb_data)
+{

refs/reftable-backend.c line 1754

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_READ, "for_each_reflog_ent_reverse");
+	struct reftable_stack *stack = stack_for(refs, refname, &refname);
+	struct reftable_merged_table *mt = NULL;
+	struct reftable_log_record log = {0};
+	struct reftable_iterator it = {0};
+	int ret;
+
+	if (refs->err < 0)
+		return refs->err;

Nit: seems like this function and the one below reftable_be_for_each_reflog_ent, share the same code apart from iteration direction. Wonder if it'd be nicer to extract out the common code from them.

Quoted 17 lines
refs/reftable-backend.c new file +17 -0
+static int reftable_be_reflog_exists(struct ref_store *ref_store,
+				     const char *refname)
+{
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_READ, "reflog_exists");
+	struct reftable_stack *stack = stack_for(refs, refname, &refname);
+	struct reftable_merged_table *mt = reftable_stack_merged_table(stack);
+	struct reftable_log_record log = {0};
+	struct reftable_iterator it = {0};
+	int ret;
+
+	ret = refs->err;
+	if (ret < 0)
+		goto done;
+
+	ret = reftable_stack_reload(stack);
+	if (ret)

refs/reftable-backend.c line 1888

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+		goto done;
+
+	ret = reftable_merged_table_seek_log(mt, &it, refname);
+	if (ret)
+		goto done;
+
+	/*
+	 * Seek the reflog to see whether it contains any reflog entries which
+	 * aren't marked for deletion.
+	 */

Shouldn't we be checking log.value_type in that case?

This email only reviews this file, I'll review the others in follow up email[s].

[1]: https://git.kernel.org/pub/scm/git/git.git/commit/?h=next&id=e7a9234a8b39bd69de12b105731413ab8e62942e

#3 lore raw
Quoted 31 lines · Karthik Nayak

Hello,

Quoted 20 lines · Patrick Steinhardt

- It becomes possible to do truly atomic writes where either all refs are committed to disk or none are. This was not possible with the "files" backend because ref updates were split across multiple loose files.

- The disk space required to store many refs is reduced, both compared to loose refs and packed-refs. This is enabled both by the reftable format being a binary format, which is more compact, and by prefix compression.

- We can ignore filesystem-specific behaviour as ref names are not encoded via paths anymore. This means there is no need to handle case sensitivity on Windows systems or Unicode precomposition on macOS.

- There is no need to rewrite the complete refdb anymore every time a ref is being deleted like it was the case for packed-refs. This means that ref deletions are now constant time instead of scaling linearly with the number of refs.

Quoted 7 lines

- We can ignore file/directory conflicts so that it becomes possible to store both "refs/heads/foo" and "refs/heads/foo/bar".

- Due to this property we can retain reflogs for deleted refs. We have previously been deleting reflogs together with their refs to avoid file/directory conflicts, which is not necessary anymore.

Quoted 5 lines

Nit: Maybe also a point about how with the current files backend doesn't have a good way to differentiate between regular files and refs. While regular refs are in the 'refs/' folder, pseudorefs have no namespace. While with the reftable implementation we know that everything within the reftable is a ref and there are no other refs we need to consider.

True, I'll add that.

Quoted 34 lines
Quoted 3 lines

Performance-wise things very much depend on the actual workload. The following benchmarks compare the "files" and "reftable" backends in the current version:

Pretty nice to have these numbers here.

Quoted 28 lines

- Creating N refs in separate transactions shows that the "files" backend is ~50% faster. This is not surprising given that creating a ref only requires us to create a single loose ref. The "reftable" backend will also perform auto compaction on updates. In real-world workloads we would likely also want to perform pack loose refs, which would likely change the picture.

Benchmark 1: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 2.1 ms ± 0.3 ms [User: 0.6 ms, System: 1.7 ms]
Range (min … max): 1.8 ms … 4.3 ms 133 runs

Benchmark 2: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 2.7 ms ± 0.1 ms [User: 0.6 ms, System: 2.2 ms]
Range (min … max): 2.4 ms … 2.9 ms 132 runs

Benchmark 3: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 1.975 s ± 0.006 s [User: 0.437 s, System: 1.535 s]
Range (min … max): 1.969 s … 1.980 s 3 runs

Benchmark 4: update-ref: create refs sequentially (refformat = reftable)
Time (mean ± σ): 2.611 s ± 0.013 s [User: 0.782 s, System: 1.825 s]
Range (min … max): 2.597 s … 2.622 s 3 runs

Benchmark 5: update-ref: create refs sequentially (refformat = files)
Time (mean ± σ): 198.442 s ± 0.241 s [User: 43.051 s, System: 155.250 s]
Range (min … max): 198.189 s … 198.670 s 3 runs

Quoted 6 lines
Quoted 4 lines
        Benchmark 6: update-ref: create refs sequentially (refformat = reftable)
          Time (mean ± σ):     294.509 s ±  4.269 s    [User: 104.046 s, System: 190.326 s]
          Range (min … max):   290.223 s … 298.761 s    3 runs

Nit: The refcount is missing in these benchmarks.

Good catch, this is an edge case of hyperfine. Because the refcount is used in the command line it's not shown anymore in its output. I'll just massage these manually.

Quoted 13 lines

refs/reftable-backend.c line 4

Quoted 10 lines
refs/reftable-backend.c new file +4 -0
new file mode 100644
index 0000000000..895de0b273
@@ -0,0 +1,2286 @@
+#include "../git-compat-util.h"
+#include "../abspath.h"
+#include "../chdir-notify.h"
+#include "../config.h"

I believe this header can be dropped.

Quoted 4 lines

Not sure about removing headers which aren't needed. The coding guidelines mentions that we should include headers that declare the functions and the types used. I found some headers here which are not required considering this rule.

It's always good hygiene to not include unneeded headers. Also helps to keep build times lower and reduce unnecessary rebuilds.

Quoted 51 lines
Quoted 4 lines
refs/reftable-backend.c new file +4 -0
+#include "../environment.h"
+#include "../gettext.h"
+#include "../hash.h"
+#include "../hex.h"

refs/reftable-backend.c line 18

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+#include "../iterator.h"
+#include "../ident.h"
+#include "../lockfile.h"
+#include "../object.h"
+#include "../path.h"
+#include "../refs.h"
+#include "../reftable/reftable-stack.h"
+#include "../reftable/reftable-record.h"
+#include "../reftable/reftable-error.h"
+#include "../reftable/reftable-blocksource.h"

This header can be dropped too.

refs/reftable-backend.c line 19

Quoted 1 line
refs/reftable-backend.c new file +1 -0
+#include "../reftable/reftable-reader.h"

This also.

refs/reftable-backend.c line 22

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+#include "../reftable/reftable-iterator.h"
+#include "../reftable/reftable-merged.h"
+#include "../reftable/reftable-generic.h"

This one as well.

refs/reftable-backend.c line 25

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+#include "../setup.h"
+#include "../strmap.h"
+#include "../worktree.h"

This one too.

refs/reftable-backend.c line 44

Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+struct reftable_ref_store {
+	struct ref_store base;
+
+	struct reftable_stack *main_stack;
+	struct reftable_stack *worktree_stack;
+	struct strmap worktree_stacks;
+	struct reftable_write_options write_options;
+
+	unsigned int store_flags;
+	int err;
+};

So, I'm assuming that main_stack here would be the primary reference db, even when inside the worktree. I'm wondering why we can't just have

    struct reftable_stack *current_stack;
    struct strmap worktree_stacks;
Quoted 6 lines

Reading further on, it becomes necessary to have both, because the user could
1. Request main-worktree/{ref}
2. Request worktrees/{worktree}/{ref}
So it's important that we have access to both the main worktree, the current worktree and also any other worktree.

Indeed. I'll add some comments to point out their respective uses.

Quoted 3 lines

Maybe we could just drop the struct reftable_stack *worktree_stack and rely on the map entirely, but I guess worktree_stack acts as a caching layer and avoids using the map unless necessary.

I doubt that the map access would be all that expensive in practice. The problem rather is that we don't know about the worktree name when we init the backend, and looking it up feels fragile to me and may involve parsing all worktree metadata. So I rather want to avoid that to the extent possible.

Quoted 7 lines

refs/reftable-backend.c line 255

Quoted 5 lines
refs/reftable-backend.c new file +4 -0
+	base_ref_store_init(&refs->base, repo, gitdir, &refs_be_reftable);
+	strmap_init(&refs->worktree_stacks);
+	refs->store_flags = store_flags;
+	refs->write_options.block_size = 4096;

Perhaps use DEFAULT_BLOCK_SIZE from "reftable/constants.h" here.

This isn't exposed given that only the "reftable/reftable-*.h" headers are considered to be public. Ideally, we'd just leave it unset and let the default block size kick in. But reftable_new_stack() doesn't set up the block size at all in that case, and that leads to failures.

I'll leave this as-is for now. We can clean it up in the future.

Quoted 13 lines
Quoted 13 lines
refs/reftable-backend.c new file +13 -0
+	strbuf_addstr(&path, "/reftable");
+	refs->err = reftable_new_stack(&refs->main_stack, path.buf,
+				       refs->write_options);
+	if (refs->err)
+		goto done;
+
+	/*
+	 * If we're in a worktree we also need to set up the worktree reftable
+	 * stack that is contained in the per-worktree GIT_DIR.
+	 */
+	if (is_worktree) {
+		strbuf_reset(&path);
+		strbuf_addf(&path, "%s/reftable", gitdir);
Quoted 8 lines

refs/reftable-backend.c line 286

Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+
+		refs->err = reftable_new_stack(&refs->worktree_stack, path.buf,
+					       refs->write_options);
+		if (refs->err)
+			goto done;
+	}

Wondering if we should also add this to the refs->worktree_stacks.

Ideally we would, but at this point we have no easy way to figure out the worktree name to the best of my knowledge.

Quoted 6 lines
Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+struct reftable_ref_iterator {
+	struct ref_iterator base;
+	struct reftable_ref_store *refs;
+	struct reftable_iterator iter;
+	struct reftable_ref_record ref;
+	struct object_id oid;
Quoted 8 lines

refs/reftable-backend.c line 335

Quoted 5 lines
refs/reftable-backend.c new file +5 -0
+
+	const char *prefix;
+	unsigned int flags;
+	int err;
+};

So the flags in this structure are for the iterator itself but the flags in base are flags related to the current ref stored in base.

Yup.

Quoted 6 lines
Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
+{
+	struct reftable_ref_iterator *iter =
+		(struct reftable_ref_iterator *)ref_iterator;
+	struct reftable_ref_store *refs = iter->refs;
+
Quoted 8 lines

refs/reftable-backend.c line 345

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+	while (!iter->err) {
+		int flags = 0;
+

Nit: I think the syntax in reftable_reflog_iterator_advance is similar but nicer to read, the usage of while(1) and returning within the while loop is better than breaking here and returning outside, either ways, I think it'd be nicer to make both of them consistent.

The problem here is that the ref iterator has multiple exit paths that need to be treated the same, so it is easier to have a common exit path. I'll thus touch up the reflog iterator to look the same. I think that's the better option anyway so that there is a single exit path, only.

Also shows that we didn't store the error into iter->err in the reflog case. Not a huge issue because callers aren't supposed to continue iterating after they've hit an error anyway. But still preferable to make errors persistent.

Quoted 34 lines

refs/reftable-backend.c line 357

Quoted 12 lines
refs/reftable-backend.c new file +12 -0
+		iter->err = reftable_iterator_next_ref(&iter->iter, &iter->ref);
+		if (iter->err)
+			break;
+
+		/*
+		 * The files backend only lists references contained in
+		 * "refs/". We emulate the same behaviour here and thus skip
+		 * all references that don't start with this prefix.
+		 */
+		if (!starts_with(iter->ref.refname, "refs/"))
+			continue;
+

Since my patch series [1] to print all refs is now merged to next, maybe you could add this in?

    diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
    index 895de0b273..3f4f905292 100644
    --- a/refs/reftable-backend.c
    +++ b/refs/reftable-backend.c
    @@ -348,11 +348,10 @@ static int

reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
break;

/*
- * The files backend only lists references contained in
- * "refs/". We emulate the same behaviour here and thus skip
- * all references that don't start with this prefix.
+ * Unless the DO_FOR_EACH_INCLUDE_ALL_REFS flag is use, we only
+ * list references contained in "refs/" to mimic the file-backend.
*/
- if (!starts_with(iter->ref.refname, "refs/"))
+ if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
!starts_with(iter->ref.refname, "refs/"))

Quoted 3 lines
     			continue;

     		if (iter->prefix &&

I'm still waiting for your patch series to hit next before making it a dependency of this series, but thanks for the patch!

Quoted 29 lines
Quoted 29 lines
refs/reftable-backend.c new file +29 -0
+static enum iterator_selection iterator_select(struct ref_iterator *iter_worktree,
+					       struct ref_iterator *iter_common,
+					       void *cb_data UNUSED)
+{
+	if (iter_worktree && !iter_common) {
+		/*
+		 * Return the worktree ref if there are no more common refs.
+		 */
+		return ITER_SELECT_0;
+	} else if (iter_common) {
+		/*
+		 * In case we have pending worktree and common refs we need to
+		 * yield them based on their lexicographical order. Worktree
+		 * refs that have the same name as common refs shadow the
+		 * latter.
+		 */
+		if (iter_worktree) {
+			int cmp = strcmp(iter_worktree->refname,
+					 iter_common->refname);
+			if (cmp < 0)
+				return ITER_SELECT_0;
+			else if (!cmp)
+				return ITER_SELECT_0_SKIP_1;
+		}
+
+		 /*
+		  * Otherwise, if we either have no worktree refs anymore or if
+		  * the common ref sorts before the next worktree ref, we need
+		  * to figure out whether the next common ref belongs to the
Quoted 8 lines

refs/reftable-backend.c line 525

Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+		  * main worktree. In that case, it should be ignored.
+		  */
+		if (parse_worktree_ref(iter_common->refname, NULL, NULL,
+				       NULL) == REF_WORKTREE_SHARED)
+			return ITER_SELECT_1;
+

I'm not sure I understand this. When would this situation occur?

There are two cases here:

- The worktree iterator is exhausted because there are common refs which sort lexicographically after all worktree refs.

- Both iterators still have refs, and the next lexicographic ref is from the common ref store.

In both case we want to return only those refs which are shared, but we want to skip over all per-worktree refs from the common ref store. This is because the per-worktree refs from the common ref store belong to the main worktree, not to ours.

I'll rework the comments a bit.

Quoted 79 lines
Quoted 65 lines
refs/reftable-backend.c new file +64 -0
+		return ITER_SKIP_1;
+	} else {
+		return ITER_DONE;
+	}
+}
+
+
+static int reftable_be_transaction_prepare(struct ref_store *ref_store,
+					   struct ref_transaction *transaction,
+					   struct strbuf *err)
+{
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare");
+	struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;
+	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
+	struct reftable_transaction_data *tx_data = NULL;
+	struct object_id head_oid;
+	unsigned int head_type = 0;
+	size_t i;
+	int ret;
+
+	ret = refs->err;
+	if (ret < 0)
+		goto done;
+
+	tx_data = xcalloc(1, sizeof(*tx_data));
+
+	/*
+	 * Preprocess all updates. For one we check that there are no duplicate
+	 * reference updates in this transaction. Second, we lock all stacks
+	 * that will be modified during the transaction.
+	 */
+	for (i = 0; i < transaction->nr; i++) {
+		ret = prepare_transaction_update(NULL, refs, tx_data,
+						 transaction->updates[i], err);
+		if (ret)
+			goto done;
+
+		string_list_append(&affected_refnames,
+				   transaction->updates[i]->refname);
+	}
+
+	/*
+	 * Now that we have counted updates per stack we can preallocate their
+	 * arrays. This avoids having to reallocate many times.
+	 */
+	for (i = 0; i < tx_data->args_nr; i++) {
+		CALLOC_ARRAY(tx_data->args[i].updates, tx_data->args[i].updates_expected);
+		tx_data->args[i].updates_alloc = tx_data->args[i].updates_expected;
+	}
+
+	/*
+	 * Fail if a refname appears more than once in the transaction.
+	 * This code is taken from the files backend and is a good candidate to
+	 * be moved into the generic layer.
+	 */
+	string_list_sort(&affected_refnames);
+	if (ref_update_reject_duplicates(&affected_refnames, err)) {
+		ret = TRANSACTION_GENERIC_ERROR;
+		goto done;
+	}
+
+	ret = read_ref_without_reload(stack_for(refs, "HEAD", NULL), "HEAD", &head_oid,
+				      &head_referent, &head_type);

refs/reftable-backend.c line 813

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+	if (ret < 0)
+		goto done;
+
+	for (i = 0; i < transaction->nr; i++) {
+		struct ref_update *u = transaction->updates[i];
+		struct object_id current_oid = {0};
+		struct reftable_stack *stack;
+		const char *rewritten_ref;
+
+		stack = stack_for(refs, u->refname, &rewritten_ref);

Wondering why we didn't just iterate over tx_data.args and args[i].updates since there we already have the required stack.

Quoted 2 lines

Update: we don't save rewritten_ref in tx_data, so that's one blocker for this.

Right, that's the reason. I was trying to do that, but also ran into some other problems caused by the ref splitting we do in the loop.

Quoted 5 lines
Quoted 5 lines
refs/reftable-backend.c new file +5 -0
+
+		if (u->type & REF_ISSYMREF) {
+			const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0,
+								       &current_oid, NULL);
+
Quoted 8 lines

refs/reftable-backend.c line 919

Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+			if (u->flags & REF_NO_DEREF) {
+				/*
+				 * The reftable stack is locked at this point
+				 * already, so it should be safe to call
+				 * `refs_resolve_ref_unsafe()` here.
+				 */

Shouldn't this comment be a few lines before?

Good catch. I moved the refs_resolve_ref_unsafe() call at one point and forgot to also move the comment.

Quoted 52 lines
Quoted 52 lines
refs/reftable-backend.c new file +52 -0
+static int write_transaction_table(struct reftable_writer *writer, void *cb_data)
+{
+	struct write_transaction_table_arg *arg = cb_data;
+	struct reftable_merged_table *mt =
+		reftable_stack_merged_table(arg->stack);
+	uint64_t ts = reftable_stack_next_update_index(arg->stack);
+	struct reftable_log_record *logs = NULL;
+	size_t logs_nr = 0, logs_alloc = 0, i;
+	int ret = 0;
+
+	QSORT(arg->updates, arg->updates_nr, transaction_update_cmp);
+
+	reftable_writer_set_limits(writer, ts, ts);
+
+	for (i = 0; i < arg->updates_nr; i++) {
+		struct reftable_transaction_update *tx_update = &arg->updates[i];
+		struct ref_update *u = tx_update->update;
+
+		/*
+		 * Write a reflog entry when updating a ref to point to
+		 * something new in either of the following cases:
+		 *
+		 * - The reference is about to be deleted. We always want to
+		 *   delete the reflog in that case.
+		 * - REF_FORCE_CREATE_REFLOG is set, asking us to always create
+		 *   the reflog entry.
+		 * - `core.logAllRefUpdates` tells us to create the reflog for
+		 *   the given ref.
+		 */
+		if (u->flags & REF_HAVE_NEW && !(u->type & REF_ISSYMREF) && is_null_oid(&u->new_oid)) {
+			struct reftable_log_record log = {0};
+			struct reftable_iterator it = {0};
+
+			/*
+			 * When deleting refs we also delete all reflog entries
+			 * with them. While it is not strictly required to
+			 * delete reflogs together with their refs, this
+			 * matches the behaviour of the files backend.
+			 *
+			 * Unfortunately, we have no better way than to delete
+			 * all reflog entries one by one.
+			 */
+			ret = reftable_merged_table_seek_log(mt, &it, u->refname);
+			while (ret == 0) {
+				struct reftable_log_record *tombstone;
+
+				ret = reftable_iterator_next_log(&it, &log);
+				if (ret < 0)
+					break;
+				if (ret > 0 || strcmp(log.refname, u->refname)) {
+					ret = 0;
+					break;
Quoted 8 lines

refs/reftable-backend.c line 1107

Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+				}
+
+				ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
+				tombstone = &logs[logs_nr++];
+				tombstone->refname = xstrdup(u->refname);
+				tombstone->value_type = REFTABLE_LOG_DELETION,

Why is this a comma? one other such instance in this file.

Typo :) Funny enough this is fine and doesn't make any difference. The comma operator will simply discard the value of the first statement (which is the assigned value) and instead return the value of the second value. We don't use either though, so no problem.

I've also fixed the second instance.

[snip]

Quoted 14 lines
Quoted 14 lines
refs/reftable-backend.c new file +14 -0
+	/*
+	 * When deleting the old branch we need to create a reflog entry on the
+	 * new branch name that indicates that the old branch has been deleted
+	 * and then recreated. This is a tad weird, but matches what the files
+	 * backend does.
+	 */
+	if (arg->delete_old) {
+		struct strbuf head_referent = STRBUF_INIT;
+		struct object_id head_oid;
+		int append_head_reflog;
+		unsigned head_type = 0;
+
+		ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
+		memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
Quoted 8 lines

refs/reftable-backend.c line 1445

Quoted 5 lines
refs/reftable-backend.c new file +5 -0
+		fill_reftable_log_record(&logs[logs_nr]);
+		logs[logs_nr].refname = (char *)arg->newname;
+		logs[logs_nr].update_index = deletion_ts;
+		logs[logs_nr].value.update.message =
+			xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);

Question: here and other places in this file, shouldn't we free memory allocated by xstrndup?

We use reftable_log_record_release(), which takes care of this.

Quoted 1 line

Also, why is it block_size / 2 ?

That's a very good question indeed. The problem we have here is that the maximum length of a record is limited by the block size. Thus, we cannot write entries into a block that exceed a certain length and we must ensure that no record exceeds that size.

Using block_size / 2 is thus a mechanism to cull the reflog message so that we can at least write a _partial_ reflog without breaking the format. The division by two is kind of arbitrary here and ensures that we have enough headroom to also write the remaining data of the record.

So this could be improved to instead trim at the exact block boundary minus the overhead of the remaining data. But I dobut that's really worth it for now given that nobody will ever write a 2kB reflog entry (famous last words).

Quoted 24 lines
Quoted 11 lines
refs/reftable-backend.c new file +11 -0
+static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftable_ref_store *refs,
+								  struct reftable_stack *stack)
+{
+	struct reftable_reflog_iterator *iter;
+	struct reftable_merged_table *mt;
+	int ret;
+
+	iter = xcalloc(1, sizeof(*iter));
+	base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);
+	iter->refs = refs;
+	iter->base.oid = &iter->oid;

refs/reftable-backend.c line 1718

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+
+	ret = reftable_stack_reload(refs->main_stack);
+	if (ret < 0)
+		goto done;
+
+	mt = reftable_stack_merged_table(stack);
+	ret = reftable_merged_table_seek_log(mt, &iter->iter, "");
+	if (ret < 0)
+		goto done;
+

Keeping it similar to ref_iterator_for_stack, perhaps:

Quoted 13 lines
    ret = refs->err;
	if (ret)
		goto done;

	ret = reftable_stack_reload(stack);
	if (ret)
		goto done;

	merged_table = reftable_stack_merged_table(stack);

	ret = reftable_merged_table_seek_ref(merged_table, &iter->iter, prefix);
	if (ret)
		goto done;

Will do.

Quoted 12 lines
Quoted 12 lines
refs/reftable-backend.c new file +12 -0
+static int reftable_be_for_each_reflog_ent_reverse(struct ref_store *ref_store,
+						   const char *refname,
+						   each_reflog_ent_fn fn,
+						   void *cb_data)
+{
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_READ, "for_each_reflog_ent_reverse");
+	struct reftable_stack *stack = stack_for(refs, refname, &refname);
+	struct reftable_merged_table *mt = NULL;
+	struct reftable_log_record log = {0};
+	struct reftable_iterator it = {0};
+	int ret;
Quoted 8 lines

refs/reftable-backend.c line 577

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+
+	if (refs->err < 0)
+		return refs->err;

Nit: seems like this function and the one below reftable_be_for_each_reflog_ent, share the same code apart from iteration direction. Wonder if it'd be nicer to extract out the common code from them.

Hm. I tried to do this once a while ago, but didn't end up with a nice solution because I basically tried to unify the iterator loops, which didn't quite work out in a nice way. But coming back with fresh eyes we can at least easily share the code that yields the log record to the callback function to remove some code duplication.

Quoted 21 lines
Quoted 21 lines
refs/reftable-backend.c new file +21 -0
+static int reftable_be_reflog_exists(struct ref_store *ref_store,
+				     const char *refname)
+{
+	struct reftable_ref_store *refs =
+		reftable_be_downcast(ref_store, REF_STORE_READ, "reflog_exists");
+	struct reftable_stack *stack = stack_for(refs, refname, &refname);
+	struct reftable_merged_table *mt = reftable_stack_merged_table(stack);
+	struct reftable_log_record log = {0};
+	struct reftable_iterator it = {0};
+	int ret;
+
+	ret = refs->err;
+	if (ret < 0)
+		goto done;
+
+	ret = reftable_stack_reload(stack);
+	if (ret)
+		goto done;
+
+	ret = reftable_merged_table_seek_log(mt, &it, refname);
+	if (ret)
Quoted 8 lines

refs/reftable-backend.c line 1888

Quoted 6 lines
refs/reftable-backend.c new file +6 -0
+		goto done;
+
+	/*
+	 * Seek the reflog to see whether it contains any reflog entries which
+	 * aren't marked for deletion.
+	 */

Shouldn't we be checking log.value_type in that case?

I think this whole loop doesn't make any sense anymore. I initially misunderstood how reflog deletion works [1]: I assumed that a reflog entry with the zero object ID as new value is supposed to mark a reflog as deleted, but that's not the case. Instead, deleting a reflog requires us to mark each record as deleted individually.

Given that the reftable stack is marked to suppress deletions (see reftable_stack_reload_once(), new_merged->suppress_deletions = 1), we don't have to check for deletions at all. All we have to do is seek the log once and check whether we get a log record matching the expected name.

[1]: https://public-inbox.org/git/ZVy0zKcmc8tjmgzs@tanuki/

Quoted 2 lines

This email only reviews this file, I'll review the others in follow up email[s].

Thanks a lot for your review! I know the code here is quite dense, so I really appreciate it.

Patrick

#7 lore raw

v2 cover refs: introduce reftable backend

In reply to Patrick Steinhardt

Hi,

this is the second version of my patch series that introduces the reftable backend.

This version addresses the feedback by Karthik. I don't think it really makes sense to spell out every change here -- the range diff should likely be easier to digest.

Thanks!

Patrick

Patrick Steinhardt (2):
refs: introduce reftable backend
ci: add jobs to test with the reftable backend

 .github/workflows/main.yml                    |    9 +
 .gitlab-ci.yml                                |    9 +
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 ci/lib.sh                                     |    2 +-
 ci/run-build-and-tests.sh                     |    3 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2295 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 17 files changed, 3246 insertions(+), 7 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh

Range-diff against v1:
1: 5598cd1307 ! 1: d6548dcfad refs: introduce reftable backend

    @@ Commit message
             previously been deleting reflogs together with their refs to avoid
             file/directory conflicts, which is not necessary anymore.
     
    +      - We can properly enumerate all refs. With the "files" backend it is
    +        not easily possible to distinguish between refs and non-refs because
    +        they may live side by side in the gitdir.
    +
         Not all of these improvements are realized with the current "reftable"
         backend implementation. At this point, the new backend is supposed to be
         a drop-in replacement for the "files" backend that is used by basically
    @@ Commit message
             workloads we would likely also want to perform pack loose refs,
             which would likely change the picture.
     
    -            Benchmark 1: update-ref: create refs sequentially (refformat = files)
    +            Benchmark 1: update-ref: create refs sequentially (refformat = files, refcount = 1)
                   Time (mean ± σ):       2.1 ms ±   0.3 ms    [User: 0.6 ms, System: 1.7 ms]
                   Range (min … max):     1.8 ms …   4.3 ms    133 runs
     
    -            Benchmark 2: update-ref: create refs sequentially (refformat = reftable)
    +            Benchmark 2: update-ref: create refs sequentially (refformat = reftable, refcount = 1)
                   Time (mean ± σ):       2.7 ms ±   0.1 ms    [User: 0.6 ms, System: 2.2 ms]
                   Range (min … max):     2.4 ms …   2.9 ms    132 runs
     
    -            Benchmark 3: update-ref: create refs sequentially (refformat = files)
    +            Benchmark 3: update-ref: create refs sequentially (refformat = files, refcount = 1000)
                   Time (mean ± σ):      1.975 s ±  0.006 s    [User: 0.437 s, System: 1.535 s]
                   Range (min … max):    1.969 s …  1.980 s    3 runs
     
    -            Benchmark 4: update-ref: create refs sequentially (refformat = reftable)
    +            Benchmark 4: update-ref: create refs sequentially (refformat = reftable, refcount = 1000)
                   Time (mean ± σ):      2.611 s ±  0.013 s    [User: 0.782 s, System: 1.825 s]
                   Range (min … max):    2.597 s …  2.622 s    3 runs
     
    -            Benchmark 5: update-ref: create refs sequentially (refformat = files)
    +            Benchmark 5: update-ref: create refs sequentially (refformat = files, refcount = 100000)
                   Time (mean ± σ):     198.442 s ±  0.241 s    [User: 43.051 s, System: 155.250 s]
                   Range (min … max):   198.189 s … 198.670 s    3 runs
     
    -            Benchmark 6: update-ref: create refs sequentially (refformat = reftable)
    +            Benchmark 6: update-ref: create refs sequentially (refformat = reftable, refcount = 100000)
                   Time (mean ± σ):     294.509 s ±  4.269 s    [User: 104.046 s, System: 190.326 s]
                   Range (min … max):   290.223 s … 298.761 s    3 runs
     
    @@ refs/reftable-backend.c (new)
     +#include "../git-compat-util.h"
     +#include "../abspath.h"
     +#include "../chdir-notify.h"
    -+#include "../config.h"
     +#include "../environment.h"
     +#include "../gettext.h"
     +#include "../hash.h"
    @@ refs/reftable-backend.c (new)
     +#include "../reftable/reftable-stack.h"
     +#include "../reftable/reftable-record.h"
     +#include "../reftable/reftable-error.h"
    -+#include "../reftable/reftable-blocksource.h"
    -+#include "../reftable/reftable-reader.h"
     +#include "../reftable/reftable-iterator.h"
     +#include "../reftable/reftable-merged.h"
    -+#include "../reftable/reftable-generic.h"
     +#include "../setup.h"
     +#include "../strmap.h"
    -+#include "../worktree.h"
     +#include "refs-internal.h"
     +
     +/*
    @@ refs/reftable-backend.c (new)
     +struct reftable_ref_store {
     +	struct ref_store base;
     +
    ++	/*
    ++	 * The main stack refers to the common dir and thus contains common
    ++	 * refs as well as refs of the main repository.
    ++	 */
     +	struct reftable_stack *main_stack;
    ++	/*
    ++	 * The worktree stack refers to the gitdir in case the refdb is opened
    ++	 * via a worktree. It thus contains the per-worktree refs.
    ++	 */
     +	struct reftable_stack *worktree_stack;
    ++	/*
    ++	 * Map of worktree stacks by their respective worktree names. The map
    ++	 * is populated lazily when we try to resolve `worktrees/$worktree` refs.
    ++	 */
     +	struct strmap worktree_stacks;
     +	struct reftable_write_options write_options;
     +
    @@ refs/reftable-backend.c (new)
     +	/*
     +	 * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.
     +	 * This stack contains both the shared and the main worktree refs.
    ++	 *
    ++	 * Note that we don't try to resolve the path in case we have a
    ++	 * worktree because `get_common_dir_noenv()` already does it for us.
     +	 */
     +	is_worktree = get_common_dir_noenv(&path, gitdir);
     +	if (!is_worktree) {
    @@ refs/reftable-backend.c (new)
     +	/*
     +	 * If we're in a worktree we also need to set up the worktree reftable
     +	 * stack that is contained in the per-worktree GIT_DIR.
    ++	 *
    ++	 * Ideally, we would also add the stack to our worktree stack map. But
    ++	 * we have no way to figure out the worktree name here and thus can't
    ++	 * do it efficiently.
     +	 */
     +	if (is_worktree) {
     +		strbuf_reset(&path);
    @@ refs/reftable-backend.c (new)
     +		}
     +
     +		 /*
    -+		  * Otherwise, if we either have no worktree refs anymore or if
    -+		  * the common ref sorts before the next worktree ref, we need
    -+		  * to figure out whether the next common ref belongs to the
    -+		  * main worktree. In that case, it should be ignored.
    ++		  * We now know that the lexicographically-next ref is a common
    ++		  * ref. When the common ref is a shared one we return it.
     +		  */
     +		if (parse_worktree_ref(iter_common->refname, NULL, NULL,
     +				       NULL) == REF_WORKTREE_SHARED)
     +			return ITER_SELECT_1;
     +
    ++		/*
    ++		 * Otherwise, if the common ref is a per-worktree ref we skip
    ++		 * it because it would belong to the main worktree, not ours.
    ++		 */
     +		return ITER_SKIP_1;
     +	} else {
     +		return ITER_DONE;
    @@ refs/reftable-backend.c (new)
     +		}
     +
     +		if (u->type & REF_ISSYMREF) {
    ++			/*
    ++			 * The reftable stack is locked at this point already,
    ++			 * so it is safe to call `refs_resolve_ref_unsafe()`
    ++			 * here without causing races.
    ++			 */
     +			const char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0,
     +								       &current_oid, NULL);
     +
     +			if (u->flags & REF_NO_DEREF) {
    -+				/*
    -+				 * The reftable stack is locked at this point
    -+				 * already, so it should be safe to call
    -+				 * `refs_resolve_ref_unsafe()` here.
    -+				 */
     +				if (u->flags & REF_HAVE_OLD && !resolved) {
     +					strbuf_addf(err, _("cannot lock ref '%s': "
     +						    "error reading reference"), u->refname);
    @@ refs/reftable-backend.c (new)
     +				ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
     +				tombstone = &logs[logs_nr++];
     +				tombstone->refname = xstrdup(u->refname);
    -+				tombstone->value_type = REFTABLE_LOG_DELETION,
    ++				tombstone->value_type = REFTABLE_LOG_DELETION;
     +				tombstone->update_index = log.update_index;
     +			}
     +
    @@ refs/reftable-backend.c (new)
     +
     +	/*
     +	 * When deleting the old reference we have to use two update indices:
    -+	 * one to delete the old ref and its reflog, and once to create the new
    -+	 * ref and its reflog. They need to be staged with two separate indices
    -+	 * because the new reflog needs to encode both the deletion of the old
    -+	 * branch and the creation of the new branch, and we cannot do two
    -+	 * changes to a reflog in a single update.
    ++	 * once to delete the old ref and its reflog, and once to create the
    ++	 * new ref and its reflog. They need to be staged with two separate
    ++	 * indices because the new reflog needs to encode both the deletion of
    ++	 * the old branch and the creation of the new branch, and we cannot do
    ++	 * two changes to a reflog in a single update.
     +	 */
     +	deletion_ts = creation_ts = reftable_stack_next_update_index(arg->stack);
     +	if (arg->delete_old)
    @@ refs/reftable-backend.c (new)
     +	struct reftable_reflog_iterator *iter =
     +		(struct reftable_reflog_iterator *)ref_iterator;
     +
    -+	if (iter->err) {
    -+		ref_iterator_abort(ref_iterator);
    -+		return iter->err;
    -+	}
    -+
    -+	while (1) {
    -+		int flags, ret;
    ++	while (!iter->err) {
    ++		int flags;
     +
    -+		ret = reftable_iterator_next_log(&iter->iter, &iter->log);
    -+		if (ret < 0) {
    -+			ref_iterator_abort(ref_iterator);
    -+			return ITER_ERROR;
    -+		}
    -+		if (ret > 0) {
    -+			if (ref_iterator_abort(ref_iterator) != ITER_DONE)
    -+				return ITER_ERROR;
    -+			return ITER_DONE;
    -+		}
    ++		iter->err = reftable_iterator_next_log(&iter->iter, &iter->log);
    ++		if (iter->err)
    ++			break;
     +
     +		/*
     +		 * We want the refnames that we have reflogs for, so we skip if
    @@ refs/reftable-backend.c (new)
     +		iter->base.refname = iter->log.refname;
     +		iter->base.oid = &iter->oid;
     +		iter->base.flags = flags;
    -+		return ITER_OK;
    ++
    ++		break;
     +	}
    ++
    ++	if (iter->err > 0) {
    ++		if (ref_iterator_abort(ref_iterator) != ITER_DONE)
    ++			return ITER_ERROR;
    ++		return ITER_DONE;
    ++	}
    ++
    ++	if (iter->err < 0) {
    ++		ref_iterator_abort(ref_iterator);
    ++		return ITER_ERROR;
    ++	}
    ++
    ++	return ITER_OK;
     +}
     +
     +static int reftable_reflog_iterator_peel(struct ref_iterator *ref_iterator,
    @@ refs/reftable-backend.c (new)
     +static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftable_ref_store *refs,
     +								  struct reftable_stack *stack)
     +{
    ++	struct reftable_merged_table *merged_table;
     +	struct reftable_reflog_iterator *iter;
    -+	struct reftable_merged_table *mt;
     +	int ret;
     +
     +	iter = xcalloc(1, sizeof(*iter));
    @@ refs/reftable-backend.c (new)
     +	iter->refs = refs;
     +	iter->base.oid = &iter->oid;
     +
    ++	ret = refs->err;
    ++	if (ret)
    ++		goto done;
    ++
     +	ret = reftable_stack_reload(refs->main_stack);
     +	if (ret < 0)
     +		goto done;
     +
    -+	mt = reftable_stack_merged_table(stack);
    -+	ret = reftable_merged_table_seek_log(mt, &iter->iter, "");
    ++	merged_table = reftable_stack_merged_table(stack);
    ++
    ++	ret = reftable_merged_table_seek_log(merged_table, &iter->iter, "");
     +	if (ret < 0)
     +		goto done;
     +
    @@ refs/reftable-backend.c (new)
     +					iterator_select, NULL);
     +}
     +
    ++static int yield_log_record(struct reftable_log_record *log,
    ++			    each_reflog_ent_fn fn,
    ++			    void *cb_data)
    ++{
    ++	struct object_id old_oid, new_oid;
    ++	const char *full_committer;
    ++
    ++	oidread(&old_oid, log->value.update.old_hash);
    ++	oidread(&new_oid, log->value.update.new_hash);
    ++
    ++	/*
    ++	 * When both the old object ID and the new object ID are null
    ++	 * then this is the reflog existence marker. The caller must
    ++	 * not be aware of it.
    ++	 */
    ++	if (is_null_oid(&old_oid) && is_null_oid(&new_oid))
    ++		return 0;
    ++
    ++	full_committer = fmt_ident(log->value.update.name, log->value.update.email,
    ++				   WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);
    ++	return fn(&old_oid, &new_oid, full_committer,
    ++		  log->value.update.time, log->value.update.tz_offset,
    ++		  log->value.update.message, cb_data);
    ++}
    ++
     +static int reftable_be_for_each_reflog_ent_reverse(struct ref_store *ref_store,
     +						   const char *refname,
     +						   each_reflog_ent_fn fn,
    @@ refs/reftable-backend.c (new)
     +	mt = reftable_stack_merged_table(stack);
     +	ret = reftable_merged_table_seek_log(mt, &it, refname);
     +	while (!ret) {
    -+		struct object_id old_oid, new_oid;
    -+		const char *full_committer;
    -+
     +		ret = reftable_iterator_next_log(&it, &log);
     +		if (ret < 0)
     +			break;
    @@ refs/reftable-backend.c (new)
     +			break;
     +		}
     +
    -+		oidread(&old_oid, log.value.update.old_hash);
    -+		oidread(&new_oid, log.value.update.new_hash);
    -+
    -+		/*
    -+		 * When both the old object ID and the new object ID are null
    -+		 * then this is the reflog existence marker. The caller must
    -+		 * not be aware of it.
    -+		 */
    -+		if (is_null_oid(&old_oid) && is_null_oid(&new_oid))
    -+			continue;
    -+
    -+		full_committer = fmt_ident(log.value.update.name, log.value.update.email,
    -+					   WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);
    -+		ret = fn(&old_oid, &new_oid, full_committer,
    -+			 log.value.update.time, log.value.update.tz_offset,
    -+			 log.value.update.message, cb_data);
    ++		ret = yield_log_record(&log, fn, cb_data);
     +		if (ret)
     +			break;
     +	}
    @@ refs/reftable-backend.c (new)
     +
     +		ret = reftable_iterator_next_log(&it, &log);
     +		if (ret < 0)
    -+			break;
    ++			goto done;
     +		if (ret > 0 || strcmp(log.refname, refname)) {
     +			reftable_log_record_release(&log);
     +			ret = 0;
    @@ refs/reftable-backend.c (new)
     +	}
     +
     +	for (i = logs_nr; i--;) {
    -+		struct reftable_log_record *log = &logs[i];
    -+		struct object_id old_oid, new_oid;
    -+		const char *full_committer = "";
    -+
    -+		oidread(&old_oid, log->value.update.old_hash);
    -+		oidread(&new_oid, log->value.update.new_hash);
    -+
    -+		/*
    -+		 * When both the old object ID and the new object ID are null
    -+		 * then this is the reflog existence marker. The caller must
    -+		 * not be aware of it.
    -+		 */
    -+		if (is_null_oid(&old_oid) && is_null_oid(&new_oid))
    -+			continue;
    -+
    -+		full_committer = fmt_ident(log->value.update.name, log->value.update.email,
    -+					   WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);
    -+		ret = fn(&old_oid, &new_oid, full_committer,
    -+			 log->value.update.time, log->value.update.tz_offset,
    -+			 log->value.update.message, cb_data);
    ++		ret = yield_log_record(&logs[i], fn, cb_data);
     +		if (ret)
    -+			break;
    ++			goto done;
     +	}
     +
    ++done:
     +	reftable_iterator_destroy(&it);
     +	for (i = 0; i < logs_nr; i++)
     +		reftable_log_record_release(&logs[i]);
    @@ refs/reftable-backend.c (new)
     +		goto done;
     +
     +	ret = reftable_stack_reload(stack);
    -+	if (ret)
    ++	if (ret < 0)
     +		goto done;
     +
     +	ret = reftable_merged_table_seek_log(mt, &it, refname);
    -+	if (ret)
    ++	if (ret < 0)
     +		goto done;
     +
     +	/*
    -+	 * Seek the reflog to see whether it contains any reflog entries which
    -+	 * aren't marked for deletion.
    ++	 * Check whether we get at least one log record for the given ref name.
    ++	 * If so, the reflog exists, otherwise it doesn't.
     +	 */
    -+	while (1) {
    -+		ret = reftable_iterator_next_log(&it, &log);
    -+		if (ret < 0)
    -+			goto done;
    -+		if (ret > 0 || strcmp(log.refname, refname)) {
    -+			ret = 0;
    -+			goto done;
    -+		}
    -+
    -+		ret = 1;
    -+		break;
    ++	ret = reftable_iterator_next_log(&it, &log);
    ++	if (ret < 0)
    ++		goto done;
    ++	if (ret > 0) {
    ++		ret = 0;
    ++		goto done;
     +	}
     +
    ++	ret = strcmp(log.refname, refname) == 0;
    ++
     +done:
     +	reftable_iterator_destroy(&it);
     +	reftable_log_record_release(&log);
    @@ refs/reftable-backend.c (new)
     +			break;
     +		}
     +
    -+		tombstone.refname = (char *)arg->refname,
    -+		tombstone.value_type = REFTABLE_LOG_DELETION,
    ++		tombstone.refname = (char *)arg->refname;
    ++		tombstone.value_type = REFTABLE_LOG_DELETION;
     +		tombstone.update_index = log.update_index;
     +
     +		ret = reftable_writer_add_log(writer, &tombstone);

2: bb694fa132 = 2: 63eafc9f5b ci: add jobs to test with the reftable backend

Signature · 1 line
2.43.GIT
#8 lore raw

PATCH v2 1/2 refs: introduce reftable backend

Due to scalability issues, Shawn Pearce has originally proposed a new "reftable" format more than six years ago [1]. Initially, this new format was implemented in JGit with promising results. Around two years ago, we have then added the "reftable" library to the Git codebase via a4bbd13be3 (Merge branch 'hn/reftable', 2021-12-15). With this we have landed all the low-level code to read and write reftables. Notably missing though was the integration of this low-level code into the Git code base in the form of a new ref backend that ties all of this together.

This gap is now finally closed by introducing a new "reftable" backend into the Git codebase. This new backend promises to bring some notable improvements to Git repositories:

- It becomes possible to do truly atomic writes where either all refs are committed to disk or none are. This was not possible with the "files" backend because ref updates were split across multiple loose files.

- The disk space required to store many refs is reduced, both compared to loose refs and packed-refs. This is enabled both by the reftable format being a binary format, which is more compact, and by prefix compression.

- We can ignore filesystem-specific behaviour as ref names are not encoded via paths anymore. This means there is no need to handle case sensitivity on Windows systems or Unicode precomposition on macOS.

- There is no need to rewrite the complete refdb anymore every time a ref is being deleted like it was the case for packed-refs. This means that ref deletions are now constant time instead of scaling linearly with the number of refs.

- We can ignore file/directory conflicts so that it becomes possible to store both "refs/heads/foo" and "refs/heads/foo/bar".

- Due to this property we can retain reflogs for deleted refs. We have previously been deleting reflogs together with their refs to avoid file/directory conflicts, which is not necessary anymore.

- We can properly enumerate all refs. With the "files" backend it is not easily possible to distinguish between refs and non-refs because they may live side by side in the gitdir.

Not all of these improvements are realized with the current "reftable" backend implementation. At this point, the new backend is supposed to be a drop-in replacement for the "files" backend that is used by basically all Git repositories nowadays. It strives for 1:1 compatibility, which means that a user can expect the same behaviour regardless of whether they use the "reftable" backend or the "files" backend for most of the part.

Most notably, this means we artificially limit the capabilities of the "reftable" backend to match the limits of the "files" backend. It is not possible to create refs that would end up with file/directory conflicts, we do not retain reflogs, we perform stricter-than-necessary checks. This is done intentionally due to two main reasons:

- It makes it significantly easier to land the "reftable" backend as tests behave the same. It would be tough to argue for each and every single test that doesn't pass with the "reftable" backend.

- It ensures compatibility between repositories that use the "files" backend and repositories that use the "reftable" backend. Like this, hosters can migrate their repositories to use the "reftable" backend without causing issues for clients that use the "files" backend in their clones.

It is expected that these artificial limitations may eventually go away in the long term.

Performance-wise things very much depend on the actual workload. The following benchmarks compare the "files" and "reftable" backends in the current version:

- Creating N refs in separate transactions shows that the "files" backend is ~50% faster. This is not surprising given that creating a ref only requires us to create a single loose ref. The "reftable" backend will also perform auto compaction on updates. In real-world workloads we would likely also want to perform pack loose refs, which would likely change the picture.

Benchmark 1: update-ref: create refs sequentially (refformat = files, refcount = 1)
Time (mean ± σ): 2.1 ms ± 0.3 ms [User: 0.6 ms, System: 1.7 ms]
Range (min … max): 1.8 ms … 4.3 ms 133 runs

Benchmark 2: update-ref: create refs sequentially (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.7 ms ± 0.1 ms [User: 0.6 ms, System: 2.2 ms]
Range (min … max): 2.4 ms … 2.9 ms 132 runs

Benchmark 3: update-ref: create refs sequentially (refformat = files, refcount = 1000)
Time (mean ± σ): 1.975 s ± 0.006 s [User: 0.437 s, System: 1.535 s]
Range (min … max): 1.969 s … 1.980 s 3 runs

Benchmark 4: update-ref: create refs sequentially (refformat = reftable, refcount = 1000)
Time (mean ± σ): 2.611 s ± 0.013 s [User: 0.782 s, System: 1.825 s]
Range (min … max): 2.597 s … 2.622 s 3 runs

Benchmark 5: update-ref: create refs sequentially (refformat = files, refcount = 100000)
Time (mean ± σ): 198.442 s ± 0.241 s [User: 43.051 s, System: 155.250 s]
Range (min … max): 198.189 s … 198.670 s 3 runs

Benchmark 6: update-ref: create refs sequentially (refformat = reftable, refcount = 100000)
Time (mean ± σ): 294.509 s ± 4.269 s [User: 104.046 s, System: 190.326 s]
Range (min … max): 290.223 s … 298.761 s 3 runs

- Creating N refs in a single transaction shows that the "files" backend is significantly slower once we start to write many refs. The "reftable" backend only needs to update two files, whereas the "files" backend needs to write one file per ref.

Benchmark 1: update-ref: create many refs (refformat = files, refcount = 1)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.4 ms, System: 1.4 ms]
Range (min … max): 1.8 ms … 2.6 ms 151 runs

Benchmark 2: update-ref: create many refs (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.5 ms ± 0.1 ms [User: 0.7 ms, System: 1.7 ms]
Range (min … max): 2.4 ms … 3.4 ms 148 runs

Benchmark 3: update-ref: create many refs (refformat = files, refcount = 1000)
Time (mean ± σ): 152.5 ms ± 5.2 ms [User: 19.1 ms, System: 133.1 ms]
Range (min … max): 148.5 ms … 167.8 ms 15 runs

Benchmark 4: update-ref: create many refs (refformat = reftable, refcount = 1000)
Time (mean ± σ): 58.0 ms ± 2.5 ms [User: 28.4 ms, System: 29.4 ms]
Range (min … max): 56.3 ms … 72.9 ms 40 runs

Benchmark 5: update-ref: create many refs (refformat = files, refcount = 1000000)
Time (mean ± σ): 152.752 s ± 0.710 s [User: 20.315 s, System: 131.310 s]
Range (min … max): 152.165 s … 153.542 s 3 runs

Benchmark 6: update-ref: create many refs (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 51.912 s ± 0.127 s [User: 26.483 s, System: 25.424 s]
Range (min … max): 51.769 s … 52.012 s 3 runs

- Deleting a ref in a fully-packed repository shows that the "files" backend scales with the number of refs. The "reftable" backend has constant-time deletions.

Benchmark 1: update-ref: delete ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.7 ms ± 0.1 ms [User: 0.4 ms, System: 1.2 ms]
Range (min … max): 1.6 ms … 2.1 ms 316 runs

Benchmark 2: update-ref: delete ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.8 ms ± 0.1 ms [User: 0.4 ms, System: 1.3 ms]
Range (min … max): 1.7 ms … 2.1 ms 294 runs

Benchmark 3: update-ref: delete ref (refformat = files, refcount = 1000)
Time (mean ± σ): 2.0 ms ± 0.1 ms [User: 0.5 ms, System: 1.4 ms]
Range (min … max): 1.9 ms … 2.5 ms 287 runs

Benchmark 4: update-ref: delete ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.5 ms, System: 1.3 ms]
Range (min … max): 1.8 ms … 2.1 ms 217 runs

Benchmark 5: update-ref: delete ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 229.8 ms ± 7.9 ms [User: 182.6 ms, System: 46.8 ms]
Range (min … max): 224.6 ms … 245.2 ms 6 runs

Benchmark 6: update-ref: delete ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 2.0 ms ± 0.0 ms [User: 0.6 ms, System: 1.3 ms]
Range (min … max): 2.0 ms … 2.1 ms 3 runs

- Listing all refs shows no significant advantage for either of the backends. The "files" backend is a bit faster, but not by a significant margin. When repositories are not packed the "reftable" backend outperforms the "files" backend because the "reftable" backend performs auto-compaction.

Benchmark 1: show-ref: print all refs (refformat = files, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1729 runs

Benchmark 2: show-ref: print all refs (refformat = reftable, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.8 ms 1816 runs

Benchmark 3: show-ref: print all refs (refformat = files, refcount = 1000, packed = true)
Time (mean ± σ): 4.3 ms ± 0.1 ms [User: 0.9 ms, System: 3.3 ms]
Range (min … max): 4.1 ms … 4.6 ms 645 runs

Benchmark 4: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = true)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.0 ms, System: 3.3 ms]
Range (min … max): 4.2 ms … 5.9 ms 643 runs

Benchmark 5: show-ref: print all refs (refformat = files, refcount = 1000000, packed = true)
Time (mean ± σ): 2.537 s ± 0.034 s [User: 0.488 s, System: 2.048 s]
Range (min … max): 2.511 s … 2.627 s 10 runs

Benchmark 6: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = true)
Time (mean ± σ): 2.712 s ± 0.017 s [User: 0.653 s, System: 2.059 s]
Range (min … max): 2.692 s … 2.752 s 10 runs

Benchmark 7: show-ref: print all refs (refformat = files, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.9 ms 1834 runs

Benchmark 8: show-ref: print all refs (refformat = reftable, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.0 ms 1840 runs

Benchmark 9: show-ref: print all refs (refformat = files, refcount = 1000, packed = false)
Time (mean ± σ): 13.8 ms ± 0.2 ms [User: 2.8 ms, System: 10.8 ms]
Range (min … max): 13.3 ms … 14.5 ms 208 runs

Benchmark 10: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = false)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.2 ms, System: 3.3 ms]
Range (min … max): 4.3 ms … 6.2 ms 624 runs

Benchmark 11: show-ref: print all refs (refformat = files, refcount = 1000000, packed = false)
Time (mean ± σ): 12.127 s ± 0.129 s [User: 2.675 s, System: 9.451 s]
Range (min … max): 11.965 s … 12.370 s 10 runs

Benchmark 12: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = false)
Time (mean ± σ): 2.799 s ± 0.022 s [User: 0.735 s, System: 2.063 s]
Range (min … max): 2.769 s … 2.836 s 10 runs

- Printing a single ref shows no real difference between the "files" and "reftable" backends.

Benchmark 1: show-ref: print single ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.4 ms, System: 1.0 ms]
Range (min … max): 1.4 ms … 1.8 ms 1779 runs

Benchmark 2: show-ref: print single ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.5 ms 1753 runs

Benchmark 3: show-ref: print single ref (refformat = files, refcount = 1000)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.3 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 1.9 ms 1840 runs

Benchmark 4: show-ref: print single ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1831 runs

Benchmark 5: show-ref: print single ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1848 runs

Benchmark 6: show-ref: print single ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1762 runs

So overall, performance depends on the usecases. Except for many sequential writes the "reftable" backend is roughly on par or significantly faster than the "files" backend though. Given that the "files" backend has received 18 years of optimizations by now this can be seen as a win. Furthermore, we can expect that the "reftable" backend will grow faster over time when attention turns more towards optimizations.

The complete test suite passes, except for those tests explicitly marked to require the REFFILES prerequisite. Some tests in t0610 are marked as failing because they depend on still-in-flight bug fixes. Tests can be run with the new backend by setting the GIT_TEST_DEFAULT_REF_FORMAT environment variable to "reftable".

There is a single known conceptual incompatibility with the dumb HTTP transport. As "info/refs" SHOULD NOT contain the HEAD reference, and because the "HEAD" file is not valid anymore, it is impossible for the remote client to figure out the default branch without changing the protocol. This shortcoming needs to be handled in a subsequent patch series.

As the reftable library has already been introduced a while ago, this commit message will not go into the details of how exactly the on-disk format works. Please refer to our preexisting technical documentation at Documentation/technical/reftable for this.

[1]: https://public-inbox.org/git/CAJo=hJtyof=HRy=2sLP0ng0uZ4=S-DpZ5dR1aF+VHVETKG20OQ@mail.gmail.com/

---
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2295 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 13 files changed, 3224 insertions(+), 6 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh
Documentation/ref-storage-format.txt +2 -0
index 1a65cac468..14fff8a9c6 100644
@@ -1 +1,3 @@
 * `files` for loose files with packed-refs. This is the default.
+* `reftable` for the reftable format. This format is experimental and its
+  internals are subject to change.
Documentation/technical/repository-version.txt +3 -2
index 27be3741e6..47281420fc 100644
@@ -103,5 +103,6 @@ GIT_COMMON_DIR/worktrees/<id>/config.worktree)
 
 ==== `refStorage`
 
-Specifies the file format for the ref database. The only valid value
-is `files` (loose references with a packed-refs file).
+Specifies the file format for the ref database. The valid values are
+`files` (loose references with a packed-refs file) and `reftable` (see
+Documentation/technical/reftable.txt).
Makefile +1 -0
index 0f748a52e6..b302516173 100644
@@ -1127,6 +1127,7 @@ LIB_OBJS += reflog.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
+LIB_OBJS += refs/reftable-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
contrib/workdir/git-new-workdir +1 -1
index 888c34a521..989197aace 100755
@@ -79,7 +79,7 @@ trap cleanup $siglist
 # create the links to the original repo.  explicitly exclude index, HEAD and
 # logs/HEAD from the list since they are purely related to the current working
 # directory, and should not be shared.
-for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn
+for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn reftable
 do
 	# create a containing directory if needed
 	case $x in
path.c +1 -1
index 0fb527918b..8bb223c92c 100644
@@ -871,7 +871,7 @@ const char *enter_repo(const char *path, int strict)
 	return NULL;
 }
 
-static int calc_shared_perm(int mode)
+int calc_shared_perm(int mode)
 {
 	int tweak;
 
path.h +1 -0
index b3233c51fa..e053effef2 100644
@@ -181,6 +181,7 @@ const char *git_path_shallow(struct repository *r);
 int ends_with_path_components(const char *path, const char *components);
 int validate_headref(const char *ref);
 
+int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
refs.c +1 -0
index c633abf284..fff343c256 100644
@@ -35,6 +35,7 @@
  */
 static const struct ref_storage_be *refs_backends[] = {
 	[REF_STORAGE_FORMAT_FILES] = &refs_be_files,
+	[REF_STORAGE_FORMAT_REFTABLE] = &refs_be_reftable,
 };
 
 static const struct ref_storage_be *find_ref_storage_backend(unsigned int ref_storage_format)
refs/refs-internal.h +1 -0
index 82219829b0..83e0f0bba3 100644
@@ -693,6 +693,7 @@ struct ref_storage_be {
 };
 
 extern struct ref_storage_be refs_be_files;
+extern struct ref_storage_be refs_be_reftable;
 extern struct ref_storage_be refs_be_packed;
 
 /*
refs/reftable-backend.c new file +2295 -0

2,295 lines not on this page · raw message

repository.h +3 -2
index 7a250a6605..1645cef518 100644
@@ -24,8 +24,9 @@ enum fetch_negotiation_setting {
 	FETCH_NEGOTIATION_NOOP,
 };
 
-#define REF_STORAGE_FORMAT_UNKNOWN 0
-#define REF_STORAGE_FORMAT_FILES   1
+#define REF_STORAGE_FORMAT_UNKNOWN  0
+#define REF_STORAGE_FORMAT_FILES    1
+#define REF_STORAGE_FORMAT_REFTABLE 2
 
 struct repo_settings {
 	int initialized;
t/t0610-reftable-basics.sh new file +887 -0

639 lines not on this page · raw message

t/t0611-reftable-httpd.sh new file +26 -0
new file mode 100755
index 0000000000..5e05b9c1f2

Lines of this file are not shown. View the raw message.

t/test-lib.sh +2 -0
index fc93aa57e6..0fb8757b7d 100644

Lines of this file are not shown. View the raw message.

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.43.GIT

Part of the diff is not shown here. View the raw message.

#9 lore raw

PATCH v2 2/2 ci: add jobs to test with the reftable backend

In reply to Patrick Steinhardt

Add CI jobs for both GitHub Workflows and GitLab CI to run Git with the new reftable backend.

---
 .github/workflows/main.yml | 9 +++++++++
 .gitlab-ci.yml             | 9 +++++++++
 ci/lib.sh                  | 2 +-
 ci/run-build-and-tests.sh  | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)
.github/workflows/main.yml +9 -0
index 4d97da57ec..1b43e49dad 100644
@@ -266,6 +266,9 @@ jobs:
           - jobname: linux-sha256
             cc: clang
             pool: ubuntu-latest
+          - jobname: linux-reftable
+            cc: clang
+            pool: ubuntu-latest
           - jobname: linux-gcc
             cc: gcc
             cc_package: gcc-8
@@ -277,6 +280,9 @@ jobs:
           - jobname: osx-clang
             cc: clang
             pool: macos-13
+          - jobname: osx-reftable
+            cc: clang
+            pool: macos-13
           - jobname: osx-gcc
             cc: gcc
             cc_package: gcc-13
@@ -287,6 +293,9 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-reftable-leaks
+            cc: gcc
+            pool: ubuntu-latest
           - jobname: linux-asan-ubsan
             cc: clang
             pool: ubuntu-latest
.gitlab-ci.yml +9 -0
index 43bfbd8834..c0fa2fe90b 100644
@@ -26,6 +26,9 @@ test:linux:
       - jobname: linux-sha256
         image: ubuntu:latest
         CC: clang
+      - jobname: linux-reftable
+        image: ubuntu:latest
+        CC: clang
       - jobname: linux-gcc
         image: ubuntu:20.04
         CC: gcc
@@ -40,6 +43,9 @@ test:linux:
       - jobname: linux-leaks
         image: ubuntu:latest
         CC: gcc
+      - jobname: linux-reftable-leaks
+        image: ubuntu:latest
+        CC: gcc
       - jobname: linux-asan-ubsan
         image: ubuntu:latest
         CC: clang
@@ -79,6 +85,9 @@ test:osx:
       - jobname: osx-clang
         image: macos-13-xcode-14
         CC: clang
+      - jobname: osx-reftable
+        image: macos-13-xcode-14
+        CC: clang
   artifacts:
     paths:
       - t/failed-test-artifacts
ci/lib.sh +1 -1
index d5dd2f2697..0a73fc7bd1 100755
@@ -367,7 +367,7 @@ linux-musl)
 	MAKEFLAGS="$MAKEFLAGS NO_REGEX=Yes ICONV_OMITS_BOM=Yes"
 	MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8"
 	;;
-linux-leaks)
+linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	export GIT_TEST_SANITIZE_LEAK_LOG=true
ci/run-build-and-tests.sh +3 -0
index 7a1466b868..c192bd613c 100755
@@ -37,6 +37,9 @@ linux-clang)
 linux-sha256)
 	export GIT_TEST_DEFAULT_HASH=sha256
 	;;
+linux-reftable|linux-reftable-leaks|osx-reftable)
+	export GIT_TEST_DEFAULT_REF_FORMAT=reftable
+	;;
 pedantic)
 	# Don't run the tests; we only care about whether Git can be
 	# built.
Signature · 1 line
2.43.GIT
#4 lore raw
In reply to Patrick Steinhardt
Quoted 37 lines · Patrick Steinhardt
Quoted 36 lines · Karthik Nayak

refs/reftable-backend.c line 357

Quoted 12 lines
refs/reftable-backend.c new file +12 -0
+		iter->err = reftable_iterator_next_ref(&iter->iter, &iter->ref);
+		if (iter->err)
+			break;
+
+		/*
+		 * The files backend only lists references contained in
+		 * "refs/". We emulate the same behaviour here and thus skip
+		 * all references that don't start with this prefix.
+		 */
+		if (!starts_with(iter->ref.refname, "refs/"))
+			continue;
+

Since my patch series [1] to print all refs is now merged to next, maybe you could add this in?

    diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
    index 895de0b273..3f4f905292 100644
    --- a/refs/reftable-backend.c
    +++ b/refs/reftable-backend.c
    @@ -348,11 +348,10 @@ static int

reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
break;

/*
- * The files backend only lists references contained in
- * "refs/". We emulate the same behaviour here and thus skip
- * all references that don't start with this prefix.
+ * Unless the DO_FOR_EACH_INCLUDE_ALL_REFS flag is use, we only
+ * list references contained in "refs/" to mimic the file-backend.
*/
- if (!starts_with(iter->ref.refname, "refs/"))
+ if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
!starts_with(iter->ref.refname, "refs/"))

     			continue;
Quoted 4 lines
Quoted 1 line
     		if (iter->prefix &&

I'm still waiting for your patch series to hit next before making it a dependency of this series, but thanks for the patch!

Oops, didn't read this carefully enough and missed that it already is in next. I'll adapt accordingly in the next version, thanks!

Patrick

#10 lore raw
In reply to Patrick Steinhardt

Hello,

Quoted 5 lines · Patrick Steinhardt

Hi,

this is the second version of my patch series that introduces the reftable backend.

The diff looks good to, apart from the tests (which I didn't get around to reviewing), everything else looks great!

Thanks,
Karthik

#11 lore raw
In reply to Patrick Steinhardt
Quoted 6 lines · Patrick Steinhardt

this is the second version of my patch series that introduces the reftable backend.

This version addresses the feedback by Karthik. I don't think it really makes sense to spell out every change here -- the range diff should likely be easier to digest.

This, when merged to 'seen' (which also has "for-each-ref now thinks an empty string is a signal to show ref-like things outside the refs/ hierarchy" topic, kn/for-all-refs), seems to break *-reftable CI tests, cf. https://github.com/git/git/actions/runs/7765401528

I'll tentatively eject the topic from 'seen', even though I have a suspicion that it byitself would pass all the tests.

Thanks.

#12 lore raw
Quoted 14 lines · Junio C Hamano
Quoted 6 lines · Patrick Steinhardt

this is the second version of my patch series that introduces the reftable backend.

This version addresses the feedback by Karthik. I don't think it really makes sense to spell out every change here -- the range diff should likely be easier to digest.

This, when merged to 'seen' (which also has "for-each-ref now thinks an empty string is a signal to show ref-like things outside the refs/ hierarchy" topic, kn/for-all-refs), seems to break *-reftable CI tests, cf. https://github.com/git/git/actions/runs/7765401528

Quoted 4 lines

I'll tentatively eject the topic from 'seen', even though I have a suspicion that it byitself would pass all the tests.

Thanks.

Yup, this is known due to the semantic conflict with kn/for-all-refs, as you point out. Karthik already sent a single-line change in this thread that fixes the issue. I'll rebase the patch series tomorrow and pull in the topic as a dependency and adapt accordingly.

Thanks!

Patrick

#13 lore raw

v3 cover refs: introduce reftable backend

In reply to Patrick Steinhardt

Hi,

this is the third version of my patch series that introduces the reftable backend.

The only real change in this version is that I've pulled in kn/for-all-refs at 693e807311 (for-each-ref: avoid filtering on empty pattern, 2024-01-29) as a dependency. This series introduces a new DO_FOR_EACH_INCLUDE_ALL_REFS flag that we need to handle in the reftable backend. With this change all tests should pass again.

Patrick

Patrick Steinhardt (2):
refs: introduce reftable backend
ci: add jobs to test with the reftable backend

 .github/workflows/main.yml                    |    9 +
 .gitlab-ci.yml                                |    9 +
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 ci/lib.sh                                     |    2 +-
 ci/run-build-and-tests.sh                     |    3 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2297 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 17 files changed, 3248 insertions(+), 7 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh

Range-diff against v2:
1: d6548dcfad ! 1: d83e66e980 refs: introduce reftable backend

    @@ refs/reftable-backend.c (new)
     +{
     +	switch (log->value_type) {
     +	case REFTABLE_LOG_UPDATE:
    -+		/* when we write log records, the hashes are owned by a struct
    -+		 * oid */
    ++		/*
    ++		 * When we write log records, the hashes are owned by the
    ++		 * caller and thus shouldn't be free'd.
    ++		 */
     +		log->value.update.old_hash = NULL;
     +		log->value.update.new_hash = NULL;
     +		break;
    @@ refs/reftable-backend.c (new)
     +			break;
     +
     +		/*
    -+		 * The files backend only lists references contained in
    -+		 * "refs/". We emulate the same behaviour here and thus skip
    -+		 * all references that don't start with this prefix.
    ++		 * Unless DO_FOR_EACH_INCLUDE_ALL_REFS is set, we only list
    ++		 * refs starting with "refs/" to mimic the "files" backend.
     +		 */
    -+		if (!starts_with(iter->ref.refname, "refs/"))
    ++		if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
    ++		    !starts_with(iter->ref.refname, "refs/"))
     +			continue;
     +
     +		if (iter->prefix &&

2: 63eafc9f5b = 2: 146bb95c03 ci: add jobs to test with the reftable backend

Signature · 1 line
2.43.GIT
#14 lore raw

PATCH v3 1/2 refs: introduce reftable backend

Due to scalability issues, Shawn Pearce has originally proposed a new "reftable" format more than six years ago [1]. Initially, this new format was implemented in JGit with promising results. Around two years ago, we have then added the "reftable" library to the Git codebase via a4bbd13be3 (Merge branch 'hn/reftable', 2021-12-15). With this we have landed all the low-level code to read and write reftables. Notably missing though was the integration of this low-level code into the Git code base in the form of a new ref backend that ties all of this together.

This gap is now finally closed by introducing a new "reftable" backend into the Git codebase. This new backend promises to bring some notable improvements to Git repositories:

- It becomes possible to do truly atomic writes where either all refs are committed to disk or none are. This was not possible with the "files" backend because ref updates were split across multiple loose files.

- The disk space required to store many refs is reduced, both compared to loose refs and packed-refs. This is enabled both by the reftable format being a binary format, which is more compact, and by prefix compression.

- We can ignore filesystem-specific behaviour as ref names are not encoded via paths anymore. This means there is no need to handle case sensitivity on Windows systems or Unicode precomposition on macOS.

- There is no need to rewrite the complete refdb anymore every time a ref is being deleted like it was the case for packed-refs. This means that ref deletions are now constant time instead of scaling linearly with the number of refs.

- We can ignore file/directory conflicts so that it becomes possible to store both "refs/heads/foo" and "refs/heads/foo/bar".

- Due to this property we can retain reflogs for deleted refs. We have previously been deleting reflogs together with their refs to avoid file/directory conflicts, which is not necessary anymore.

- We can properly enumerate all refs. With the "files" backend it is not easily possible to distinguish between refs and non-refs because they may live side by side in the gitdir.

Not all of these improvements are realized with the current "reftable" backend implementation. At this point, the new backend is supposed to be a drop-in replacement for the "files" backend that is used by basically all Git repositories nowadays. It strives for 1:1 compatibility, which means that a user can expect the same behaviour regardless of whether they use the "reftable" backend or the "files" backend for most of the part.

Most notably, this means we artificially limit the capabilities of the "reftable" backend to match the limits of the "files" backend. It is not possible to create refs that would end up with file/directory conflicts, we do not retain reflogs, we perform stricter-than-necessary checks. This is done intentionally due to two main reasons:

- It makes it significantly easier to land the "reftable" backend as tests behave the same. It would be tough to argue for each and every single test that doesn't pass with the "reftable" backend.

- It ensures compatibility between repositories that use the "files" backend and repositories that use the "reftable" backend. Like this, hosters can migrate their repositories to use the "reftable" backend without causing issues for clients that use the "files" backend in their clones.

It is expected that these artificial limitations may eventually go away in the long term.

Performance-wise things very much depend on the actual workload. The following benchmarks compare the "files" and "reftable" backends in the current version:

- Creating N refs in separate transactions shows that the "files" backend is ~50% faster. This is not surprising given that creating a ref only requires us to create a single loose ref. The "reftable" backend will also perform auto compaction on updates. In real-world workloads we would likely also want to perform pack loose refs, which would likely change the picture.

Benchmark 1: update-ref: create refs sequentially (refformat = files, refcount = 1)
Time (mean ± σ): 2.1 ms ± 0.3 ms [User: 0.6 ms, System: 1.7 ms]
Range (min … max): 1.8 ms … 4.3 ms 133 runs

Benchmark 2: update-ref: create refs sequentially (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.7 ms ± 0.1 ms [User: 0.6 ms, System: 2.2 ms]
Range (min … max): 2.4 ms … 2.9 ms 132 runs

Benchmark 3: update-ref: create refs sequentially (refformat = files, refcount = 1000)
Time (mean ± σ): 1.975 s ± 0.006 s [User: 0.437 s, System: 1.535 s]
Range (min … max): 1.969 s … 1.980 s 3 runs

Benchmark 4: update-ref: create refs sequentially (refformat = reftable, refcount = 1000)
Time (mean ± σ): 2.611 s ± 0.013 s [User: 0.782 s, System: 1.825 s]
Range (min … max): 2.597 s … 2.622 s 3 runs

Benchmark 5: update-ref: create refs sequentially (refformat = files, refcount = 100000)
Time (mean ± σ): 198.442 s ± 0.241 s [User: 43.051 s, System: 155.250 s]
Range (min … max): 198.189 s … 198.670 s 3 runs

Benchmark 6: update-ref: create refs sequentially (refformat = reftable, refcount = 100000)
Time (mean ± σ): 294.509 s ± 4.269 s [User: 104.046 s, System: 190.326 s]
Range (min … max): 290.223 s … 298.761 s 3 runs

- Creating N refs in a single transaction shows that the "files" backend is significantly slower once we start to write many refs. The "reftable" backend only needs to update two files, whereas the "files" backend needs to write one file per ref.

Benchmark 1: update-ref: create many refs (refformat = files, refcount = 1)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.4 ms, System: 1.4 ms]
Range (min … max): 1.8 ms … 2.6 ms 151 runs

Benchmark 2: update-ref: create many refs (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.5 ms ± 0.1 ms [User: 0.7 ms, System: 1.7 ms]
Range (min … max): 2.4 ms … 3.4 ms 148 runs

Benchmark 3: update-ref: create many refs (refformat = files, refcount = 1000)
Time (mean ± σ): 152.5 ms ± 5.2 ms [User: 19.1 ms, System: 133.1 ms]
Range (min … max): 148.5 ms … 167.8 ms 15 runs

Benchmark 4: update-ref: create many refs (refformat = reftable, refcount = 1000)
Time (mean ± σ): 58.0 ms ± 2.5 ms [User: 28.4 ms, System: 29.4 ms]
Range (min … max): 56.3 ms … 72.9 ms 40 runs

Benchmark 5: update-ref: create many refs (refformat = files, refcount = 1000000)
Time (mean ± σ): 152.752 s ± 0.710 s [User: 20.315 s, System: 131.310 s]
Range (min … max): 152.165 s … 153.542 s 3 runs

Benchmark 6: update-ref: create many refs (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 51.912 s ± 0.127 s [User: 26.483 s, System: 25.424 s]
Range (min … max): 51.769 s … 52.012 s 3 runs

- Deleting a ref in a fully-packed repository shows that the "files" backend scales with the number of refs. The "reftable" backend has constant-time deletions.

Benchmark 1: update-ref: delete ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.7 ms ± 0.1 ms [User: 0.4 ms, System: 1.2 ms]
Range (min … max): 1.6 ms … 2.1 ms 316 runs

Benchmark 2: update-ref: delete ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.8 ms ± 0.1 ms [User: 0.4 ms, System: 1.3 ms]
Range (min … max): 1.7 ms … 2.1 ms 294 runs

Benchmark 3: update-ref: delete ref (refformat = files, refcount = 1000)
Time (mean ± σ): 2.0 ms ± 0.1 ms [User: 0.5 ms, System: 1.4 ms]
Range (min … max): 1.9 ms … 2.5 ms 287 runs

Benchmark 4: update-ref: delete ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.5 ms, System: 1.3 ms]
Range (min … max): 1.8 ms … 2.1 ms 217 runs

Benchmark 5: update-ref: delete ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 229.8 ms ± 7.9 ms [User: 182.6 ms, System: 46.8 ms]
Range (min … max): 224.6 ms … 245.2 ms 6 runs

Benchmark 6: update-ref: delete ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 2.0 ms ± 0.0 ms [User: 0.6 ms, System: 1.3 ms]
Range (min … max): 2.0 ms … 2.1 ms 3 runs

- Listing all refs shows no significant advantage for either of the backends. The "files" backend is a bit faster, but not by a significant margin. When repositories are not packed the "reftable" backend outperforms the "files" backend because the "reftable" backend performs auto-compaction.

Benchmark 1: show-ref: print all refs (refformat = files, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1729 runs

Benchmark 2: show-ref: print all refs (refformat = reftable, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.8 ms 1816 runs

Benchmark 3: show-ref: print all refs (refformat = files, refcount = 1000, packed = true)
Time (mean ± σ): 4.3 ms ± 0.1 ms [User: 0.9 ms, System: 3.3 ms]
Range (min … max): 4.1 ms … 4.6 ms 645 runs

Benchmark 4: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = true)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.0 ms, System: 3.3 ms]
Range (min … max): 4.2 ms … 5.9 ms 643 runs

Benchmark 5: show-ref: print all refs (refformat = files, refcount = 1000000, packed = true)
Time (mean ± σ): 2.537 s ± 0.034 s [User: 0.488 s, System: 2.048 s]
Range (min … max): 2.511 s … 2.627 s 10 runs

Benchmark 6: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = true)
Time (mean ± σ): 2.712 s ± 0.017 s [User: 0.653 s, System: 2.059 s]
Range (min … max): 2.692 s … 2.752 s 10 runs

Benchmark 7: show-ref: print all refs (refformat = files, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.9 ms 1834 runs

Benchmark 8: show-ref: print all refs (refformat = reftable, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.0 ms 1840 runs

Benchmark 9: show-ref: print all refs (refformat = files, refcount = 1000, packed = false)
Time (mean ± σ): 13.8 ms ± 0.2 ms [User: 2.8 ms, System: 10.8 ms]
Range (min … max): 13.3 ms … 14.5 ms 208 runs

Benchmark 10: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = false)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.2 ms, System: 3.3 ms]
Range (min … max): 4.3 ms … 6.2 ms 624 runs

Benchmark 11: show-ref: print all refs (refformat = files, refcount = 1000000, packed = false)
Time (mean ± σ): 12.127 s ± 0.129 s [User: 2.675 s, System: 9.451 s]
Range (min … max): 11.965 s … 12.370 s 10 runs

Benchmark 12: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = false)
Time (mean ± σ): 2.799 s ± 0.022 s [User: 0.735 s, System: 2.063 s]
Range (min … max): 2.769 s … 2.836 s 10 runs

- Printing a single ref shows no real difference between the "files" and "reftable" backends.

Benchmark 1: show-ref: print single ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.4 ms, System: 1.0 ms]
Range (min … max): 1.4 ms … 1.8 ms 1779 runs

Benchmark 2: show-ref: print single ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.5 ms 1753 runs

Benchmark 3: show-ref: print single ref (refformat = files, refcount = 1000)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.3 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 1.9 ms 1840 runs

Benchmark 4: show-ref: print single ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1831 runs

Benchmark 5: show-ref: print single ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1848 runs

Benchmark 6: show-ref: print single ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1762 runs

So overall, performance depends on the usecases. Except for many sequential writes the "reftable" backend is roughly on par or significantly faster than the "files" backend though. Given that the "files" backend has received 18 years of optimizations by now this can be seen as a win. Furthermore, we can expect that the "reftable" backend will grow faster over time when attention turns more towards optimizations.

The complete test suite passes, except for those tests explicitly marked to require the REFFILES prerequisite. Some tests in t0610 are marked as failing because they depend on still-in-flight bug fixes. Tests can be run with the new backend by setting the GIT_TEST_DEFAULT_REF_FORMAT environment variable to "reftable".

There is a single known conceptual incompatibility with the dumb HTTP transport. As "info/refs" SHOULD NOT contain the HEAD reference, and because the "HEAD" file is not valid anymore, it is impossible for the remote client to figure out the default branch without changing the protocol. This shortcoming needs to be handled in a subsequent patch series.

As the reftable library has already been introduced a while ago, this commit message will not go into the details of how exactly the on-disk format works. Please refer to our preexisting technical documentation at Documentation/technical/reftable for this.

[1]: https://public-inbox.org/git/CAJo=hJtyof=HRy=2sLP0ng0uZ4=S-DpZ5dR1aF+VHVETKG20OQ@mail.gmail.com/

---
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2297 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 13 files changed, 3226 insertions(+), 6 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh
Documentation/ref-storage-format.txt +2 -0
index 1a65cac468..14fff8a9c6 100644
@@ -1 +1,3 @@
 * `files` for loose files with packed-refs. This is the default.
+* `reftable` for the reftable format. This format is experimental and its
+  internals are subject to change.
Documentation/technical/repository-version.txt +3 -2
index 27be3741e6..47281420fc 100644
@@ -103,5 +103,6 @@ GIT_COMMON_DIR/worktrees/<id>/config.worktree)
 
 ==== `refStorage`
 
-Specifies the file format for the ref database. The only valid value
-is `files` (loose references with a packed-refs file).
+Specifies the file format for the ref database. The valid values are
+`files` (loose references with a packed-refs file) and `reftable` (see
+Documentation/technical/reftable.txt).
Makefile +1 -0
index 0f748a52e6..b302516173 100644
@@ -1127,6 +1127,7 @@ LIB_OBJS += reflog.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
+LIB_OBJS += refs/reftable-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
contrib/workdir/git-new-workdir +1 -1
index 888c34a521..989197aace 100755
@@ -79,7 +79,7 @@ trap cleanup $siglist
 # create the links to the original repo.  explicitly exclude index, HEAD and
 # logs/HEAD from the list since they are purely related to the current working
 # directory, and should not be shared.
-for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn
+for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn reftable
 do
 	# create a containing directory if needed
 	case $x in
path.c +1 -1
index 0fb527918b..8bb223c92c 100644
@@ -871,7 +871,7 @@ const char *enter_repo(const char *path, int strict)
 	return NULL;
 }
 
-static int calc_shared_perm(int mode)
+int calc_shared_perm(int mode)
 {
 	int tweak;
 
path.h +1 -0
index b3233c51fa..e053effef2 100644
@@ -181,6 +181,7 @@ const char *git_path_shallow(struct repository *r);
 int ends_with_path_components(const char *path, const char *components);
 int validate_headref(const char *ref);
 
+int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
refs.c +1 -0
index 3190df8d81..7e6970ca1d 100644
@@ -35,6 +35,7 @@
  */
 static const struct ref_storage_be *refs_backends[] = {
 	[REF_STORAGE_FORMAT_FILES] = &refs_be_files,
+	[REF_STORAGE_FORMAT_REFTABLE] = &refs_be_reftable,
 };
 
 static const struct ref_storage_be *find_ref_storage_backend(unsigned int ref_storage_format)
refs/refs-internal.h +1 -0
index be15c19c02..2285abe1ec 100644
@@ -700,6 +700,7 @@ struct ref_storage_be {
 };
 
 extern struct ref_storage_be refs_be_files;
+extern struct ref_storage_be refs_be_reftable;
 extern struct ref_storage_be refs_be_packed;
 
 /*
refs/reftable-backend.c new file +2297 -0

2,297 lines not on this page · raw message

repository.h +3 -2
index 7a250a6605..1645cef518 100644
@@ -24,8 +24,9 @@ enum fetch_negotiation_setting {
 	FETCH_NEGOTIATION_NOOP,
 };
 
-#define REF_STORAGE_FORMAT_UNKNOWN 0
-#define REF_STORAGE_FORMAT_FILES   1
+#define REF_STORAGE_FORMAT_UNKNOWN  0
+#define REF_STORAGE_FORMAT_FILES    1
+#define REF_STORAGE_FORMAT_REFTABLE 2
 
 struct repo_settings {
 	int initialized;
t/t0610-reftable-basics.sh new file +887 -0

637 lines not on this page · raw message

t/t0611-reftable-httpd.sh new file +26 -0
new file mode 100755
index 0000000000..5e05b9c1f2

Lines of this file are not shown. View the raw message.

t/test-lib.sh +2 -0
index fc93aa57e6..0fb8757b7d 100644

Lines of this file are not shown. View the raw message.

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.43.GIT

Part of the diff is not shown here. View the raw message.

#15 lore raw

PATCH v3 2/2 ci: add jobs to test with the reftable backend

In reply to Patrick Steinhardt

Add CI jobs for both GitHub Workflows and GitLab CI to run Git with the new reftable backend.

---
 .github/workflows/main.yml | 9 +++++++++
 .gitlab-ci.yml             | 9 +++++++++
 ci/lib.sh                  | 2 +-
 ci/run-build-and-tests.sh  | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)
.github/workflows/main.yml +9 -0
index 4d97da57ec..1b43e49dad 100644
@@ -266,6 +266,9 @@ jobs:
           - jobname: linux-sha256
             cc: clang
             pool: ubuntu-latest
+          - jobname: linux-reftable
+            cc: clang
+            pool: ubuntu-latest
           - jobname: linux-gcc
             cc: gcc
             cc_package: gcc-8
@@ -277,6 +280,9 @@ jobs:
           - jobname: osx-clang
             cc: clang
             pool: macos-13
+          - jobname: osx-reftable
+            cc: clang
+            pool: macos-13
           - jobname: osx-gcc
             cc: gcc
             cc_package: gcc-13
@@ -287,6 +293,9 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-reftable-leaks
+            cc: gcc
+            pool: ubuntu-latest
           - jobname: linux-asan-ubsan
             cc: clang
             pool: ubuntu-latest
.gitlab-ci.yml +9 -0
index 43bfbd8834..c0fa2fe90b 100644
@@ -26,6 +26,9 @@ test:linux:
       - jobname: linux-sha256
         image: ubuntu:latest
         CC: clang
+      - jobname: linux-reftable
+        image: ubuntu:latest
+        CC: clang
       - jobname: linux-gcc
         image: ubuntu:20.04
         CC: gcc
@@ -40,6 +43,9 @@ test:linux:
       - jobname: linux-leaks
         image: ubuntu:latest
         CC: gcc
+      - jobname: linux-reftable-leaks
+        image: ubuntu:latest
+        CC: gcc
       - jobname: linux-asan-ubsan
         image: ubuntu:latest
         CC: clang
@@ -79,6 +85,9 @@ test:osx:
       - jobname: osx-clang
         image: macos-13-xcode-14
         CC: clang
+      - jobname: osx-reftable
+        image: macos-13-xcode-14
+        CC: clang
   artifacts:
     paths:
       - t/failed-test-artifacts
ci/lib.sh +1 -1
index d5dd2f2697..0a73fc7bd1 100755
@@ -367,7 +367,7 @@ linux-musl)
 	MAKEFLAGS="$MAKEFLAGS NO_REGEX=Yes ICONV_OMITS_BOM=Yes"
 	MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8"
 	;;
-linux-leaks)
+linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	export GIT_TEST_SANITIZE_LEAK_LOG=true
ci/run-build-and-tests.sh +3 -0
index 7a1466b868..c192bd613c 100755
@@ -37,6 +37,9 @@ linux-clang)
 linux-sha256)
 	export GIT_TEST_DEFAULT_HASH=sha256
 	;;
+linux-reftable|linux-reftable-leaks|osx-reftable)
+	export GIT_TEST_DEFAULT_REF_FORMAT=reftable
+	;;
 pedantic)
 	# Don't run the tests; we only care about whether Git can be
 	# built.
Signature · 1 line
2.43.GIT
#16 lore raw
In reply to Patrick Steinhardt

Hello,

Quoted 70 lines · Patrick Steinhardt

Hi,

this is the third version of my patch series that introduces the reftable backend.

The only real change in this version is that I've pulled in kn/for-all-refs at 693e807311 (for-each-ref: avoid filtering on empty pattern, 2024-01-29) as a dependency. This series introduces a new DO_FOR_EACH_INCLUDE_ALL_REFS flag that we need to handle in the reftable backend. With this change all tests should pass again.

Patrick

Patrick Steinhardt (2):
refs: introduce reftable backend
ci: add jobs to test with the reftable backend

 .github/workflows/main.yml                    |    9 +
 .gitlab-ci.yml                                |    9 +
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 ci/lib.sh                                     |    2 +-
 ci/run-build-and-tests.sh                     |    3 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2297 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 17 files changed, 3248 insertions(+), 7 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh

Range-diff against v2:
1: d6548dcfad ! 1: d83e66e980 refs: introduce reftable backend

    @@ refs/reftable-backend.c (new)
     +{
     +	switch (log->value_type) {
     +	case REFTABLE_LOG_UPDATE:
    -+		/* when we write log records, the hashes are owned by a struct
    -+		 * oid */
    ++		/*
    ++		 * When we write log records, the hashes are owned by the
    ++		 * caller and thus shouldn't be free'd.
    ++		 */
     +		log->value.update.old_hash = NULL;
     +		log->value.update.new_hash = NULL;
     +		break;
    @@ refs/reftable-backend.c (new)
     +			break;
     +
     +		/*
    -+		 * The files backend only lists references contained in
    -+		 * "refs/". We emulate the same behaviour here and thus skip
    -+		 * all references that don't start with this prefix.
    ++		 * Unless DO_FOR_EACH_INCLUDE_ALL_REFS is set, we only list
    ++		 * refs starting with "refs/" to mimic the "files" backend.
     +		 */
    -+		if (!starts_with(iter->ref.refname, "refs/"))
    ++		if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
    ++		    !starts_with(iter->ref.refname, "refs/"))
     +			continue;
     +
     +		if (iter->prefix &&
Quoted 1 line

2: 63eafc9f5b = 2: 146bb95c03 ci: add jobs to test with the reftable backend

This range-diff looks good to me. I'd already reviewed the previous version, so overall looks good now.

#17 lore raw

v4 cover refs: introduce reftable backend

In reply to Patrick Steinhardt

Hi,

this is the fourth version of my patch series that introduces the reftable backend.

Changes compared to v3:

- kn/for-all-refs has been reverted due to discussions around its user interface. I have thus evicted the patch series as a dependency and dropped handling of DO_FOR_EACH_INCLUDE_ALL_REFS.

- ps/reftable-compacted-tables-permission-fix has been merged to "master" now, fixing permissions on "tables.list" when compacting reftables. I've marked the corresponding test as succeeding.

- jc/reftable-core-fsync has been merged to "master" now, which adds fsyncing logic to reftable. I've marked the corresponding test as succeeding.

- I noticed that the fsync tests fail on macOS because there we use a different fsync method by default. I fixed that by explicitly saying which fsync method should be used in the corresponding tests.

- I also noticed that the second fsync test reused "trace2.txt" from the first fsync test because it only appends to the file. Thus, we saw two fsync events instead of one. I fixed that by truncating the file.

The patch series is based on the current "master" branch at 235986be82 (The fourteenth batch, 2024-02-06).

Thanks!

Patrick

Patrick Steinhardt (2):
refs: introduce reftable backend
ci: add jobs to test with the reftable backend

 .github/workflows/main.yml                    |    9 +
 .gitlab-ci.yml                                |    9 +
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 ci/lib.sh                                     |    2 +-
 ci/run-build-and-tests.sh                     |    3 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2297 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 17 files changed, 3248 insertions(+), 7 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh

Range-diff against v3:
1: d83e66e980 ! 1: 5de60d46bd refs: introduce reftable backend

    @@ refs/reftable-backend.c (new)
     +			break;
     +
     +		/*
    -+		 * Unless DO_FOR_EACH_INCLUDE_ALL_REFS is set, we only list
    -+		 * refs starting with "refs/" to mimic the "files" backend.
    ++		 * The files backend only lists references contained in
    ++		 * "refs/". We emulate the same behaviour here and thus skip
    ++		 * all references that don't start with this prefix.
     +		 */
    -+		if (!(iter->flags & DO_FOR_EACH_INCLUDE_ALL_REFS) &&
    -+		    !starts_with(iter->ref.refname, "refs/"))
    ++		if (!starts_with(iter->ref.refname, "refs/"))
     +			continue;
     +
     +		if (iter->prefix &&
    @@ t/t0610-reftable-basics.sh (new)
     +	test_cmp expect actual
     +}
     +
    -+# A fix for this is in flight via jc/reftable-core-fsync.
    -+test_expect_failure 'ref transaction: writes are synced' '
    ++test_expect_success 'ref transaction: writes are synced' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +	test_commit -C repo initial &&
     +
     +	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
     +	GIT_TEST_FSYNC=true \
    -+		git -C repo -c core.fsync=reference update-ref refs/heads/branch HEAD &&
    ++		git -C repo -c core.fsync=reference \
    ++		-c core.fsyncMethod=fsync update-ref refs/heads/branch HEAD &&
     +	check_fsync_events trace2.txt <<-EOF
     +	"name":"hardware-flush","count":2
     +	EOF
    @@ t/t0610-reftable-basics.sh (new)
     +
     +for umask in 002 022
     +do
    -+	# A fix for this is in flight via ps/reftable-compacted-tables-permission-fix.
    -+	test_expect_failure POSIXPERM 'pack-refs: honors core.sharedRepository' '
    ++	test_expect_success POSIXPERM 'pack-refs: honors core.sharedRepository' '
     +		test_when_finished "rm -rf repo" &&
     +		(
     +			umask $umask &&
    @@ t/t0610-reftable-basics.sh (new)
     +	'
     +done
     +
    -+# A fix for this is in flight.
    -+test_expect_failure 'packed-refs: writes are synced' '
    ++test_expect_success 'packed-refs: writes are synced' '
     +	test_when_finished "rm -rf repo" &&
     +	git init repo &&
     +	test_commit -C repo initial &&
     +	test_line_count = 2 table-files &&
     +
    ++	: >trace2.txt &&
     +	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" \
     +	GIT_TEST_FSYNC=true \
    -+		git -C repo -c core.fsync=reference pack-refs &&
    ++		git -C repo -c core.fsync=reference \
    ++		-c core.fsyncMethod=fsync pack-refs &&
     +	check_fsync_events trace2.txt <<-EOF
     +	"name":"hardware-flush","count":2
     +	EOF

2: 146bb95c03 = 2: 30e5feb28c ci: add jobs to test with the reftable backend

base-commit: 235986be822c9f8689be2e9a0b7804d0b1b6d821

Signature · 1 line
2.43.GIT
#18 lore raw

PATCH v4 1/2 refs: introduce reftable backend

Due to scalability issues, Shawn Pearce has originally proposed a new "reftable" format more than six years ago [1]. Initially, this new format was implemented in JGit with promising results. Around two years ago, we have then added the "reftable" library to the Git codebase via a4bbd13be3 (Merge branch 'hn/reftable', 2021-12-15). With this we have landed all the low-level code to read and write reftables. Notably missing though was the integration of this low-level code into the Git code base in the form of a new ref backend that ties all of this together.

This gap is now finally closed by introducing a new "reftable" backend into the Git codebase. This new backend promises to bring some notable improvements to Git repositories:

- It becomes possible to do truly atomic writes where either all refs are committed to disk or none are. This was not possible with the "files" backend because ref updates were split across multiple loose files.

- The disk space required to store many refs is reduced, both compared to loose refs and packed-refs. This is enabled both by the reftable format being a binary format, which is more compact, and by prefix compression.

- We can ignore filesystem-specific behaviour as ref names are not encoded via paths anymore. This means there is no need to handle case sensitivity on Windows systems or Unicode precomposition on macOS.

- There is no need to rewrite the complete refdb anymore every time a ref is being deleted like it was the case for packed-refs. This means that ref deletions are now constant time instead of scaling linearly with the number of refs.

- We can ignore file/directory conflicts so that it becomes possible to store both "refs/heads/foo" and "refs/heads/foo/bar".

- Due to this property we can retain reflogs for deleted refs. We have previously been deleting reflogs together with their refs to avoid file/directory conflicts, which is not necessary anymore.

- We can properly enumerate all refs. With the "files" backend it is not easily possible to distinguish between refs and non-refs because they may live side by side in the gitdir.

Not all of these improvements are realized with the current "reftable" backend implementation. At this point, the new backend is supposed to be a drop-in replacement for the "files" backend that is used by basically all Git repositories nowadays. It strives for 1:1 compatibility, which means that a user can expect the same behaviour regardless of whether they use the "reftable" backend or the "files" backend for most of the part.

Most notably, this means we artificially limit the capabilities of the "reftable" backend to match the limits of the "files" backend. It is not possible to create refs that would end up with file/directory conflicts, we do not retain reflogs, we perform stricter-than-necessary checks. This is done intentionally due to two main reasons:

- It makes it significantly easier to land the "reftable" backend as tests behave the same. It would be tough to argue for each and every single test that doesn't pass with the "reftable" backend.

- It ensures compatibility between repositories that use the "files" backend and repositories that use the "reftable" backend. Like this, hosters can migrate their repositories to use the "reftable" backend without causing issues for clients that use the "files" backend in their clones.

It is expected that these artificial limitations may eventually go away in the long term.

Performance-wise things very much depend on the actual workload. The following benchmarks compare the "files" and "reftable" backends in the current version:

- Creating N refs in separate transactions shows that the "files" backend is ~50% faster. This is not surprising given that creating a ref only requires us to create a single loose ref. The "reftable" backend will also perform auto compaction on updates. In real-world workloads we would likely also want to perform pack loose refs, which would likely change the picture.

Benchmark 1: update-ref: create refs sequentially (refformat = files, refcount = 1)
Time (mean ± σ): 2.1 ms ± 0.3 ms [User: 0.6 ms, System: 1.7 ms]
Range (min … max): 1.8 ms … 4.3 ms 133 runs

Benchmark 2: update-ref: create refs sequentially (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.7 ms ± 0.1 ms [User: 0.6 ms, System: 2.2 ms]
Range (min … max): 2.4 ms … 2.9 ms 132 runs

Benchmark 3: update-ref: create refs sequentially (refformat = files, refcount = 1000)
Time (mean ± σ): 1.975 s ± 0.006 s [User: 0.437 s, System: 1.535 s]
Range (min … max): 1.969 s … 1.980 s 3 runs

Benchmark 4: update-ref: create refs sequentially (refformat = reftable, refcount = 1000)
Time (mean ± σ): 2.611 s ± 0.013 s [User: 0.782 s, System: 1.825 s]
Range (min … max): 2.597 s … 2.622 s 3 runs

Benchmark 5: update-ref: create refs sequentially (refformat = files, refcount = 100000)
Time (mean ± σ): 198.442 s ± 0.241 s [User: 43.051 s, System: 155.250 s]
Range (min … max): 198.189 s … 198.670 s 3 runs

Benchmark 6: update-ref: create refs sequentially (refformat = reftable, refcount = 100000)
Time (mean ± σ): 294.509 s ± 4.269 s [User: 104.046 s, System: 190.326 s]
Range (min … max): 290.223 s … 298.761 s 3 runs

- Creating N refs in a single transaction shows that the "files" backend is significantly slower once we start to write many refs. The "reftable" backend only needs to update two files, whereas the "files" backend needs to write one file per ref.

Benchmark 1: update-ref: create many refs (refformat = files, refcount = 1)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.4 ms, System: 1.4 ms]
Range (min … max): 1.8 ms … 2.6 ms 151 runs

Benchmark 2: update-ref: create many refs (refformat = reftable, refcount = 1)
Time (mean ± σ): 2.5 ms ± 0.1 ms [User: 0.7 ms, System: 1.7 ms]
Range (min … max): 2.4 ms … 3.4 ms 148 runs

Benchmark 3: update-ref: create many refs (refformat = files, refcount = 1000)
Time (mean ± σ): 152.5 ms ± 5.2 ms [User: 19.1 ms, System: 133.1 ms]
Range (min … max): 148.5 ms … 167.8 ms 15 runs

Benchmark 4: update-ref: create many refs (refformat = reftable, refcount = 1000)
Time (mean ± σ): 58.0 ms ± 2.5 ms [User: 28.4 ms, System: 29.4 ms]
Range (min … max): 56.3 ms … 72.9 ms 40 runs

Benchmark 5: update-ref: create many refs (refformat = files, refcount = 1000000)
Time (mean ± σ): 152.752 s ± 0.710 s [User: 20.315 s, System: 131.310 s]
Range (min … max): 152.165 s … 153.542 s 3 runs

Benchmark 6: update-ref: create many refs (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 51.912 s ± 0.127 s [User: 26.483 s, System: 25.424 s]
Range (min … max): 51.769 s … 52.012 s 3 runs

- Deleting a ref in a fully-packed repository shows that the "files" backend scales with the number of refs. The "reftable" backend has constant-time deletions.

Benchmark 1: update-ref: delete ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.7 ms ± 0.1 ms [User: 0.4 ms, System: 1.2 ms]
Range (min … max): 1.6 ms … 2.1 ms 316 runs

Benchmark 2: update-ref: delete ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.8 ms ± 0.1 ms [User: 0.4 ms, System: 1.3 ms]
Range (min … max): 1.7 ms … 2.1 ms 294 runs

Benchmark 3: update-ref: delete ref (refformat = files, refcount = 1000)
Time (mean ± σ): 2.0 ms ± 0.1 ms [User: 0.5 ms, System: 1.4 ms]
Range (min … max): 1.9 ms … 2.5 ms 287 runs

Benchmark 4: update-ref: delete ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.9 ms ± 0.1 ms [User: 0.5 ms, System: 1.3 ms]
Range (min … max): 1.8 ms … 2.1 ms 217 runs

Benchmark 5: update-ref: delete ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 229.8 ms ± 7.9 ms [User: 182.6 ms, System: 46.8 ms]
Range (min … max): 224.6 ms … 245.2 ms 6 runs

Benchmark 6: update-ref: delete ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 2.0 ms ± 0.0 ms [User: 0.6 ms, System: 1.3 ms]
Range (min … max): 2.0 ms … 2.1 ms 3 runs

- Listing all refs shows no significant advantage for either of the backends. The "files" backend is a bit faster, but not by a significant margin. When repositories are not packed the "reftable" backend outperforms the "files" backend because the "reftable" backend performs auto-compaction.

Benchmark 1: show-ref: print all refs (refformat = files, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1729 runs

Benchmark 2: show-ref: print all refs (refformat = reftable, refcount = 1, packed = true)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.8 ms 1816 runs

Benchmark 3: show-ref: print all refs (refformat = files, refcount = 1000, packed = true)
Time (mean ± σ): 4.3 ms ± 0.1 ms [User: 0.9 ms, System: 3.3 ms]
Range (min … max): 4.1 ms … 4.6 ms 645 runs

Benchmark 4: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = true)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.0 ms, System: 3.3 ms]
Range (min … max): 4.2 ms … 5.9 ms 643 runs

Benchmark 5: show-ref: print all refs (refformat = files, refcount = 1000000, packed = true)
Time (mean ± σ): 2.537 s ± 0.034 s [User: 0.488 s, System: 2.048 s]
Range (min … max): 2.511 s … 2.627 s 10 runs

Benchmark 6: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = true)
Time (mean ± σ): 2.712 s ± 0.017 s [User: 0.653 s, System: 2.059 s]
Range (min … max): 2.692 s … 2.752 s 10 runs

Benchmark 7: show-ref: print all refs (refformat = files, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 1.9 ms 1834 runs

Benchmark 8: show-ref: print all refs (refformat = reftable, refcount = 1, packed = false)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.0 ms 1840 runs

Benchmark 9: show-ref: print all refs (refformat = files, refcount = 1000, packed = false)
Time (mean ± σ): 13.8 ms ± 0.2 ms [User: 2.8 ms, System: 10.8 ms]
Range (min … max): 13.3 ms … 14.5 ms 208 runs

Benchmark 10: show-ref: print all refs (refformat = reftable, refcount = 1000, packed = false)
Time (mean ± σ): 4.5 ms ± 0.2 ms [User: 1.2 ms, System: 3.3 ms]
Range (min … max): 4.3 ms … 6.2 ms 624 runs

Benchmark 11: show-ref: print all refs (refformat = files, refcount = 1000000, packed = false)
Time (mean ± σ): 12.127 s ± 0.129 s [User: 2.675 s, System: 9.451 s]
Range (min … max): 11.965 s … 12.370 s 10 runs

Benchmark 12: show-ref: print all refs (refformat = reftable, refcount = 1000000, packed = false)
Time (mean ± σ): 2.799 s ± 0.022 s [User: 0.735 s, System: 2.063 s]
Range (min … max): 2.769 s … 2.836 s 10 runs

- Printing a single ref shows no real difference between the "files" and "reftable" backends.

Benchmark 1: show-ref: print single ref (refformat = files, refcount = 1)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.4 ms, System: 1.0 ms]
Range (min … max): 1.4 ms … 1.8 ms 1779 runs

Benchmark 2: show-ref: print single ref (refformat = reftable, refcount = 1)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 2.5 ms 1753 runs

Benchmark 3: show-ref: print single ref (refformat = files, refcount = 1000)
Time (mean ± σ): 1.5 ms ± 0.1 ms [User: 0.3 ms, System: 1.1 ms]
Range (min … max): 1.4 ms … 1.9 ms 1840 runs

Benchmark 4: show-ref: print single ref (refformat = reftable, refcount = 1000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.0 ms 1831 runs

Benchmark 5: show-ref: print single ref (refformat = files, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1848 runs

Benchmark 6: show-ref: print single ref (refformat = reftable, refcount = 1000000)
Time (mean ± σ): 1.6 ms ± 0.1 ms [User: 0.4 ms, System: 1.1 ms]
Range (min … max): 1.5 ms … 2.1 ms 1762 runs

So overall, performance depends on the usecases. Except for many sequential writes the "reftable" backend is roughly on par or significantly faster than the "files" backend though. Given that the "files" backend has received 18 years of optimizations by now this can be seen as a win. Furthermore, we can expect that the "reftable" backend will grow faster over time when attention turns more towards optimizations.

The complete test suite passes, except for those tests explicitly marked to require the REFFILES prerequisite. Some tests in t0610 are marked as failing because they depend on still-in-flight bug fixes. Tests can be run with the new backend by setting the GIT_TEST_DEFAULT_REF_FORMAT environment variable to "reftable".

There is a single known conceptual incompatibility with the dumb HTTP transport. As "info/refs" SHOULD NOT contain the HEAD reference, and because the "HEAD" file is not valid anymore, it is impossible for the remote client to figure out the default branch without changing the protocol. This shortcoming needs to be handled in a subsequent patch series.

As the reftable library has already been introduced a while ago, this commit message will not go into the details of how exactly the on-disk format works. Please refer to our preexisting technical documentation at Documentation/technical/reftable for this.

[1]: https://public-inbox.org/git/CAJo=hJtyof=HRy=2sLP0ng0uZ4=S-DpZ5dR1aF+VHVETKG20OQ@mail.gmail.com/

---
 Documentation/ref-storage-format.txt          |    2 +
 .../technical/repository-version.txt          |    5 +-
 Makefile                                      |    1 +
 contrib/workdir/git-new-workdir               |    2 +-
 path.c                                        |    2 +-
 path.h                                        |    1 +
 refs.c                                        |    1 +
 refs/refs-internal.h                          |    1 +
 refs/reftable-backend.c                       | 2297 +++++++++++++++++
 repository.h                                  |    5 +-
 t/t0610-reftable-basics.sh                    |  887 +++++++
 t/t0611-reftable-httpd.sh                     |   26 +
 t/test-lib.sh                                 |    2 +
 13 files changed, 3226 insertions(+), 6 deletions(-)
 create mode 100644 refs/reftable-backend.c
 create mode 100755 t/t0610-reftable-basics.sh
 create mode 100755 t/t0611-reftable-httpd.sh
Documentation/ref-storage-format.txt +2 -0
index 1a65cac468..14fff8a9c6 100644
@@ -1 +1,3 @@
 * `files` for loose files with packed-refs. This is the default.
+* `reftable` for the reftable format. This format is experimental and its
+  internals are subject to change.
Documentation/technical/repository-version.txt +3 -2
index 27be3741e6..47281420fc 100644
@@ -103,5 +103,6 @@ GIT_COMMON_DIR/worktrees/<id>/config.worktree)
 
 ==== `refStorage`
 
-Specifies the file format for the ref database. The only valid value
-is `files` (loose references with a packed-refs file).
+Specifies the file format for the ref database. The valid values are
+`files` (loose references with a packed-refs file) and `reftable` (see
+Documentation/technical/reftable.txt).
Makefile +1 -0
index 302b863548..dd7b1cf9dd 100644
@@ -1127,6 +1127,7 @@ LIB_OBJS += reflog.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/debug.o
 LIB_OBJS += refs/files-backend.o
+LIB_OBJS += refs/reftable-backend.o
 LIB_OBJS += refs/iterator.o
 LIB_OBJS += refs/packed-backend.o
 LIB_OBJS += refs/ref-cache.o
contrib/workdir/git-new-workdir +1 -1
index 888c34a521..989197aace 100755
@@ -79,7 +79,7 @@ trap cleanup $siglist
 # create the links to the original repo.  explicitly exclude index, HEAD and
 # logs/HEAD from the list since they are purely related to the current working
 # directory, and should not be shared.
-for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn
+for x in config refs logs/refs objects info hooks packed-refs remotes rr-cache svn reftable
 do
 	# create a containing directory if needed
 	case $x in
path.c +1 -1
index 0fb527918b..8bb223c92c 100644
@@ -871,7 +871,7 @@ const char *enter_repo(const char *path, int strict)
 	return NULL;
 }
 
-static int calc_shared_perm(int mode)
+int calc_shared_perm(int mode)
 {
 	int tweak;
 
path.h +1 -0
index b3233c51fa..e053effef2 100644
@@ -181,6 +181,7 @@ const char *git_path_shallow(struct repository *r);
 int ends_with_path_components(const char *path, const char *components);
 int validate_headref(const char *ref);
 
+int calc_shared_perm(int mode);
 int adjust_shared_perm(const char *path);
 
 char *interpolate_path(const char *path, int real_home);
refs.c +1 -0
index c633abf284..fff343c256 100644
@@ -35,6 +35,7 @@
  */
 static const struct ref_storage_be *refs_backends[] = {
 	[REF_STORAGE_FORMAT_FILES] = &refs_be_files,
+	[REF_STORAGE_FORMAT_REFTABLE] = &refs_be_reftable,
 };
 
 static const struct ref_storage_be *find_ref_storage_backend(unsigned int ref_storage_format)
refs/refs-internal.h +1 -0
index 82219829b0..83e0f0bba3 100644
@@ -693,6 +693,7 @@ struct ref_storage_be {
 };
 
 extern struct ref_storage_be refs_be_files;
+extern struct ref_storage_be refs_be_reftable;
 extern struct ref_storage_be refs_be_packed;
 
 /*
refs/reftable-backend.c new file +2297 -0

2,297 lines not on this page · raw message

repository.h +3 -2
index 7a250a6605..1645cef518 100644
@@ -24,8 +24,9 @@ enum fetch_negotiation_setting {
 	FETCH_NEGOTIATION_NOOP,
 };
 
-#define REF_STORAGE_FORMAT_UNKNOWN 0
-#define REF_STORAGE_FORMAT_FILES   1
+#define REF_STORAGE_FORMAT_UNKNOWN  0
+#define REF_STORAGE_FORMAT_FILES    1
+#define REF_STORAGE_FORMAT_REFTABLE 2
 
 struct repo_settings {
 	int initialized;
t/t0610-reftable-basics.sh new file +887 -0

637 lines not on this page · raw message

t/t0611-reftable-httpd.sh new file +26 -0
new file mode 100755
index 0000000000..5e05b9c1f2

Lines of this file are not shown. View the raw message.

t/test-lib.sh +2 -0
index 042f557a6f..c8af8dab79 100644

Lines of this file are not shown. View the raw message.

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.43.GIT

Part of the diff is not shown here. View the raw message.

#22 lore raw

PATCH v4 2/2 ci: add jobs to test with the reftable backend

In reply to Patrick Steinhardt

Add CI jobs for both GitHub Workflows and GitLab CI to run Git with the new reftable backend.

---
 .github/workflows/main.yml | 9 +++++++++
 .gitlab-ci.yml             | 9 +++++++++
 ci/lib.sh                  | 2 +-
 ci/run-build-and-tests.sh  | 3 +++
 4 files changed, 22 insertions(+), 1 deletion(-)
.github/workflows/main.yml +9 -0
index 4d97da57ec..1b43e49dad 100644
@@ -266,6 +266,9 @@ jobs:
           - jobname: linux-sha256
             cc: clang
             pool: ubuntu-latest
+          - jobname: linux-reftable
+            cc: clang
+            pool: ubuntu-latest
           - jobname: linux-gcc
             cc: gcc
             cc_package: gcc-8
@@ -277,6 +280,9 @@ jobs:
           - jobname: osx-clang
             cc: clang
             pool: macos-13
+          - jobname: osx-reftable
+            cc: clang
+            pool: macos-13
           - jobname: osx-gcc
             cc: gcc
             cc_package: gcc-13
@@ -287,6 +293,9 @@ jobs:
           - jobname: linux-leaks
             cc: gcc
             pool: ubuntu-latest
+          - jobname: linux-reftable-leaks
+            cc: gcc
+            pool: ubuntu-latest
           - jobname: linux-asan-ubsan
             cc: clang
             pool: ubuntu-latest
.gitlab-ci.yml +9 -0
index 43bfbd8834..c0fa2fe90b 100644
@@ -26,6 +26,9 @@ test:linux:
       - jobname: linux-sha256
         image: ubuntu:latest
         CC: clang
+      - jobname: linux-reftable
+        image: ubuntu:latest
+        CC: clang
       - jobname: linux-gcc
         image: ubuntu:20.04
         CC: gcc
@@ -40,6 +43,9 @@ test:linux:
       - jobname: linux-leaks
         image: ubuntu:latest
         CC: gcc
+      - jobname: linux-reftable-leaks
+        image: ubuntu:latest
+        CC: gcc
       - jobname: linux-asan-ubsan
         image: ubuntu:latest
         CC: clang
@@ -79,6 +85,9 @@ test:osx:
       - jobname: osx-clang
         image: macos-13-xcode-14
         CC: clang
+      - jobname: osx-reftable
+        image: macos-13-xcode-14
+        CC: clang
   artifacts:
     paths:
       - t/failed-test-artifacts
ci/lib.sh +1 -1
index d5dd2f2697..0a73fc7bd1 100755
@@ -367,7 +367,7 @@ linux-musl)
 	MAKEFLAGS="$MAKEFLAGS NO_REGEX=Yes ICONV_OMITS_BOM=Yes"
 	MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8"
 	;;
-linux-leaks)
+linux-leaks|linux-reftable-leaks)
 	export SANITIZE=leak
 	export GIT_TEST_PASSING_SANITIZE_LEAK=true
 	export GIT_TEST_SANITIZE_LEAK_LOG=true
ci/run-build-and-tests.sh +3 -0
index 7a1466b868..c192bd613c 100755
@@ -37,6 +37,9 @@ linux-clang)
 linux-sha256)
 	export GIT_TEST_DEFAULT_HASH=sha256
 	;;
+linux-reftable|linux-reftable-leaks|osx-reftable)
+	export GIT_TEST_DEFAULT_REF_FORMAT=reftable
+	;;
 pedantic)
 	# Don't run the tests; we only care about whether Git can be
 	# built.
Signature · 1 line
2.43.GIT
#19 lore raw
In reply to Patrick Steinhardt
Quoted 14 lines · Patrick Steinhardt
refs/reftable-backend.c new file +13 -0
+static int write_copy_table(struct reftable_writer *writer, void *cb_data)
+{
[...]
+	/*
+	 * Create the reflog entry for the newly created branch.
+	 */
+	ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
+	memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
+	fill_reftable_log_record(&logs[logs_nr]);
+	logs[logs_nr].refname = (char *)arg->newname;
+	logs[logs_nr].update_index = creation_ts;
+	logs[logs_nr].value.update.message =
+		xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
+	logs[logs_nr].value.update.new_hash = old_ref.value.val1;

refs/reftable-backend.c line 1506

Quoted 10 lines
refs/reftable-backend.c new file +10 -0
+	logs_nr++;
+
+	/*
+	 * In addition to writing the reflog entry for the new branch, we also
+	 * copy over all log entries from the old reflog. Last but not least,
+	 * when renaming we also have to delete all the old reflog entries.
+	 */
+	ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
+	if (ret < 0)
+		return ret;

Should this last line be "goto done" as is used elsewhere in the function? Otherwise we are at least leaking the "logs" array (and possibly some of the other cleanup is important, too).

refs/reftable-backend.c line 1515

Quoted 8 lines
refs/reftable-backend.c new file +8 -0
+	while (1) {
+		ret = reftable_iterator_next_log(&it, &old_log);
+		if (ret < 0)
+			goto done;
+		if (ret > 0 || strcmp(old_log.refname, arg->oldname)) {
+			ret = 0;
+			break;
+		}

This "ret = 0" doesn't have any effect. We break out of the loop, and then...

refs/reftable-backend.c line 1196

Quoted 5 lines
refs/reftable-backend.c new file +5 -0
+	}
+
+	ret = reftable_writer_add_logs(writer, logs, logs_nr);
+	if (ret < 0)
+		goto done;

...the first thing we do is write over it. I dunno if it's worth keeping as a maintenance precaution, though (if the code after the loop changed to omit that assignment, then setting "ret" would become important).

Both were noticed by Coverity (along with several other false positives).

-Peff

#20 lore raw
Quoted 22 lines · Jeff King
Quoted 20 lines · Patrick Steinhardt
refs/reftable-backend.c new file +19 -0
+static int write_copy_table(struct reftable_writer *writer, void *cb_data)
+{
[...]
+	/*
+	 * Create the reflog entry for the newly created branch.
+	 */
+	ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
+	memset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));
+	fill_reftable_log_record(&logs[logs_nr]);
+	logs[logs_nr].refname = (char *)arg->newname;
+	logs[logs_nr].update_index = creation_ts;
+	logs[logs_nr].value.update.message =
+		xstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);
+	logs[logs_nr].value.update.new_hash = old_ref.value.val1;
+	logs_nr++;
+
+	/*
+	 * In addition to writing the reflog entry for the new branch, we also
+	 * copy over all log entries from the old reflog. Last but not least,
+	 * when renaming we also have to delete all the old reflog entries.
Quoted 8 lines

refs/reftable-backend.c line 1506

Quoted 4 lines
refs/reftable-backend.c new file +4 -0
+	 */
+	ret = reftable_merged_table_seek_log(mt, &it, arg->oldname);
+	if (ret < 0)
+		return ret;

Should this last line be "goto done" as is used elsewhere in the function? Otherwise we are at least leaking the "logs" array (and possibly some of the other cleanup is important, too).

Indeed we should.

Quoted 12 lines

refs/reftable-backend.c line 1515

Quoted 8 lines
refs/reftable-backend.c new file +8 -0
+	while (1) {
+		ret = reftable_iterator_next_log(&it, &old_log);
+		if (ret < 0)
+			goto done;
+		if (ret > 0 || strcmp(old_log.refname, arg->oldname)) {
+			ret = 0;
+			break;
+		}

This "ret = 0" doesn't have any effect. We break out of the loop, and then...

Quoted 9 lines

refs/reftable-backend.c line 1196

Quoted 5 lines
refs/reftable-backend.c new file +5 -0
+	}
+
+	ret = reftable_writer_add_logs(writer, logs, logs_nr);
+	if (ret < 0)
+		goto done;

...the first thing we do is write over it. I dunno if it's worth keeping as a maintenance precaution, though (if the code after the loop changed to omit that assignment, then setting "ret" would become important).

Yeah, I think this one we should keep. It's also a repeating pattern that we have in many other places, so it helps lower the mental burden when it looks similar to all the others.

Quoted 2 lines

Both were noticed by Coverity (along with several other false positives).

Is the Coverity instance publicly accessible? If not, can you maybe invite me so that I get a better feeling for them?

I'll send a patch to fix the missing goto done above, thanks!

Patrick

#21 lore raw
Quoted 11 lines · Patrick Steinhardt
Quoted 7 lines

refs/reftable-backend.c line 1196

Quoted 3 lines
refs/reftable-backend.c new file +3 -0
+	ret = reftable_writer_add_logs(writer, logs, logs_nr);
+	if (ret < 0)
+		goto done;

...the first thing we do is write over it. I dunno if it's worth keeping as a maintenance precaution, though (if the code after the loop changed to omit that assignment, then setting "ret" would become important).

Yeah, I think this one we should keep. It's also a repeating pattern that we have in many other places, so it helps lower the mental burden when it looks similar to all the others.

That sounds quite reasonable to me.

Quoted 5 lines
Quoted 2 lines

Both were noticed by Coverity (along with several other false positives).

Is the Coverity instance publicly accessible? If not, can you maybe invite me so that I get a better feeling for them?

I don't think there's a way to make it publicly accessible. Probably I could invite you to the project if you sign up for a Coverity account. But it's a build of next plus my personal in-progress topics, which is probably not ideal for other people to look at.

There are instructions for setting up your own scan in a56b6230d0 (ci:
add a GitHub workflow to submit Coverity scans, 2023-09-25). The unsaid part there is signing up for Coverity and registering a project to get the necessary tokens. Probably start here:

https://scan.coverity.com/users/sign_up

but I don't remember the details.

The Actions workflow is in "master", so in theory we could register a project for git.git and get automatic builds when Junio pushes to 'next', etc. In practice I have found that it requires a human looking over the results, but if people at least had access they could poke around.

(I suspect it would also be easy to port the workflow over to run on GitLab CI, as well, if you wanted to).

-Peff