{"thread":{"id":"62404","subject":"[PATCH 0/5] When fetching from a promisor remote, repack local objects referenced","startedAt":"2024-10-24T18:08:50Z","lastAt":"2024-11-20T06:29:21Z","messageCount":37,"participants":["Jonathan Tan","Han Young","Taylor Blau","Josh Steadmon","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"506056","messageId":"cover.1729792911.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":null,"subject":"[PATCH 0/5] When fetching from a promisor remote, repack local objects referenced","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-24T18:08:39Z","receivedAt":"2024-10-24T18:08:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a polished version of [1], also with all the test failures\ndebugged and addressed.\n\nThe first 4 patches are cleanups and addressing issues with tests, and\nthe last patch contains the actual change.\n\nThis aims to solve the same problem as [2]. Some issues with it have\nbeen brought up in [3] (e.g. not being able to identify if an object is\nmissing due to repo corruption or legitimately missing because it's been\npromised, and also GC not removing any local object); these patches do\nnot have those issues. (Admittedly, these patches may have other issues\n- mainly, more work needs to be done during fetch, and that work may\nresult in duplicate objects on disk, but I think that both the work and\nthe disk space used will be minimal, and the extra disk space used will\ngo away after a GC.)\n\n[1] https://lore.kernel.org/git/cover.1729549127.git.jonathantanmy@google.com/\n[2] https://lore.kernel.org/git/20240925072021.77078-1-hanyang.tony@bytedance.com/\n[3] https://lore.kernel.org/git/a5e3322d-4e63-4b8c-84af-6578fe257cad@gmail.com/\n\nJonathan Tan (5):\n pack-objects: make variable non-static\n t0410: make test description clearer\n t0410: use from-scratch server\n t5300: move --window clamp test next to unclamped\n index-pack: repack local links into promisor packs\n\n Documentation/git-index-pack.txt | 5 ++\n builtin/index-pack.c | 110 ++++++++++++++++++++++++++++++-\n builtin/pack-objects.c | 31 ++++++++-\n t/t0410-partial-clone.sh | 6 +-\n t/t5300-pack-object.sh | 10 +--\n t/t5616-partial-clone.sh | 30 +++++++++\n 6 files changed, 180 insertions(+), 12 deletions(-)\n\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506057","messageId":"b2c76c207d3ea880c612f115f733a91bc6b529a3.1729792911.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"[PATCH 1/5] pack-objects: make variable non-static","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-24T18:08:40Z","receivedAt":"2024-10-24T18:08:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/pack-objects.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fc0680b40..e15fbaeb21 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -238,8 +238,6 @@ static enum {\n } write_bitmap_index;\n static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n \n-static int exclude_promisor_objects;\n-\n static int use_delta_islands;\n \n static unsigned long delta_cache_size = 0;\n@@ -4327,6 +4325,7 @@ int cmd_pack_objects(int argc,\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n+\tint exclude_promisor_objects = 0;\n \n \tstruct option pack_objects_options[] = {\n \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506058","messageId":"c220e77ccf4e0884067a27b40e862075d64f8c4b.1729792911.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"[PATCH 2/5] t0410: make test description clearer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-24T18:08:41Z","receivedAt":"2024-10-24T18:08:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 9a4c507886 (t0410: test fetching from many promisor remotes,\n2019-06-25) adds some tests that demonstrate not the automatic fetching\nof missing objects, but the direct fetching from another promisor remote\n(configured explicitly in one test and implicitly via --filter on the\n\"git fetch\" CLI invocation in the other test) - thus demonstrating\nsupport for multiple promisor remotes, as described in the commit\nmessage.\n\nChange the test descriptions accordingly to make this clearer.\n\nSigned-off-by: Jonathan Tan \n---\n t/t0410-partial-clone.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 818700fbec..eadb69473f 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -241,7 +241,7 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \tgrep \"fetch< fetch=.*ref-in-want\" trace\n '\n \n-test_expect_success 'fetching of missing objects from another promisor remote' '\n+test_expect_success 'fetching from another promisor remote' '\n \tgit clone \"file://$(pwd)/server\" server2 &&\n \ttest_commit -C server2 bar &&\n \tgit -C server2 repack -a -d --write-bitmap-index &&\n@@ -264,7 +264,7 @@ test_expect_success 'fetching of missing objects from another promisor remote' '\n \tgrep \"$HASH2\" out\n '\n \n-test_expect_success 'fetching of missing objects configures a promisor remote' '\n+test_expect_success 'fetching with --filter configures a promisor remote' '\n \tgit clone \"file://$(pwd)/server\" server3 &&\n \ttest_commit -C server3 baz &&\n \tgit -C server3 repack -a -d --write-bitmap-index &&\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506059","messageId":"08750988e0498db7de9b3c8766137592adb84194.1729792911.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"[PATCH 3/5] t0410: use from-scratch server","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-24T18:08:42Z","receivedAt":"2024-10-24T18:08:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will add functionality: when fetching from a\npromisor remote, existing non-promisor objects that are ancestors of any\nfetched object will be repacked into promisor packs (since if a promisor\nremote has an object, it also has all its ancestors).\n\nThis means that sometimes, a fetch from a promisor remote results in 2\nnew promisor packs (instead of the 1 that you would expect). There is a\ntest that fetches a descendant of a local object from a promisor remote,\nbut also specifically tests that there is exactly 1 promisor pack as\na result of the fetch. This means that this test will fail when the\nsubsequent commit is added.\n\nSince the ancestry of the fetched object is not the concern of this\ntest, make the fetched objects have no ancestry in common with the\nobjets in the client repo. This is done by making the server from\nscratch, instead of using an existing repo that has objects in common\nwith the client.\n\nSigned-off-by: Jonathan Tan \n---\n t/t0410-partial-clone.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex eadb69473f..e2b317db65 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -265,7 +265,7 @@ test_expect_success 'fetching from another promisor remote' '\n '\n \n test_expect_success 'fetching with --filter configures a promisor remote' '\n-\tgit clone \"file://$(pwd)/server\" server3 &&\n+\ttest_create_repo server3 &&\n \ttest_commit -C server3 baz &&\n \tgit -C server3 repack -a -d --write-bitmap-index &&\n \tHASH3=$(git -C server3 rev-parse baz) &&\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506060","messageId":"85fc3fa77e1df835721bc702c6e1df92e7b5b4bf.1729792911.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"[PATCH 4/5] t5300: move --window clamp test next to unclamped","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-24T18:08:43Z","receivedAt":"2024-10-24T18:08:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will change the behavior of \"git index-pack\n--promisor\", which is exercised in \"build pack index for an existing\npack\", causing the unclamped and clamped versions of the --window\ntest to exhibit different behavior. Move the clamp test closer to the\nunclamped test that it references.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5300-pack-object.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 3b9dae331a..aff164ddf8 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -156,6 +156,11 @@ test_expect_success 'pack without delta' '\n \tcheck_deltas stderr = 0\n '\n \n+test_expect_success 'negative window clamps to 0' '\n+\tgit pack-objects --progress --window=-1 neg-window stderr &&\n+\tcheck_deltas stderr = 0\n+'\n+\n test_expect_success 'pack-objects with bogus arguments' '\n \ttest_must_fail git pack-objects --window=0 test-1 blah blah stderr &&\n-\tcheck_deltas stderr = 0\n-'\n-\n for hash in sha1 sha256\n do\n \ttest_expect_success \"verify-pack with $hash packfile\" '\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506061","messageId":"5dd7fdc16df7757ee1b24997ad9fbe3f923d5e93.1729792911.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"[PATCH 5/5] index-pack: repack local links into promisor packs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-24T18:08:44Z","receivedAt":"2024-10-24T18:08:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach index-pack to, when processing the objects in a pack with\n--promisor specified on the CLI, repack local objects (and the local\nobjects that they refer to, recursively) referenced by these objects\ninto promisor packs.\n\nThis prevents the situation in which, when fetching from a promisor\nremote, we end up with promisor objects (newly fetched) referring\nto non-promisor objects (locally created prior to the fetch). This\nsituation may arise if the client had previously pushed objects to the\nremote, for example. One issue that arises in this situation is that,\nif the non-promisor objects become inaccessible except through promisor\nobjects (for example, if the branch pointing to them has moved to\npoint to the promisor object that refers to them), then GC will garbage\ncollect them. There are other ways to solve this, but the simplest\nseems to be to enforce the invariant that we don't have promisor objects\nreferring to non-promisor objects.\n\nThis repacking is done from index-pack to minimize the performance\nimpact. During a fetch, the only time most objects are fully inflated\nin memory is when their object ID is computed, so we also scan the\nobjects (to see which objects they refer to) during this time.\n\nAlso to minimize the performance impact, an object is calculated to be\nlocal if it's a loose object or present in a non-promisor pack. (If it's\nalso in a promisor pack or referred to by an object in a promisor pack,\nit is technically already a promisor object. But a misidentification\nof a promisor object as a non-promisor object is relatively benign\nhere - we will thus repack that promisor object into a promisor pack,\nduplicating it in the object store, but there is no correctness issue,\njust an issue of inefficiency.)\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-index-pack.txt | 5 ++\n builtin/index-pack.c | 110 ++++++++++++++++++++++++++++++-\n builtin/pack-objects.c | 28 ++++++++\n t/t5616-partial-clone.sh | 30 +++++++++\n 4 files changed, 171 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 5a20deefd5..4be09e58e7 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -139,6 +139,11 @@ include::object-format-disclaimer.txt[]\n \twritten. If a `` is provided, then that content will be\n \twritten to the .promisor file for future reference. See\n \tlink:technical/partial-clone.html[partial clone] for more information.\n++\n+Also, if there are objects in the given pack that references non-promisor\n+objects (in the repo), repacks those non-promisor objects into a promisor\n+pack. This avoids a situation in which a repo has non-promisor objects that are\n+accessible through promisor objects.\n \n NOTES\n -----\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9d23b41b3a..e4afd6725f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -9,6 +9,7 @@\n #include \"csum-file.h\"\n #include \"blob.h\"\n #include \"commit.h\"\n+#include \"tag.h\"\n #include \"tree.h\"\n #include \"progress.h\"\n #include \"fsck.h\"\n@@ -20,9 +21,14 @@\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n #include \"oid-array.h\"\n+#include \"oidset.h\"\n+#include \"path.h\"\n #include \"replace-object.h\"\n+#include \"tree-walk.h\"\n #include \"promisor-remote.h\"\n+#include \"run-command.h\"\n #include \"setup.h\"\n+#include \"strvec.h\"\n \n static const char index_pack_usage[] =\n \"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n@@ -148,6 +154,13 @@ static uint32_t input_crc32;\n static int input_fd, output_fd;\n static const char *curr_pack;\n \n+/*\n+ * local_links is guarded by read_mutex, and record_local_links is read-only in\n+ * a thread.\n+ */\n+static struct oidset local_links = OIDSET_INIT;\n+static int record_local_links;\n+\n static struct thread_local *thread_data;\n static int nr_dispatched;\n static int threads_active;\n@@ -799,6 +812,44 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n+static void record_if_local_object(const struct object_id *oid)\n+{\n+\tstruct object_info info = OBJECT_INFO_INIT;\n+\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n+\t\t/* Missing; assume it is a promisor object */\n+\t\treturn;\n+\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n+\t\treturn;\n+\toidset_insert(&local_links, oid);\n+}\n+\n+static void do_record_local_links(struct object *obj)\n+{\n+\tif (obj->type == OBJ_TREE) {\n+\t\tstruct tree *tree = (struct tree *)obj;\n+\t\tstruct tree_desc desc;\n+\t\tstruct name_entry entry;\n+\t\tif (init_tree_desc_gently(&desc, &tree->object.oid,\n+\t\t\t\t\t tree->buffer, tree->size, 0))\n+\t\t\t/*\n+\t\t\t * Error messages are given when packs are\n+\t\t\t * verified, so do not print any here.\n+\t\t\t */\n+\t\t\treturn;\n+\t\twhile (tree_entry_gently(&desc, &entry))\n+\t\t\trecord_if_local_object(&entry.oid);\n+\t} else if (obj->type == OBJ_COMMIT) {\n+\t\tstruct commit *commit = (struct commit *) obj;\n+\t\tstruct commit_list *parents = commit->parents;\n+\n+\t\tfor (; parents; parents = parents->next)\n+\t\t\trecord_if_local_object(&parents->item->object.oid);\n+\t} else if (obj->type == OBJ_TAG) {\n+\t\tstruct tag *tag = (struct tag *) obj;\n+\t\trecord_if_local_object(get_tagged_oid(tag));\n+\t}\n+}\n+\n static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tunsigned long size, enum object_type type,\n \t\t\tconst struct object_id *oid)\n@@ -845,7 +896,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict || do_fsck_object) {\n+\tif (strict || do_fsck_object || record_local_links) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n@@ -877,6 +928,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tdie(_(\"fsck error in packed object\"));\n \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n+\t\t\tif (record_local_links)\n+\t\t\t\tdo_record_local_links(obj);\n \n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n@@ -1719,6 +1772,57 @@ static void show_pack_info(int stat_only)\n \tfree(chain_histogram);\n }\n \n+static void repack_local_links(void)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *out;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tstruct oidset_iter iter;\n+\tstruct object_id *oid;\n+\tchar *base_name;\n+\n+\tif (!oidset_size(&local_links))\n+\t\treturn;\n+\n+\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n+\n+\tstrvec_push(&cmd.args, \"pack-objects\");\n+\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n+\tstrvec_push(&cmd.args, base_name);\n+\tcmd.git_cmd = 1;\n+\tcmd.in = -1;\n+\tcmd.out = -1;\n+\tif (start_command(&cmd))\n+\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\n+\toidset_iter_init(&local_links, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n+\t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n+\t}\n+\tclose(cmd.in);\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tunsigned char binary[GIT_MAX_RAWSZ];\n+\t\tif (line.len != the_hash_algo->hexsz ||\n+\t\t !hex_to_bytes(binary, line.buf, line.len))\n+\t\t\tdie(_(\"index-pack: Expecting full hex object ID lines only from pack-objects.\"));\n+\n+\t\t/*\n+\t\t * pack-objects creates the .pack and .idx files, but not the\n+\t\t * .promisor file. Create the .promisor file, which is empty.\n+\t\t */\n+\t\twrite_special_file(\"promisor\", \"\", NULL, binary, NULL);\n+\t}\n+\n+\tfclose(out);\n+\tif (finish_command(&cmd))\n+\t\tdie(_(\"could not finish pack-objects to repack local links\"));\n+\tstrbuf_release(&line);\n+}\n+\n int cmd_index_pack(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -1794,7 +1898,7 @@ int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n-\t\t\t\t; /* already parsed */\n+\t\t\t\trecord_local_links = 1;\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n@@ -1970,6 +2074,8 @@ int cmd_index_pack(int argc,\n \t\tfree((void *) curr_index);\n \tfree(curr_rev_index);\n \n+\trepack_local_links();\n+\n \t/*\n \t * Let the caller know this pack is not self contained\n \t */\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex e15fbaeb21..a565ab9b40 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4310,6 +4310,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n \treturn 0;\n }\n \n+static int should_include_obj(struct object *obj, void *data UNUSED)\n+{\n+\tstruct object_info info = OBJECT_INFO_INIT;\n+\tif (oid_object_info_extended(the_repository, &obj->oid, &info, 0))\n+\t\tBUG(\"should_include_obj should only be called on existing objects\");\n+\treturn info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;\n+}\n+\n+static int should_include(struct commit *commit, void *data) {\n+\treturn should_include_obj((struct object *) commit, data);\n+}\n+\n int cmd_pack_objects(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -4326,6 +4338,7 @@ int cmd_pack_objects(int argc,\n \tstruct list_objects_filter_options filter_options =\n \t\tLIST_OBJECTS_FILTER_INIT;\n \tint exclude_promisor_objects = 0;\n+\tint exclude_promisor_objects_best_effort = 0;\n \n \tstruct option pack_objects_options[] = {\n \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n@@ -4423,6 +4436,9 @@ int cmd_pack_objects(int argc,\n \t\t option_parse_missing_action),\n \t\tOPT_BOOL(0, \"exclude-promisor-objects\", &exclude_promisor_objects,\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n+\t\tOPT_BOOL(0, \"exclude-promisor-objects-best-effort\",\n+\t\t\t &exclude_promisor_objects_best_effort,\n+\t\t\t N_(\"implies --missing=allow-any\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n \t\tOPT_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n@@ -4503,10 +4519,18 @@ int cmd_pack_objects(int argc,\n \t\tstrvec_push(&rp, \"--unpacked\");\n \t}\n \n+\tif (exclude_promisor_objects && exclude_promisor_objects_best_effort)\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t \"--exclude-promisor-objects\", \"--exclude-promisor-objects-best-effort\");\n \tif (exclude_promisor_objects) {\n \t\tuse_internal_rev_list = 1;\n \t\tfetch_if_missing = 0;\n \t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n+\t} else if (exclude_promisor_objects_best_effort) {\n+\t\tuse_internal_rev_list = 1;\n+\t\tfetch_if_missing = 0;\n+\t\toption_parse_missing_action(NULL, \"allow-any\", 0);\n+\t\t/* revs configured below */\n \t}\n \tif (unpack_unreachable || keep_unreachable || pack_loose_unreachable)\n \t\tuse_internal_rev_list = 1;\n@@ -4626,6 +4650,10 @@ int cmd_pack_objects(int argc,\n \n \t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n+\t\tif (exclude_promisor_objects_best_effort) {\n+\t\t\trevs.include_check = should_include;\n+\t\t\trevs.include_check_obj = should_include_obj;\n+\t\t}\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t\trelease_revisions(&revs);\n \t}\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex c53e93be2f..2e67f59f89 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -694,6 +694,36 @@ test_expect_success 'lazy-fetch in submodule succeeds' '\n \tgit -C client restore --recurse-submodules --source=HEAD^ :/\n '\n \n+test_expect_success 'after fetching descendants of non-promisor commits, gc works' '\n+\t# Setup\n+\tgit init full &&\n+\tgit -C full config uploadpack.allowfilter 1 &&\n+ \tgit -C full config uploadpack.allowanysha1inwant 1 &&\n+\ttouch full/foo &&\n+\tgit -C full add foo &&\n+\tgit -C full commit -m \"commit 1\" &&\n+\tgit -C full checkout --detach &&\n+\n+\t# Partial clone and push commit to remote\n+\tgit clone \"file://$(pwd)/full\" --filter=blob:none partial &&\n+\techo \"hello\" > partial/foo &&\n+\tgit -C partial commit -a -m \"commit 2\" &&\n+\tgit -C partial push &&\n+\n+\t# gc in partial repo\n+\tgit -C partial gc --prune=now &&\n+\n+\t# Create another commit in normal repo\n+\tgit -C full checkout main &&\n+\techo \" world\" >> full/foo &&\n+\tgit -C full commit -a -m \"commit 3\" &&\n+\n+\t# Pull from remote in partial repo, and run gc again\n+\tgit -C partial pull &&\n+\tgit -C partial gc --prune=now\n+'\n+\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506079","messageId":"CAG1j3zHXThL_JXP=9xqvg=wg0R1wZYnA-okfFxqmcUQ9w0M36g@mail.gmail.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"Re: [External] [PATCH 0/5] When fetching from a promisor remote, repack local objects referenced","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2024-10-25T06:04:21Z","receivedAt":"2024-10-25T06:04:33Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Fri, Oct 25, 2024 at 2:09 AM Jonathan Tan wrote:\n>\n> This is a polished version of [1], also with all the test failures\n> debugged and addressed.\n\nThanks! I think I can drop my \"repack all\" patches now. :)\n"},{"id":"506121","messageId":"ZxwIhDsM+19nAZkT@nand.local","threadId":"62404","inReplyTo":"CAG1j3zHXThL_JXP=9xqvg=wg0R1wZYnA-okfFxqmcUQ9w0M36g@mail.gmail.com","subject":"Re: [External] [PATCH 0/5] When fetching from a promisor remote, repack local objects referenced","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:07:16Z","receivedAt":"2024-10-25T21:07:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Oct 25, 2024 at 02:04:21PM +0800, Han Young wrote:\n> On Fri, Oct 25, 2024 at 2:09 AM Jonathan Tan wrote:\n> >\n> > This is a polished version of [1], also with all the test failures\n> > debugged and addressed.\n>\n> Thanks! I think I can drop my \"repack all\" patches now. :)\n\nThanks for saying so, I dropped the 'hy/partial-repack-fix' branch from\nmy tree.\n\nThanks,\nTaylor\n"},{"id":"506122","messageId":"ZxwIkPJLuDDxfSgE@nand.local","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/5] When fetching from a promisor remote, repack local objects referenced","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-25T21:07:28Z","receivedAt":"2024-10-25T21:07:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 24, 2024 at 11:08:39AM -0700, Jonathan Tan wrote:\n> Jonathan Tan (5):\n> pack-objects: make variable non-static\n> t0410: make test description clearer\n> t0410: use from-scratch server\n> t5300: move --window clamp test next to unclamped\n> index-pack: repack local links into promisor packs\n\nThanks, will queue.\n\nThanks,\nTaylor\n"},{"id":"506184","messageId":"Zx7bEq5DVG4CmokI@nand.local","threadId":"62404","inReplyTo":"b2c76c207d3ea880c612f115f733a91bc6b529a3.1729792911.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/5] pack-objects: make variable non-static","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-28T00:30:10Z","receivedAt":"2024-10-28T00:30:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 24, 2024 at 11:08:40AM -0700, Jonathan Tan wrote:\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/pack-objects.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 0fc0680b40..e15fbaeb21 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -238,8 +238,6 @@ static enum {\n> } write_bitmap_index;\n> static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n>\n> -static int exclude_promisor_objects;\n> -\n> static int use_delta_islands;\n>\n> static unsigned long delta_cache_size = 0;\n> @@ -4327,6 +4325,7 @@ int cmd_pack_objects(int argc,\n> \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n> \tstruct list_objects_filter_options filter_options =\n> \t\tLIST_OBJECTS_FILTER_INIT;\n> +\tint exclude_promisor_objects = 0;\n>\n> \tstruct option pack_objects_options[] = {\n> \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n> --\n> 2.47.0.163.g1226f6d8fa-goog\n\nThis patch appears to conflict with ds/path-walk, which wants to read\nthe exclude_promisor_objects variable from outside of cmd_pack_objects()\n(but elsewhere within the builtin/pack-objects.c compilation unit).\n\nIs this refactoring a necessary step, or just cleanup? If the former, it\nmay be good for you and Stolee (CC'd) to work together to figure out how\nto eliminate the conflict from your two series. If the latter, it may be\nworth dropping this patch.\n\nThanks,\nTaylor\n"},{"id":"506251","messageId":"20241028193409.3648734-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"Zx7bEq5DVG4CmokI@nand.local","subject":"Re: [PATCH 1/5] pack-objects: make variable non-static","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-28T19:34:09Z","receivedAt":"2024-10-28T19:34:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau writes:\n> This patch appears to conflict with ds/path-walk, which wants to read\n> the exclude_promisor_objects variable from outside of cmd_pack_objects()\n> (but elsewhere within the builtin/pack-objects.c compilation unit).\n> \n> Is this refactoring a necessary step, or just cleanup? If the former, it\n> may be good for you and Stolee (CC'd) to work together to figure out how\n> to eliminate the conflict from your two series. If the latter, it may be\n> worth dropping this patch.\n> \n> Thanks,\n> Taylor\n\nIt's just cleanup. I've dropped this patch in my local copy but will\nwait for reviews before sending the next one (probably not worth sending\nit now since it's a relatively trivial change).\n\nI've also looked briefly at ds/path-walk - will reply with a few\ncomments on that email thread.\n"},{"id":"506253","messageId":"Zx/q/HztQRT4eLMQ@nand.local","threadId":"62404","inReplyTo":"20241028193409.3648734-1-jonathantanmy@google.com","subject":"Re: [PATCH 1/5] pack-objects: make variable non-static","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-28T19:50:20Z","receivedAt":"2024-10-28T19:50:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 28, 2024 at 12:34:09PM -0700, Jonathan Tan wrote:\n> Taylor Blau writes:\n> > This patch appears to conflict with ds/path-walk, which wants to read\n> > the exclude_promisor_objects variable from outside of cmd_pack_objects()\n> > (but elsewhere within the builtin/pack-objects.c compilation unit).\n> >\n> > Is this refactoring a necessary step, or just cleanup? If the former, it\n> > may be good for you and Stolee (CC'd) to work together to figure out how\n> > to eliminate the conflict from your two series. If the latter, it may be\n> > worth dropping this patch.\n> >\n> > Thanks,\n> > Taylor\n>\n> It's just cleanup. I've dropped this patch in my local copy but will\n> wait for reviews before sending the next one (probably not worth sending\n> it now since it's a relatively trivial change).\n>\n> I've also looked briefly at ds/path-walk - will reply with a few\n> comments on that email thread.\n\nGreat, thanks on both.\n\nThanks,\nTaylor\n\nP.S.: it's good to see you back on the list again :-).\n"},{"id":"506268","messageId":"20241028230410.4154271-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"Zx/q/HztQRT4eLMQ@nand.local","subject":"Re: [PATCH 1/5] pack-objects: make variable non-static","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-10-28T23:04:10Z","receivedAt":"2024-10-28T23:04:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Taylor Blau writes:\n> Great, thanks on both.\n> \n> Thanks,\n> Taylor\n\nThanks also for your work in coordinating the patches from various\nauthors.\n\n> P.S.: it's good to see you back on the list again :-).\n\nThank you :)\n"},{"id":"506342","messageId":"radxsrv6sjemdzl2mw5zzkieyim6xfikrevwggjmzi774g2sob@4nx7fwcjfk32","threadId":"62404","inReplyTo":"5dd7fdc16df7757ee1b24997ad9fbe3f923d5e93.1729792911.git.jonathantanmy@google.com","subject":"Re: [PATCH 5/5] index-pack: repack local links into promisor packs","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-10-30T22:29:59Z","receivedAt":"2024-10-30T22:30:05Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Mostly looks good, just one point of confusion on my part, and one nit.\nThanks for the patch! (comments inline below)\n\n\nOn 2024.10.24 11:08, Jonathan Tan wrote:\n> Teach index-pack to, when processing the objects in a pack with\n> --promisor specified on the CLI, repack local objects (and the local\n> objects that they refer to, recursively) referenced by these objects\n> into promisor packs.\n> \n> This prevents the situation in which, when fetching from a promisor\n> remote, we end up with promisor objects (newly fetched) referring\n> to non-promisor objects (locally created prior to the fetch). This\n> situation may arise if the client had previously pushed objects to the\n> remote, for example. One issue that arises in this situation is that,\n> if the non-promisor objects become inaccessible except through promisor\n> objects (for example, if the branch pointing to them has moved to\n> point to the promisor object that refers to them), then GC will garbage\n> collect them. There are other ways to solve this, but the simplest\n> seems to be to enforce the invariant that we don't have promisor objects\n> referring to non-promisor objects.\n> \n> This repacking is done from index-pack to minimize the performance\n> impact. During a fetch, the only time most objects are fully inflated\n> in memory is when their object ID is computed, so we also scan the\n> objects (to see which objects they refer to) during this time.\n> \n> Also to minimize the performance impact, an object is calculated to be\n> local if it's a loose object or present in a non-promisor pack. (If it's\n> also in a promisor pack or referred to by an object in a promisor pack,\n> it is technically already a promisor object. But a misidentification\n> of a promisor object as a non-promisor object is relatively benign\n> here - we will thus repack that promisor object into a promisor pack,\n> duplicating it in the object store, but there is no correctness issue,\n> just an issue of inefficiency.)\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> Documentation/git-index-pack.txt | 5 ++\n> builtin/index-pack.c | 110 ++++++++++++++++++++++++++++++-\n> builtin/pack-objects.c | 28 ++++++++\n> t/t5616-partial-clone.sh | 30 +++++++++\n> 4 files changed, 171 insertions(+), 2 deletions(-)\n> \n> diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\n> index 5a20deefd5..4be09e58e7 100644\n> --- a/Documentation/git-index-pack.txt\n> +++ b/Documentation/git-index-pack.txt\n> @@ -139,6 +139,11 @@ include::object-format-disclaimer.txt[]\n> \twritten. If a `` is provided, then that content will be\n> \twritten to the .promisor file for future reference. See\n> \tlink:technical/partial-clone.html[partial clone] for more information.\n> ++\n> +Also, if there are objects in the given pack that references non-promisor\n> +objects (in the repo), repacks those non-promisor objects into a promisor\n> +pack. This avoids a situation in which a repo has non-promisor objects that are\n> +accessible through promisor objects.\n> \n> NOTES\n> -----\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 9d23b41b3a..e4afd6725f 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -9,6 +9,7 @@\n> #include \"csum-file.h\"\n> #include \"blob.h\"\n> #include \"commit.h\"\n> +#include \"tag.h\"\n> #include \"tree.h\"\n> #include \"progress.h\"\n> #include \"fsck.h\"\n> @@ -20,9 +21,14 @@\n> #include \"object-file.h\"\n> #include \"object-store-ll.h\"\n> #include \"oid-array.h\"\n> +#include \"oidset.h\"\n> +#include \"path.h\"\n> #include \"replace-object.h\"\n> +#include \"tree-walk.h\"\n> #include \"promisor-remote.h\"\n> +#include \"run-command.h\"\n> #include \"setup.h\"\n> +#include \"strvec.h\"\n> \n> static const char index_pack_usage[] =\n> \"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n> @@ -148,6 +154,13 @@ static uint32_t input_crc32;\n> static int input_fd, output_fd;\n> static const char *curr_pack;\n> \n> +/*\n> + * local_links is guarded by read_mutex, and record_local_links is read-only in\n> + * a thread.\n> + */\n> +static struct oidset local_links = OIDSET_INIT;\n> +static int record_local_links;\n> +\n> static struct thread_local *thread_data;\n> static int nr_dispatched;\n> static int threads_active;\n> @@ -799,6 +812,44 @@ static int check_collison(struct object_entry *entry)\n> \treturn 0;\n> }\n> \n> +static void record_if_local_object(const struct object_id *oid)\n> +{\n> +\tstruct object_info info = OBJECT_INFO_INIT;\n> +\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n> +\t\t/* Missing; assume it is a promisor object */\n> +\t\treturn;\n> +\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n> +\t\treturn;\n> +\toidset_insert(&local_links, oid);\n> +}\n> +\n> +static void do_record_local_links(struct object *obj)\n> +{\n> +\tif (obj->type == OBJ_TREE) {\n> +\t\tstruct tree *tree = (struct tree *)obj;\n> +\t\tstruct tree_desc desc;\n> +\t\tstruct name_entry entry;\n> +\t\tif (init_tree_desc_gently(&desc, &tree->object.oid,\n> +\t\t\t\t\t tree->buffer, tree->size, 0))\n\nThis part confused me for a bit, since I didn't see how simply casting\na `struct object *` to a `struct tree *` could possibly guarantee that\n`tree->buffer` or `tree->size` pointed to valid memory. But the object\npointers in question have all been created via `parse_object_buffer` and\nallocated as the appropriate blob/tree/commit/tag structs, and were\npreviously cast to `struct object *` by that function. So all looks good\nhere.\n\n\n> +\t\t\t/*\n> +\t\t\t * Error messages are given when packs are\n> +\t\t\t * verified, so do not print any here.\n> +\t\t\t */\n> +\t\t\treturn;\n> +\t\twhile (tree_entry_gently(&desc, &entry))\n> +\t\t\trecord_if_local_object(&entry.oid);\n> +\t} else if (obj->type == OBJ_COMMIT) {\n> +\t\tstruct commit *commit = (struct commit *) obj;\n> +\t\tstruct commit_list *parents = commit->parents;\n> +\n> +\t\tfor (; parents; parents = parents->next)\n> +\t\t\trecord_if_local_object(&parents->item->object.oid);\n> +\t} else if (obj->type == OBJ_TAG) {\n> +\t\tstruct tag *tag = (struct tag *) obj;\n> +\t\trecord_if_local_object(get_tagged_oid(tag));\n> +\t}\n\nWe only ever `record_if_local_object()` on things referenced by `obj`\nhere but not on `obj` itself. We should only be calling this on objects\nthat are inside a promisor pack, so all good here I think.\n\n> +}\n> +\n> static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\t\tunsigned long size, enum object_type type,\n> \t\t\tconst struct object_id *oid)\n> @@ -845,7 +896,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\tfree(has_data);\n> \t}\n> \n> -\tif (strict || do_fsck_object) {\n> +\tif (strict || do_fsck_object || record_local_links) {\n> \t\tread_lock();\n> \t\tif (type == OBJ_BLOB) {\n> \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n> @@ -877,6 +928,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\t\t\tdie(_(\"fsck error in packed object\"));\n> \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n> \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n> +\t\t\tif (record_local_links)\n> +\t\t\t\tdo_record_local_links(obj);\n> \n> \t\t\tif (obj->type == OBJ_TREE) {\n> \t\t\t\tstruct tree *item = (struct tree *) obj;\n> @@ -1719,6 +1772,57 @@ static void show_pack_info(int stat_only)\n> \tfree(chain_histogram);\n> }\n> \n> +static void repack_local_links(void)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tFILE *out;\n> +\tstruct strbuf line = STRBUF_INIT;\n> +\tstruct oidset_iter iter;\n> +\tstruct object_id *oid;\n> +\tchar *base_name;\n> +\n> +\tif (!oidset_size(&local_links))\n> +\t\treturn;\n> +\n> +\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n> +\n> +\tstrvec_push(&cmd.args, \"pack-objects\");\n> +\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n> +\tstrvec_push(&cmd.args, base_name);\n> +\tcmd.git_cmd = 1;\n> +\tcmd.in = -1;\n> +\tcmd.out = -1;\n> +\tif (start_command(&cmd))\n> +\t\tdie(_(\"could not start pack-objects to repack local links\"));\n> +\n> +\toidset_iter_init(&local_links, &iter);\n> +\twhile ((oid = oidset_iter_next(&iter))) {\n> +\t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> +\t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n> +\t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n> +\t}\n> +\tclose(cmd.in);\n> +\n> +\tout = xfdopen(cmd.out, \"r\");\n> +\twhile (strbuf_getline_lf(&line, out) != EOF) {\n> +\t\tunsigned char binary[GIT_MAX_RAWSZ];\n> +\t\tif (line.len != the_hash_algo->hexsz ||\n> +\t\t !hex_to_bytes(binary, line.buf, line.len))\n> +\t\t\tdie(_(\"index-pack: Expecting full hex object ID lines only from pack-objects.\"));\n\nI'm not sure why we check the pack-objects output here, is this just to\ndetect errors? Could we instead just check the exit status of\npack-objects, and discard the output?\n\n\n> +\n> +\t\t/*\n> +\t\t * pack-objects creates the .pack and .idx files, but not the\n> +\t\t * .promisor file. Create the .promisor file, which is empty.\n> +\t\t */\n> +\t\twrite_special_file(\"promisor\", \"\", NULL, binary, NULL);\n> +\t}\n> +\n> +\tfclose(out);\n> +\tif (finish_command(&cmd))\n> +\t\tdie(_(\"could not finish pack-objects to repack local links\"));\n> +\tstrbuf_release(&line);\n> +}\n> +\n> int cmd_index_pack(int argc,\n> \t\t const char **argv,\n> \t\t const char *prefix,\n> @@ -1794,7 +1898,7 @@ int cmd_index_pack(int argc,\n> \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n> \t\t\t\t; /* nothing to do */\n> \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n> -\t\t\t\t; /* already parsed */\n> +\t\t\t\trecord_local_links = 1;\n> \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n> \t\t\t\tchar *end;\n> \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n> @@ -1970,6 +2074,8 @@ int cmd_index_pack(int argc,\n> \t\tfree((void *) curr_index);\n> \tfree(curr_rev_index);\n> \n> +\trepack_local_links();\n> +\n> \t/*\n> \t * Let the caller know this pack is not self contained\n> \t */\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index e15fbaeb21..a565ab9b40 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4310,6 +4310,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n> \treturn 0;\n> }\n> \n> +static int should_include_obj(struct object *obj, void *data UNUSED)\n> +{\n> +\tstruct object_info info = OBJECT_INFO_INIT;\n> +\tif (oid_object_info_extended(the_repository, &obj->oid, &info, 0))\n> +\t\tBUG(\"should_include_obj should only be called on existing objects\");\n> +\treturn info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;\n> +}\n> +\n> +static int should_include(struct commit *commit, void *data) {\n> +\treturn should_include_obj((struct object *) commit, data);\n> +}\n> +\n\nNit: these two functions could be named a bit more descriptively.\n\n\n> int cmd_pack_objects(int argc,\n> \t\t const char **argv,\n> \t\t const char *prefix,\n> @@ -4326,6 +4338,7 @@ int cmd_pack_objects(int argc,\n> \tstruct list_objects_filter_options filter_options =\n> \t\tLIST_OBJECTS_FILTER_INIT;\n> \tint exclude_promisor_objects = 0;\n> +\tint exclude_promisor_objects_best_effort = 0;\n> \n> \tstruct option pack_objects_options[] = {\n> \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n> @@ -4423,6 +4436,9 @@ int cmd_pack_objects(int argc,\n> \t\t option_parse_missing_action),\n> \t\tOPT_BOOL(0, \"exclude-promisor-objects\", &exclude_promisor_objects,\n> \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n> +\t\tOPT_BOOL(0, \"exclude-promisor-objects-best-effort\",\n> +\t\t\t &exclude_promisor_objects_best_effort,\n> +\t\t\t N_(\"implies --missing=allow-any\")),\n> \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n> \t\t\t N_(\"respect islands during delta compression\")),\n> \t\tOPT_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n> @@ -4503,10 +4519,18 @@ int cmd_pack_objects(int argc,\n> \t\tstrvec_push(&rp, \"--unpacked\");\n> \t}\n> \n> +\tif (exclude_promisor_objects && exclude_promisor_objects_best_effort)\n> +\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n> +\t\t \"--exclude-promisor-objects\", \"--exclude-promisor-objects-best-effort\");\n> \tif (exclude_promisor_objects) {\n> \t\tuse_internal_rev_list = 1;\n> \t\tfetch_if_missing = 0;\n> \t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n> +\t} else if (exclude_promisor_objects_best_effort) {\n> +\t\tuse_internal_rev_list = 1;\n> +\t\tfetch_if_missing = 0;\n> +\t\toption_parse_missing_action(NULL, \"allow-any\", 0);\n> +\t\t/* revs configured below */\n> \t}\n> \tif (unpack_unreachable || keep_unreachable || pack_loose_unreachable)\n> \t\tuse_internal_rev_list = 1;\n> @@ -4626,6 +4650,10 @@ int cmd_pack_objects(int argc,\n> \n> \t\trepo_init_revisions(the_repository, &revs, NULL);\n> \t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n> +\t\tif (exclude_promisor_objects_best_effort) {\n> +\t\t\trevs.include_check = should_include;\n> +\t\t\trevs.include_check_obj = should_include_obj;\n> +\t\t}\n> \t\tget_object_list(&revs, rp.nr, rp.v);\n> \t\trelease_revisions(&revs);\n> \t}\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index c53e93be2f..2e67f59f89 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -694,6 +694,36 @@ test_expect_success 'lazy-fetch in submodule succeeds' '\n> \tgit -C client restore --recurse-submodules --source=HEAD^ :/\n> '\n> \n> +test_expect_success 'after fetching descendants of non-promisor commits, gc works' '\n> +\t# Setup\n> +\tgit init full &&\n> +\tgit -C full config uploadpack.allowfilter 1 &&\n> + \tgit -C full config uploadpack.allowanysha1inwant 1 &&\n> +\ttouch full/foo &&\n> +\tgit -C full add foo &&\n> +\tgit -C full commit -m \"commit 1\" &&\n> +\tgit -C full checkout --detach &&\n> +\n> +\t# Partial clone and push commit to remote\n> +\tgit clone \"file://$(pwd)/full\" --filter=blob:none partial &&\n> +\techo \"hello\" > partial/foo &&\n> +\tgit -C partial commit -a -m \"commit 2\" &&\n> +\tgit -C partial push &&\n> +\n> +\t# gc in partial repo\n> +\tgit -C partial gc --prune=now &&\n> +\n> +\t# Create another commit in normal repo\n> +\tgit -C full checkout main &&\n> +\techo \" world\" >> full/foo &&\n> +\tgit -C full commit -a -m \"commit 3\" &&\n> +\n> +\t# Pull from remote in partial repo, and run gc again\n> +\tgit -C partial pull &&\n> +\tgit -C partial gc --prune=now\n> +'\n> +\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n> \n> -- \n> 2.47.0.163.g1226f6d8fa-goog\n> \n> \n"},{"id":"506465","messageId":"cover.1730491845.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1729792911.git.jonathantanmy@google.com","subject":"[PATCH v2 0/4] When fetching from a promisor remote, repack local objects referenced","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T20:11:44Z","receivedAt":"2024-11-01T20:11:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for looking at it. Here's version 2.\n\nJonathan Tan (4):\n t0410: make test description clearer\n t0410: use from-scratch server\n t5300: move --window clamp test next to unclamped\n index-pack: repack local links into promisor packs\n\n Documentation/git-index-pack.txt | 5 ++\n builtin/index-pack.c | 110 ++++++++++++++++++++++++++++++-\n builtin/pack-objects.c | 28 ++++++++\n t/t0410-partial-clone.sh | 6 +-\n t/t5300-pack-object.sh | 10 +--\n t/t5616-partial-clone.sh | 30 +++++++++\n 6 files changed, 179 insertions(+), 10 deletions(-)\n\nRange-diff against v1:\n1: b2c76c207d < -: ---------- pack-objects: make variable non-static\n2: c220e77ccf = 1: f405c9c9aa t0410: make test description clearer\n3: 08750988e0 = 2: ce9d5af42a t0410: use from-scratch server\n4: 85fc3fa77e = 3: 1526a59e2d t5300: move --window clamp test next to unclamped\n5: 5dd7fdc16d ! 4: c51fac33fb index-pack: repack local links into promisor packs\n @@ builtin/index-pack.c: int cmd_index_pack(int argc,\n \t */\n \n ## builtin/pack-objects.c ##\n +@@ builtin/pack-objects.c: static enum {\n + static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n + \n + static int exclude_promisor_objects;\n ++static int exclude_promisor_objects_best_effort;\n + \n + static int use_delta_islands;\n + \n @@ builtin/pack-objects.c: static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n \treturn 0;\n }\n \n -+static int should_include_obj(struct object *obj, void *data UNUSED)\n ++static int is_not_in_promisor_pack_obj(struct object *obj, void *data UNUSED)\n +{\n +\tstruct object_info info = OBJECT_INFO_INIT;\n +\tif (oid_object_info_extended(the_repository, &obj->oid, &info, 0))\n @@ builtin/pack-objects.c: static int option_parse_cruft_expiration(const struct op\n +\treturn info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;\n +}\n +\n -+static int should_include(struct commit *commit, void *data) {\n -+\treturn should_include_obj((struct object *) commit, data);\n ++static int is_not_in_promisor_pack(struct commit *commit, void *data) {\n ++\treturn is_not_in_promisor_pack_obj((struct object *) commit, data);\n +}\n +\n int cmd_pack_objects(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n -@@ builtin/pack-objects.c: int cmd_pack_objects(int argc,\n - \tstruct list_objects_filter_options filter_options =\n - \t\tLIST_OBJECTS_FILTER_INIT;\n - \tint exclude_promisor_objects = 0;\n -+\tint exclude_promisor_objects_best_effort = 0;\n - \n - \tstruct option pack_objects_options[] = {\n - \t\tOPT_CALLBACK_F('q', \"quiet\", &progress, NULL,\n @@ builtin/pack-objects.c: int cmd_pack_objects(int argc,\n \t\t option_parse_missing_action),\n \t\tOPT_BOOL(0, \"exclude-promisor-objects\", &exclude_promisor_objects,\n @@ builtin/pack-objects.c: int cmd_pack_objects(int argc,\n \t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n +\t\tif (exclude_promisor_objects_best_effort) {\n -+\t\t\trevs.include_check = should_include;\n -+\t\t\trevs.include_check_obj = should_include_obj;\n ++\t\t\trevs.include_check = is_not_in_promisor_pack;\n ++\t\t\trevs.include_check_obj = is_not_in_promisor_pack_obj;\n +\t\t}\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t\trelease_revisions(&revs);\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506466","messageId":"f405c9c9aab39984d27ff31ed03186a713f79035.1730491845.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1730491845.git.jonathantanmy@google.com","subject":"[PATCH v2 1/4] t0410: make test description clearer","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T20:11:45Z","receivedAt":"2024-11-01T20:11:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 9a4c507886 (t0410: test fetching from many promisor remotes,\n2019-06-25) adds some tests that demonstrate not the automatic fetching\nof missing objects, but the direct fetching from another promisor remote\n(configured explicitly in one test and implicitly via --filter on the\n\"git fetch\" CLI invocation in the other test) - thus demonstrating\nsupport for multiple promisor remotes, as described in the commit\nmessage.\n\nChange the test descriptions accordingly to make this clearer.\n\nSigned-off-by: Jonathan Tan \n---\n t/t0410-partial-clone.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 818700fbec..eadb69473f 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -241,7 +241,7 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \tgrep \"fetch< fetch=.*ref-in-want\" trace\n '\n \n-test_expect_success 'fetching of missing objects from another promisor remote' '\n+test_expect_success 'fetching from another promisor remote' '\n \tgit clone \"file://$(pwd)/server\" server2 &&\n \ttest_commit -C server2 bar &&\n \tgit -C server2 repack -a -d --write-bitmap-index &&\n@@ -264,7 +264,7 @@ test_expect_success 'fetching of missing objects from another promisor remote' '\n \tgrep \"$HASH2\" out\n '\n \n-test_expect_success 'fetching of missing objects configures a promisor remote' '\n+test_expect_success 'fetching with --filter configures a promisor remote' '\n \tgit clone \"file://$(pwd)/server\" server3 &&\n \ttest_commit -C server3 baz &&\n \tgit -C server3 repack -a -d --write-bitmap-index &&\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506467","messageId":"ce9d5af42a50ba115fb9b11f9063f207b471b672.1730491845.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1730491845.git.jonathantanmy@google.com","subject":"[PATCH v2 2/4] t0410: use from-scratch server","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T20:11:46Z","receivedAt":"2024-11-01T20:11:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will add functionality: when fetching from a\npromisor remote, existing non-promisor objects that are ancestors of any\nfetched object will be repacked into promisor packs (since if a promisor\nremote has an object, it also has all its ancestors).\n\nThis means that sometimes, a fetch from a promisor remote results in 2\nnew promisor packs (instead of the 1 that you would expect). There is a\ntest that fetches a descendant of a local object from a promisor remote,\nbut also specifically tests that there is exactly 1 promisor pack as\na result of the fetch. This means that this test will fail when the\nsubsequent commit is added.\n\nSince the ancestry of the fetched object is not the concern of this\ntest, make the fetched objects have no ancestry in common with the\nobjets in the client repo. This is done by making the server from\nscratch, instead of using an existing repo that has objects in common\nwith the client.\n\nSigned-off-by: Jonathan Tan \n---\n t/t0410-partial-clone.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex eadb69473f..e2b317db65 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -265,7 +265,7 @@ test_expect_success 'fetching from another promisor remote' '\n '\n \n test_expect_success 'fetching with --filter configures a promisor remote' '\n-\tgit clone \"file://$(pwd)/server\" server3 &&\n+\ttest_create_repo server3 &&\n \ttest_commit -C server3 baz &&\n \tgit -C server3 repack -a -d --write-bitmap-index &&\n \tHASH3=$(git -C server3 rev-parse baz) &&\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506468","messageId":"1526a59e2d4ace2761fd8935c63350f0a41985c6.1730491845.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1730491845.git.jonathantanmy@google.com","subject":"[PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T20:11:47Z","receivedAt":"2024-11-01T20:11:57Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent commit will change the behavior of \"git index-pack\n--promisor\", which is exercised in \"build pack index for an existing\npack\", causing the unclamped and clamped versions of the --window\ntest to exhibit different behavior. Move the clamp test closer to the\nunclamped test that it references.\n\nSigned-off-by: Jonathan Tan \n---\n t/t5300-pack-object.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 3b9dae331a..aff164ddf8 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -156,6 +156,11 @@ test_expect_success 'pack without delta' '\n \tcheck_deltas stderr = 0\n '\n \n+test_expect_success 'negative window clamps to 0' '\n+\tgit pack-objects --progress --window=-1 neg-window stderr &&\n+\tcheck_deltas stderr = 0\n+'\n+\n test_expect_success 'pack-objects with bogus arguments' '\n \ttest_must_fail git pack-objects --window=0 test-1 blah blah stderr &&\n-\tcheck_deltas stderr = 0\n-'\n-\n for hash in sha1 sha256\n do\n \ttest_expect_success \"verify-pack with $hash packfile\" '\n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506469","messageId":"c51fac33fb68b75c28da16005b0e76f5fa2b37f0.1730491845.git.jonathantanmy@google.com","threadId":"62404","inReplyTo":"cover.1730491845.git.jonathantanmy@google.com","subject":"[PATCH v2 4/4] index-pack: repack local links into promisor packs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T20:11:48Z","receivedAt":"2024-11-01T20:12:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach index-pack to, when processing the objects in a pack with\n--promisor specified on the CLI, repack local objects (and the local\nobjects that they refer to, recursively) referenced by these objects\ninto promisor packs.\n\nThis prevents the situation in which, when fetching from a promisor\nremote, we end up with promisor objects (newly fetched) referring\nto non-promisor objects (locally created prior to the fetch). This\nsituation may arise if the client had previously pushed objects to the\nremote, for example. One issue that arises in this situation is that,\nif the non-promisor objects become inaccessible except through promisor\nobjects (for example, if the branch pointing to them has moved to\npoint to the promisor object that refers to them), then GC will garbage\ncollect them. There are other ways to solve this, but the simplest\nseems to be to enforce the invariant that we don't have promisor objects\nreferring to non-promisor objects.\n\nThis repacking is done from index-pack to minimize the performance\nimpact. During a fetch, the only time most objects are fully inflated\nin memory is when their object ID is computed, so we also scan the\nobjects (to see which objects they refer to) during this time.\n\nAlso to minimize the performance impact, an object is calculated to be\nlocal if it's a loose object or present in a non-promisor pack. (If it's\nalso in a promisor pack or referred to by an object in a promisor pack,\nit is technically already a promisor object. But a misidentification\nof a promisor object as a non-promisor object is relatively benign\nhere - we will thus repack that promisor object into a promisor pack,\nduplicating it in the object store, but there is no correctness issue,\njust an issue of inefficiency.)\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-index-pack.txt | 5 ++\n builtin/index-pack.c | 110 ++++++++++++++++++++++++++++++-\n builtin/pack-objects.c | 28 ++++++++\n t/t5616-partial-clone.sh | 30 +++++++++\n 4 files changed, 171 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 5a20deefd5..4be09e58e7 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -139,6 +139,11 @@ include::object-format-disclaimer.txt[]\n \twritten. If a `` is provided, then that content will be\n \twritten to the .promisor file for future reference. See\n \tlink:technical/partial-clone.html[partial clone] for more information.\n++\n+Also, if there are objects in the given pack that references non-promisor\n+objects (in the repo), repacks those non-promisor objects into a promisor\n+pack. This avoids a situation in which a repo has non-promisor objects that are\n+accessible through promisor objects.\n \n NOTES\n -----\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9d23b41b3a..e4afd6725f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -9,6 +9,7 @@\n #include \"csum-file.h\"\n #include \"blob.h\"\n #include \"commit.h\"\n+#include \"tag.h\"\n #include \"tree.h\"\n #include \"progress.h\"\n #include \"fsck.h\"\n@@ -20,9 +21,14 @@\n #include \"object-file.h\"\n #include \"object-store-ll.h\"\n #include \"oid-array.h\"\n+#include \"oidset.h\"\n+#include \"path.h\"\n #include \"replace-object.h\"\n+#include \"tree-walk.h\"\n #include \"promisor-remote.h\"\n+#include \"run-command.h\"\n #include \"setup.h\"\n+#include \"strvec.h\"\n \n static const char index_pack_usage[] =\n \"git index-pack [-v] [-o ] [--keep | --keep=] [--[no-]rev-index] [--verify] [--strict[==...]] [--fsck-objects[==...]] ( | --stdin [--fix-thin] [])\";\n@@ -148,6 +154,13 @@ static uint32_t input_crc32;\n static int input_fd, output_fd;\n static const char *curr_pack;\n \n+/*\n+ * local_links is guarded by read_mutex, and record_local_links is read-only in\n+ * a thread.\n+ */\n+static struct oidset local_links = OIDSET_INIT;\n+static int record_local_links;\n+\n static struct thread_local *thread_data;\n static int nr_dispatched;\n static int threads_active;\n@@ -799,6 +812,44 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n+static void record_if_local_object(const struct object_id *oid)\n+{\n+\tstruct object_info info = OBJECT_INFO_INIT;\n+\tif (oid_object_info_extended(the_repository, oid, &info, 0))\n+\t\t/* Missing; assume it is a promisor object */\n+\t\treturn;\n+\tif (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)\n+\t\treturn;\n+\toidset_insert(&local_links, oid);\n+}\n+\n+static void do_record_local_links(struct object *obj)\n+{\n+\tif (obj->type == OBJ_TREE) {\n+\t\tstruct tree *tree = (struct tree *)obj;\n+\t\tstruct tree_desc desc;\n+\t\tstruct name_entry entry;\n+\t\tif (init_tree_desc_gently(&desc, &tree->object.oid,\n+\t\t\t\t\t tree->buffer, tree->size, 0))\n+\t\t\t/*\n+\t\t\t * Error messages are given when packs are\n+\t\t\t * verified, so do not print any here.\n+\t\t\t */\n+\t\t\treturn;\n+\t\twhile (tree_entry_gently(&desc, &entry))\n+\t\t\trecord_if_local_object(&entry.oid);\n+\t} else if (obj->type == OBJ_COMMIT) {\n+\t\tstruct commit *commit = (struct commit *) obj;\n+\t\tstruct commit_list *parents = commit->parents;\n+\n+\t\tfor (; parents; parents = parents->next)\n+\t\t\trecord_if_local_object(&parents->item->object.oid);\n+\t} else if (obj->type == OBJ_TAG) {\n+\t\tstruct tag *tag = (struct tag *) obj;\n+\t\trecord_if_local_object(get_tagged_oid(tag));\n+\t}\n+}\n+\n static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tunsigned long size, enum object_type type,\n \t\t\tconst struct object_id *oid)\n@@ -845,7 +896,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict || do_fsck_object) {\n+\tif (strict || do_fsck_object || record_local_links) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(the_repository, oid);\n@@ -877,6 +928,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tdie(_(\"fsck error in packed object\"));\n \t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n+\t\t\tif (record_local_links)\n+\t\t\t\tdo_record_local_links(obj);\n \n \t\t\tif (obj->type == OBJ_TREE) {\n \t\t\t\tstruct tree *item = (struct tree *) obj;\n@@ -1719,6 +1772,57 @@ static void show_pack_info(int stat_only)\n \tfree(chain_histogram);\n }\n \n+static void repack_local_links(void)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *out;\n+\tstruct strbuf line = STRBUF_INIT;\n+\tstruct oidset_iter iter;\n+\tstruct object_id *oid;\n+\tchar *base_name;\n+\n+\tif (!oidset_size(&local_links))\n+\t\treturn;\n+\n+\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n+\n+\tstrvec_push(&cmd.args, \"pack-objects\");\n+\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n+\tstrvec_push(&cmd.args, base_name);\n+\tcmd.git_cmd = 1;\n+\tcmd.in = -1;\n+\tcmd.out = -1;\n+\tif (start_command(&cmd))\n+\t\tdie(_(\"could not start pack-objects to repack local links\"));\n+\n+\toidset_iter_init(&local_links, &iter);\n+\twhile ((oid = oidset_iter_next(&iter))) {\n+\t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n+\t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n+\t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n+\t}\n+\tclose(cmd.in);\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tunsigned char binary[GIT_MAX_RAWSZ];\n+\t\tif (line.len != the_hash_algo->hexsz ||\n+\t\t !hex_to_bytes(binary, line.buf, line.len))\n+\t\t\tdie(_(\"index-pack: Expecting full hex object ID lines only from pack-objects.\"));\n+\n+\t\t/*\n+\t\t * pack-objects creates the .pack and .idx files, but not the\n+\t\t * .promisor file. Create the .promisor file, which is empty.\n+\t\t */\n+\t\twrite_special_file(\"promisor\", \"\", NULL, binary, NULL);\n+\t}\n+\n+\tfclose(out);\n+\tif (finish_command(&cmd))\n+\t\tdie(_(\"could not finish pack-objects to repack local links\"));\n+\tstrbuf_release(&line);\n+}\n+\n int cmd_index_pack(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -1794,7 +1898,7 @@ int cmd_index_pack(int argc,\n \t\t\t} else if (skip_to_optional_arg(arg, \"--keep\", &keep_msg)) {\n \t\t\t\t; /* nothing to do */\n \t\t\t} else if (skip_to_optional_arg(arg, \"--promisor\", &promisor_msg)) {\n-\t\t\t\t; /* already parsed */\n+\t\t\t\trecord_local_links = 1;\n \t\t\t} else if (starts_with(arg, \"--threads=\")) {\n \t\t\t\tchar *end;\n \t\t\t\tnr_threads = strtoul(arg+10, &end, 0);\n@@ -1970,6 +2074,8 @@ int cmd_index_pack(int argc,\n \t\tfree((void *) curr_index);\n \tfree(curr_rev_index);\n \n+\trepack_local_links();\n+\n \t/*\n \t * Let the caller know this pack is not self contained\n \t */\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fc0680b40..51d2ffe490 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -239,6 +239,7 @@ static enum {\n static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n \n static int exclude_promisor_objects;\n+static int exclude_promisor_objects_best_effort;\n \n static int use_delta_islands;\n \n@@ -4312,6 +4313,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n \treturn 0;\n }\n \n+static int is_not_in_promisor_pack_obj(struct object *obj, void *data UNUSED)\n+{\n+\tstruct object_info info = OBJECT_INFO_INIT;\n+\tif (oid_object_info_extended(the_repository, &obj->oid, &info, 0))\n+\t\tBUG(\"should_include_obj should only be called on existing objects\");\n+\treturn info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;\n+}\n+\n+static int is_not_in_promisor_pack(struct commit *commit, void *data) {\n+\treturn is_not_in_promisor_pack_obj((struct object *) commit, data);\n+}\n+\n int cmd_pack_objects(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -4424,6 +4437,9 @@ int cmd_pack_objects(int argc,\n \t\t option_parse_missing_action),\n \t\tOPT_BOOL(0, \"exclude-promisor-objects\", &exclude_promisor_objects,\n \t\t\t N_(\"do not pack objects in promisor packfiles\")),\n+\t\tOPT_BOOL(0, \"exclude-promisor-objects-best-effort\",\n+\t\t\t &exclude_promisor_objects_best_effort,\n+\t\t\t N_(\"implies --missing=allow-any\")),\n \t\tOPT_BOOL(0, \"delta-islands\", &use_delta_islands,\n \t\t\t N_(\"respect islands during delta compression\")),\n \t\tOPT_STRING_LIST(0, \"uri-protocol\", &uri_protocols,\n@@ -4504,10 +4520,18 @@ int cmd_pack_objects(int argc,\n \t\tstrvec_push(&rp, \"--unpacked\");\n \t}\n \n+\tif (exclude_promisor_objects && exclude_promisor_objects_best_effort)\n+\t\tdie(_(\"options '%s' and '%s' cannot be used together\"),\n+\t\t \"--exclude-promisor-objects\", \"--exclude-promisor-objects-best-effort\");\n \tif (exclude_promisor_objects) {\n \t\tuse_internal_rev_list = 1;\n \t\tfetch_if_missing = 0;\n \t\tstrvec_push(&rp, \"--exclude-promisor-objects\");\n+\t} else if (exclude_promisor_objects_best_effort) {\n+\t\tuse_internal_rev_list = 1;\n+\t\tfetch_if_missing = 0;\n+\t\toption_parse_missing_action(NULL, \"allow-any\", 0);\n+\t\t/* revs configured below */\n \t}\n \tif (unpack_unreachable || keep_unreachable || pack_loose_unreachable)\n \t\tuse_internal_rev_list = 1;\n@@ -4627,6 +4651,10 @@ int cmd_pack_objects(int argc,\n \n \t\trepo_init_revisions(the_repository, &revs, NULL);\n \t\tlist_objects_filter_copy(&revs.filter, &filter_options);\n+\t\tif (exclude_promisor_objects_best_effort) {\n+\t\t\trevs.include_check = is_not_in_promisor_pack;\n+\t\t\trevs.include_check_obj = is_not_in_promisor_pack_obj;\n+\t\t}\n \t\tget_object_list(&revs, rp.nr, rp.v);\n \t\trelease_revisions(&revs);\n \t}\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex c53e93be2f..2e67f59f89 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -694,6 +694,36 @@ test_expect_success 'lazy-fetch in submodule succeeds' '\n \tgit -C client restore --recurse-submodules --source=HEAD^ :/\n '\n \n+test_expect_success 'after fetching descendants of non-promisor commits, gc works' '\n+\t# Setup\n+\tgit init full &&\n+\tgit -C full config uploadpack.allowfilter 1 &&\n+ \tgit -C full config uploadpack.allowanysha1inwant 1 &&\n+\ttouch full/foo &&\n+\tgit -C full add foo &&\n+\tgit -C full commit -m \"commit 1\" &&\n+\tgit -C full checkout --detach &&\n+\n+\t# Partial clone and push commit to remote\n+\tgit clone \"file://$(pwd)/full\" --filter=blob:none partial &&\n+\techo \"hello\" > partial/foo &&\n+\tgit -C partial commit -a -m \"commit 2\" &&\n+\tgit -C partial push &&\n+\n+\t# gc in partial repo\n+\tgit -C partial gc --prune=now &&\n+\n+\t# Create another commit in normal repo\n+\tgit -C full checkout main &&\n+\techo \" world\" >> full/foo &&\n+\tgit -C full commit -a -m \"commit 3\" &&\n+\n+\t# Pull from remote in partial repo, and run gc again\n+\tgit -C partial pull &&\n+\tgit -C partial gc --prune=now\n+'\n+\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.47.0.163.g1226f6d8fa-goog\n\n"},{"id":"506470","messageId":"20241101201448.1695516-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"radxsrv6sjemdzl2mw5zzkieyim6xfikrevwggjmzi774g2sob@4nx7fwcjfk32","subject":"Re: [PATCH 5/5] index-pack: repack local links into promisor packs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-01T20:14:48Z","receivedAt":"2024-11-01T20:14:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Josh Steadmon writes:\n> > @@ -1719,6 +1772,57 @@ static void show_pack_info(int stat_only)\n> > \tfree(chain_histogram);\n> > }\n> > \n> > +static void repack_local_links(void)\n> > +{\n> > +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> > +\tFILE *out;\n> > +\tstruct strbuf line = STRBUF_INIT;\n> > +\tstruct oidset_iter iter;\n> > +\tstruct object_id *oid;\n> > +\tchar *base_name;\n> > +\n> > +\tif (!oidset_size(&local_links))\n> > +\t\treturn;\n> > +\n> > +\tbase_name = mkpathdup(\"%s/pack/pack\", repo_get_object_directory(the_repository));\n> > +\n> > +\tstrvec_push(&cmd.args, \"pack-objects\");\n> > +\tstrvec_push(&cmd.args, \"--exclude-promisor-objects-best-effort\");\n> > +\tstrvec_push(&cmd.args, base_name);\n> > +\tcmd.git_cmd = 1;\n> > +\tcmd.in = -1;\n> > +\tcmd.out = -1;\n> > +\tif (start_command(&cmd))\n> > +\t\tdie(_(\"could not start pack-objects to repack local links\"));\n> > +\n> > +\toidset_iter_init(&local_links, &iter);\n> > +\twhile ((oid = oidset_iter_next(&iter))) {\n> > +\t\tif (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||\n> > +\t\t write_in_full(cmd.in, \"\\n\", 1) < 0)\n> > +\t\t\tdie(_(\"failed to feed local object to pack-objects\"));\n> > +\t}\n> > +\tclose(cmd.in);\n> > +\n> > +\tout = xfdopen(cmd.out, \"r\");\n> > +\twhile (strbuf_getline_lf(&line, out) != EOF) {\n> > +\t\tunsigned char binary[GIT_MAX_RAWSZ];\n> > +\t\tif (line.len != the_hash_algo->hexsz ||\n> > +\t\t !hex_to_bytes(binary, line.buf, line.len))\n> > +\t\t\tdie(_(\"index-pack: Expecting full hex object ID lines only from pack-objects.\"));\n> \n> I'm not sure why we check the pack-objects output here, is this just to\n> detect errors? Could we instead just check the exit status of\n> pack-objects, and discard the output?\n\nThe output is a hex object ID that tells us what we need to name\nthe .promisor file. (Later in this function, \"binary\" is used.) So we\ncan't discard it.\n\n> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> > index e15fbaeb21..a565ab9b40 100644\n> > --- a/builtin/pack-objects.c\n> > +++ b/builtin/pack-objects.c\n> > @@ -4310,6 +4310,18 @@ static int option_parse_cruft_expiration(const struct option *opt UNUSED,\n> > \treturn 0;\n> > }\n> > \n> > +static int should_include_obj(struct object *obj, void *data UNUSED)\n> > +{\n> > +\tstruct object_info info = OBJECT_INFO_INIT;\n> > +\tif (oid_object_info_extended(the_repository, &obj->oid, &info, 0))\n> > +\t\tBUG(\"should_include_obj should only be called on existing objects\");\n> > +\treturn info.whence != OI_PACKED || !info.u.packed.pack->pack_promisor;\n> > +}\n> > +\n> > +static int should_include(struct commit *commit, void *data) {\n> > +\treturn should_include_obj((struct object *) commit, data);\n> > +}\n> > +\n> \n> Nit: these two functions could be named a bit more descriptively.\n\nOK, done.\n"},{"id":"506482","messageId":"xmqqwmhlyl7n.fsf@gitster.g","threadId":"62404","inReplyTo":"ZxwIhDsM+19nAZkT@nand.local","subject":"Re: [External] [PATCH 0/5] When fetching from a promisor remote, repack local objects referenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-02T10:38:20Z","receivedAt":"2024-11-02T10:38:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Fri, Oct 25, 2024 at 02:04:21PM +0800, Han Young wrote:\n>> On Fri, Oct 25, 2024 at 2:09 AM Jonathan Tan wrote:\n>> >\n>> > This is a polished version of [1], also with all the test failures\n>> > debugged and addressed.\n>>\n>> Thanks! I think I can drop my \"repack all\" patches now. :)\n>\n> Thanks for saying so, I dropped the 'hy/partial-repack-fix' branch from\n> my tree.\n\nI'll drop the topic, together with its \"Need review.\" comment, from\nthe \"What's Cooking\" draft, then. Thanks, all.\n\nJTan's 4-patch series should be queued instead, I presume, which\nI'll look into next.\n\n\n\n"},{"id":"506500","messageId":"xmqq7c9jyhjb.fsf@gitster.g","threadId":"62404","inReplyTo":"cover.1730491845.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] When fetching from a promisor remote, repack local objects referenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-04T00:22:16Z","receivedAt":"2024-11-04T00:22:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks everyone for looking at it. Here's version 2.\n>\n> Jonathan Tan (4):\n> t0410: make test description clearer\n> t0410: use from-scratch server\n> t5300: move --window clamp test next to unclamped\n> index-pack: repack local links into promisor packs\n>\n> Documentation/git-index-pack.txt | 5 ++\n> builtin/index-pack.c | 110 ++++++++++++++++++++++++++++++-\n> builtin/pack-objects.c | 28 ++++++++\n> t/t0410-partial-clone.sh | 6 +-\n> t/t5300-pack-object.sh | 10 +--\n> t/t5616-partial-clone.sh | 30 +++++++++\n> 6 files changed, 179 insertions(+), 10 deletions(-)\n\nThe reasoning behind each commit seems to be very well described.\n\nThanks, queued.\n\nYou may already have noticed this, but with this topic merged,\n'seen' seems to start failing leak checker jobs e.g.\n\nhttps://github.com/git/git/actions/runs/11642458705\n https://github.com/git/git/actions/runs/11642458705/job/32422074222#step:4:1964\n https://github.com/git/git/actions/runs/11642458705/job/32422074132#step:4:1963\n\n"},{"id":"506501","messageId":"xmqq34k7ycry.fsf@gitster.g","threadId":"62404","inReplyTo":"xmqq7c9jyhjb.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] When fetching from a promisor remote, repack local objects referenced","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-04T02:05:05Z","receivedAt":"2024-11-04T02:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> You may already have noticed this, but with this topic merged,\n> 'seen' seems to start failing leak checker jobs e.g.\n>\n> https://github.com/git/git/actions/runs/11642458705\n> https://github.com/git/git/actions/runs/11642458705/job/32422074222#step:4:1964\n> https://github.com/git/git/actions/runs/11642458705/job/32422074132#step:4:1963\n\nWith this patch, t5300 no longer seems to leak (when the topic is\ntested alone, at least).\n\n builtin/index-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git c/builtin/index-pack.c w/builtin/index-pack.c\nindex e4afd6725f..08b340552f 100644\n--- c/builtin/index-pack.c\n+++ w/builtin/index-pack.c\n@@ -1821,6 +1821,7 @@ static void repack_local_links(void)\n \tif (finish_command(&cmd))\n \t\tdie(_(\"could not finish pack-objects to repack local links\"));\n \tstrbuf_release(&line);\n+\tfree(base_name);\n }\n \n int cmd_index_pack(int argc,\n"},{"id":"507188","messageId":"20241113073500.GA587228@coredump.intra.peff.net","threadId":"62404","inReplyTo":"1526a59e2d4ace2761fd8935c63350f0a41985c6.1730491845.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-13T07:35:00Z","receivedAt":"2024-11-13T07:35:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 01, 2024 at 01:11:47PM -0700, Jonathan Tan wrote:\n\n> A subsequent commit will change the behavior of \"git index-pack\n> --promisor\", which is exercised in \"build pack index for an existing\n> pack\", causing the unclamped and clamped versions of the --window\n> test to exhibit different behavior. Move the clamp test closer to the\n> unclamped test that it references.\n\nHmm. The change in patch 4 broke another similar --window test I had in\na topic in flight. I can probably move it to match what you've done\nhere, but I feel like this may be papering over a bigger issue.\n\nThe reason these window tests are broken is that the earlier \"build pack\nindex for an existing pack\" is now finding and storing deltas in a new\npack when it does this:\n\n git index-pack --promisor=message test-3.pack &&\n\nBut that command is indexing a pack that is not even in the repository's\nobject store at all! Yet it triggers a call to pack-objects that repacks\nwithin that object store.\n\nHere's an even more extreme version. You do not need to have a\nrepository at all to run index-pack. So doing:\n\n mkdir /tmp/foo\n cd /tmp/foo\n cp /some/repo/.git/objects/pack/*.pack .\n for i in *.pack; do\n git index-pack -v --promisor=foo $i\n done\n\nused to work, but with your patches will segfault (because the repo\npointer is NULL). Granted it's odd to pass --promisor when you are not\nin a repo, but certainly we should never segfault.\n\nSo I think at the very least that index-pack should not try to modify\nthe repository's object database unless we are indexing a pack that is\nwithin it, which would fix both of those issues.\n\nI'd guess in the real world, we'd only pass that option when indexing\npacks that we just fetched. But as a bystander to this feature, it feels\nquite odd to me that index-pack, which I generally consider a \"read\nonly\" operation except for the index it was asked to write, would be\ncreating a new pack like this. I didn't follow the topic closely enough\nto comment more intelligently, but would it be possible for the caller\nof index-pack to trigger the repack as an independent step?\n\n-Peff\n"},{"id":"507229","messageId":"20241113182656.2135341-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"20241113073500.GA587228@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-13T18:26:56Z","receivedAt":"2024-11-13T18:26:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> On Fri, Nov 01, 2024 at 01:11:47PM -0700, Jonathan Tan wrote:\n> \n> > A subsequent commit will change the behavior of \"git index-pack\n> > --promisor\", which is exercised in \"build pack index for an existing\n> > pack\", causing the unclamped and clamped versions of the --window\n> > test to exhibit different behavior. Move the clamp test closer to the\n> > unclamped test that it references.\n> \n> Hmm. The change in patch 4 broke another similar --window test I had in\n> a topic in flight. I can probably move it to match what you've done\n> here, but I feel like this may be papering over a bigger issue.\n> \n> The reason these window tests are broken is that the earlier \"build pack\n> index for an existing pack\" is now finding and storing deltas in a new\n> pack when it does this:\n> \n> git index-pack --promisor=message test-3.pack &&\n> \n> But that command is indexing a pack that is not even in the repository's\n> object store at all! Yet it triggers a call to pack-objects that repacks\n> within that object store.\n\nAs far as I know, index-pack, when run as part of fetch, indexes a pack\nthat's not in the repository's object store; it indexes a packfile in a\ntemp directory. (So I don't think this is a strange thing to do.)\n\n> Here's an even more extreme version. You do not need to have a\n> repository at all to run index-pack. So doing:\n> \n> mkdir /tmp/foo\n> cd /tmp/foo\n> cp /some/repo/.git/objects/pack/*.pack .\n> for i in *.pack; do\n> git index-pack -v --promisor=foo $i\n> done\n> \n> used to work, but with your patches will segfault (because the repo\n> pointer is NULL). Granted it's odd to pass --promisor when you are not\n> in a repo, but certainly we should never segfault.\n\nAh, good catch.\n\n> So I think at the very least that index-pack should not try to modify\n> the repository's object database unless we are indexing a pack that is\n> within it, which would fix both of those issues.\n> \n> I'd guess in the real world, we'd only pass that option when indexing\n> packs that we just fetched. But as a bystander to this feature, it feels\n> quite odd to me that index-pack, which I generally consider a \"read\n> only\" operation except for the index it was asked to write, would be\n> creating a new pack like this. I didn't follow the topic closely enough\n> to comment more intelligently, but would it be possible for the caller\n> of index-pack to trigger the repack as an independent step?\n> \n> -Peff\n\nI thought of that, but as far as I know, during a fetch, index-pack is\nthe only time in which the objects in the fetched pack are uncompressed\nin memory. There have been concerns about the performance of various\nways of solving the promisor-object-and-GC bug, so I took an approach\nthat minimizes the performance hit as much as possible, by avoiding yet\nanother uncompression (we need to uncompress the objects to find their\noutgoing links, so that we know what to repack).\n\nWe definitely should prevent the segfault, but I think that's better\ndone by making --promisor only work if we run index-pack from within\na repo. I don't think we can restrict the repacking to run only if\nwe're indexing a pack within the repo, because in our fetch case, we're\nindexing a new pack - not one within the repo.\n\nMaybe we could conceptualize \"index-pack --promisor\" as the pack giving\n\"testimony\" about objects that its objects link to, so we can update our\nown records.\n"},{"id":"507243","messageId":"20241114005652.GC1140565@coredump.intra.peff.net","threadId":"62404","inReplyTo":"20241113182656.2135341-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-14T00:56:52Z","receivedAt":"2024-11-14T00:56:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 13, 2024 at 10:26:56AM -0800, Jonathan Tan wrote:\n\n> > The reason these window tests are broken is that the earlier \"build pack\n> > index for an existing pack\" is now finding and storing deltas in a new\n> > pack when it does this:\n> > \n> > git index-pack --promisor=message test-3.pack &&\n> > \n> > But that command is indexing a pack that is not even in the repository's\n> > object store at all! Yet it triggers a call to pack-objects that repacks\n> > within that object store.\n> \n> As far as I know, index-pack, when run as part of fetch, indexes a pack\n> that's not in the repository's object store; it indexes a packfile in a\n> temp directory. (So I don't think this is a strange thing to do.)\n\nWhen fetching (or receiving a push), we use \"index-pack --stdin\" and do\nwrite the resulting pack into the repository (and the command will\ncomplain if there is no repository).\n\nSo I think what the test is doing above (using --promisor on a random\npackfile) is questionable. It goes back to 1f52cdfacb (index-pack:\ndocument and test the --promisor option, 2022-03-09). I wonder if that\ntest is actually valuable now that the --promisor option is actually\ntriggered via fetch.\n\nIf we restricted --promisor to work only with --stdin, that would deal\nwith both of the issues I saw. And I suspect (but didn't dig deeply or\ntest) that would be sufficient for how it is called within git. (I\nwondered briefly if bundles might index in-place, but they seem to use\n--stdin also).\n\n\nWhere it gets weirder to me is with quarantine directories (and maybe\nthis is what you meant above). On receiving a push, we \"index --stdin\"\ninto a temporary quarantine directory. If that kicks off a pack-objects\nrun, where does that pack-objects put its new pack? Within the\nquarantined index-pack we set GIT_OBJECT_DIRECTORY to the quarantine and\nadd the original repo as an alternate. So I _think_ both the pushed-up\npack and the repacked promisor pack would go into the quarantine dir,\nand then we'd migrate both (or neither) when we commit to the push.\n\nWhich is OK, but I don't know that I thought that far ahead when writing\nthe quarantine stuff long ago.\n\nIt's probably somewhat academic right now, as I'm not sure if you can\neven push reliably into a promisor repo (and it doesn't look like\nreceive-pack knows about passing --promisor anyway). We don't quarantine\non fetch right now, though we have discussed it in the past (and I think\nwe should consider doing it).\n\nSo this may become more real in the future. I wonder if there is a way\nto add a test to future-proof against changes to how the quarantine\nsystem works. The theoretical problem case is if we did quarantine\nfetches, but accidentally wrote the new promisor pack into the main\nrepo instead of the quarantine, and then a fetch rejected the incoming\npack (because of a hook, failed connectivity check, etc). Then we'd end\nup with the new promisor pack when we shouldn't, which I guess could\nmove objects from that incoming pack that we rejected into the main\nrepo, despite the quarantine?\n\nI can't think of a way to test that now, without the quarantine-on-fetch\nfeature existing.\n\n> > I'd guess in the real world, we'd only pass that option when indexing\n> > packs that we just fetched. But as a bystander to this feature, it feels\n> > quite odd to me that index-pack, which I generally consider a \"read\n> > only\" operation except for the index it was asked to write, would be\n> > creating a new pack like this. I didn't follow the topic closely enough\n> > to comment more intelligently, but would it be possible for the caller\n> > of index-pack to trigger the repack as an independent step?\n> \n> I thought of that, but as far as I know, during a fetch, index-pack is\n> the only time in which the objects in the fetched pack are uncompressed\n> in memory. There have been concerns about the performance of various\n> ways of solving the promisor-object-and-GC bug, so I took an approach\n> that minimizes the performance hit as much as possible, by avoiding yet\n> another uncompression (we need to uncompress the objects to find their\n> outgoing links, so that we know what to repack).\n\nHmm, yeah. I can see the appeal of doing the processing there. Kicking\noff a pack-objects can be similarly expensive in the worst case, but in\npractice it should only be dealing with a few new objects (and we want\nto cheaply find out what those objects are, if there are even any at\nall).\n\n> We definitely should prevent the segfault, but I think that's better\n> done by making --promisor only work if we run index-pack from within a\n> repo. I don't think we can restrict the repacking to run only if we're\n> indexing a pack within the repo, because in our fetch case, we're\n> indexing a new pack - not one within the repo.\n\nI think the \"--stdin\" thing above neatly solves this.\n\n> Maybe we could conceptualize \"index-pack --promisor\" as the pack\n> giving \"testimony\" about objects that its objects link to, so we can\n> update our own records.\n\nYeah, I guess the fundamental thing here is that anybody who isn't\npassing \"--promisor\" is not going to be affected, so that at least\nlimits the opportunity for surprise.\n\nThe quarantine discussion above is an example of how there could be\nunexpected consequences. I _think_ it's OK based on what I wrote, but\nhopefully that explains my general feeling of surprise. I dunno. It\nstill may be the least bad thing.\n\n-Peff\n"},{"id":"507244","messageId":"20241114005901.GD1140565@coredump.intra.peff.net","threadId":"62404","inReplyTo":"20241113073500.GA587228@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-14T00:59:01Z","receivedAt":"2024-11-14T00:59:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 13, 2024 at 02:35:00AM -0500, Jeff King wrote:\n\n> On Fri, Nov 01, 2024 at 01:11:47PM -0700, Jonathan Tan wrote:\n> \n> > A subsequent commit will change the behavior of \"git index-pack\n> > --promisor\", which is exercised in \"build pack index for an existing\n> > pack\", causing the unclamped and clamped versions of the --window\n> > test to exhibit different behavior. Move the clamp test closer to the\n> > unclamped test that it references.\n> \n> Hmm. The change in patch 4 broke another similar --window test I had in\n> a topic in flight. I can probably move it to match what you've done\n> here, but I feel like this may be papering over a bigger issue.\n> \n> The reason these window tests are broken is that the earlier \"build pack\n> index for an existing pack\" is now finding and storing deltas in a new\n> pack when it does this:\n> \n> git index-pack --promisor=message test-3.pack &&\n\nBTW, an alternate fix instead of moving the test is below. But maybe not\nworth revisiting since it's already in next.\n\n-- >8 --\nSubject: [PATCH] t5300: use --no-reuse-delta for --window test\n\nIn the test added for 953aa54e1a (pack-objects: clamp negative window\nsize to 0, 2021-05-01), we expect that dropping the --window parameter\nwill mean the resulting pack does not have any deltas. But this\nexpectation would not hold if there are deltas from an on-disk pack that\nare reused.\n\nThis makes the test fragile with respect to the existing repository\nstate. It works reliably now, but changes to earlier tests could produce\npacks that violate the assumption.\n\nWe can make the test more reliable by passing --no-reuse-delta, meaning\nwe will only output deltas we find in the current run (using --window).\n\nSigned-off-by: Jeff King \n---\n t/t5300-pack-object.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 3b9dae331a..392d6a4d41 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -631,7 +631,7 @@ test_expect_success 'prefetch objects' '\n '\n \n test_expect_success 'negative window clamps to 0' '\n-\tgit pack-objects --progress --window=-1 neg-window stderr &&\n+\tgit pack-objects --progress --no-reuse-delta --window=-1 neg-window stderr &&\n \tcheck_deltas stderr = 0\n '\n \n-- \n2.47.0.527.gfb211c7f3b\n\n"},{"id":"507252","messageId":"xmqqiksq71x7.fsf@gitster.g","threadId":"62404","inReplyTo":"20241114005652.GC1140565@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-14T06:41:08Z","receivedAt":"2024-11-14T06:41:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> As far as I know, index-pack, when run as part of fetch, indexes a pack\n>> that's not in the repository's object store; it indexes a packfile in a\n>> temp directory. (So I don't think this is a strange thing to do.)\n>\n> When fetching (or receiving a push), we use \"index-pack --stdin\" and do\n> write the resulting pack into the repository (and the command will\n> complain if there is no repository).\n> ...\n>> We definitely should prevent the segfault, but I think that's better\n>> done by making --promisor only work if we run index-pack from within a\n>> repo. I don't think we can restrict the repacking to run only if we're\n>> indexing a pack within the repo, because in our fetch case, we're\n>> indexing a new pack - not one within the repo.\n>\n> I think the \"--stdin\" thing above neatly solves this.\n> ...\n> Yeah, I guess the fundamental thing here is that anybody who isn't\n> passing \"--promisor\" is not going to be affected, so that at least\n> limits the opportunity for surprise.\n>\n> The quarantine discussion above is an example of how there could be\n> unexpected consequences. I _think_ it's OK based on what I wrote, but\n> hopefully that explains my general feeling of surprise. I dunno. It\n> still may be the least bad thing.\n\nTying this extra processing to the use of \"--stdin\" is not exactly\nintuitive, in that a \"--stdin\" user is not necessarily doing a fetch\n(even though a fetch may always use \"--stdin\"), but I guess it is a\ngood enough approximation (and the best one easily available to us)\nif we want to safeguard the use of this \"--promisor\" logic only to\nfetch client.\n\nAs to future potential mis-interaction between quarantined fetch and\nthe effect of this \"repack local objects that can be reached by\nobjects in a promisor pack\" feature, I do not offhand think of a\ngood way to future-proof it with tests.\n\nThanks for the discussion, both of you.\n\n\n"},{"id":"507353","messageId":"20241115095244.GC1749331@coredump.intra.peff.net","threadId":"62404","inReplyTo":"xmqqiksq71x7.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-15T09:52:44Z","receivedAt":"2024-11-15T09:52:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 14, 2024 at 03:41:08PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> As far as I know, index-pack, when run as part of fetch, indexes a pack\n> >> that's not in the repository's object store; it indexes a packfile in a\n> >> temp directory. (So I don't think this is a strange thing to do.)\n> >\n> > When fetching (or receiving a push), we use \"index-pack --stdin\" and do\n> > write the resulting pack into the repository (and the command will\n> > complain if there is no repository).\n> > ...\n> >> We definitely should prevent the segfault, but I think that's better\n> >> done by making --promisor only work if we run index-pack from within a\n> >> repo. I don't think we can restrict the repacking to run only if we're\n> >> indexing a pack within the repo, because in our fetch case, we're\n> >> indexing a new pack - not one within the repo.\n> >\n> > I think the \"--stdin\" thing above neatly solves this.\n> [...]\n> \n> Tying this extra processing to the use of \"--stdin\" is not exactly\n> intuitive, in that a \"--stdin\" user is not necessarily doing a fetch\n> (even though a fetch may always use \"--stdin\"), but I guess it is a\n> good enough approximation (and the best one easily available to us)\n> if we want to safeguard the use of this \"--promisor\" logic only to\n> fetch client.\n\nI think the \"--stdin\" thing is a bit more general than that. Even though\nwe expect to use it with --promisor only under fetch, the real rule is\nmore like: only do the extra --promisor repacking when indexing a pack\nthat will be made available in the repository. With --stdin, we know\nthe result will be available because index-pack itself will write the\npack into the repository. And that is true whether it is fetch, push, or\nsome other script driving it.\n\nWhen being fed a path to a pack on the command line, then \"is it\navailable in the repo\" would involve some path comparisons to see if\nit's in the object database directory. Probably not that hard, but not\nentirely trivial (due to normalization, etc). But since we care only\nabout fetch and --stdin, it seemed like an easy cheat to simply disallow\nthe other case for now, erring on the conservative side.\n\n-Peff\n"},{"id":"507374","messageId":"20241115195503.3395744-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"20241114005652.GC1140565@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-15T19:55:03Z","receivedAt":"2024-11-15T19:55:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> Where it gets weirder to me is with quarantine directories (and maybe\n> this is what you meant above). On receiving a push, we \"index --stdin\"\n> into a temporary quarantine directory. If that kicks off a pack-objects\n> run, where does that pack-objects put its new pack? Within the\n> quarantined index-pack we set GIT_OBJECT_DIRECTORY to the quarantine and\n> add the original repo as an alternate. So I _think_ both the pushed-up\n> pack and the repacked promisor pack would go into the quarantine dir,\n> and then we'd migrate both (or neither) when we commit to the push.\n> \n> Which is OK, but I don't know that I thought that far ahead when writing\n> the quarantine stuff long ago.\n> \n> It's probably somewhat academic right now, as I'm not sure if you can\n> even push reliably into a promisor repo (and it doesn't look like\n> receive-pack knows about passing --promisor anyway).\n\nThanks for this description. Such a push would be an \"I am pushing a\npack with missing objects to you, and you can later get those missing\nobjects from me\" situation. Not completely implausible, but doesn't seem\nhigh-priority to me.\n\n> We don't quarantine\n> on fetch right now, though we have discussed it in the past (and I think\n> we should consider doing it).\n> \n> So this may become more real in the future. I wonder if there is a way\n> to add a test to future-proof against changes to how the quarantine\n> system works. The theoretical problem case is if we did quarantine\n> fetches, but accidentally wrote the new promisor pack into the main\n> repo instead of the quarantine, and then a fetch rejected the incoming\n> pack (because of a hook, failed connectivity check, etc). Then we'd end\n> up with the new promisor pack when we shouldn't, which I guess could\n> move objects from that incoming pack that we rejected into the main\n> repo, despite the quarantine?\n> \n> I can't think of a way to test that now, without the quarantine-on-fetch\n> feature existing.\n\nQuarantine on fetch does seem like a good idea. I also can't think of\na way to test that now. Although, for the fetch case, my patch set is\nnot the first time that an extra packfile (that is, a packfile not in\nthe \"packfile\" section of the fetch response) could be written during\na fetch: packfile-uris and bundle-uris already exist. So I would hope\nthat the implementor of the fetch quarantine feature would be aware of\nat least one of these extra features, and design the test to check that\nabsolutely no packfiles are written if the fetch is rejected. (So I\ndon't think the future needs to be \"proofed\" so much.)\n\n"},{"id":"507391","messageId":"20241116032352.GA1782794@coredump.intra.peff.net","threadId":"62404","inReplyTo":"20241115195503.3395744-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 3/4] t5300: move --window clamp test next to unclamped","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-16T03:23:52Z","receivedAt":"2024-11-16T03:23:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 15, 2024 at 11:55:03AM -0800, Jonathan Tan wrote:\n\n> > So this may become more real in the future. I wonder if there is a way\n> > to add a test to future-proof against changes to how the quarantine\n> > system works. The theoretical problem case is if we did quarantine\n> > fetches, but accidentally wrote the new promisor pack into the main\n> > repo instead of the quarantine, and then a fetch rejected the incoming\n> > pack (because of a hook, failed connectivity check, etc). Then we'd end\n> > up with the new promisor pack when we shouldn't, which I guess could\n> > move objects from that incoming pack that we rejected into the main\n> > repo, despite the quarantine?\n> > \n> > I can't think of a way to test that now, without the quarantine-on-fetch\n> > feature existing.\n> \n> Quarantine on fetch does seem like a good idea. I also can't think of\n> a way to test that now. Although, for the fetch case, my patch set is\n> not the first time that an extra packfile (that is, a packfile not in\n> the \"packfile\" section of the fetch response) could be written during\n> a fetch: packfile-uris and bundle-uris already exist. So I would hope\n> that the implementor of the fetch quarantine feature would be aware of\n> at least one of these extra features, and design the test to check that\n> absolutely no packfiles are written if the fetch is rejected. (So I\n> don't think the future needs to be \"proofed\" so much.)\n\nGood point. I think we have to just leave it until that hypothetical\nfuture and hope that person is careful. :)\n\n-Peff\n"},{"id":"507530","messageId":"20241118190210.772105-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"20241116032352.GA1782794@coredump.intra.peff.net","subject":"[PATCH] index-pack: teach --promisor to require --stdin","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-18T19:02:06Z","receivedAt":"2024-11-18T19:02:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently,\n\n - Running \"index-pack --promisor\" outside a repo segfaults.\n - It may be confusing to a user that running \"index-pack --promisor\"\n within a repo may make changes to the repo's object DB, especially\n since the packs indexed by the index-pack invocation may not even be\n related to the repo.\n\nAs discussed in [1], teaching --promisor to require --stdin and forbid a\npackfile name solves both these problems. This combination of arguments\nrequires a repo (since we are writing the resulting .pack and .idx to\nit) and it is clear that the files are related to the repo.\n\nCurrently, Git uses \"index-pack --promisor\" only when fetching into\na repo, so it could be argued that we should teach \"index-pack\" a new\nargument (say, \"--fetching-mode\") instead of tying --promisor to a\ngeneric argument like \"--stdin\". However, this --promisor feature could\nconceivably be used whenever we have a packfile that is known to come\nfrom the promisor remote (whether obtained through Git's fetch protocol\nor through other means) so it seems reasonable to use --stdin here -\none could envision a user-made script obtaining a packfile and then\nrunning \"index-pack --promisor --stdin\", for example. In fact, it might\nbe possible to relax the restriction further (say, by also allowing\n--promisor when indexing a packfile that is in the object DB), but\nrelaxing the restriction is backwards-compatible so we can revisit that\nlater.\n\nOne thing to watch out for is the possibility of a future Git feature\nthat indexes a pack in the context of a repo, but does not necessarily\nwrite the resulting pack to it (and does not necessarily desire to\nmake any changes to the object DB). One such feature would be fetch\nquarantine, which might need the repo context in order to detect\nhash collisions, but would also need to ensure that the object DB\nis undisturbed in case the fetch fails for whatever reason, even if\nthe reason occurs only after the indexing is complete. It may not be\nobvious to the implementer of such a feature that \"index-pack\" could\nsometimes write packs other than the indexed pack to the object DB,\nbut there are already other ways that \"fetch\" could write to the object\nDB (in particular, packfile URIs and bundle URIs), so hopefully the\nimplementation of this future feature would already include a test that\nthe object DB be undisturbed.\n\nThis change requires the change to t5300 by 1f52cdfacb (index-pack:\ndocument and test the --promisor option, 2022-03-09) to be undone.\n(--promisor is already tested indirectly, so we don't need the explicit\ntest here any more.)\n\n[1] https://lore.kernel.org/git/20241114005652.GC1140565@coredump.intra.peff.net/\n\nSigned-off-by: Jonathan Tan \n---\nThis is on jt/repack-local-promisor.\n\nLooking into it further, I think that we also need to require no\npackfile name to be given (so that we are writing the file to the\nrepository). Therefore, I've added that requirement both in the code and\nin the documentation.\n\nI've tried to summarize our conversation in the commit message - if you\nnotice anything missing or incorrect, feel free to let me know.\n---\n Documentation/git-index-pack.txt | 2 ++\n builtin/index-pack.c | 4 ++++\n t/t5300-pack-object.sh | 4 +---\n 3 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 4be09e58e7..ac96935d73 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -144,6 +144,8 @@ Also, if there are objects in the given pack that references non-promisor\n objects (in the repo), repacks those non-promisor objects into a promisor\n pack. This avoids a situation in which a repo has non-promisor objects that are\n accessible through promisor objects.\n++\n+Requires --stdin, and requires to not be specified.\n \n NOTES\n -----\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 08b340552f..c46b6e4061 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1970,6 +1970,10 @@ int cmd_index_pack(int argc,\n \t\tusage(index_pack_usage);\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--fix-thin\", \"--stdin\");\n+\tif (promisor_msg && !from_stdin)\n+\t\tdie(_(\"the option '%s' requires '%s'\"), \"--promisor\", \"--stdin\");\n+\tif (promisor_msg && pack_name)\n+\t\tdie(_(\"--promisor cannot be used with a pack name\"));\n \tif (from_stdin && !startup_info->have_repository)\n \t\tdie(_(\"--stdin requires a git repository\"));\n \tif (from_stdin && hash_algo)\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex aff164ddf8..c53f355e48 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -332,10 +332,8 @@ test_expect_success 'build pack index for an existing pack' '\n \tgit index-pack -o tmp.idx test-3.pack &&\n \tcmp tmp.idx test-1-${packname_1}.idx &&\n \n-\tgit index-pack --promisor=message test-3.pack &&\n+\tgit index-pack test-3.pack &&\n \tcmp test-3.idx test-1-${packname_1}.idx &&\n-\techo message >expect &&\n-\ttest_cmp expect test-3.promisor &&\n \n \tcat test-2-${packname_2}.pack >test-3.pack &&\n \tgit index-pack -o tmp.idx test-2-${packname_2}.pack &&\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"507564","messageId":"xmqq1pz7gaua.fsf@gitster.g","threadId":"62404","inReplyTo":"20241118190210.772105-1-jonathantanmy@google.com","subject":"Re: [PATCH] index-pack: teach --promisor to require --stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-19T03:29:33Z","receivedAt":"2024-11-19T03:29:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Currently,\n>\n> - Running \"index-pack --promisor\" outside a repo segfaults.\n> - It may be confusing to a user that running \"index-pack --promisor\"\n> within a repo may make changes to the repo's object DB, especially\n> since the packs indexed by the index-pack invocation may not even be\n> related to the repo.\n>\n> As discussed in [1], teaching --promisor to require --stdin and forbid a\n> packfile name solves both these problems. This combination of arguments\n> requires a repo (since we are writing the resulting .pack and .idx to\n> it) and it is clear that the files are related to the repo.\n\nMakes sense.\n\n> This change requires the change to t5300 by 1f52cdfacb (index-pack:\n> document and test the --promisor option, 2022-03-09) to be undone.\n> (--promisor is already tested indirectly, so we don't need the explicit\n> test here any more.)\n\nOK.\n\n> This is on jt/repack-local-promisor.\n> diff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\n> index 4be09e58e7..ac96935d73 100644\n> --- a/Documentation/git-index-pack.txt\n> +++ b/Documentation/git-index-pack.txt\n> @@ -144,6 +144,8 @@ Also, if there are objects in the given pack that references non-promisor\n> objects (in the repo), repacks those non-promisor objects into a promisor\n> pack. This avoids a situation in which a repo has non-promisor objects that are\n> accessible through promisor objects.\n> ++\n> +Requires --stdin, and requires to not be specified.\n> \n> NOTES\n> -----\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 08b340552f..c46b6e4061 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1970,6 +1970,10 @@ int cmd_index_pack(int argc,\n> \t\tusage(index_pack_usage);\n> \tif (fix_thin_pack && !from_stdin)\n> \t\tdie(_(\"the option '%s' requires '%s'\"), \"--fix-thin\", \"--stdin\");\n> +\tif (promisor_msg && !from_stdin)\n> +\t\tdie(_(\"the option '%s' requires '%s'\"), \"--promisor\", \"--stdin\");\n> +\tif (promisor_msg && pack_name)\n> +\t\tdie(_(\"--promisor cannot be used with a pack name\"));\n> \tif (from_stdin && !startup_info->have_repository)\n> \t\tdie(_(\"--stdin requires a git repository\"));\n\nOK. Thanks, will queue.\n"},{"id":"507637","messageId":"20241119185345.GB15723@coredump.intra.peff.net","threadId":"62404","inReplyTo":"20241118190210.772105-1-jonathantanmy@google.com","subject":"Re: [PATCH] index-pack: teach --promisor to require --stdin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-19T18:53:45Z","receivedAt":"2024-11-19T18:53:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 18, 2024 at 11:02:06AM -0800, Jonathan Tan wrote:\n\n> Currently, Git uses \"index-pack --promisor\" only when fetching into\n> a repo, so it could be argued that we should teach \"index-pack\" a new\n> argument (say, \"--fetching-mode\") instead of tying --promisor to a\n> generic argument like \"--stdin\". However, this --promisor feature could\n> conceivably be used whenever we have a packfile that is known to come\n> from the promisor remote (whether obtained through Git's fetch protocol\n> or through other means) so it seems reasonable to use --stdin here -\n> one could envision a user-made script obtaining a packfile and then\n> running \"index-pack --promisor --stdin\", for example. In fact, it might\n> be possible to relax the restriction further (say, by also allowing\n> --promisor when indexing a packfile that is in the object DB), but\n> relaxing the restriction is backwards-compatible so we can revisit that\n> later.\n\nYeah, I agree with this summary.\n\n> This change requires the change to t5300 by 1f52cdfacb (index-pack:\n> document and test the --promisor option, 2022-03-09) to be undone.\n> (--promisor is already tested indirectly, so we don't need the explicit\n> test here any more.)\n\nOK, I think this is reasonable.\n\n> Looking into it further, I think that we also need to require no\n> packfile name to be given (so that we are writing the file to the\n> repository). Therefore, I've added that requirement both in the code and\n> in the documentation.\n\nHmm. I didn't realize that you could specify a pack name _and_ --stdin,\nbut I guess it makes sense if you wanted to write the result to a\nnon-standard location (though curiously --stdin requires a repo, which\nfeels overly restrictive if you give a pack name).\n\nBut I think that makes the --stdin check redundant. I.e., here:\n\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 08b340552f..c46b6e4061 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -1970,6 +1970,10 @@ int cmd_index_pack(int argc,\n> \t\tusage(index_pack_usage);\n> \tif (fix_thin_pack && !from_stdin)\n> \t\tdie(_(\"the option '%s' requires '%s'\"), \"--fix-thin\", \"--stdin\");\n> +\tif (promisor_msg && !from_stdin)\n> +\t\tdie(_(\"the option '%s' requires '%s'\"), \"--promisor\", \"--stdin\");\n> +\tif (promisor_msg && pack_name)\n> +\t\tdie(_(\"--promisor cannot be used with a pack name\"));\n\n...just the second one would be sufficient, because the context just\nabove this has:\n\n\tif (!pack_name && !from_stdin)\n\t\tusage(index_pack_usage);\n\nSo if there isn't a pack name then from_stdin must be set anyway.\n\nWhat you've written won't behave incorrectly, but I wonder if this means\nwe can explain the rule in a more simple way:\n\n - the --promisor option requires that we be indexing a pack in the\n object database\n\n - when not given a pack name on the command line, we know this is true\n (because we generate the name ourselves internally)\n\n - when given a pack name on the command line, we _could_ check that it\n is inside the object directory, but we don't currently do so and\n just bail. That could be changed in the future.\n\nAnd then there is no mention of --stdin at all (though of course it is\nan implication of the second point, since we have to get input somehow).\n\n-Peff\n"},{"id":"507641","messageId":"20241119201016.22713-1-jonathantanmy@google.com","threadId":"62404","inReplyTo":"20241116032352.GA1782794@coredump.intra.peff.net","subject":"[PATCH v2] index-pack: teach --promisor to forbid pack name","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2024-11-19T20:10:15Z","receivedAt":"2024-11-19T20:10:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently,\n\n - Running \"index-pack --promisor\" outside a repo segfaults.\n - It may be confusing to a user that running \"index-pack --promisor\"\n within a repo may make changes to the repo's object DB, especially\n since the packs indexed by the index-pack invocation may not even be\n related to the repo.\n\nAs discussed in [1] and [2], teaching --promisor to forbid a packfile\nname solves both these problems. This combination of arguments requires\na repo (since we are writing the resulting .pack and .idx to it) and it\nis clear that the files are related to the repo.\n\nCurrently, Git uses \"index-pack --promisor\" only when fetching into\na repo, so it could be argued that we should teach \"index-pack\" a\nnew argument (say, \"--fetching-mode\") instead of tying --promisor to\na generic argument like the packfile name. However, this --promisor\nfeature could conceivably be used whenever we have a packfile that is\nknown to come from the promisor remote (whether obtained through Git's\nfetch protocol or through other means) so not using a new argument seems\nreasonable - one could envision a user-made script obtaining a packfile\nand then running \"index-pack --promisor --stdin\", for example. In fact,\nit might be possible to relax the restriction further (say, by also\nallowing --promisor when indexing a packfile that is in the object DB),\nbut relaxing the restriction is backwards-compatible so we can revisit\nthat later.\n\nOne thing to watch out for is the possibility of a future Git feature\nthat indexes a pack in the context of a repo, but does not necessarily\nwrite the resulting pack to it (and does not necessarily desire to\nmake any changes to the object DB). One such feature would be fetch\nquarantine, which might need the repo context in order to detect\nhash collisions, but would also need to ensure that the object DB\nis undisturbed in case the fetch fails for whatever reason, even if\nthe reason occurs only after the indexing is complete. It may not be\nobvious to the implementer of such a feature that \"index-pack\" could\nsometimes write packs other than the indexed pack to the object DB,\nbut there are already other ways that \"fetch\" could write to the object\nDB (in particular, packfile URIs and bundle URIs), so hopefully the\nimplementation of this future feature would already include a test that\nthe object DB be undisturbed.\n\nThis change requires the change to t5300 by 1f52cdfacb (index-pack:\ndocument and test the --promisor option, 2022-03-09) to be undone.\n(--promisor is already tested indirectly, so we don't need the explicit\ntest here any more.)\n\n[1] https://lore.kernel.org/git/20241114005652.GC1140565@coredump.intra.peff.net/\n[2] https://lore.kernel.org/git/20241119185345.GB15723@coredump.intra.peff.net/\n\nSigned-off-by: Jonathan Tan \n---\nThis is on jt/repack-local-promisor.\n\nThanks, Peff, for the catch. Here's an updated patch, with an updated\ncommit message.\n---\n Documentation/git-index-pack.txt | 2 ++\n builtin/index-pack.c | 2 ++\n t/t5300-pack-object.sh | 4 +---\n 3 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 4be09e58e7..58dd5b5f0e 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -144,6 +144,8 @@ Also, if there are objects in the given pack that references non-promisor\n objects (in the repo), repacks those non-promisor objects into a promisor\n pack. This avoids a situation in which a repo has non-promisor objects that are\n accessible through promisor objects.\n++\n+Requires to not be specified.\n \n NOTES\n -----\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 08b340552f..05758a2f3e 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1970,6 +1970,8 @@ int cmd_index_pack(int argc,\n \t\tusage(index_pack_usage);\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--fix-thin\", \"--stdin\");\n+\tif (promisor_msg && pack_name)\n+\t\tdie(_(\"--promisor cannot be used with a pack name\"));\n \tif (from_stdin && !startup_info->have_repository)\n \t\tdie(_(\"--stdin requires a git repository\"));\n \tif (from_stdin && hash_algo)\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex aff164ddf8..c53f355e48 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -332,10 +332,8 @@ test_expect_success 'build pack index for an existing pack' '\n \tgit index-pack -o tmp.idx test-3.pack &&\n \tcmp tmp.idx test-1-${packname_1}.idx &&\n \n-\tgit index-pack --promisor=message test-3.pack &&\n+\tgit index-pack test-3.pack &&\n \tcmp test-3.idx test-1-${packname_1}.idx &&\n-\techo message >expect &&\n-\ttest_cmp expect test-3.promisor &&\n \n \tcat test-2-${packname_2}.pack >test-3.pack &&\n \tgit index-pack -o tmp.idx test-2-${packname_2}.pack &&\n\nRange-diff against v1:\n1: b5a0012531 ! 1: 226a627c25 index-pack: teach --promisor to require --stdin\n @@ Metadata\n Author: Jonathan Tan \n \n ## Commit message ##\n - index-pack: teach --promisor to require --stdin\n + index-pack: teach --promisor to forbid pack name\n \n Currently,\n \n @@ Commit message\n since the packs indexed by the index-pack invocation may not even be\n related to the repo.\n \n - As discussed in [1], teaching --promisor to require --stdin and forbid a\n - packfile name solves both these problems. This combination of arguments\n - requires a repo (since we are writing the resulting .pack and .idx to\n - it) and it is clear that the files are related to the repo.\n + As discussed in [1] and [2], teaching --promisor to forbid a packfile\n + name solves both these problems. This combination of arguments requires\n + a repo (since we are writing the resulting .pack and .idx to it) and it\n + is clear that the files are related to the repo.\n \n Currently, Git uses \"index-pack --promisor\" only when fetching into\n - a repo, so it could be argued that we should teach \"index-pack\" a new\n - argument (say, \"--fetching-mode\") instead of tying --promisor to a\n - generic argument like \"--stdin\". However, this --promisor feature could\n - conceivably be used whenever we have a packfile that is known to come\n - from the promisor remote (whether obtained through Git's fetch protocol\n - or through other means) so it seems reasonable to use --stdin here -\n - one could envision a user-made script obtaining a packfile and then\n - running \"index-pack --promisor --stdin\", for example. In fact, it might\n - be possible to relax the restriction further (say, by also allowing\n - --promisor when indexing a packfile that is in the object DB), but\n - relaxing the restriction is backwards-compatible so we can revisit that\n - later.\n + a repo, so it could be argued that we should teach \"index-pack\" a\n + new argument (say, \"--fetching-mode\") instead of tying --promisor to\n + a generic argument like the packfile name. However, this --promisor\n + feature could conceivably be used whenever we have a packfile that is\n + known to come from the promisor remote (whether obtained through Git's\n + fetch protocol or through other means) so not using a new argument seems\n + reasonable - one could envision a user-made script obtaining a packfile\n + and then running \"index-pack --promisor --stdin\", for example. In fact,\n + it might be possible to relax the restriction further (say, by also\n + allowing --promisor when indexing a packfile that is in the object DB),\n + but relaxing the restriction is backwards-compatible so we can revisit\n + that later.\n \n One thing to watch out for is the possibility of a future Git feature\n that indexes a pack in the context of a repo, but does not necessarily\n @@ Commit message\n test here any more.)\n \n [1] https://lore.kernel.org/git/20241114005652.GC1140565@coredump.intra.peff.net/\n + [2] https://lore.kernel.org/git/20241119185345.GB15723@coredump.intra.peff.net/\n \n Signed-off-by: Jonathan Tan \n ---\n This is on jt/repack-local-promisor.\n \n - Looking into it further, I think that we also need to require no\n - packfile name to be given (so that we are writing the file to the\n - repository). Therefore, I've added that requirement both in the code and\n - in the documentation.\n -\n - I've tried to summarize our conversation in the commit message - if you\n - notice anything missing or incorrect, feel free to let me know.\n + Thanks, Peff, for the catch. Here's an updated patch, with an updated\n + commit message.\n \n ## Documentation/git-index-pack.txt ##\n @@ Documentation/git-index-pack.txt: Also, if there are objects in the given pack that references non-promisor\n @@ Documentation/git-index-pack.txt: Also, if there are objects in the given pack t\n pack. This avoids a situation in which a repo has non-promisor objects that are\n accessible through promisor objects.\n ++\n -+Requires --stdin, and requires to not be specified.\n ++Requires to not be specified.\n \n NOTES\n -----\n @@ builtin/index-pack.c: int cmd_index_pack(int argc,\n \t\tusage(index_pack_usage);\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--fix-thin\", \"--stdin\");\n -+\tif (promisor_msg && !from_stdin)\n -+\t\tdie(_(\"the option '%s' requires '%s'\"), \"--promisor\", \"--stdin\");\n +\tif (promisor_msg && pack_name)\n +\t\tdie(_(\"--promisor cannot be used with a pack name\"));\n \tif (from_stdin && !startup_info->have_repository)\n-- \n2.47.0.338.g60cca15819-goog\n\n"},{"id":"507673","messageId":"xmqqcyiqadtf.fsf@gitster.g","threadId":"62404","inReplyTo":"20241119185345.GB15723@coredump.intra.peff.net","subject":"Re: [PATCH] index-pack: teach --promisor to require --stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-20T01:34:04Z","receivedAt":"2024-11-20T01:34:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I think that makes the --stdin check redundant. I.e., here:\n>\n>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n>> index 08b340552f..c46b6e4061 100644\n>> --- a/builtin/index-pack.c\n>> +++ b/builtin/index-pack.c\n>> @@ -1970,6 +1970,10 @@ int cmd_index_pack(int argc,\n>> \t\tusage(index_pack_usage);\n>> \tif (fix_thin_pack && !from_stdin)\n>> \t\tdie(_(\"the option '%s' requires '%s'\"), \"--fix-thin\", \"--stdin\");\n>> +\tif (promisor_msg && !from_stdin)\n>> +\t\tdie(_(\"the option '%s' requires '%s'\"), \"--promisor\", \"--stdin\");\n>> +\tif (promisor_msg && pack_name)\n>> +\t\tdie(_(\"--promisor cannot be used with a pack name\"));\n>\n> ...just the second one would be sufficient, because the context just\n> above this has:\n>\n> \tif (!pack_name && !from_stdin)\n> \t\tusage(index_pack_usage);\n>\n> So if there isn't a pack name then from_stdin must be set anyway.\n\nNice findings that leads to ... \n\n> What you've written won't behave incorrectly, but I wonder if this means\n> we can explain the rule in a more simple way:\n>\n> - the --promisor option requires that we be indexing a pack in the\n> object database\n>\n> - when not given a pack name on the command line, we know this is true\n> (because we generate the name ourselves internally)\n>\n> - when given a pack name on the command line, we _could_ check that it\n> is inside the object directory, but we don't currently do so and\n> just bail. That could be changed in the future.\n>\n> And then there is no mention of --stdin at all (though of course it is\n> an implication of the second point, since we have to get input somehow).\n\n... a good simplification. Not of the implementation---as it is\nalready simple enough---but of the concept, and simplification of\nthe latter counts a lot more ;-)\n\nThanks, both, for working on this.\n"},{"id":"507683","messageId":"20241120062919.GA4564@coredump.intra.peff.net","threadId":"62404","inReplyTo":"20241119201016.22713-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] index-pack: teach --promisor to forbid pack name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-20T06:29:19Z","receivedAt":"2024-11-20T06:29:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 19, 2024 at 12:10:15PM -0800, Jonathan Tan wrote:\n\n> Thanks, Peff, for the catch. Here's an updated patch, with an updated\n> commit message.\n\nThis looks good to me, thanks.\n\n> Range-diff against v1:\n> [...]\n> @@ Commit message\n> test here any more.)\n> \n> [1] https://lore.kernel.org/git/20241114005652.GC1140565@coredump.intra.peff.net/\n> + [2] https://lore.kernel.org/git/20241119185345.GB15723@coredump.intra.peff.net/\n> \n> Signed-off-by: Jonathan Tan \n> ---\n> This is on jt/repack-local-promisor.\n> \n> - Looking into it further, I think that we also need to require no\n> - packfile name to be given (so that we are writing the file to the\n> - repository). Therefore, I've added that requirement both in the code and\n> - in the documentation.\n> -\n> - I've tried to summarize our conversation in the commit message - if you\n> - notice anything missing or incorrect, feel free to let me know.\n> + Thanks, Peff, for the catch. Here's an updated patch, with an updated\n> + commit message.\n\nHeh, I guess you stick your notes directly into the commit message. ;) I\ndo that sometimes, too. A long time ago I had a patch that would let you\nwrite \"---\" in the commit message editor and then auto-convert that into\nactual notes. Probably not that big a deal, though.\n\n-Peff\n"}]}