From 82ddde639f104d0e7a963020e215d656cc8d9adf Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Fri, 21 Oct 2022 10:19:34 -0400 Subject: [PATCH 01/10] hashfile: allow skipping the hash function The hashfile API is useful for generating files that include a trailing hash of the file's contents up to that point. Using such a hash is helpful for verifying the file for corruption-at-rest, such as a faulty drive causing flipped bits. Since the commit-graph and multi-pack-index files both use this trailing hash, the chunk-format API uses a 'struct hashfile' to handle the I/O to the file. This was very convenient to allow using the hashfile methods during these operations. However, hashing the file contents during write comes at a performance penalty. It's slower to hash the bytes on their way to the disk than without that step. If we wish to use the chunk-format API to upgrade other file types, then this hashing is a performance penalty that might not be worth the benefit of a trailing hash. For example, if we create a chunk-format version of the packed-refs file, then the file format could shrink by using raw object IDs instead of hexadecimal representations in ASCII. That reduction in size is not enough to counteract the performance penalty of hashing the file contents. In cases such as deleting a reference that appears in the packed-refs file, that write-time performance is critical. This is in contrast to the commit-graph and multi-pack-index files which are mainly updated in non-critical paths such as background maintenance. One way to allow future chunked formats to not suffer this penalty would be to create an abstraction layer around the 'struct hashfile' using a vtable of function pointers. This would allow placing a different representation in place of the hashfile. This option would be cumbersome for a few reasons. First, the hashfile's buffered writes are already highly optimized and would need to be duplicated in another code path. The second is that the chunk-format API calls the chunk_write_fn pointers using a hashfile. If we change that to an abstraction layer, then those that _do_ use the hashfile API would need to change all of their instances of hashwrite(), hashwrite_be32(), and others to use the new abstraction layer. Instead, this change opts for a simpler change. Introduce a new 'skip_hash' option to 'struct hashfile'. When set, the update_fn and final_fn members of the_hash_algo are skipped. When finalizing the hashfile, the trailing hash is replaced with the null hash. This use of a trailing null hash would be desireable in either case, since we do not want to special case a file format to have a different length depending on whether it was hashed or not. When the final bytes of a file are all zero, we can infer that it was written without hashing, and thus that verification is not available as a check for file consistency. This also means that we could easily toggle hashing for any file format we desire. For the commit-graph and multi-pack-index file, it may be possible to allow the null hash without incrementing the file format version, since it technically fits the structure of the file format. The only issue is that older versions would trigger a failure during 'git fsck'. For these file formats, we may want to delay such a change until it is justified. However, the index file is written in critical paths. It is also frequently updated, so corruption at rest is less likely to be an issue than in those other file formats. This could be a good candidate to create an option that skips the hashing operation. A version of this patch has existed in the microsoft/git fork since 2017 [1] (the linked commit was rebased in 2018, but the original dates back to January 2017). Here, the change to make the index use this fast path is delayed until a later change. [1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201 Co-authored-by: Kevin Willford Signed-off-by: Kevin Willford Signed-off-by: Derrick Stolee --- csum-file.c | 14 +++++++++++--- csum-file.h | 7 +++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/csum-file.c b/csum-file.c index 59ef3398ca2b01..3243473c3d7e45 100644 --- a/csum-file.c +++ b/csum-file.c @@ -45,7 +45,8 @@ void hashflush(struct hashfile *f) unsigned offset = f->offset; if (offset) { - the_hash_algo->update_fn(&f->ctx, f->buffer, offset); + if (!f->skip_hash) + the_hash_algo->update_fn(&f->ctx, f->buffer, offset); flush(f, f->buffer, offset); f->offset = 0; } @@ -64,7 +65,12 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result, int fd; hashflush(f); - the_hash_algo->final_fn(f->buffer, &f->ctx); + + if (f->skip_hash) + memset(f->buffer, 0, the_hash_algo->rawsz); + else + the_hash_algo->final_fn(f->buffer, &f->ctx); + if (result) hashcpy(result, f->buffer); if (flags & CSUM_HASH_IN_STREAM) @@ -108,7 +114,8 @@ void hashwrite(struct hashfile *f, const void *buf, unsigned int count) * the hashfile's buffer. In this block, * f->offset is necessarily zero. */ - the_hash_algo->update_fn(&f->ctx, buf, nr); + if (!f->skip_hash) + the_hash_algo->update_fn(&f->ctx, buf, nr); flush(f, buf, nr); } else { /* @@ -153,6 +160,7 @@ static struct hashfile *hashfd_internal(int fd, const char *name, f->tp = tp; f->name = name; f->do_crc = 0; + f->skip_hash = 0; the_hash_algo->init_fn(&f->ctx); f->buffer_len = buffer_len; diff --git a/csum-file.h b/csum-file.h index 0d29f528fbcb51..29468067f81880 100644 --- a/csum-file.h +++ b/csum-file.h @@ -20,6 +20,13 @@ struct hashfile { size_t buffer_len; unsigned char *buffer; unsigned char *check_buffer; + + /** + * If set to 1, skip_hash indicates that we should + * not actually compute the hash for this hashfile and + * instead only use it as a buffered write. + */ + unsigned int skip_hash; }; /* Checkpoint */ From ca339b0b7d0d395a0c03eb16d552bd1f9f5c6de7 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 24 Oct 2022 15:54:15 -0400 Subject: [PATCH 02/10] read-cache: add index.computeHash config option MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous change allowed skipping the hashing portion of the hashwrite API, using it instead as a buffered write API. Disabling the hashwrite can be particularly helpful when the write operation is in a critical path. One such critical path is the writing of the index. This operation is so critical that the sparse index was created specifically to reduce the size of the index to make these writes (and reads) faster. Following a similar approach to one used in the microsoft/git fork [1], add a new config option that allows disabling this hashing during the index write. The cost is that we can no longer validate the contents for corruption-at-rest using the trailing hash. [1] https://github.com/microsoft/git/commit/21fed2d91410f45d85279467f21d717a2db45201 While older Git versions will not recognize the null hash as a special case, the file format itself is still being met in terms of its structure. Using this null hash will still allow Git operations to function across older versions. The one exception is 'git fsck' which checks the hash of the index file. Here, we disable this check if the trailing hash is all zeroes. We add a warning to the config option that this may cause undesirable behavior with older Git versions. As a quick comparison, I tested 'git update-index --force-write' with and without index.computHash=false on a copy of the Linux kernel repository. Benchmark 1: with hash Time (mean ± σ): 46.3 ms ± 13.8 ms [User: 34.3 ms, System: 11.9 ms] Range (min … max): 34.3 ms … 79.1 ms 82 runs Benchmark 2: without hash Time (mean ± σ): 26.0 ms ± 7.9 ms [User: 11.8 ms, System: 14.2 ms] Range (min … max): 16.3 ms … 42.0 ms 69 runs Summary 'without hash' ran 1.78 ± 0.76 times faster than 'with hash' These performance benefits are substantial enough to allow users the ability to opt-in to this feature, even with the potential confusion with older 'git fsck' versions. Signed-off-by: Derrick Stolee --- Documentation/config/index.txt | 8 ++++++++ read-cache.c | 22 +++++++++++++++++++++- t/t1600-index.sh | 8 ++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/Documentation/config/index.txt b/Documentation/config/index.txt index 75f3a2d1054146..709ba72f62208b 100644 --- a/Documentation/config/index.txt +++ b/Documentation/config/index.txt @@ -30,3 +30,11 @@ index.version:: Specify the version with which new index files should be initialized. This does not affect existing repositories. If `feature.manyFiles` is enabled, then the default is 4. + +index.computeHash:: + When enabled, compute the hash of the index file as it is written + and store the hash at the end of the content. This is enabled by + default. ++ +If you disable `index.computHash`, then older Git clients may report that +your index is corrupt during `git fsck`. diff --git a/read-cache.c b/read-cache.c index 32024029274828..f24d96de4d360a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1817,6 +1817,8 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size) git_hash_ctx c; unsigned char hash[GIT_MAX_RAWSZ]; int hdr_version; + int all_zeroes = 1; + unsigned char *start, *end; if (hdr->hdr_signature != htonl(CACHE_SIGNATURE)) return error(_("bad signature 0x%08x"), hdr->hdr_signature); @@ -1827,10 +1829,23 @@ static int verify_hdr(const struct cache_header *hdr, unsigned long size) if (!verify_index_checksum) return 0; + end = (unsigned char *)hdr + size; + start = end - the_hash_algo->rawsz; + while (start < end) { + if (*start != 0) { + all_zeroes = 0; + break; + } + start++; + } + + if (all_zeroes) + return 0; + the_hash_algo->init_fn(&c); the_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz); the_hash_algo->final_fn(hash, &c); - if (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz)) + if (!hasheq(hash, end - the_hash_algo->rawsz)) return error(_("bad index file sha1 signature")); return 0; } @@ -2917,9 +2932,14 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile, int ieot_entries = 1; struct index_entry_offset_table *ieot = NULL; int nr, nr_threads; + int compute_hash; f = hashfd(tempfile->fd, tempfile->filename.buf); + if (!git_config_get_maybe_bool("index.computehash", &compute_hash) && + !compute_hash) + f->skip_hash = 1; + for (i = removed = extended = 0; i < entries; i++) { if (cache[i]->ce_flags & CE_REMOVE) removed++; diff --git a/t/t1600-index.sh b/t/t1600-index.sh index 010989f90e63f9..24ab90ca0478fe 100755 --- a/t/t1600-index.sh +++ b/t/t1600-index.sh @@ -103,4 +103,12 @@ test_expect_success 'index version config precedence' ' test_index_version 0 true 2 2 ' +test_expect_success 'index.computeHash config option' ' + ( + rm -f .git/index && + git -c index.computeHash=false add a && + git fsck + ) +' + test_done From b3e18dccd8892e5dc8690a22b39b1a83520efacf Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 10 Oct 2022 15:50:55 -0400 Subject: [PATCH 03/10] extensions: add refFormat extension Git's reference storage is critical to its function. Creating new storage formats for references requires adding an extension. This prevents third-party tools that do not understand that format from operating incorrectly on the repository. This makes updating ref formats more difficult than other optional indexes, such as the commit-graph or multi-pack-index. However, there are a number of potential ref storage enhancements that are underway or could be created. Git needs an established mechanism for coordinating between these different options. The first obvious format update is the reftable format as documented in Documentation/technical/reftable.txt. This format has much of its implementation already in Git, but its connection as a ref backend is not complete. This change is similar to some changes within one of the patches intended for the reftable effort [1]. [1] https://lore.kernel.org/git/pull.1215.git.git.1644351400761.gitgitgadget@gmail.com/ However, this change makes a distinct strategy change from the one recommended by reftable. Here, the extensions.refFormat extension is provided as a multi-valued list. In the reftable RFC, the extension has a single value, "files" or "reftable" and explicitly states that this should not change after 'git init' or 'git clone'. The single-valued approach has some major drawbacks, including the idea that the "files" backend cannot coexist with the "reftable" backend at the same time. In this way, it would not be possible to create a repository that can write loose references and combine them into a reftable in the background. With the multi-valued approach, we could integrate reftable as a drop-in replacement for the packed-refs file and allow that to be a faster way to do the integration since the test suite would only need updates when the test is explicitly testing packed-refs. When upgrading a repository from the "files" backend to the "reftable" backend, it can help to have a transition period where both are present, then finally removing the "files" backend after all loose refs are collected into the reftable. But the reftable is not the only approach available. One obvious improvement could be a new file format version for the packed-refs file. Its current plaintext-based format is inefficient due to storing object IDs as hexadecimal representations instead of in their raw format. This extra cost will get worse with SHA-256. In addition, binary searches need to guess a position and scan to find newlines for a refname entry. A structured binary format could allow for more compact representation and faster access. Adding such a format could be seen as "files-v2", but it is really "packed-v2". The reftable approach has a concept of a "stack" of reftable files. This idea would also work for a stack of packed-refs files (in v1 or v2 format). It would be helpful to describe that the refs could be stored in a stack of packed-ref files independently of whether that is in file format v1 or v2. Even in these two options, it might be helpful to indicate whether or not loose ref files are present. That is one reason to not make them appear as "files-v2" or "files-v3" options in a single-valued extension. Even as "packed-v2" or "packed-v3" options, this approach would require third-party tools to understand the "v2" version if they want to support the "v3" options. Instead, by splitting the format from the layout, we can allow third-party tools to integrate only with the most-desired format options. For these reasons, this change is defining the extensions.refFormat extension as well as how the two existing values interact. By default, Git will assume "files" and "packed" in the list. If any other value is provided, then the extension is marked as unrecognized. Add tests that check the behavior of extensions.refFormat, both in that it requires core.repositoryFormatVersion=1, and Git will refuse to work with an unknown value of the extension. There is a gap in the current implementation, though. What happens if exactly one of "files" or "packed" is provided? The presence of only one would imply that the other is not available. A later change can communicate the list contents to the repository struct and then the reference backend could ignore one of these two layers. Specifically, having only "files" would mean that Git should not read or write the packed-refs file and instead only read and write loose ref files. By contrast, having only "packed" would mean that Git should not read or write loose ref files and instead always update the packed-refs file on every ref update. Signed-off-by: Derrick Stolee --- Documentation/config/extensions.txt | 25 +++++++++++++++++++++++++ setup.c | 5 +++++ t/t3212-ref-formats.sh | 27 +++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100755 t/t3212-ref-formats.sh diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt index bccaec7a963679..6eb9a380ba9c0e 100644 --- a/Documentation/config/extensions.txt +++ b/Documentation/config/extensions.txt @@ -7,6 +7,31 @@ Note that this setting should only be set by linkgit:git-init[1] or linkgit:git-clone[1]. Trying to change it after initialization will not work and will produce hard-to-diagnose issues. +extensions.refFormat:: + Specify the reference storage mechanisms used by the repoitory as a + multi-valued list. The acceptable values are `files` and `packed`. + If not specified, the list of `files` and `packed` is assumed. It + is an error to specify this key unless `core.repositoryFormatVersion` + is 1. ++ +As new ref formats are added, Git commands may modify this list before and +after upgrading the on-disk reference storage files. The specific values +indicate the existence of different layers: ++ +* `files`: When present, references may be stored as "loose" reference files + in the `$GIT_DIR/refs/` directory. The name of the reference corresponds + to the filename after `$GIT_DIR` and the file contains an object ID as + a hexadecimal string. If a loose reference file exists, then its value + takes precedence over all other formats. ++ +* `packed`: When present, references may be stored as a group in a + `packed-refs` file in its version 1 format. When grouped with `"files"` + or provided on its own, this file is located at `$GIT_DIR/packed-refs`. + This file contains a list of distinct reference names, paired with their + object IDs. When combined with `files`, the `packed` format will only be + used to group multiple loose object files upon request via the + `git pack-refs` command or via the `pack-refs` maintenance task. + extensions.worktreeConfig:: If enabled, then worktrees will load config settings from the `$GIT_DIR/config.worktree` file in addition to the diff --git a/setup.c b/setup.c index cefd5f63c4680f..f5eb50c969a9d5 100644 --- a/setup.c +++ b/setup.c @@ -577,6 +577,11 @@ static enum extension_result handle_extension(const char *var, "extensions.objectformat", value); data->hash_algo = format; return EXTENSION_OK; + } else if (!strcmp(ext, "refformat")) { + if (strcmp(value, "files") && strcmp(value, "packed")) + return error(_("invalid value for '%s': '%s'"), + "extensions.refFormat", value); + return EXTENSION_OK; } return EXTENSION_UNKNOWN; } diff --git a/t/t3212-ref-formats.sh b/t/t3212-ref-formats.sh new file mode 100755 index 00000000000000..bc554e7c7011c5 --- /dev/null +++ b/t/t3212-ref-formats.sh @@ -0,0 +1,27 @@ +#!/bin/sh + +test_description='test across ref formats' + +. ./test-lib.sh + +test_expect_success 'extensions.refFormat requires core.repositoryFormatVersion=1' ' + test_when_finished rm -rf broken && + + # Force sha1 to ensure GIT_TEST_DEFAULT_HASH does + # not imply a value of core.repositoryFormatVersion. + git init --object-format=sha1 broken && + git -C broken config extensions.refFormat files && + test_must_fail git -C broken status 2>err && + grep "repo version is 0, but v1-only extension found" err +' + +test_expect_success 'invalid extensions.refFormat' ' + test_when_finished rm -rf broken && + git init broken && + git -C broken config core.repositoryFormatVersion 1 && + git -C broken config extensions.refFormat bogus && + test_must_fail git -C broken status 2>err && + grep "invalid value for '\''extensions.refFormat'\'': '\''bogus'\''" err +' + +test_done From 8e3cad1ac515adbbff0f3d8e4d30856a3b18b981 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 24 Oct 2022 10:46:37 -0400 Subject: [PATCH 04/10] repository: wire ref extensions to ref backends The previous change introduced the extensions.refFormat config option. It is a multi-valued config option that currently understands "files" and "packed", with both values assumed by default. If any value is provided explicitly, this default is ignored and the provided settings are used instead. The multi-valued nature of this extension presents a way to allow a user to specify that they never want a packed-refs file (only use "files") or that they never want loose reference files (only use "packed"). However, that functionality is not currently connected. Before actually modifying the files backend to understand these extension settings, do the basic wiring that connects the extensions.refFormat parsing to the creation of the ref backend. A future change will actually change the ref backend initialization based on these settings, but this communication of the extension is sufficiently complicated to be worth an isolated change. For now, also forbid the setting of only "packed". This is done by redirecting the choice of backend to the packed backend when that selection is made. A later change will make the "files"-only extension value ignore the packed backend. Signed-off-by: Derrick Stolee --- cache.h | 2 ++ refs.c | 22 ++++++++++++++++++++-- refs/files-backend.c | 2 +- refs/refs-internal.h | 3 +++ repository.c | 2 ++ repository.h | 6 ++++++ setup.c | 18 +++++++++++++++++- t/t3212-ref-formats.sh | 12 ++++++++++++ 8 files changed, 63 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index 26ed03bd6de626..13e9c251ac3262 100644 --- a/cache.h +++ b/cache.h @@ -1155,6 +1155,8 @@ struct repository_format { int hash_algo; int sparse_index; char *work_tree; + int ref_format_count; + enum ref_format_flags ref_format; struct string_list unknown_extensions; struct string_list v1_only_extensions; }; diff --git a/refs.c b/refs.c index c89d558892569b..0cbe96bedf23b0 100644 --- a/refs.c +++ b/refs.c @@ -1956,6 +1956,15 @@ static struct ref_store *lookup_ref_store_map(struct hashmap *map, return entry ? entry->refs : NULL; } +static int add_ref_format_flags(enum ref_format_flags flags, int caps) { + if (flags & REF_FORMAT_FILES) + caps |= REF_STORE_FORMAT_FILES; + if (flags & REF_FORMAT_PACKED) + caps |= REF_STORE_FORMAT_PACKED; + + return caps; +} + /* * Create, record, and return a ref_store instance for the specified * gitdir. @@ -1965,9 +1974,17 @@ static struct ref_store *ref_store_init(struct repository *repo, unsigned int flags) { const char *be_name = "files"; - struct ref_storage_be *be = find_ref_storage_backend(be_name); + struct ref_storage_be *be; struct ref_store *refs; + flags = add_ref_format_flags(repo->ref_format, flags); + + if (!(flags & REF_STORE_FORMAT_FILES) && + (flags & REF_STORE_FORMAT_PACKED)) + be_name = "packed"; + + be = find_ref_storage_backend(be_name); + if (!be) BUG("reference backend %s is unknown", be_name); @@ -1983,7 +2000,8 @@ struct ref_store *get_main_ref_store(struct repository *r) if (!r->gitdir) BUG("attempting to get main_ref_store outside of repository"); - r->refs_private = ref_store_init(r, r->gitdir, REF_STORE_ALL_CAPS); + r->refs_private = ref_store_init(r, r->gitdir, + REF_STORE_ALL_CAPS); r->refs_private = maybe_debug_wrap_ref_store(r->gitdir, r->refs_private); return r->refs_private; } diff --git a/refs/files-backend.c b/refs/files-backend.c index e4009b3c421f5b..97d6deae95afad 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3282,7 +3282,7 @@ static int files_init_db(struct ref_store *ref_store, struct strbuf *err UNUSED) } struct ref_storage_be refs_be_files = { - .next = NULL, + .next = &refs_be_packed, .name = "files", .init = files_ref_store_create, .init_db = files_init_db, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 69f93b0e2ac9fa..41520c945e4862 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -521,6 +521,9 @@ struct ref_store; REF_STORE_ODB | \ REF_STORE_MAIN) +#define REF_STORE_FORMAT_FILES (1 << 8) /* can use loose ref files */ +#define REF_STORE_FORMAT_PACKED (1 << 9) /* can use packed-refs file */ + /* * Initialize the ref_store for the specified gitdir. These functions * should call base_ref_store_init() to initialize the shared part of diff --git a/repository.c b/repository.c index 5d166b692c8aa8..96533fc76be3a6 100644 --- a/repository.c +++ b/repository.c @@ -182,6 +182,8 @@ int repo_init(struct repository *repo, repo->repository_format_partial_clone = format.partial_clone; format.partial_clone = NULL; + repo->ref_format = format.ref_format; + if (worktree) repo_set_worktree(repo, worktree); diff --git a/repository.h b/repository.h index 24316ac944edcd..5cfde4282c50c0 100644 --- a/repository.h +++ b/repository.h @@ -61,6 +61,11 @@ struct repo_path_cache { char *shallow; }; +enum ref_format_flags { + REF_FORMAT_FILES = (1 << 0), + REF_FORMAT_PACKED = (1 << 1), +}; + struct repository { /* Environment */ /* @@ -95,6 +100,7 @@ struct repository { * the ref object. */ struct ref_store *refs_private; + enum ref_format_flags ref_format; /* * Contains path to often used file names. diff --git a/setup.c b/setup.c index f5eb50c969a9d5..a5e63479558a94 100644 --- a/setup.c +++ b/setup.c @@ -578,9 +578,14 @@ static enum extension_result handle_extension(const char *var, data->hash_algo = format; return EXTENSION_OK; } else if (!strcmp(ext, "refformat")) { - if (strcmp(value, "files") && strcmp(value, "packed")) + if (!strcmp(value, "files")) + data->ref_format |= REF_FORMAT_FILES; + else if (!strcmp(value, "packed")) + data->ref_format |= REF_FORMAT_PACKED; + else return error(_("invalid value for '%s': '%s'"), "extensions.refFormat", value); + data->ref_format_count++; return EXTENSION_OK; } return EXTENSION_UNKNOWN; @@ -723,6 +728,11 @@ int read_repository_format(struct repository_format *format, const char *path) git_config_from_file(check_repo_format, path, format); if (format->version == -1) clear_repository_format(format); + + /* Set default ref_format if no extensions.refFormat exists. */ + if (!format->ref_format_count) + format->ref_format = REF_FORMAT_FILES | REF_FORMAT_PACKED; + return format->version; } @@ -1425,6 +1435,9 @@ int discover_git_directory(struct strbuf *commondir, candidate.partial_clone; candidate.partial_clone = NULL; + /* take ownership of candidate.ref_format */ + the_repository->ref_format = candidate.ref_format; + clear_repository_format(&candidate); return 0; } @@ -1561,6 +1574,8 @@ const char *setup_git_directory_gently(int *nongit_ok) the_repository->repository_format_partial_clone = repo_fmt.partial_clone; repo_fmt.partial_clone = NULL; + + the_repository->ref_format = repo_fmt.ref_format; } } /* @@ -1650,6 +1665,7 @@ void check_repository_format(struct repository_format *fmt) repo_set_hash_algo(the_repository, fmt->hash_algo); the_repository->repository_format_partial_clone = xstrdup_or_null(fmt->partial_clone); + the_repository->ref_format = fmt->ref_format; clear_repository_format(&repo_fmt); } diff --git a/t/t3212-ref-formats.sh b/t/t3212-ref-formats.sh index bc554e7c7011c5..8c4e70196a021a 100755 --- a/t/t3212-ref-formats.sh +++ b/t/t3212-ref-formats.sh @@ -24,4 +24,16 @@ test_expect_success 'invalid extensions.refFormat' ' grep "invalid value for '\''extensions.refFormat'\'': '\''bogus'\''" err ' +test_expect_success 'extensions.refFormat=packed only' ' + git init only-packed && + ( + cd only-packed && + git config core.repositoryFormatVersion 1 && + git config extensions.refFormat packed && + test_commit A && + test_path_exists .git/packed-refs && + test_path_is_missing .git/refs/tags/A + ) +' + test_done From 196d9e8d780adcd21fc9f516b0808500f6a183f8 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Mon, 24 Oct 2022 12:21:25 -0400 Subject: [PATCH 05/10] refs: allow loose files without packed-refs The extensions.refFormat extension is a multi-valued config that specifies which ref formats are available to the current repository. By default, Git assumes the list of "files" and "packed", unless there is at least one of these extensions specified. With the current values, it is possible for a user to specify only "files" or only "packed". The only-"packed" option was already ruled as invalid since Git's current code has too many places that require a loose reference. This could change in the future. However, we can now allow the user to specify extensions.refFormat=files alone, making it impossible to create a packed-refs file (or to read one that might exist). Signed-off-by: Derrick Stolee --- refs/files-backend.c | 6 ++++++ refs/packed-backend.c | 3 +++ refs/refs-internal.h | 5 +++++ t/t3212-ref-formats.sh | 20 ++++++++++++++++++++ 4 files changed, 34 insertions(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index 97d6deae95afad..9848c010ea94d4 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1207,6 +1207,12 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags) struct strbuf err = STRBUF_INIT; struct ref_transaction *transaction; + if (!packed_refs_enabled(refs->store_flags)) { + warning(_("refusing to create '%s' file because '%s' is not set"), + "packed-refs", "extensions.refFormat=packed"); + return -1; + } + transaction = ref_store_transaction_begin(refs->packed_ref_store, &err); if (!transaction) return -1; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 43cdb97f8b3775..120500050cf26f 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -478,6 +478,9 @@ static int load_contents(struct snapshot *snapshot) size_t size; ssize_t bytes_read; + if (!packed_refs_enabled(snapshot->refs->store_flags)) + return 0; + fd = open(snapshot->refs->path, O_RDONLY); if (fd < 0) { if (errno == ENOENT) { diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 41520c945e4862..a1900848a878e4 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -524,6 +524,11 @@ struct ref_store; #define REF_STORE_FORMAT_FILES (1 << 8) /* can use loose ref files */ #define REF_STORE_FORMAT_PACKED (1 << 9) /* can use packed-refs file */ +static inline int packed_refs_enabled(int flags) +{ + return flags & REF_STORE_FORMAT_PACKED; +} + /* * Initialize the ref_store for the specified gitdir. These functions * should call base_ref_store_init() to initialize the shared part of diff --git a/t/t3212-ref-formats.sh b/t/t3212-ref-formats.sh index 8c4e70196a021a..67aa65c116f642 100755 --- a/t/t3212-ref-formats.sh +++ b/t/t3212-ref-formats.sh @@ -36,4 +36,24 @@ test_expect_success 'extensions.refFormat=packed only' ' ) ' +test_expect_success 'extensions.refFormat=files only' ' + test_commit T && + git pack-refs --all && + git init only-loose && + ( + cd only-loose && + git config core.repositoryFormatVersion 1 && + git config extensions.refFormat files && + test_commit A && + test_commit B && + test_must_fail git pack-refs 2>err && + grep "refusing to create" err && + test_path_is_missing .git/packed-refs && + + # Refuse to parse a packed-refs file. + cp ../.git/packed-refs .git/packed-refs && + test_must_fail git rev-parse refs/tags/T + ) +' + test_done From 9f33e4e2add9853024ead6505a220dff8e5cc70a Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 1 Nov 2022 08:29:51 -0400 Subject: [PATCH 06/10] chunk-format: number of chunks is optional Even though the commit-graph and multi-pack-index file formats specify a number of chunks in their header information, this is optional. The table of contents terminates with a null chunk ID, which can be used instead. The extra value is helpful for some checks, but is ultimately not necessary for the format. This will be important in some future formats. Signed-off-by: Derrick Stolee --- Documentation/gitformat-chunk.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/gitformat-chunk.txt b/Documentation/gitformat-chunk.txt index 57202ede273ad2..c01f5567c4f7af 100644 --- a/Documentation/gitformat-chunk.txt +++ b/Documentation/gitformat-chunk.txt @@ -24,8 +24,9 @@ how they use the chunks to describe structured data. A chunk-based file format begins with some header information custom to that format. That header should include enough information to identify -the file type, format version, and number of chunks in the file. From this -information, that file can determine the start of the chunk-based region. +the file type, format version, and (optionally) the number of chunks in +the file. From this information, that file can determine the start of the +chunk-based region. The chunk-based region starts with a table of contents describing where each chunk starts and ends. This consists of (C+1) rows of 12 bytes each, From 2c4987215834b7e337609478cb35315280d7a9b9 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 1 Nov 2022 08:35:42 -0400 Subject: [PATCH 07/10] chunk-format: document trailing table of contents It will be helpful to allow a trailing table of contents when writing some file types with the chunk-format API. The main reason is that it allows dynamically computing the chunk sizes while writing the file. This can use fewer resources than precomputing all chunk sizes in advance. Signed-off-by: Derrick Stolee --- Documentation/gitformat-chunk.txt | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/Documentation/gitformat-chunk.txt b/Documentation/gitformat-chunk.txt index c01f5567c4f7af..ee3718c4306d14 100644 --- a/Documentation/gitformat-chunk.txt +++ b/Documentation/gitformat-chunk.txt @@ -52,8 +52,27 @@ The final entry in the table of contents must be four zero bytes. This confirms that the table of contents is ending and provides the offset for the end of the chunk-based data. +The default chunk format assumes the table of contents appears at the +beginning of the file (after the header information) and the chunks are +ordered by increasing offset. Alternatively, the chunk format allows a +table of contents that is placed at the end of the file (before the +trailing hash) and the offsets are in descending order. In this trailing +table of contents case, the data in order looks instead like the following +table: + + | Chunk ID (4 bytes) | Chunk Offset (8 bytes) | + |--------------------|------------------------| + | 0x0000 | OFFSET[C+1] | + | ID[C] | OFFSET[C] | + | ... | ... | + | ID[0] | OFFSET[0] | + +The concrete file format that uses the chunk format will mention that it +uses a trailing table of contents if it uses it. By default, the table of +contents is in ascending order before all chunk data. + Note: The chunk-based format expects that the file contains _at least_ a -trailing hash after `OFFSET[C+1]`. +trailing hash after either `OFFSET[C+1]` or the trailing table of contents. Functions for working with chunk-based file formats are declared in `chunk-format.h`. Using these methods provide extra checks that assist From 239eab411b396fa007da73794c63389110fdbdb4 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 5 Oct 2022 10:26:58 -0400 Subject: [PATCH 08/10] chunk-format: store chunk offset during write As a preparatory step to allowing trailing table of contents, store the offsets of each chunk as we write them. This replaces an existing use of a local variable, but the stored value will be used in the next change. Signed-off-by: Derrick Stolee --- chunk-format.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/chunk-format.c b/chunk-format.c index 0275b74a895a17..f1b2c8a8b36f87 100644 --- a/chunk-format.c +++ b/chunk-format.c @@ -13,6 +13,7 @@ struct chunk_info { chunk_write_fn write_fn; const void *start; + off_t offset; }; struct chunkfile { @@ -78,16 +79,16 @@ int write_chunkfile(struct chunkfile *cf, void *data) hashwrite_be64(cf->f, cur_offset); for (i = 0; i < cf->chunks_nr; i++) { - off_t start_offset = hashfile_total(cf->f); + cf->chunks[i].offset = hashfile_total(cf->f); result = cf->chunks[i].write_fn(cf->f, data); if (result) goto cleanup; - if (hashfile_total(cf->f) - start_offset != cf->chunks[i].size) + if (hashfile_total(cf->f) - cf->chunks[i].offset != cf->chunks[i].size) BUG("expected to write %"PRId64" bytes to chunk %"PRIx32", but wrote %"PRId64" instead", cf->chunks[i].size, cf->chunks[i].id, - hashfile_total(cf->f) - start_offset); + hashfile_total(cf->f) - cf->chunks[i].offset); } cleanup: From 7fca0e5c9e7f2258047bd2f3e18c1ec4c15260df Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 5 Oct 2022 10:12:13 -0400 Subject: [PATCH 09/10] chunk-format: allow trailing table of contents The existing chunk formats use the table of contents at the beginning of the file. This is intended as a way to speed up the initial loading of the file, but comes at a cost during writes. Each example needs to fully compute how big each chunk will be in advance, which usually requires storing the full file contents in memory. Future file formats may want to use the chunk format API in cases where the writing stage is critical to performance, so we may want to stream updates from an existing file and then only write the table of contents at the end. Add a new 'flags' parameter to write_chunkfile() that allows this behavior. When this is specified, the defensive programming that checks that the chunks are written with the precomputed sizes is disabled. Then, the table of contents is written in reverse order at the end of the hashfile, so a parser can read the chunk list starting from the end of the file (minus the hash). The parsing of these table of contents will come in a later change. Signed-off-by: Derrick Stolee --- chunk-format.c | 53 +++++++++++++++++++++++++++++++++++--------------- chunk-format.h | 9 ++++++++- commit-graph.c | 2 +- midx.c | 2 +- 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/chunk-format.c b/chunk-format.c index f1b2c8a8b36f87..3f5cc9b5ddf48c 100644 --- a/chunk-format.c +++ b/chunk-format.c @@ -57,26 +57,31 @@ void add_chunk(struct chunkfile *cf, cf->chunks_nr++; } -int write_chunkfile(struct chunkfile *cf, void *data) +int write_chunkfile(struct chunkfile *cf, + enum chunkfile_flags flags, + void *data) { int i, result = 0; - uint64_t cur_offset = hashfile_total(cf->f); trace2_region_enter("chunkfile", "write", the_repository); - /* Add the table of contents to the current offset */ - cur_offset += (cf->chunks_nr + 1) * CHUNK_TOC_ENTRY_SIZE; + if (!(flags & CHUNKFILE_TRAILING_TOC)) { + uint64_t cur_offset = hashfile_total(cf->f); - for (i = 0; i < cf->chunks_nr; i++) { - hashwrite_be32(cf->f, cf->chunks[i].id); - hashwrite_be64(cf->f, cur_offset); + /* Add the table of contents to the current offset */ + cur_offset += (cf->chunks_nr + 1) * CHUNK_TOC_ENTRY_SIZE; - cur_offset += cf->chunks[i].size; - } + for (i = 0; i < cf->chunks_nr; i++) { + hashwrite_be32(cf->f, cf->chunks[i].id); + hashwrite_be64(cf->f, cur_offset); - /* Trailing entry marks the end of the chunks */ - hashwrite_be32(cf->f, 0); - hashwrite_be64(cf->f, cur_offset); + cur_offset += cf->chunks[i].size; + } + + /* Trailing entry marks the end of the chunks */ + hashwrite_be32(cf->f, 0); + hashwrite_be64(cf->f, cur_offset); + } for (i = 0; i < cf->chunks_nr; i++) { cf->chunks[i].offset = hashfile_total(cf->f); @@ -85,10 +90,26 @@ int write_chunkfile(struct chunkfile *cf, void *data) if (result) goto cleanup; - if (hashfile_total(cf->f) - cf->chunks[i].offset != cf->chunks[i].size) - BUG("expected to write %"PRId64" bytes to chunk %"PRIx32", but wrote %"PRId64" instead", - cf->chunks[i].size, cf->chunks[i].id, - hashfile_total(cf->f) - cf->chunks[i].offset); + if (!(flags & CHUNKFILE_TRAILING_TOC)) { + if (hashfile_total(cf->f) - cf->chunks[i].offset != cf->chunks[i].size) + BUG("expected to write %"PRId64" bytes to chunk %"PRIx32", but wrote %"PRId64" instead", + cf->chunks[i].size, cf->chunks[i].id, + hashfile_total(cf->f) - cf->chunks[i].offset); + } + + cf->chunks[i].size = hashfile_total(cf->f) - cf->chunks[i].offset; + } + + if (flags & CHUNKFILE_TRAILING_TOC) { + size_t last_chunk_tail = hashfile_total(cf->f); + /* First entry marks the end of the chunks */ + hashwrite_be32(cf->f, 0); + hashwrite_be64(cf->f, last_chunk_tail); + + for (i = cf->chunks_nr - 1; i >= 0; i--) { + hashwrite_be32(cf->f, cf->chunks[i].id); + hashwrite_be64(cf->f, cf->chunks[i].offset); + } } cleanup: diff --git a/chunk-format.h b/chunk-format.h index 7885aa084878dd..39e8967e95075b 100644 --- a/chunk-format.h +++ b/chunk-format.h @@ -31,7 +31,14 @@ void add_chunk(struct chunkfile *cf, uint32_t id, size_t size, chunk_write_fn fn); -int write_chunkfile(struct chunkfile *cf, void *data); + +enum chunkfile_flags { + CHUNKFILE_TRAILING_TOC = (1 << 0), +}; + +int write_chunkfile(struct chunkfile *cf, + enum chunkfile_flags flags, + void *data); int read_table_of_contents(struct chunkfile *cf, const unsigned char *mfile, diff --git a/commit-graph.c b/commit-graph.c index a7d8755932884c..c927b81250d617 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1932,7 +1932,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) get_num_chunks(cf) * ctx->commits.nr); } - write_chunkfile(cf, ctx); + write_chunkfile(cf, 0, ctx); stop_progress(&ctx->progress); strbuf_release(&progress_title); diff --git a/midx.c b/midx.c index c27d0e5f1510c3..fed25b02af8f9a 100644 --- a/midx.c +++ b/midx.c @@ -1480,7 +1480,7 @@ static int write_midx_internal(const char *object_dir, } write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs); - write_chunkfile(cf, &ctx); + write_chunkfile(cf, 0, &ctx); finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA, CSUM_FSYNC | CSUM_HASH_IN_STREAM); From d198fd89cf36d47acff07c0e6f5af6c3cbe9e98b Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 5 Oct 2022 10:38:40 -0400 Subject: [PATCH 10/10] chunk-format: parse trailing table of contents The new read_trailing_table_of_contents() mimics read_table_of_contents() except that it reads the table of contents in reverse from the end of the given hashfile. The file is given as a memory-mapped section of memory and a size. Automatically calculate the start of the trailing hash and read the table of contents in revers from that position. The errors come along from those in read_table_of_contents(). The one exception is that the chunk_offset cannot be checked as going into the table of contents since we do not have that length automatically. That may have some surprising results for some narrow forms of corruption. However, we do still limit the size to the size of the file plus the part of the table of contents read so far. At minimum, the given sizes can be used to limit parsing within the file itself. Signed-off-by: Derrick Stolee --- chunk-format.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++ chunk-format.h | 9 +++++++++ 2 files changed, 62 insertions(+) diff --git a/chunk-format.c b/chunk-format.c index 3f5cc9b5ddf48c..e836a121c5ca81 100644 --- a/chunk-format.c +++ b/chunk-format.c @@ -173,6 +173,59 @@ int read_table_of_contents(struct chunkfile *cf, return 0; } +int read_trailing_table_of_contents(struct chunkfile *cf, + const unsigned char *mfile, + size_t mfile_size) +{ + int i; + uint32_t chunk_id; + const unsigned char *table_of_contents = mfile + mfile_size - the_hash_algo->rawsz; + + while (1) { + uint64_t chunk_offset; + + table_of_contents -= CHUNK_TOC_ENTRY_SIZE; + + chunk_id = get_be32(table_of_contents); + chunk_offset = get_be64(table_of_contents + 4); + + /* Calculate the previous chunk size, if it exists. */ + if (cf->chunks_nr) { + off_t previous_offset = cf->chunks[cf->chunks_nr - 1].offset; + + if (chunk_offset < previous_offset || + chunk_offset > table_of_contents - mfile) { + error(_("improper chunk offset(s) %"PRIx64" and %"PRIx64""), + previous_offset, chunk_offset); + return -1; + } + + cf->chunks[cf->chunks_nr - 1].size = chunk_offset - previous_offset; + } + + /* Stop at the null chunk. We only need it for the last size. */ + if (!chunk_id) + break; + + for (i = 0; i < cf->chunks_nr; i++) { + if (cf->chunks[i].id == chunk_id) { + error(_("duplicate chunk ID %"PRIx32" found"), + chunk_id); + return -1; + } + } + + ALLOC_GROW(cf->chunks, cf->chunks_nr + 1, cf->chunks_alloc); + + cf->chunks[cf->chunks_nr].id = chunk_id; + cf->chunks[cf->chunks_nr].start = mfile + chunk_offset; + cf->chunks[cf->chunks_nr].offset = chunk_offset; + cf->chunks_nr++; + } + + return 0; +} + static int pair_chunk_fn(const unsigned char *chunk_start, size_t chunk_size, void *data) diff --git a/chunk-format.h b/chunk-format.h index 39e8967e95075b..acb8dfbce8039d 100644 --- a/chunk-format.h +++ b/chunk-format.h @@ -46,6 +46,15 @@ int read_table_of_contents(struct chunkfile *cf, uint64_t toc_offset, int toc_length); +/** + * Read the given chunkfile, but read the table of contents from the + * end of the given mfile. The file is expected to be a hashfile with + * the_hash_file->rawsz bytes at the end storing the hash. + */ +int read_trailing_table_of_contents(struct chunkfile *cf, + const unsigned char *mfile, + size_t mfile_size); + #define CHUNK_NOT_FOUND (-2) /*