From b0026daf1e60ebff7f1d4a840c158b3a05ecffcd Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 24 Apr 2025 16:10:47 -0700 Subject: [PATCH 1/8] ci: skip unavailable external software The ci/install-dependencies.sh script used in a very early phase of our CI jobs downloads Perforce, Git-LFS, and JGit, used for running the test scripts. The test framework is prepared to properly skip the tests that depend on these external software, but the CI script is unnecessarily strict (due to its use of "set -e" in ci/lib.sh) and fails the entire CI run before even starting to test the rest of the system. Notice a failure to download to any of these external software, but keep going. We need to be careful about cleaning after a failed wget, as a later part of the script that does: if type jgit >/dev/null 2>&1 then echo "$(tput setaf 6)JGit Version$(tput sgr0)" jgit version else echo >&2 "WARNING: JGit wasn't installed, see above for clues why" fi will (surprise!) succeed running "type jgit", and then fail with "jgit version", taking the whole thing down due to "set -e". Signed-off-by: Junio C Hamano --- ci/install-dependencies.sh | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 0df74610d063fb..e51304c3b0eed2 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -66,16 +66,29 @@ ubuntu-*|i386/ubuntu-*|debian-*) mkdir --parents "$CUSTOM_PATH" wget --quiet --directory-prefix="$CUSTOM_PATH" \ - "$P4WHENCE/bin.linux26x86_64/p4d" "$P4WHENCE/bin.linux26x86_64/p4" - chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" - - wget --quiet "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" + "$P4WHENCE/bin.linux26x86_64/p4d" \ + "$P4WHENCE/bin.linux26x86_64/p4" && + chmod a+x "$CUSTOM_PATH/p4d" "$CUSTOM_PATH/p4" || { + rm -f "$CUSTOM_PATH/p4" + rm -f "$CUSTOM_PATH/p4d" + echo >&2 "P4 download (optional) failed" + } + + wget --quiet \ + "$LFSWHENCE/git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" && tar -xzf "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" \ - -C "$CUSTOM_PATH" --strip-components=1 "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" - rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" - - wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" - chmod a+x "$CUSTOM_PATH/jgit" + -C "$CUSTOM_PATH" --strip-components=1 \ + "git-lfs-$LINUX_GIT_LFS_VERSION/git-lfs" && + rm "git-lfs-linux-amd64-$LINUX_GIT_LFS_VERSION.tar.gz" || { + rm -f "$CUSTOM_PATH/git-lfs" + echo >&2 "LFS download (optional) failed" + } + + wget --quiet "$JGITWHENCE" --output-document="$CUSTOM_PATH/jgit" && + chmod a+x "$CUSTOM_PATH/jgit" || { + rm -f "$CUSTOM_PATH/jgit" + echo >&2 "JGit download (optional) failed" + } ;; esac ;; From c41653b90270454517292b175ad74bb1661cf509 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 16 Apr 2025 07:17:24 +0200 Subject: [PATCH 2/8] ci(pedantic): ensure that awk is installed The image pointed to by the fedora:latest tag has moved from fedora 41 to 42. The fedora 41 container images have awk installed while the fedora 42 images do not. That change is most likely just part of reducing the size of the base container images. In both AlmaLinux and Fedora (as well as other RHEL derivatives/relatives), awk is provided by the gawk package. On Fedora, `dnf install awk` would work, but for unintended reasons! It uses the package filelist data to determine that /usr/bin/awk is provided by gawk and installs gawk as a result. On AlmaLinux (8 & 9, by my quick testing), that is not the case and you'd need to use `dnf install gawk` or `dnf install '*bin/awk'` to get it installed. Having said that, awk _is_ included in the current AlmaLinux 8 and 9 images, so it isn't strictly needed. But it's probably better to be explicit that we need it installed, as a defense against some future change to the AlmaLinux container removing awk. Using the package name "gawk" is the right thing to do. Note that even '*bin/awk' would have worked, but it is less specific. And who knows, maybe in the far future a BSD variant of awk is offered, too, and would then cause ambiguities. Best to avoid that. Suggested-by: Todd Zullinger Signed-off-by: Johannes Schindelin --- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 8700c0f2924d7f..be9ba5e30a477b 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -31,7 +31,7 @@ alpine-*) ;; fedora-*|almalinux-*) dnf -yq update >/dev/null && - dnf -yq install shadow-utils sudo make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null + dnf -yq install shadow-utils sudo make gcc findutils diffutils perl python3 gawk gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null ;; ubuntu-*|i386/ubuntu-*|debian-*) # Required so that apt doesn't wait for user input on certain packages. From 61fbdd33a096bdeb61aaab033286b9a0c27eb982 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 25 Apr 2025 09:27:14 +0200 Subject: [PATCH 3/8] ci(jgit): use a more reliable link to download JGit Git's dependency story is improvable: When, say, JGit cannot be downloaded, it currently fails the entire job without even running all the tests that do not require JGit (which is the vast, vast majority of them). It would be better to handle that gracefully by warning about the JGit download failure and then skipping the respective tests. This JGit download problem is far from theoretical: There is an incident on eclipse.org that has now lasted for an extended amount of time: https://www.eclipsestatus.io/incident/549796. While upstream Git finally chose to improve their CI story in b0026daf1e60 (ci: skip unavailable external software, 2025-04-24), we should heed the suggestion by the JGit maintainer, Matthias Sohn, in https://discord.com/channels/1042895022950994071/1364872237710184520/1364885895865696479, to switch to the far more reliable Maven Central link (according to https://status.maven.org/uptime, there has not been any major downtime recently that wasn't resolved within 5 minutes, a far cry from the 30h of the current eclipse.org incident as of time of writing). Signed-off-by: Johannes Schindelin --- ci/install-dependencies.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index be9ba5e30a477b..7fe96110c63a88 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -9,7 +9,7 @@ begin_group "Install dependencies" P4WHENCE=https://cdist2.perforce.com/perforce/r23.2 LFSWHENCE=https://github.com/github/git-lfs/releases/download/v$LINUX_GIT_LFS_VERSION -JGITWHENCE=https://repo.eclipse.org/content/groups/releases//org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh +JGITWHENCE=https://repo1.maven.org/maven2/org/eclipse/jgit/org.eclipse.jgit.pgm/6.8.0.202311291450-r/org.eclipse.jgit.pgm-6.8.0.202311291450-r.sh # Make sudo a no-op and execute the command directly when running as root. # While using sudo would be fine on most platforms when we are root already, From 6bc610b82b611edf253e01faa2956d217999a1d6 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 23 Apr 2025 11:01:31 -0400 Subject: [PATCH 4/8] test-tool: add pack-deltas helper When trying to demonstrate certain behavior in tests, it can be helpful to create packfiles that have specific delta structures. 'git pack-objects' uses various algorithms to select deltas based on their compression rates, but that does not always demonstrate all possible packfile shapes. This becomes especially important when wanting to test 'git index-pack' and its ability to parse certain pack shapes. We have prior art in t/lib-pack.sh, where certain delta structures are produced by manually writing certain opaque pack contents. However, producing these script updates is cumbersome and difficult to do as a contributor. Instead, create a new test-tool, 'test-tool pack-deltas', that reads a list of instructions for which objects to include in a packfile and how those objects should be written in delta form. At the moment, this only supports REF_DELTAs as those are the kinds of deltas needed to exercise a bug in 'git index-pack'. Signed-off-by: Derrick Stolee --- Makefile | 1 + t/helper/meson.build | 1 + t/helper/test-pack-deltas.c | 140 ++++++++++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + 5 files changed, 144 insertions(+) create mode 100644 t/helper/test-pack-deltas.c diff --git a/Makefile b/Makefile index 898a2fa5dd99d0..f82a203d895dca 100644 --- a/Makefile +++ b/Makefile @@ -827,6 +827,7 @@ TEST_BUILTINS_OBJS += test-mergesort.o TEST_BUILTINS_OBJS += test-mktemp.o TEST_BUILTINS_OBJS += test-name-hash.o TEST_BUILTINS_OBJS += test-online-cpus.o +TEST_BUILTINS_OBJS += test-pack-deltas.o TEST_BUILTINS_OBJS += test-pack-mtimes.o TEST_BUILTINS_OBJS += test-parse-options.o TEST_BUILTINS_OBJS += test-parse-pathspec-file.o diff --git a/t/helper/meson.build b/t/helper/meson.build index 92853260d6a4ca..aca3692332e685 100644 --- a/t/helper/meson.build +++ b/t/helper/meson.build @@ -37,6 +37,7 @@ test_tool_sources = [ 'test-mktemp.c', 'test-name-hash.c', 'test-online-cpus.c', + 'test-pack-deltas.c', 'test-pack-mtimes.c', 'test-parse-options.c', 'test-parse-pathspec-file.c', diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c new file mode 100644 index 00000000000000..28cbb5f4242ded --- /dev/null +++ b/t/helper/test-pack-deltas.c @@ -0,0 +1,140 @@ +#define USE_THE_REPOSITORY_VARIABLE + +#include "test-tool.h" +#include "git-compat-util.h" +#include "delta.h" +#include "git-zlib.h" +#include "hash.h" +#include "hex.h" +#include "pack.h" +#include "pack-objects.h" +#include "setup.h" +#include "strbuf.h" +#include "string-list.h" + +static const char usage_str[] = "test-tool pack-deltas "; + +static unsigned long do_compress(void **pptr, unsigned long size) +{ + git_zstream stream; + void *in, *out; + unsigned long maxsize; + + git_deflate_init(&stream, 1); + maxsize = git_deflate_bound(&stream, size); + + in = *pptr; + out = xmalloc(maxsize); + *pptr = out; + + stream.next_in = in; + stream.avail_in = size; + stream.next_out = out; + stream.avail_out = maxsize; + while (git_deflate(&stream, Z_FINISH) == Z_OK) + ; /* nothing */ + git_deflate_end(&stream); + + free(in); + return stream.total_out; +} + +static void write_ref_delta(struct hashfile *f, + struct object_id *oid, + struct object_id *base) +{ + unsigned char header[MAX_PACK_OBJECT_HEADER]; + unsigned long size, base_size, delta_size, compressed_size, hdrlen; + enum object_type type; + void *base_buf, *delta_buf; + void *buf = repo_read_object_file(the_repository, + oid, &type, + &size); + + if (!buf) + die("unable to read %s", oid_to_hex(oid)); + + base_buf = repo_read_object_file(the_repository, + base, &type, + &base_size); + + if (!base_buf) + die("unable to read %s", oid_to_hex(base)); + + delta_buf = diff_delta(base_buf, base_size, + buf, size, &delta_size, 0); + + compressed_size = do_compress(&delta_buf, delta_size); + + hdrlen = encode_in_pack_object_header(header, sizeof(header), + OBJ_REF_DELTA, delta_size); + hashwrite(f, header, hdrlen); + hashwrite(f, base->hash, the_repository->hash_algo->rawsz); + hashwrite(f, delta_buf, compressed_size); + + free(buf); + free(base_buf); + free(delta_buf); +} + +int cmd__pack_deltas(int argc, const char **argv) +{ + int N; + struct hashfile *f; + struct strbuf line = STRBUF_INIT; + + if (argc != 2) { + usage(usage_str); + return -1; + } + + N = atoi(argv[1]); + + setup_git_directory(); + + f = hashfd(1, ""); + write_pack_header(f, N); + + /* Read each line from stdin into 'line' */ + while (strbuf_getline_lf(&line, stdin) != EOF) { + const char *type_str, *content_oid_str, *base_oid_str = NULL; + struct object_id content_oid, base_oid; + struct string_list items = STRING_LIST_INIT_NODUP; + /* + * Tokenize into two or three parts: + * 1. REF_DELTA, OFS_DELTA, or FULL. + * 2. The object ID for the content object. + * 3. The object ID for the base object (optional). + */ + if (string_list_split_in_place(&items, line.buf, " ", 3) < 0) + die("invalid input format: %s", line.buf); + + if (items.nr < 2) + die("invalid input format: %s", line.buf); + + type_str = items.items[0].string; + content_oid_str = items.items[1].string; + + if (get_oid_hex(content_oid_str, &content_oid)) + die("invalid object: %s", content_oid_str); + if (items.nr >= 3) { + base_oid_str = items.items[2].string; + if (get_oid_hex(base_oid_str, &base_oid)) + die("invalid object: %s", base_oid_str); + } + + if (!strcmp(type_str, "REF_DELTA")) + write_ref_delta(f, &content_oid, &base_oid); + else if (!strcmp(type_str, "OFS_DELTA")) + die("OFS_DELTA not implemented"); + else if (!strcmp(type_str, "FULL")) + die("FULL not implemented"); + else + die("unknown pack type: %s", type_str); + } + + finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK, + CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE); + strbuf_release(&line); + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index c5fb9935d8fafd..19ca2f2b6bd7e9 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -47,6 +47,7 @@ static struct test_cmd cmds[] = { { "mktemp", cmd__mktemp }, { "name-hash", cmd__name_hash }, { "online-cpus", cmd__online_cpus }, + { "pack-deltas", cmd__pack_deltas }, { "pack-mtimes", cmd__pack_mtimes }, { "parse-options", cmd__parse_options }, { "parse-options-flags", cmd__parse_options_flags }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 4e7fe4c875d91e..623d89beebeff5 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -40,6 +40,7 @@ int cmd__mergesort(int argc, const char **argv); int cmd__mktemp(int argc, const char **argv); int cmd__name_hash(int argc, const char **argv); int cmd__online_cpus(int argc, const char **argv); +int cmd__pack_deltas(int argc, const char **argv); int cmd__pack_mtimes(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); int cmd__parse_options_flags(int argc, const char **argv); From 87db66763dbf2a0d3141f337055a1667ebe2e55f Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 23 Apr 2025 11:05:48 -0400 Subject: [PATCH 5/8] t5309: create failing test for 'git index-pack' This new test demonstrates some behavior where a valid packfile is being rejected by the Git client due to the order in which it is resolving REF_DELTAs. The thin packfile has a REF_DELTA chain A->B->C where C is not included in the packfile. However, the client repository contains both C and B already. Thus, 'git index-pack' is able to resolve A before resolving B. When resolving B, it then attempts to resolve any other REF_DELTAs that are pointing to B as a base. This "revisits" A and complains as if there is a cycle, but it did not actually detect a cycle. A fix will arrive in the next change. Signed-off-by: Derrick Stolee --- t/t5309-pack-delta-cycles.sh | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh index 60fc710bacb20e..9029f8a0dda7bf 100755 --- a/t/t5309-pack-delta-cycles.sh +++ b/t/t5309-pack-delta-cycles.sh @@ -75,4 +75,30 @@ test_expect_success 'failover to a duplicate object in the same pack' ' test_must_fail git index-pack --fix-thin --stdin B->C with B on disk' ' + git init server && + ( + cd server && + test_commit_bulk 4 + ) && + + A=$(git -C server rev-parse HEAD^{tree}) && + B=$(git -C server rev-parse HEAD~1^{tree}) && + C=$(git -C server rev-parse HEAD~2^{tree}) && + git -C server reset --hard HEAD~1 && + + cat >in <<-EOF && + REF_DELTA $A $B + REF_DELTA $B $C + EOF + + test-tool -C server pack-deltas 2 thin.pack && + + git clone "file://$(pwd)/server" client && + ( + cd client && + git index-pack --fix-thin --stdin <../thin.pack + ) +' + test_done From f64bf3ba70560594099f06ac54f3f266aad17305 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 23 Apr 2025 11:36:10 -0400 Subject: [PATCH 6/8] index-pack: allow revisiting REF_DELTA chains As detailed in the previous changes to t5309-pack-delta-cycles.sh, the logic within 'git index-pack' to analyze an incoming thin packfile with REF_DELTAs is suspect. The algorithm is overly cautious around delta cycles, and that leads in fact to failing even when there is no cycle. This change adjusts the algorithm to no longer fail in these cases. In fact, these cycle cases will no longer fail but more importantly the valid cases will no longer fail, either. The resulting packfile from the --fix-thin operation will not have cycles either since REF_DELTAs are forbidden from the on-disk format and OFS_DELTAs are impossible to write as a cycle. The crux of the matter is how the algorithm works when the REF_DELTAs point to base objects that exist in the local repository. When reading the thin packfile, the object IDs for the delta objects are unknown so we do not have the delta chain structure automatically. Instead, we need to start somewhere by selecting a delta whose base is inside our current object database. Consider the case where the packfile has two REF_DELTA objects, A and B, and the delta chain looks like "A depends on B" and "B depends on C" for some third object C, where C is already in the current repository. The algorithm _should_ start with all objects that depend on C, finding B, and then moving on to all objects depending on B, finding A. However, if the repository also already has object B, then the delta chain can be analyzed in a different order. The deltas with base B can be analyzed first, finding A, and then the deltas with base C are analyzed, finding B. The algorithm currently continues to look for objects that depend on B, finding A again. This fails due to A's 'real_type' member already being overwritten from OBJ_REF_DELTA to the correct object type. This scenario is possible in a typical 'git fetch' where the client does not advertise B as a 'have' but requests A as a 'want' (and C is noticed as a common object based on other 'have's). The reason this isn't typically seen is that most Git servers use OFS_DELTAs to represent deltas within a packfile. However, if a server uses only REF_DELTAs, then this kind of issue can occur. There is nothing in the explicit packfile format that states this use of inter-pack REF_DELTA is incorrect, only that REF_DELTAs should not be used in the on-disk representation to avoid cycles. This die() was introduced in ab791dd138 (index-pack: fix race condition with duplicate bases, 2014-08-29). Several refactors have adjusted the error message and the surrounding logic, but this issue has existed for a longer time as that was only a conversion from an assert(). The tests in t5309 originated in 3b910d0c5e (add tests for indexing packs with delta cycles, 2013-08-23) and b2ef3d9ebb (test index-pack on packs with recoverable delta cycles, 2013-08-23). These changes make note that the current behavior of handling "resolvable" cycles is mostly a documentation-only test, not that this behavior is the best way for Git to handle the situation. The fix here is somewhat complicated due to the amount of state being adjusted by the loop within threaded_second_pass(). Instead of trying to resume the start of the loop while adjusting the necessary context, I chose to scan the REF_DELTAs depending on the current 'parent' and skip any that have already been processed. This necessarily leaves us in a state where 'child' and 'child_obj' could be left as NULL and that must be handled later. There is also some careful handling around skipping REF_DELTAs when there are also OFS_DELTAs depending on that parent. There may be value in extending 'test-tool pack-deltas' to allow writing OFS_DELTAs in order to exercise this logic across the delta types. Signed-off-by: Derrick Stolee --- builtin/index-pack.c | 58 ++++++++++++++++++++---------------- t/t5309-pack-delta-cycles.sh | 12 ++++++-- 2 files changed, 41 insertions(+), 29 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 8df66cb521b38d..4ac93eb8c08329 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1107,8 +1107,8 @@ static void *threaded_second_pass(void *data) set_thread_data(data); for (;;) { struct base_data *parent = NULL; - struct object_entry *child_obj; - struct base_data *child; + struct object_entry *child_obj = NULL; + struct base_data *child = NULL; counter_lock(); display_progress(progress, nr_resolved_deltas); @@ -1135,15 +1135,18 @@ static void *threaded_second_pass(void *data) parent = list_first_entry(&work_head, struct base_data, list); - if (parent->ref_first <= parent->ref_last) { + while (parent->ref_first <= parent->ref_last) { int offset = ref_deltas[parent->ref_first++].obj_no; child_obj = objects + offset; - if (child_obj->real_type != OBJ_REF_DELTA) - die("REF_DELTA at offset %"PRIuMAX" already resolved (duplicate base %s?)", - (uintmax_t) child_obj->idx.offset, - oid_to_hex(&parent->obj->idx.oid)); + if (child_obj->real_type != OBJ_REF_DELTA) { + child_obj = NULL; + continue; + } child_obj->real_type = parent->obj->real_type; - } else { + break; + } + + if (!child_obj && parent->ofs_first <= parent->ofs_last) { child_obj = objects + ofs_deltas[parent->ofs_first++].obj_no; assert(child_obj->real_type == OBJ_OFS_DELTA); @@ -1176,29 +1179,32 @@ static void *threaded_second_pass(void *data) } work_unlock(); - if (parent) { - child = resolve_delta(child_obj, parent); - if (!child->children_remaining) - FREE_AND_NULL(child->data); - } else { - child = make_base(child_obj, NULL); - if (child->children_remaining) { - /* - * Since this child has its own delta children, - * we will need this data in the future. - * Inflate now so that future iterations will - * have access to this object's data while - * outside the work mutex. - */ - child->data = get_data_from_pack(child_obj); - child->size = child_obj->size; + if (child_obj) { + if (parent) { + child = resolve_delta(child_obj, parent); + if (!child->children_remaining) + FREE_AND_NULL(child->data); + } else{ + child = make_base(child_obj, NULL); + if (child->children_remaining) { + /* + * Since this child has its own delta children, + * we will need this data in the future. + * Inflate now so that future iterations will + * have access to this object's data while + * outside the work mutex. + */ + child->data = get_data_from_pack(child_obj); + child->size = child_obj->size; + } } } work_lock(); if (parent) parent->retain_data--; - if (child->data) { + + if (child && child->data) { /* * This child has its own children, so add it to * work_head. @@ -1207,7 +1213,7 @@ static void *threaded_second_pass(void *data) base_cache_used += child->size; prune_base_data(NULL); free_base_data(child); - } else { + } else if (child) { /* * This child does not have its own children. It may be * the last descendant of its ancestors; free those diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh index 9029f8a0dda7bf..fc3594aaae95a3 100755 --- a/t/t5309-pack-delta-cycles.sh +++ b/t/t5309-pack-delta-cycles.sh @@ -60,7 +60,10 @@ test_expect_success 'index-pack detects REF_DELTA cycles' ' test_expect_success 'failover to an object in another pack' ' clear_packs && git index-pack --stdin recoverable.pack && pack_trailer recoverable.pack && - test_must_fail git index-pack --fix-thin --stdin B->C with B on disk' ' +test_expect_success 'index-pack works with thin pack A->B->C with B on disk' ' git init server && ( cd server && From d1e9900f9b47da05d6399b5ac24e7b82d1a11f21 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 24 Apr 2025 12:41:58 -0700 Subject: [PATCH 7/8] fixup! test-tool: add pack-deltas helper Junio says in https://lore.kernel.org/git/xmqq34dxuz21.fsf@gitster.g: > I needed this to make > > $ SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true make > $ cd t && sh t5309-pack-delta-cycles.sh > > pass. Signed-off-by: Johannes Schindelin --- t/helper/test-pack-deltas.c | 1 + 1 file changed, 1 insertion(+) diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c index 28cbb5f4242ded..4af69bdc05d395 100644 --- a/t/helper/test-pack-deltas.c +++ b/t/helper/test-pack-deltas.c @@ -122,6 +122,7 @@ int cmd__pack_deltas(int argc, const char **argv) if (get_oid_hex(base_oid_str, &base_oid)) die("invalid object: %s", base_oid_str); } + string_list_clear(&items, 0); if (!strcmp(type_str, "REF_DELTA")) write_ref_delta(f, &content_oid, &base_oid); From 638c49acd7ef8827b295ce7a2c3779aca27354c7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Fri, 25 Apr 2025 11:04:13 +0200 Subject: [PATCH 8/8] fixup! test-tool: add pack-deltas helper Let's make the command-line parsing a bit more stringent. We _could_ use `parse_options()`, but that would be overkill for a single, non-optional argument. Besides, it would not bring any benefit, as the parsed value needs to fit in the `uint32_t` type, and `parse_options()` has no provision to ensure that. Signed-off-by: Johannes Schindelin --- t/helper/test-pack-deltas.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/t/helper/test-pack-deltas.c b/t/helper/test-pack-deltas.c index 4af69bdc05d395..f95d8ee16768c5 100644 --- a/t/helper/test-pack-deltas.c +++ b/t/helper/test-pack-deltas.c @@ -8,11 +8,12 @@ #include "hex.h" #include "pack.h" #include "pack-objects.h" +#include "parse.h" #include "setup.h" #include "strbuf.h" #include "string-list.h" -static const char usage_str[] = "test-tool pack-deltas "; +static const char usage_str[] = "test-tool pack-deltas "; static unsigned long do_compress(void **pptr, unsigned long size) { @@ -79,7 +80,7 @@ static void write_ref_delta(struct hashfile *f, int cmd__pack_deltas(int argc, const char **argv) { - int N; + unsigned long n; struct hashfile *f; struct strbuf line = STRBUF_INIT; @@ -88,12 +89,13 @@ int cmd__pack_deltas(int argc, const char **argv) return -1; } - N = atoi(argv[1]); + if (!git_parse_ulong(argv[1], &n) || n != (uint32_t)n) + die("invalid number of objects: %s", argv[1]); setup_git_directory(); f = hashfd(1, ""); - write_pack_header(f, N); + write_pack_header(f, n); /* Read each line from stdin into 'line' */ while (strbuf_getline_lf(&line, stdin) != EOF) {