{"thread":{"id":"58609","subject":"[PATCH 0/3] Implement filtering repacks","startedAt":"2022-10-12T13:52:11Z","lastAt":"2024-06-03T17:30:03Z","messageCount":46,"participants":["Christian Couder","Junio C Hamano","Taylor Blau","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"464714","messageId":"20221012135114.294680-1-christian.couder@gmail.com","threadId":"58609","inReplyTo":null,"subject":"[PATCH 0/3] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-12T13:51:11Z","receivedAt":"2022-10-12T13:52:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Earlier this year, John Cai sent 2 versions of a patch series to\nimplement `git repack --filter=`:\n\nhttps://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/\n\nWe tried to \"sell\" it as a way to use partial clone on a Git server to\noffload large blobs to, for example, an http server, while using\nmultiple promisor remotes on the client side.\n\nEven though it is still our end goal, it seems a bit far fetched for\nnow and unnecessary as `git repack --filter=` could be\nuseful on the client side too.\n\nFor example one might want to clone with a filter to avoid too many\nspace to be taken by some large blobs, and one might realize after\nsome time that a number of the large blobs have still be downloaded\nbecause some old branches referencing them were checked out. In this\ncase a filtering repack could remove some of those large blobs.\n\nSome of the comments on the patch series that John sent were related\nto the possible data loss and repo corruption that a filtering repack\ncould cause. It's indeed true that it could be very dangerous, and we\nagree that improvements were needed in this area.\n\nTo address this, in the patch 2/3 introducing --filter, we warn users\nlaunching such a repack on the command line and ask them if they\nreally want to do it. If such a repack is not launched from a\nterminal, we die().\n\nA new patch 3/3, though, introduces --force to allow users to launch\nsuch a repack without a terminal and without having to confirm it on\nthe command line.\n\nPatch 1/3 is a preparatory patch.\n\nIn short, this small patch series tries to reboot the previous one\nwith a focus on the client side and a focus on safety.\n\nThanks to John Cai, who worked on the previous versions, and to\nJonathan Nieder, Jonathan Tan and Taylor Blau, who recently discussed\nthis with me at the Git Merge and Contributor Summit.\n\nChristian Couder (3):\n pack-objects: allow --filter without --stdout\n repack: add --filter= option\n repack: introduce --force to force filtering\n\n Documentation/git-repack.txt | 17 ++++++++++++++\n builtin/pack-objects.c | 6 +----\n builtin/repack.c | 44 ++++++++++++++++++++++++++++++------\n t/t7700-repack.sh | 20 ++++++++++++++++\n 4 files changed, 75 insertions(+), 12 deletions(-)\n\n-- \n2.38.0.4.g7f9724c7bf.dirty\n\n"},{"id":"464715","messageId":"20221012135114.294680-2-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221012135114.294680-1-christian.couder@gmail.com","subject":"[PATCH 1/3] pack-objects: allow --filter without --stdout","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-12T13:51:12Z","receivedAt":"2022-10-12T13:52:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\n9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)\ntaught pack-objects to use --filter, but required the use of\n--stdout since a partial clone mechanism was not yet in place to\nhandle missing objects. Since then, changes like 9e27beaa23\n(promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)\nand others added support to dynamically fetch objects that were missing.\n\nRemove the --stdout requirement so that in a follow-up commit, repack\ncan pass --filter to pack-objects to omit certain objects from the\nresulting packfile.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n builtin/pack-objects.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3658c05caf..d707e6230d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4385,12 +4385,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n-\t\tif (!pack_to_stdout)\n-\t\t\tdie(_(\"cannot use --filter without --stdout\"));\n-\t\tif (stdin_packs)\n+\tif (pfd.have_revs && pfd.revs.filter.choice && stdin_packs)\n \t\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n-\t}\n \n \tif (stdin_packs && use_internal_rev_list)\n \t\tdie(_(\"cannot use internal rev list with --stdin-packs\"));\n-- \n2.38.0.4.g7f9724c7bf.dirty\n\n"},{"id":"464716","messageId":"20221012135114.294680-3-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221012135114.294680-1-christian.couder@gmail.com","subject":"[PATCH 2/3] repack: add --filter= option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-12T13:51:13Z","receivedAt":"2022-10-12T13:52:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nAfter cloning with --filter=, for example to avoid\ngetting unneeded large files on a user machine, it's possible\nthat some of these large files still get fetched for some reasons\n(like checking out old branches) over time.\n\nIn this case the repo size could grow too much for no good reason\nand `git repack --filter=` would be useful to remove\nthe unneeded large files.\n\nThis command could be dangerous to use though, as it might remove\nlocal objects that haven't been pushed which would lose data and\ncorrupt the repo. On a server this command could also corrupt a\nrepo unless ALL the removed objects aren't already available in\nanother remote that clients can access.\n\nTo avoid as much as possible data to be lost and repos to be\ncorrupted, let's warn users and ask them to confirm that they\nreally want to use this command.\n\nIf no terminal is used, let's just die() for now. A follow-up\ncommit will introduce a --force option that will allow using\nthis option when no terminal is available.\n\nIt will be easier to test that --filter is working correctly\nin the follow-up commit that adds --force, so let's just test\nthat we die() if no terminal is used for now.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n Documentation/git-repack.txt | 7 ++++++\n builtin/repack.c | 41 ++++++++++++++++++++++++++++++------\n t/t7700-repack.sh | 5 +++++\n 3 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 0bf13893d8..230f176e10 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -137,6 +137,13 @@ depth is 4095.\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n \n+--filter=::\n+\tOmits certain objects (usually blobs) from the resulting\n+\tpackfile. WARNING: this could easily corrupt the current repo\n+\tand lose data if ANY of the omitted objects hasn't been\n+\talready pushed to a remote. See linkgit:git-rev-list[1] for\n+\tvalid `` forms.\n+\n -b::\n --write-bitmap-index::\n \tWrite a reachability bitmap index as part of the repack. This\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..0a93f38da4 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -17,6 +17,7 @@\n #include \"pack.h\"\n #include \"pack-bitmap.h\"\n #include \"refs.h\"\n+#include \"prompt.h\"\n \n #define ALL_INTO_ONE 1\n #define LOOSEN_UNREACHABLE 2\n@@ -50,6 +51,7 @@ struct pack_objects_args {\n \tconst char *depth;\n \tconst char *threads;\n \tconst char *max_pack_size;\n+\tconst char *filter;\n \tint no_reuse_delta;\n \tint no_reuse_object;\n \tint quiet;\n@@ -201,6 +203,8 @@ static void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n \tif (args->max_pack_size)\n \t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\tif (args->filter)\n+\t\tstrvec_pushf(&cmd->args, \"--filter=%s\", args->filter);\n \tif (args->no_reuse_delta)\n \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n \tif (args->no_reuse_object)\n@@ -268,6 +272,13 @@ static unsigned populate_pack_exts(char *name)\n \treturn ret;\n }\n \n+static void write_promisor_file_1(char *p)\n+{\n+\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp, p);\n+\twrite_promisor_file(promisor_name, NULL, 0);\n+\tfree(promisor_name);\n+}\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t struct string_list *names)\n {\n@@ -299,7 +310,6 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tstruct string_list_item *item;\n-\t\tchar *promisor_name;\n \n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n@@ -316,13 +326,8 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t * concatenate the contents of all .promisor files instead of\n \t\t * just creating a new empty file.\n \t\t */\n-\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n-\t\t\t\t\t line.buf);\n-\t\twrite_promisor_file(promisor_name, NULL, 0);\n-\n+\t\twrite_promisor_file_1(line.buf);\n \t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n-\n-\t\tfree(promisor_name);\n \t}\n \tfclose(out);\n \tif (finish_command(&cmd))\n@@ -786,6 +791,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_STRING(0, \"filter\", &po_args.filter, N_(\"args\"),\n+\t\t\t\tN_(\"object filtering\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n@@ -818,6 +825,24 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n \t}\n \n+\tif (po_args.filter) {\n+\t\tconst char *yesno;\n+\n+\t\tif (!isatty(STDIN_FILENO))\n+\t\t\tdie (_(\"Repacking with a filter is not allowed \"\n+\t\t\t \"yet unless a terminal is used!\"));\n+\n+\t\t/*\n+\t\t * TRANSLATORS: Make sure to include [y] and [N] in your translation.\n+\t\t * The program will only accept English input at this point.\n+\t\t */\n+\t\tyesno = git_prompt(_(\"Repacking with a filter will lose data and corrupt the repo\\n\"\n+\t\t\t\t \"if ANY of the filtered out object hasn't been already pushed!\\n\"\n+\t\t\t\t \"Repack with a filter anyway [y/N]? \"), PROMPT_ECHO);\n+\t\tif (tolower(*yesno) != 'y')\n+\t\t\texit(1);\n+\t}\n+\n \tif (write_bitmaps < 0) {\n \t\tif (!write_midx &&\n \t\t (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n@@ -955,6 +980,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n \t\tstring_list_append(&names, line.buf);\n+\t\tif (po_args.filter)\n+\t\t\twrite_promisor_file_1(line.buf);\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..f8764d1dd9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -237,6 +237,11 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'repacking with a filter is not allowed' '\n+\ttest_must_fail git repack -a -d --filter=blob:none 2>actual &&\n+\ttest_i18ngrep \"Repacking with a filter is not allowed\" actual\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \n2.38.0.4.g7f9724c7bf.dirty\n\n"},{"id":"464717","messageId":"20221012135114.294680-4-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221012135114.294680-1-christian.couder@gmail.com","subject":"[PATCH 3/3] repack: introduce --force to force filtering","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-12T13:51:14Z","receivedAt":"2022-10-12T13:52:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"A previous commit introduced --filter=, but disallowed it\nwhen stdin doesn't refer to a terminal, and asked the user when it does\nrefer to a terminal. This is because this option is dangerous as it\ncould lose data and corrupt the repo.\n\nIn some cases people might want to run this command automatically when\nstdin desn't refer to a terminal or when no question should be asked.\nLet's introduce --force for this purpose.\n\nUnfortunately, we cannot use OPT__FORCE() to implement this because -f is\nalready a repack option (to pass --no-reuse-delta to git-pack-objects),\nso we use OPT_BOOL() instead.\n\nThis is also a good time to test that --filter works properly as it\nwasn't done in the previous commit that introduced --filter.\n\nSigned-off-by: Christian Couder \n---\n Documentation/git-repack.txt | 12 +++++++++++-\n builtin/repack.c | 7 +++++--\n t/t7700-repack.sh | 15 +++++++++++++++\n 3 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 230f176e10..574f569fbe 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -142,7 +142,17 @@ depth is 4095.\n \tpackfile. WARNING: this could easily corrupt the current repo\n \tand lose data if ANY of the omitted objects hasn't been\n \talready pushed to a remote. See linkgit:git-rev-list[1] for\n-\tvalid `` forms.\n+\tvalid `` forms. See also `--force` option below.\n+\n+--force::\n+\tBy default when using `--filter=` and stdin\n+\trefers to a terminal, the user will be warned and asked if the\n+\tfiltering repack should really be launched. This tries to\n+\tavoid possible data loss and repo corruption. See `--filter`\n+\toption above. If stdin doesn't refer to a terminal, the repack\n+\tis aborted. `--force` allows the repack to go on anyway when\n+\tstdin doesn't refer to a terminal or when no question should\n+\tbe asked.\n \n -b::\n --write-bitmap-index::\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 0a93f38da4..3660dbb7a7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -56,6 +56,7 @@ struct pack_objects_args {\n \tint no_reuse_object;\n \tint quiet;\n \tint local;\n+\tint force;\n };\n \n static int repack_config(const char *var, const char *value, void *cb)\n@@ -793,6 +794,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"maximum size of each packfile\")),\n \t\tOPT_STRING(0, \"filter\", &po_args.filter, N_(\"args\"),\n \t\t\t\tN_(\"object filtering\")),\n+\t\tOPT_BOOL(0, \"force\", &po_args.force,\n+\t\t\t\tN_(\"force object filtering\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n@@ -825,12 +828,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n \t}\n \n-\tif (po_args.filter) {\n+\tif (po_args.filter && !po_args.force) {\n \t\tconst char *yesno;\n \n \t\tif (!isatty(STDIN_FILENO))\n \t\t\tdie (_(\"Repacking with a filter is not allowed \"\n-\t\t\t \"yet unless a terminal is used!\"));\n+\t\t\t \"unless a terminal is used or --force is passed!\"));\n \n \t\t/*\n \t\t * TRANSLATORS: Make sure to include [y] and [N] in your translation.\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex f8764d1dd9..20727112b5 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -242,6 +242,21 @@ test_expect_success 'repacking with a filter is not allowed' '\n \ttest_i18ngrep \"Repacking with a filter is not allowed\" actual\n '\n \n+test_expect_success 'repacking with a filter works when --force is passed' '\n+\ttest_when_finished \"rm -rf server client\" &&\n+\ttest_create_repo server &&\n+\tgit -C server config uploadpack.allowFilter true &&\n+\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n+\ttest_commit -C server 1 &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep \"^?\" objects | wc -l) = 0 &&\n+\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none --force &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep \"^?\" objects | wc -l) = 1\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \n2.38.0.4.g7f9724c7bf.dirty\n\n"},{"id":"464965","messageId":"xmqqilkm9wv6.fsf@gitster.g","threadId":"58609","inReplyTo":"20221012135114.294680-1-christian.couder@gmail.com","subject":"Re: [PATCH 0/3] Implement filtering repacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T16:46:21Z","receivedAt":"2022-10-14T16:46:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> For example one might want to clone with a filter to avoid too many\n> space to be taken by some large blobs, and one might realize after\n> some time that a number of the large blobs have still be downloaded\n> because some old branches referencing them were checked out. In this\n> case a filtering repack could remove some of those large blobs.\n>\n> Some of the comments on the patch series that John sent were related\n> to the possible data loss and repo corruption that a filtering repack\n> could cause. It's indeed true that it could be very dangerous, and we\n> agree that improvements were needed in this area.\n\nThe wish is understandable, but I do not think this gives a good UI.\n\nThis feature is, from an end-user's point of view, very similar to\n\"git prune-packed\", in that we prune data that is not necessary due\nto redundancy. Nobody runs \"prune-packed\" directly; most people are\neven unaware of it being run on their behalf when they run \"git gc\".\n\nReusing pack-objects as an underlying mechanism is OK, but this\nneeds to be plumbed through to \"git gc\" for a more consistent\nexperience for the end users.\n\nIs there a way to check if the \"promisor remote\" is still willing to\nkeep the previous promise it made, so that the users do not have to\nsee \"we may corrupt the repository as the result of this operation,\nyou have been warned\", by the way? Possibly with a protocol\nextension?\n\nIn a sense, once you made a partial clone, your repository is at the\nmercy of the remote. They can disappear any time with majority of\nthe data you depend on, leaving only what you created locally and\nhaven't pruned away, in a repository that may technically pass\n\"fsck\", because the things that are supposed to exist locally\nexists, but may not be usable in practice. So from that point of\nview, a simple check that asks \"I earlier fetched from you with this\nfilter and these tips of histories; are you still willing to support\nme?\" and gets yes/no answer might be sufficient. A remote that is\nnot trustworthy can say \"yes\" and still change their mind later, so\nsuch a check may not even be needed.\n\nThe above two big paragraphs is a way to say that I am not all that\nworried about losing objects that we should be able to refetch again\nby adding this feature. The perceived need for \"--force\" or \"must\nrun from terminal\" may be overblown. I do not think this negatively\naffects correctness or robustness at all (as long as the pruning is\nnot buggy, of course).\n\nHOWEVER\n\nUnlike prune-packed, pruning objects that could be refetched has\nnegative performance impact. So adding an option to enable/disable\nsuch pruning is needed. If the frontmost UI entry point were \"gc\",\nit needs an opt-in option to invoke the \"filtering repack\", in other\nwords. \"pack-objects\" should not need any more work than what you\nhave here (with the \"terminal\" and \"force\" discarded), as \"--filter\"\nis such an opt-in option already.\n\n"},{"id":"465287","messageId":"CAP8UFD2HX6rK4TRP6ynUzWn4eoHa1FrbiFOtxBaxX-ZkBF3FJw@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqilkm9wv6.fsf@gitster.g","subject":"Re: [PATCH 0/3] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-20T11:23:02Z","receivedAt":"2022-10-20T11:23:20Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 14, 2022 at 6:46 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > For example one might want to clone with a filter to avoid too many\n> > space to be taken by some large blobs, and one might realize after\n> > some time that a number of the large blobs have still be downloaded\n> > because some old branches referencing them were checked out. In this\n> > case a filtering repack could remove some of those large blobs.\n> >\n> > Some of the comments on the patch series that John sent were related\n> > to the possible data loss and repo corruption that a filtering repack\n> > could cause. It's indeed true that it could be very dangerous, and we\n> > agree that improvements were needed in this area.\n>\n> The wish is understandable, but I do not think this gives a good UI.\n>\n> This feature is, from an end-user's point of view, very similar to\n> \"git prune-packed\", in that we prune data that is not necessary due\n> to redundancy. Nobody runs \"prune-packed\" directly; most people are\n> even unaware of it being run on their behalf when they run \"git gc\".\n\nI am Ok with adding the --filter option to `git gc`, or a config\noption with a similar effect. I wonder how `git gc` should implement\nthat option though.\n\nIf we implement a new command called for example `git filter-packed`,\nsimilar to `git prune-packed`, then this new command will call `git\npack-objects --filter=...`.\n\n`git gc` is already running `git repack` under the hood in a number of\ncases though. So running `git gc --filter=...` would in many cases\ncall `git pack-objects` twice, as it would call it once through git\nrepack and once through `git filter-packed`. Or am I missing something\nhere?\n\nIf on the other hand --filter was implemented in some way in `git\nrepack`, then `git gc --filter=...` could just call `git repack` once.\n\nSo even if the new feature should be run only through `git gc` and\nperhaps a new command possibly called `git filter-packed`, I think it\nmight make sense for efficiency to implement it in some ways, like\nmaybe with some undocumented option or flag, in `git repack`.\n\n> Reusing pack-objects as an underlying mechanism is OK, but this\n> needs to be plumbed through to \"git gc\" for a more consistent\n> experience for the end users.\n\nIt seems to me that `git prune-packed` might only remove objects that\nare already in pack files. So there is no risk of losing data or\ncorrupting the repo.\n\nInstead, the new feature could in some cases lose data and corrupt the\nrepo if some removed objects haven't yet been pushed. So on the client\nside, it seems dangerous to me to make it run automatically without a\ncheck that everything has been pushed.\n\nUnfortunately some users might already run `git gc` automatically, in\ncron scripts for example, and they might be tempted to just add the\n`--filter=...` to their `git gc` script, or to set up a config option\nwith a similar effect without always properly checking that everything\nhas been pushed.\n\nSo I am Ok with trying to make the experience consistent, but I would\nbe worrying that it would let people shoot themselves in the foot too\neasily.\n\nI feel that an obscure `git repack` option would be less likely to be\nrun automatically.\n\n> Is there a way to check if the \"promisor remote\" is still willing to\n> keep the previous promise it made, so that the users do not have to\n> see \"we may corrupt the repository as the result of this operation,\n> you have been warned\", by the way? Possibly with a protocol\n> extension?\n>\n> In a sense, once you made a partial clone, your repository is at the\n> mercy of the remote. They can disappear any time with majority of\n> the data you depend on, leaving only what you created locally and\n> haven't pruned away, in a repository that may technically pass\n> \"fsck\", because the things that are supposed to exist locally\n> exists, but may not be usable in practice.\n\nYeah, when a user clones using --filter=..., the remote can disappear\nanytime, and we haven't been very worried about that.\n\n> So from that point of\n> view, a simple check that asks \"I earlier fetched from you with this\n> filter and these tips of histories; are you still willing to support\n> me?\" and gets yes/no answer might be sufficient. A remote that is\n> not trustworthy can say \"yes\" and still change their mind later, so\n> such a check may not even be needed.\n\nYeah, or a remote that is using some kind of high availability system\nunderneath might consider that it's too expensive and useless to check\nif everything is properly saved everywhere, as the underlying system\nhas been designed for that purpose and already runs enough internal\nchecks.\n\n> The above two big paragraphs is a way to say that I am not all that\n> worried about losing objects that we should be able to refetch again\n> by adding this feature.\n\nI agree. I think it's more important to worry about objects that might\nhave been added locally to the repo, but might not have been pushed\nsomewhere else (yet).\n\n> The perceived need for \"--force\" or \"must\n> run from terminal\" may be overblown. I do not think this negatively\n> affects correctness or robustness at all (as long as the pruning is\n> not buggy, of course).\n\nI am Ok to remove the \"must run from terminal\" and \"--force\" if we\nconsider that people using this feature should know what they are\ndoing.\n\n> HOWEVER\n>\n> Unlike prune-packed, pruning objects that could be refetched has\n> negative performance impact. So adding an option to enable/disable\n> such pruning is needed.\n\nI think a command line option like `--filter=...` is what makes it the\nmost obvious that something special is going on, compared to a config\noption.\n\n> If the frontmost UI entry point were \"gc\",\n> it needs an opt-in option to invoke the \"filtering repack\", in other\n> words. \"pack-objects\" should not need any more work than what you\n> have here (with the \"terminal\" and \"force\" discarded), as \"--filter\"\n> is such an opt-in option already.\n\nYeah. So to sum up, it looks like you are Ok with `git gc\n--filter=...` which is fine for me, even if I wonder if `git repack\n--filter=...` could be a good first step as it is less likely to be\nused automatically (so safer in a way) and it might be better for\nimplementation related performance reasons.\n"},{"id":"465700","messageId":"20221025122856.20204-1-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221012135114.294680-1-christian.couder@gmail.com","subject":"[PATCH v2 0/2] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-25T12:28:54Z","receivedAt":"2022-10-25T12:29:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Earlier this year, John Cai sent 2 versions of a patch series to\nimplement `git repack --filter=`:\n\nhttps://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/\n\nWe tried to \"sell\" it as a way to use partial clone on a Git server to\noffload large blobs to, for example, an http server, while using\nmultiple promisor remotes on the client side.\n\nEven though it is still our end goal, it seems a bit far fetched for\nnow and unnecessary as `git repack --filter=` could be\nuseful on the client side too.\n\nFor example one might want to clone with a filter to avoid too many\nspace to be taken by some large blobs, and one might realize after\nsome time that a number of the large blobs have still be downloaded\nbecause some old branches referencing them were checked out. In this\ncase a filtering repack could remove some of those large blobs.\n\nSome of the comments on the patch series that John sent were related\nto the possible data loss and repo corruption that a filtering repack\ncould cause. It's indeed true that it could be very dangerous, so the\nfirst version of this patch series asked the user to confirm the\ncommand, either by answering 'Y' on the command line or by passing\n`--force`.\n\nIn the discussion with Junio following that first version though, it\nappeared that asking for such confirmation might not be necessary, so\nthe differences in this v2 compared to the v1 are:\n\n - the patch 3/3 that introduced the `--force` option has been\n removed,\n \n - the test that checked that the command couldn't be launched\n without a terminal and without --force has been removed,\n\n - the code that checked if the command was launched from a terminal\n and that asked for confirmation on the command line has been\n removed,\n\n - the documentation of --filter has been improved to make it clearer\n that objects created locally which haven't been pushed could be\n deleted.\n\nSo there are only 2 patches now in this v2 series:\n\n - Patch 1/2 is a preparatory patch.\n\n - Patch 2/2 introduces the `--filter=` option.\n\nIn the discussion with Junio following the first version, Junio\nsuggested adding `--filter=` to `git gc` and I am still\nOk with doing it, either later in a followup patch or in a v3. I\nhaven't done it yet, as I think it's better for performance reasons if\nanyway the filtering is done underneath by `git repack` (even if for\nexample the --filter option is undocumented in `git repack`), and I'd\nlike the approach in this v2 series to be reviewed first.\n\nThanks to Junio for discussing the v1, to John Cai, who worked\non the previous versions, to Jonathan Nieder, Jonathan Tan and Taylor\nBlau, who discussed this with me at the Git Merge and Contributor\nSummit, and to Stolee, Taylor, Robert Coup and Junio who discussed the\nversions John sent.\n\nNot sure it's very useful as it's quite big, but here is the\nrange-diff compared to v1:\n\n1: 0ac7ebf48c = 1: d1c65ff1f5 pack-objects: allow --filter without --stdout\n2: 144356a97e ! 2: ac21b4ec8f repack: add --filter= option\n @@ Commit message\n \n This command could be dangerous to use though, as it might remove\n local objects that haven't been pushed which would lose data and\n - corrupt the repo. On a server this command could also corrupt a\n + corrupt the repo. On a server, this command could also corrupt a\n repo unless ALL the removed objects aren't already available in\n another remote that clients can access.\n \n - To avoid as much as possible data to be lost and repos to be\n - corrupted, let's warn users and ask them to confirm that they\n - really want to use this command.\n -\n - If no terminal is used, let's just die() for now. A follow-up\n - commit will introduce a --force option that will allow using\n - this option when no terminal is available.\n -\n - It will be easier to test that --filter is working correctly\n - in the follow-up commit that adds --force, so let's just test\n - that we die() if no terminal is used for now.\n -\n Signed-off-by: John Cai \n Signed-off-by: Christian Couder \n \n @@ Documentation/git-repack.txt: depth is 4095.\n +--filter=::\n + Omits certain objects (usually blobs) from the resulting\n + packfile. WARNING: this could easily corrupt the current repo\n -+ and lose data if ANY of the omitted objects hasn't been\n -+ already pushed to a remote. See linkgit:git-rev-list[1] for\n -+ valid `` forms.\n ++ and lose data if ANY of the omitted objects hasn't been already\n ++ pushed to a remote. Be very careful about objects that might\n ++ have been created locally! See linkgit:git-rev-list[1] for valid\n ++ `` forms.\n +\n -b::\n --write-bitmap-index::\n Write a reachability bitmap index as part of the repack. This\n \n ## builtin/repack.c ##\n -@@\n - #include \"pack.h\"\n - #include \"pack-bitmap.h\"\n - #include \"refs.h\"\n -+#include \"prompt.h\"\n - \n - #define ALL_INTO_ONE 1\n - #define LOOSEN_UNREACHABLE 2\n @@ builtin/repack.c: struct pack_objects_args {\n const char *depth;\n const char *threads;\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n OPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n N_(\"repack objects in packs marked with .keep\")),\n OPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n -@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n - die(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n - }\n - \n -+ if (po_args.filter) {\n -+ const char *yesno;\n -+\n -+ if (!isatty(STDIN_FILENO))\n -+ die (_(\"Repacking with a filter is not allowed \"\n -+ \"yet unless a terminal is used!\"));\n -+\n -+ /*\n -+ * TRANSLATORS: Make sure to include [y] and [N] in your translation.\n -+ * The program will only accept English input at this point.\n -+ */\n -+ yesno = git_prompt(_(\"Repacking with a filter will lose data and corrupt the repo\\n\"\n -+ \"if ANY of the filtered out object hasn't been already pushed!\\n\"\n -+ \"Repack with a filter anyway [y/N]? \"), PROMPT_ECHO);\n -+ if (tolower(*yesno) != 'y')\n -+ exit(1);\n -+ }\n -+\n - if (write_bitmaps < 0) {\n - if (!write_midx &&\n - (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n if (line.len != the_hash_algo->hexsz)\n die(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila\n test_must_be_empty actual\n '\n \n -+test_expect_success 'repacking with a filter is not allowed' '\n -+ test_must_fail git repack -a -d --filter=blob:none 2>actual &&\n -+ test_i18ngrep \"Repacking with a filter is not allowed\" actual\n ++test_expect_success 'repacking with a filter works' '\n ++ test_when_finished \"rm -rf server client\" &&\n ++ test_create_repo server &&\n ++ git -C server config uploadpack.allowFilter true &&\n ++ git -C server config uploadpack.allowAnySHA1InWant true &&\n ++ test_commit -C server 1 &&\n ++ git clone --bare --no-local server client &&\n ++ git -C client config remote.origin.promisor true &&\n ++ git -C client rev-list --objects --all --missing=print >objects &&\n ++ test $(grep \"^?\" objects | wc -l) = 0 &&\n ++ git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n ++ git -C client rev-list --objects --all --missing=print >objects &&\n ++ test $(grep \"^?\" objects | wc -l) = 1\n +'\n +\n objdir=.git/objects\n3: a23e19796e < -: ---------- repack: introduce --force to force filtering\n\nChristian Couder (2):\n pack-objects: allow --filter without --stdout\n repack: add --filter= option\n\n Documentation/git-repack.txt | 8 ++++++++\n builtin/pack-objects.c | 6 +-----\n builtin/repack.c | 22 +++++++++++++++-------\n t/t7700-repack.sh | 15 +++++++++++++++\n 4 files changed, 39 insertions(+), 12 deletions(-)\n\n-- \n2.38.1.145.g80cce38e46.dirty\n\n"},{"id":"465701","messageId":"20221025122856.20204-2-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221025122856.20204-1-christian.couder@gmail.com","subject":"[PATCH v2 1/2] pack-objects: allow --filter without --stdout","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-25T12:28:55Z","receivedAt":"2022-10-25T12:29:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\n9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)\ntaught pack-objects to use --filter, but required the use of\n--stdout since a partial clone mechanism was not yet in place to\nhandle missing objects. Since then, changes like 9e27beaa23\n(promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)\nand others added support to dynamically fetch objects that were missing.\n\nRemove the --stdout requirement so that in a follow-up commit, repack\ncan pass --filter to pack-objects to omit certain objects from the\nresulting packfile.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n builtin/pack-objects.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3658c05caf..d707e6230d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4385,12 +4385,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n-\t\tif (!pack_to_stdout)\n-\t\t\tdie(_(\"cannot use --filter without --stdout\"));\n-\t\tif (stdin_packs)\n+\tif (pfd.have_revs && pfd.revs.filter.choice && stdin_packs)\n \t\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n-\t}\n \n \tif (stdin_packs && use_internal_rev_list)\n \t\tdie(_(\"cannot use internal rev list with --stdin-packs\"));\n-- \n2.38.1.145.g80cce38e46.dirty\n\n"},{"id":"465702","messageId":"20221025122856.20204-3-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221025122856.20204-1-christian.couder@gmail.com","subject":"[PATCH v2 2/2] repack: add --filter= option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-10-25T12:28:56Z","receivedAt":"2022-10-25T12:29:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nAfter cloning with --filter=, for example to avoid\ngetting unneeded large files on a user machine, it's possible\nthat some of these large files still get fetched for some reasons\n(like checking out old branches) over time.\n\nIn this case the repo size could grow too much for no good reason\nand `git repack --filter=` would be useful to remove\nthe unneeded large files.\n\nThis command could be dangerous to use though, as it might remove\nlocal objects that haven't been pushed which would lose data and\ncorrupt the repo. On a server, this command could also corrupt a\nrepo unless ALL the removed objects aren't already available in\nanother remote that clients can access.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n Documentation/git-repack.txt | 8 ++++++++\n builtin/repack.c | 22 +++++++++++++++-------\n t/t7700-repack.sh | 15 +++++++++++++++\n 3 files changed, 38 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 0bf13893d8..d28bb68cfc 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -137,6 +137,14 @@ depth is 4095.\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n \n+--filter=::\n+\tOmits certain objects (usually blobs) from the resulting\n+\tpackfile. WARNING: this could easily corrupt the current repo\n+\tand lose data if ANY of the omitted objects hasn't been already\n+\tpushed to a remote. Be very careful about objects that might\n+\thave been created locally! See linkgit:git-rev-list[1] for valid\n+\t`` forms.\n+\n -b::\n --write-bitmap-index::\n \tWrite a reachability bitmap index as part of the repack. This\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..d8b8a58f73 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -50,6 +50,7 @@ struct pack_objects_args {\n \tconst char *depth;\n \tconst char *threads;\n \tconst char *max_pack_size;\n+\tconst char *filter;\n \tint no_reuse_delta;\n \tint no_reuse_object;\n \tint quiet;\n@@ -201,6 +202,8 @@ static void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n \tif (args->max_pack_size)\n \t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\tif (args->filter)\n+\t\tstrvec_pushf(&cmd->args, \"--filter=%s\", args->filter);\n \tif (args->no_reuse_delta)\n \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n \tif (args->no_reuse_object)\n@@ -268,6 +271,13 @@ static unsigned populate_pack_exts(char *name)\n \treturn ret;\n }\n \n+static void write_promisor_file_1(char *p)\n+{\n+\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp, p);\n+\twrite_promisor_file(promisor_name, NULL, 0);\n+\tfree(promisor_name);\n+}\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t struct string_list *names)\n {\n@@ -299,7 +309,6 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tstruct string_list_item *item;\n-\t\tchar *promisor_name;\n \n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n@@ -316,13 +325,8 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t * concatenate the contents of all .promisor files instead of\n \t\t * just creating a new empty file.\n \t\t */\n-\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n-\t\t\t\t\t line.buf);\n-\t\twrite_promisor_file(promisor_name, NULL, 0);\n-\n+\t\twrite_promisor_file_1(line.buf);\n \t\titem->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n-\n-\t\tfree(promisor_name);\n \t}\n \tfclose(out);\n \tif (finish_command(&cmd))\n@@ -786,6 +790,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_STRING(0, \"filter\", &po_args.filter, N_(\"args\"),\n+\t\t\t\tN_(\"object filtering\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n@@ -955,6 +961,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n \t\tstring_list_append(&names, line.buf);\n+\t\tif (po_args.filter)\n+\t\t\twrite_promisor_file_1(line.buf);\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex ca45c4cd2c..363eaf8fea 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -237,6 +237,21 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'repacking with a filter works' '\n+\ttest_when_finished \"rm -rf server client\" &&\n+\ttest_create_repo server &&\n+\tgit -C server config uploadpack.allowFilter true &&\n+\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n+\ttest_commit -C server 1 &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep \"^?\" objects | wc -l) = 0 &&\n+\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep \"^?\" objects | wc -l) = 1\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \n2.38.1.145.g80cce38e46.dirty\n\n"},{"id":"465970","messageId":"Y1wyVpHprWGxEDi/@nand.local","threadId":"58609","inReplyTo":"CAP8UFD2HX6rK4TRP6ynUzWn4eoHa1FrbiFOtxBaxX-ZkBF3FJw@mail.gmail.com","subject":"Re: [PATCH 0/3] Implement filtering repacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-28T19:49:42Z","receivedAt":"2022-10-28T19:50:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 20, 2022 at 01:23:02PM +0200, Christian Couder wrote:\n> On Fri, Oct 14, 2022 at 6:46 PM Junio C Hamano wrote:\n> >\n> > Christian Couder writes:\n> >\n> > > For example one might want to clone with a filter to avoid too many\n> > > space to be taken by some large blobs, and one might realize after\n> > > some time that a number of the large blobs have still be downloaded\n> > > because some old branches referencing them were checked out. In this\n> > > case a filtering repack could remove some of those large blobs.\n> > >\n> > > Some of the comments on the patch series that John sent were related\n> > > to the possible data loss and repo corruption that a filtering repack\n> > > could cause. It's indeed true that it could be very dangerous, and we\n> > > agree that improvements were needed in this area.\n> >\n> > The wish is understandable, but I do not think this gives a good UI.\n> >\n> > This feature is, from an end-user's point of view, very similar to\n> > \"git prune-packed\", in that we prune data that is not necessary due\n> > to redundancy. Nobody runs \"prune-packed\" directly; most people are\n> > even unaware of it being run on their behalf when they run \"git gc\".\n>\n> I am Ok with adding the --filter option to `git gc`, or a config\n> option with a similar effect. I wonder how `git gc` should implement\n> that option though.\n>\n> If we implement a new command called for example `git filter-packed`,\n> similar to `git prune-packed`, then this new command will call `git\n> pack-objects --filter=...`.\n\nConceptually, yes, the two are similar. Though `prune-filtered` is\nnecessarily going to differ in implementation from `prune-packed`, since\nwe will have to write new pack(s), not just delete loose objects which\nappear in packs already.\n\nSo it's really not just a matter of purely deleting redundant loose\ncopies of objects like in the case of prune-packed. Here we really do\ncare about potentially writing a new set of packs to satisfy the new\nfilter constraint.\n\nPresumably that tool would implement creating the new packs according to\nthe given --filter, and would similarly delete existing packs. That is\nbasically what your implementation in repack already does, so I am not\nsure what the difference would be.\n\n> Yeah. So to sum up, it looks like you are Ok with `git gc\n> --filter=...` which is fine for me, even if I wonder if `git repack\n> --filter=...` could be a good first step as it is less likely to be\n> used automatically (so safer in a way) and it might be better for\n> implementation related performance reasons.\n\nIf we don't intend to have `git repack --filter` part of our backwards\ncompatibility guarantee, then I would prefer to see the implementation\njust live in git-gc from start to finish.\n\nThanks,\nTaylor\n"},{"id":"465971","messageId":"Y1wzgOTiKdBcBba0@nand.local","threadId":"58609","inReplyTo":"20221025122856.20204-1-christian.couder@gmail.com","subject":"Re: [PATCH v2 0/2] Implement filtering repacks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-28T19:54:40Z","receivedAt":"2022-10-28T19:54:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Oct 25, 2022 at 02:28:54PM +0200, Christian Couder wrote:\n> So there are only 2 patches now in this v2 series:\n>\n> - Patch 1/2 is a preparatory patch.\n>\n> - Patch 2/2 introduces the `--filter=` option.\n\nOne thing that I wasn't clear on in this or the previous round(s) was\nhow we handle setting remote..promisor and partialclonefilter.\n\nIf there is a single remote, then it's obvious that we should set\npromisor to \"true\" and partialCloneFilter to whatever value of\n`--filter` the user provided when repacking / GCing.\n\nBut what happens if there are multiple remotes? Which get the new\nconfiguration settings modified?\n\nI wonder what breakage happens if we fail to do that (and why such\nbreakage isn't yet noticed by CI).\n\nThanks,\nTaylor\n"},{"id":"465981","messageId":"xmqqwn8jheyb.fsf@gitster.g","threadId":"58609","inReplyTo":"Y1wyVpHprWGxEDi/@nand.local","subject":"Re: [PATCH 0/3] Implement filtering repacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-28T20:26:52Z","receivedAt":"2022-10-28T20:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n>> > This feature is, from an end-user's point of view, very similar to\n>> > \"git prune-packed\", in that we prune data that is not necessary due\n>> > to redundancy. Nobody runs \"prune-packed\" directly; most people are\n>> > even unaware of it being run on their behalf when they run \"git gc\".\n>> ...\n> If we don't intend to have `git repack --filter` part of our backwards\n> compatibility guarantee, then I would prefer to see the implementation\n> just live in git-gc from start to finish.\n\nI am OK with that, too. We do not have to expose the option to\nusers of pack-objects at all. Just make \"gc\" the single entry point\nand that should be quite clean.\n"},{"id":"466643","messageId":"CAP8UFD31vHzV6fmvXLPadVFk3a_-MHgfQGUnO6tAftQxu7KE0w@mail.gmail.com","threadId":"58609","inReplyTo":"Y1wyVpHprWGxEDi/@nand.local","subject":"Re: [PATCH 0/3] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-11-07T09:00:52Z","receivedAt":"2022-11-07T09:01:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 28, 2022 at 9:49 PM Taylor Blau wrote:\n>\n> On Thu, Oct 20, 2022 at 01:23:02PM +0200, Christian Couder wrote:\n> > On Fri, Oct 14, 2022 at 6:46 PM Junio C Hamano wrote:\n> > >\n> > > Christian Couder writes:\n> > >\n> > > > For example one might want to clone with a filter to avoid too many\n> > > > space to be taken by some large blobs, and one might realize after\n> > > > some time that a number of the large blobs have still be downloaded\n> > > > because some old branches referencing them were checked out. In this\n> > > > case a filtering repack could remove some of those large blobs.\n> > > >\n> > > > Some of the comments on the patch series that John sent were related\n> > > > to the possible data loss and repo corruption that a filtering repack\n> > > > could cause. It's indeed true that it could be very dangerous, and we\n> > > > agree that improvements were needed in this area.\n> > >\n> > > The wish is understandable, but I do not think this gives a good UI.\n> > >\n> > > This feature is, from an end-user's point of view, very similar to\n> > > \"git prune-packed\", in that we prune data that is not necessary due\n> > > to redundancy. Nobody runs \"prune-packed\" directly; most people are\n> > > even unaware of it being run on their behalf when they run \"git gc\".\n> >\n> > I am Ok with adding the --filter option to `git gc`, or a config\n> > option with a similar effect. I wonder how `git gc` should implement\n> > that option though.\n> >\n> > If we implement a new command called for example `git filter-packed`,\n> > similar to `git prune-packed`, then this new command will call `git\n> > pack-objects --filter=...`.\n>\n> Conceptually, yes, the two are similar. Though `prune-filtered` is\n> necessarily going to differ in implementation from `prune-packed`, since\n> we will have to write new pack(s), not just delete loose objects which\n> appear in packs already.\n\nYeah, that's why I say `prune-filtered` will call `git pack-objects\n--filter=...`.\n\n> So it's really not just a matter of purely deleting redundant loose\n> copies of objects like in the case of prune-packed. Here we really do\n> care about potentially writing a new set of packs to satisfy the new\n> filter constraint.\n\nYeah, I agree.\n\n> Presumably that tool would implement creating the new packs according to\n> the given --filter, and would similarly delete existing packs. That is\n> basically what your implementation in repack already does, so I am not\n> sure what the difference would be.\n\nIndeed, there wouldn't be much difference implementation wise between\na new `git filter-packed` command like Junio suggested and the current\nimplementation I sent which implements the feature in `git repack`. (A\nnew `git filter-packed` would just duplicate the repack features that\nare needed and just call `git pack-objects --filter=...`). That's why\nI don't really see the point of a new `git filter-packed` command and\nthe version 2 I sent still implements the feature in `git repack`.\n\nSo I have a hard time understanding your comment unless you just agree with me.\n\n> > Yeah. So to sum up, it looks like you are Ok with `git gc\n> > --filter=...` which is fine for me, even if I wonder if `git repack\n> > --filter=...` could be a good first step as it is less likely to be\n> > used automatically (so safer in a way) and it might be better for\n> > implementation related performance reasons.\n>\n> If we don't intend to have `git repack --filter` part of our backwards\n> compatibility guarantee, then I would prefer to see the implementation\n> just live in git-gc from start to finish.\n\nAbout the implementation living in `git gc` I wrote the following:\n\n>>> `git gc` is already running `git repack` under the hood in a number of\n>>> cases though. So running `git gc --filter=...` would in many cases\n>>> call `git pack-objects` twice, as it would call it once through git\n>>> repack and once through `git filter-packed`. Or am I missing something\n>>> here?\n\nEven if we don't have a `git filter-packed` command, if the feature is\nimplemented in `git gc` (but not in `git repack`) it would just call\n`git pack-objects --filter=...` from there, which means that `git\npack-objects` would be called twice (once through `git repack` and\nonce for this new feature) by `git gc` in some cases, instead of just\nonce if the feature was implemented in `git repack` as `git gc` could\nthen just calls `git repack ... --filter=...` once.\n\nThat's why I think it's better for performance reasons if the feature\nis implemented in `git repack`. If you don't want for some reason to\nhave `git repack --filter=...` part of our backwards compatibility\nguarantee, then --filter can be a hidden and undocumented option in\n`git repack`. Or maybe we could use a new env variable to instruct\n`git repack` to pass some --filter option to `git pack-objects`, but\nmy opinion is that it's much simpler to just accept --filter to be a\nregular, though dangerous, `git repack` option, and then add --filter\nto `git gc`.\n\nI am also Ok with adding --filter to `git gc` in this patch series and\nhave the doc say that it's better to use `git gc --filter` instead of\n`git repack --filter` so that users could learn right away to use the\nfeature through `git gc` instead of through `git repack`.\n"},{"id":"466644","messageId":"CAP8UFD04HWO0kwuxzuRKutuWuHn26iWhC-CCuyaJz3-jjHzOWw@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqwn8jheyb.fsf@gitster.g","subject":"Re: [PATCH 0/3] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-11-07T09:12:15Z","receivedAt":"2022-11-07T09:12:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 28, 2022 at 10:26 PM Junio C Hamano wrote:\n>\n> Taylor Blau writes:\n>\n> >> > This feature is, from an end-user's point of view, very similar to\n> >> > \"git prune-packed\", in that we prune data that is not necessary due\n> >> > to redundancy. Nobody runs \"prune-packed\" directly; most people are\n> >> > even unaware of it being run on their behalf when they run \"git gc\".\n> >> ...\n> > If we don't intend to have `git repack --filter` part of our backwards\n> > compatibility guarantee, then I would prefer to see the implementation\n> > just live in git-gc from start to finish.\n>\n> I am OK with that, too. We do not have to expose the option to\n> users of pack-objects at all.\n\nYou might mean \"to users of `git repack`\", as `git pack-objects`\nalready has a `--filter=` option.\n\n> Just make \"gc\" the single entry point\n> and that should be quite clean.\n\nI am Ok with that if there is a simple way to not call `git\npack-objects` twice in some cases (once through git repack and once\nthrough this new feature). I may be missing something, but\nunfortunately I don't see one.\n\n(Or maybe you mean that `--filter` in `git gc` should be an option\nthat is incompatible with other `git gc` options and configuration\nparameters, so that when `git gc --filter=XXX` is called it only\nperforms what `git repack --filter=XXX` performs in the implementation\nI sent, but then people might want to call `git gc` twice instead of\nonce, if they also want other usual gc housekeeping tasks to be\nperformed. I don't think that's the way `git gc` has been designed for\nnow though.)\n"},{"id":"466645","messageId":"CAP8UFD2jjQxMsN7VzticiFkrOSLVK6f-f4R1fZU1wnEBpuHapQ@mail.gmail.com","threadId":"58609","inReplyTo":"Y1wzgOTiKdBcBba0@nand.local","subject":"Re: [PATCH v2 0/2] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-11-07T09:29:51Z","receivedAt":"2022-11-07T09:30:11Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Oct 28, 2022 at 9:54 PM Taylor Blau wrote:\n>\n> On Tue, Oct 25, 2022 at 02:28:54PM +0200, Christian Couder wrote:\n> > So there are only 2 patches now in this v2 series:\n> >\n> > - Patch 1/2 is a preparatory patch.\n> >\n> > - Patch 2/2 introduces the `--filter=` option.\n>\n> One thing that I wasn't clear on in this or the previous round(s) was\n> how we handle setting remote..promisor and partialclonefilter.\n\nYeah, I agree that it's an interesting question that I overlooked.\n\n> If there is a single remote, then it's obvious that we should set\n> promisor to \"true\" and partialCloneFilter to whatever value of\n> `--filter` the user provided when repacking / GCing.\n\nI would be Ok to setting remote..promisor to true in this case,\nbut I am not sure we really need to do it.\n\nMaybe the user is mostly interested in reducing the size of the repo\nfor now and plans to set up a promisor remote afterwards.\n\nAnother perhaps better way to handle this would be to just die() if no\nremote..promisor is set to true. This way we can make sure that\nusers will not forget to set up at least one promisor remote. This\ncould also give users the opportunity to think about whether their\nconfigured remotes contain all the objects they are going to remove.\n\nAbout remote..partialclonefilter I don't think we need to do\nanything. Maybe the user would be Ok with having different filters\nwhen fetching and when cleaning up.\n\n> But what happens if there are multiple remotes? Which get the new\n> configuration settings modified?\n\nI agree that if we want this feature to modify settings, then there is\nno good and simple solution in this case.\n\n> I wonder what breakage happens if we fail to do that (and why such\n> breakage isn't yet noticed by CI).\n\nIf we want to avoid breakages as much as possible, then die()ing when\nno remote..promisor is set to true seems to be the best\nsolution, instead of trying to modify settings.\n"},{"id":"467782","messageId":"20221122175150.366828-1-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221025122856.20204-1-christian.couder@gmail.com","subject":"[PATCH v3 0/2] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-11-22T17:51:48Z","receivedAt":"2022-11-22T17:55:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Earlier this year, John Cai sent 2 versions of a patch series to\nimplement `git repack --filter=`:\n\nhttps://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/\n\nWe tried to \"sell\" it as a way to use partial clone on a Git server to\noffload large blobs to, for example, an http server, while using\nmultiple promisor remotes on the client side.\n\nEven though it is still our end goal, it seems a bit far fetched for\nnow and unnecessary as `git repack --filter=` could be\nuseful on the client side too.\n\nFor example one might want to clone with a filter to avoid too many\nspace to be taken by some large blobs, and one might realize after\nsome time that a number of the large blobs have still be downloaded\nbecause some old branches referencing them were checked out. In this\ncase a filtering repack could remove some of those large blobs.\n\nSome of the comments on the patch series that John sent were related\nto the possible data loss and repo corruption that a filtering repack\ncould cause. It's indeed true that it could be very dangerous, so the\nfirst version of this patch series asked the user to confirm the\ncommand, either by answering 'Y' on the command line or by passing\n`--force`.\n\nIn the discussion with Junio following that first version though, it\nappeared that asking for such confirmation might not be necessary, so\nthe v2 removed those checks. Taylor though asked what would happen to\nthe remote..promisor and remote..partialclonefilter config\nvariables when a filtering repack is run. I replied that it seems to\nme that we should just check that a promisor remote has been\nconfigured and fail if that's not the case.\n\nIn the discussions with Junio and Taylor following the first and\nsecond versions, Junio suggested adding `--filter=` to\n`git gc` and I am still Ok with doing it, either later in a followup\npatch or in a v4. I haven't done it yet, as it's not clear how to\nimplement it efficiently only in `git gc`.\n\n`git gc` is already running `git repack` when it's passed some options\n(either on the command line or via the config), so it would be just\nsimpler for `git gc` to just pass on the --filter= it\nwould be given to `git repack`. If `git gc` would implement that by\ncalling `git pack-objects` directly, then `git pack-objects` would\npossibly be called twice from `git gc`: once throught `git repack` and\nonce triggered by the --filter option.\n\nSo the only changes in this v3 compared to v2 are the following:\n\n - rebased on top of a0789512c5 (The thirteenth batch, 2022-11-18) to\n avoid a simple conflict,\n\n - patch 2/2 uses has_promisor_remote() to check if a promisor remote\n is configured and die() otherwise.\n\nThanks to Junio and Taylor for discussing the v1 and v2, to John Cai,\nwho worked on the previous versions, to Jonathan Nieder, Jonathan Tan\nand Taylor Blau, who discussed this with me at the Git Merge and\nContributor Summit, and to Stolee, Taylor, Robert Coup and Junio who\ndiscussed the versions John sent.\n\nRange diff with v2:\n\n1: d1c65ff1f5 = 1: 1e64cac782 pack-objects: allow --filter without --stdout\n2: ac21b4ec8f ! 2: 7216a7bc05 repack: add --filter= option\n @@ Commit message\n repo unless ALL the removed objects aren't already available in\n another remote that clients can access.\n \n + To mitigate that risk, we check that a promisor remote has at\n + least been configured.\n +\n Signed-off-by: John Cai \n Signed-off-by: Christian Couder \n \n @@ builtin/repack.c: static void prepare_pack_objects(struct child_process *cmd,\n if (args->no_reuse_delta)\n strvec_pushf(&cmd->args, \"--no-reuse-delta\");\n if (args->no_reuse_object)\n -@@ builtin/repack.c: static unsigned populate_pack_exts(char *name)\n - return ret;\n +@@ builtin/repack.c: static struct generated_pack_data *populate_pack_exts(const char *name)\n + return data;\n }\n \n +static void write_promisor_file_1(char *p)\n @@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_\n - write_promisor_file(promisor_name, NULL, 0);\n -\n + write_promisor_file_1(line.buf);\n - item->util = (void *)(uintptr_t)populate_pack_exts(item->string);\n + item->util = populate_pack_exts(item->string);\n -\n - free(promisor_name);\n }\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n OPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n N_(\"repack objects in packs marked with .keep\")),\n OPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n +@@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n + die(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n + }\n + \n ++ if (po_args.filter && !has_promisor_remote())\n ++ die(\"a promisor remote must be setup\\n\"\n ++ \"Also please push all the objects \"\n ++ \"that might be filtered to that remote!\\n\"\n ++ \"Otherwise they will be lost!\");\n ++\n + if (write_bitmaps < 0) {\n + if (!write_midx &&\n + (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)\n if (line.len != the_hash_algo->hexsz)\n die(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n - string_list_append(&names, line.buf);\n + item = string_list_append(&names, line.buf);\n + if (po_args.filter)\n + write_promisor_file_1(line.buf);\n + item->util = populate_pack_exts(item->string);\n }\n fclose(out);\n - ret = finish_command(&cmd);\n \n ## t/t7700-repack.sh ##\n @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavailable' '\n\n\nChristian Couder (2):\n pack-objects: allow --filter without --stdout\n repack: add --filter= option\n\n Documentation/git-repack.txt | 8 ++++++++\n builtin/pack-objects.c | 6 +-----\n builtin/repack.c | 28 +++++++++++++++++++++-------\n t/t7700-repack.sh | 15 +++++++++++++++\n 4 files changed, 45 insertions(+), 12 deletions(-)\n\n-- \n2.38.1.475.g7216a7bc05\n\n"},{"id":"467783","messageId":"20221122175150.366828-2-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221122175150.366828-1-christian.couder@gmail.com","subject":"[PATCH v3 1/2] pack-objects: allow --filter without --stdout","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-11-22T17:51:49Z","receivedAt":"2022-11-22T17:55:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\n9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)\ntaught pack-objects to use --filter, but required the use of\n--stdout since a partial clone mechanism was not yet in place to\nhandle missing objects. Since then, changes like 9e27beaa23\n(promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)\nand others added support to dynamically fetch objects that were missing.\n\nRemove the --stdout requirement so that in a follow-up commit, repack\ncan pass --filter to pack-objects to omit certain objects from the\nresulting packfile.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n builtin/pack-objects.c | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 573d0b20b7..a4dccca4cd 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4385,12 +4385,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n-\tif (pfd.have_revs && pfd.revs.filter.choice) {\n-\t\tif (!pack_to_stdout)\n-\t\t\tdie(_(\"cannot use --filter without --stdout\"));\n-\t\tif (stdin_packs)\n+\tif (pfd.have_revs && pfd.revs.filter.choice && stdin_packs)\n \t\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n-\t}\n \n \tif (stdin_packs && use_internal_rev_list)\n \t\tdie(_(\"cannot use internal rev list with --stdin-packs\"));\n-- \n2.38.1.475.g7216a7bc05\n\n"},{"id":"467784","messageId":"20221122175150.366828-3-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221122175150.366828-1-christian.couder@gmail.com","subject":"[PATCH v3 2/2] repack: add --filter= option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-11-22T17:51:50Z","receivedAt":"2022-11-22T17:55:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nAfter cloning with --filter=, for example to avoid\ngetting unneeded large files on a user machine, it's possible\nthat some of these large files still get fetched for some reasons\n(like checking out old branches) over time.\n\nIn this case the repo size could grow too much for no good reason\nand `git repack --filter=` would be useful to remove\nthe unneeded large files.\n\nThis command could be dangerous to use though, as it might remove\nlocal objects that haven't been pushed which would lose data and\ncorrupt the repo. On a server, this command could also corrupt a\nrepo unless ALL the removed objects aren't already available in\nanother remote that clients can access.\n\nTo mitigate that risk, we check that a promisor remote has at\nleast been configured.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n Documentation/git-repack.txt | 8 ++++++++\n builtin/repack.c | 28 +++++++++++++++++++++-------\n t/t7700-repack.sh | 15 +++++++++++++++\n 3 files changed, 44 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 4017157949..2539ee0a02 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -143,6 +143,14 @@ depth is 4095.\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n \n+--filter=::\n+\tOmits certain objects (usually blobs) from the resulting\n+\tpackfile. WARNING: this could easily corrupt the current repo\n+\tand lose data if ANY of the omitted objects hasn't been already\n+\tpushed to a remote. Be very careful about objects that might\n+\thave been created locally! See linkgit:git-rev-list[1] for valid\n+\t`` forms.\n+\n -b::\n --write-bitmap-index::\n \tWrite a reachability bitmap index as part of the repack. This\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 65eb1b8bd2..3586d97f3f 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -49,6 +49,7 @@ struct pack_objects_args {\n \tconst char *depth;\n \tconst char *threads;\n \tconst char *max_pack_size;\n+\tconst char *filter;\n \tint no_reuse_delta;\n \tint no_reuse_object;\n \tint quiet;\n@@ -163,6 +164,8 @@ static void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n \tif (args->max_pack_size)\n \t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\tif (args->filter)\n+\t\tstrvec_pushf(&cmd->args, \"--filter=%s\", args->filter);\n \tif (args->no_reuse_delta)\n \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n \tif (args->no_reuse_object)\n@@ -234,6 +237,13 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n \treturn data;\n }\n \n+static void write_promisor_file_1(char *p)\n+{\n+\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp, p);\n+\twrite_promisor_file(promisor_name, NULL, 0);\n+\tfree(promisor_name);\n+}\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t struct string_list *names)\n {\n@@ -265,7 +275,6 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tstruct string_list_item *item;\n-\t\tchar *promisor_name;\n \n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n@@ -282,13 +291,8 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t * concatenate the contents of all .promisor files instead of\n \t\t * just creating a new empty file.\n \t\t */\n-\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n-\t\t\t\t\t line.buf);\n-\t\twrite_promisor_file(promisor_name, NULL, 0);\n-\n+\t\twrite_promisor_file_1(line.buf);\n \t\titem->util = populate_pack_exts(item->string);\n-\n-\t\tfree(promisor_name);\n \t}\n \tfclose(out);\n \tif (finish_command(&cmd))\n@@ -800,6 +804,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_STRING(0, \"filter\", &po_args.filter, N_(\"args\"),\n+\t\t\t\tN_(\"object filtering\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n@@ -834,6 +840,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n \t}\n \n+\tif (po_args.filter && !has_promisor_remote())\n+\t\tdie(\"a promisor remote must be setup\\n\"\n+\t\t \"Also please push all the objects \"\n+\t\t \"that might be filtered to that remote!\\n\"\n+\t\t \"Otherwise they will be lost!\");\n+\n \tif (write_bitmaps < 0) {\n \t\tif (!write_midx &&\n \t\t (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n@@ -971,6 +983,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n \t\titem = string_list_append(&names, line.buf);\n+\t\tif (po_args.filter)\n+\t\t\twrite_promisor_file_1(line.buf);\n \t\titem->util = populate_pack_exts(item->string);\n \t}\n \tfclose(out);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex c630e0d52d..fef144bace 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -237,6 +237,21 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'repacking with a filter works' '\n+\ttest_when_finished \"rm -rf server client\" &&\n+\ttest_create_repo server &&\n+\tgit -C server config uploadpack.allowFilter true &&\n+\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n+\ttest_commit -C server 1 &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep \"^?\" objects | wc -l) = 0 &&\n+\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep \"^?\" objects | wc -l) = 1\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \n2.38.1.475.g7216a7bc05\n\n"},{"id":"467836","messageId":"xmqqa64ipl4r.fsf@gitster.g","threadId":"58609","inReplyTo":"20221122175150.366828-1-christian.couder@gmail.com","subject":"Re: [PATCH v3 0/2] Implement filtering repacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-23T00:31:00Z","receivedAt":"2022-11-23T00:31:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> In the discussions with Junio and Taylor following the first and\n> second versions, Junio suggested adding `--filter=` to\n> `git gc` and I am still Ok with doing it, either later in a followup\n> patch or in a v4. I haven't done it yet, as it's not clear how to\n> implement it efficiently only in `git gc`.\n\nDid I make such a suggestion? I only said --filter=\ndoes not seem like a good option to surface and stress at the\nend-user level for \"git pack-objects\".\n\nThe similarity of \"git prune-packed\" with this new operation was\nbrought up to illustrate the point that users of \"git gc\" do not\nwant to even know about having to remove redundantly available\nobjects via \"prune-packed\" by giving an extra option to \"git gc\"\n(hence --prune is the default), and honoring the filter spec when\nobjects are known to be available via the configured promisor remote\nis likely what users at the \"git gc\" level would want to see happen\nautomatically. IOW, adding more options to \"gc\" would be the last\nthing I would want to see.\n"},{"id":"467837","messageId":"xmqq5yf6pkws.fsf@gitster.g","threadId":"58609","inReplyTo":"20221122175150.366828-1-christian.couder@gmail.com","subject":"Re: [PATCH v3 0/2] Implement filtering repacks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-23T00:35:47Z","receivedAt":"2022-11-23T00:35:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> So the only changes in this v3 compared to v2 are the following:\n>\n> - rebased on top of a0789512c5 (The thirteenth batch, 2022-11-18) to\n> avoid a simple conflict,\n>\n> - patch 2/2 uses has_promisor_remote() to check if a promisor remote\n> is configured and die() otherwise.\n\nBoth sounds like sensible things to do. Thanks.\n"},{"id":"469411","messageId":"CAP8UFD2_ZbrOh4Tto2V80wn4ki+3j6Giegx+QyrLqd7uH6U4GQ@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqa64ipl4r.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-12-21T03:53:50Z","receivedAt":"2022-12-21T03:54:19Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Nov 23, 2022 at 1:31 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > In the discussions with Junio and Taylor following the first and\n> > second versions, Junio suggested adding `--filter=` to\n> > `git gc` and I am still Ok with doing it, either later in a followup\n> > patch or in a v4. I haven't done it yet, as it's not clear how to\n> > implement it efficiently only in `git gc`.\n>\n> Did I make such a suggestion? I only said --filter=\n> does not seem like a good option to surface and stress at the\n> end-user level for \"git pack-objects\".\n\nIn https://lore.kernel.org/git/xmqqilkm9wv6.fsf@gitster.g/ you wrote:\n\n-> Unlike prune-packed, pruning objects that could be refetched has\n-> negative performance impact. So adding an option to enable/disable\n-> such pruning is needed. If the frontmost UI entry point were \"gc\",\n-> it needs an opt-in option to invoke the \"filtering repack\", in other\n-> words. \"pack-objects\" should not need any more work than what you\n-> have here (with the \"terminal\" and \"force\" discarded), as \"--filter\"\n-> is such an opt-in option already.\n\nI understood the above to mean that you would be Ok with adding\na \"--filter\" flag to `git gc`.\n\n> The similarity of \"git prune-packed\" with this new operation was\n> brought up to illustrate the point that users of \"git gc\" do not\n> want to even know about having to remove redundantly available\n> objects via \"prune-packed\" by giving an extra option to \"git gc\"\n> (hence --prune is the default), and honoring the filter spec when\n> objects are known to be available via the configured promisor remote\n> is likely what users at the \"git gc\" level would want to see happen\n> automatically. IOW, adding more options to \"gc\" would be the last\n> thing I would want to see.\n\nI still think that users should be able to control if `git gc` performs\nsuch a filtering when repacking, and I don't see how it could be done\nwithout any new option at all. I think that for example only checking\nthe 'remote..partialclonefilter' config variable to decide if it\nshould be done or not is just too dangerous for people already using\nthis variable.\n\nIn the v4 I will send really soon now, I tentatively implemented a\n'gc.repackFilter' config option for the purpose of telling `git gc` that it\nshould perform filtering repacks with the specified filter. I hope that\nthis will help move the discussion forward.\n\nBTW in the latest \"What's cooking in git.git\" emails there is the\nfollowing about this patch series:\n\n-> Seems to break CI.\n-> cf. https://github.com/git/git/actions/runs/3560918726\n\nI took a look at the failures in the different failing tests and they\ndon't seem related to this patch\nseries and seem quite random:\n\n=== Failed test: t3106-ls-tree-pattern ===\nThe full logs are in the 'print test failures' step below.\nSee also the 'failed-tests-*' artifacts attached to this run.\nError: failed: t3106.3 combine with \"--object-only\"\nfailure: t3106.3 combine with \"--object-only\"\nError: failed: t3106.5 combine with \"--long\"\nfailure: t3106.5 combine with \"--long\"\n\n=== Failed test: t5601-clone ===\nThe full logs are in the 'print test failures' step below.\nSee also the 'failed-tests-*' artifacts attached to this run.\nskip: t5601.60 clone c:temp is dos drive\nskip: t5601.101 colliding file detection\nError: failed: t5601.110 auto-discover bundle URI from HTTP clone\nfailure: t5601.110 auto-discover bundle URI from HTTP clone\nError: failed: t5601.111 auto-discover multiple bundles from HTTP clone\nfailure: t5601.111 auto-discover multiple bundles from HTTP clone\n\n=== Failed test: t5556-http-auth ===\nThe full logs are in the 'print test failures' step below.\nSee also the 'failed-tests-*' artifacts attached to this run.\nError: failed: t5556.2 http auth anonymous no challenge\nfailure: t5556.2 http auth anonymous no challenge\nError: failed: t5556.3 http auth www-auth headers to credential helper\nbearer valid\nfailure: t5556.3 http auth www-auth headers to credential helper bearer valid\n\n=== Failed test: t4203-mailmap ===\nThe full logs are in the 'print test failures' step below.\nSee also the 'failed-tests-*' artifacts attached to this run.\nError: failed: t4203.66 git cat-file -s returns correct size with --use-mailmap\nfailure: t4203.66 git cat-file -s returns correct size with --use-mailmap\nError: failed: t4203.67 git cat-file -s returns correct size with\n--use-mailmap for tag objects\nfailure: t4203.67 git cat-file -s returns correct size with\n--use-mailmap for tag objects\n\netc\n\nI tried different build flags but can't reproduce locally.\n"},{"id":"469412","messageId":"20221221040446.2860985-1-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221122175150.366828-1-christian.couder@gmail.com","subject":"[PATCH v4 0/3] Implement filtering repacks","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-12-21T04:04:43Z","receivedAt":"2022-12-21T04:05:21Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Earlier this year, John Cai sent 2 versions of a patch series to\nimplement `git repack --filter=`:\n\nhttps://lore.kernel.org/git/pull.1206.git.git.1643248180.gitgitgadget@gmail.com/\n\nWe tried to \"sell\" it as a way to use partial clone on a Git server to\noffload large blobs to, for example, an http server, while using\nmultiple promisor remotes on the client side.\n\nEven though it is still our end goal, it seems a bit far fetched for\nnow and unnecessary as `git repack --filter=` could be\nuseful on the client side too.\n\nFor example one might want to clone with a filter to avoid too many\nspace to be taken by some large blobs, and one might realize after\nsome time that a number of the large blobs have still be downloaded\nbecause some old branches referencing them were checked out. In this\ncase a filtering repack could remove some of those large blobs.\n\nSome of the comments on the patch series that John sent were related\nto the possible data loss and repo corruption that a filtering repack\ncould cause. It's indeed true that it could be very dangerous, so the\nfirst version of this patch series asked the user to confirm the\ncommand, either by answering 'Y' on the command line or by passing\n`--force`.\n\nIn the discussion with Junio following that first version though, it\nappeared that asking for such confirmation might not be necessary, so\nthe v2 removed those checks.\n\nTaylor though asked what would happen to the 'remote..promisor'\nand 'remote..partialclonefilter' config variables when a\nfiltering repack is run. As it seemed to me that we should just check\nthat a promisor remote has been configured and fail if that's not the\ncase, that was implemented in the third version of this patch series.\n\nIn the discussions following the first, second and third versions,\nJunio commented that `git gc` was a better way for users to launch\nfiltering repacks then `git repack`, so in this v4 a new\n'gc.repackFilter' config option is implemented that allows `git gc` to\nperform filtering repacks. When this config option is set to a non\nempty string, `git gc` will just add a `--filter=`\nargument to the repack processes it launches, with '' set\nto the value of 'gc.repackFilter'.\n\nSo the changes in this v4 compared to v3 are the following:\n\n - rebased on top of 57e2c6ebbe (Start the 2.40 cycle, 2022-12-14) to\n avoid a simple conflict,\n\n - simplified the test in patch 2/3 by using `grep -c ...` instead of\n `grep ... | wc -l`,\n\n - added patch 3/3 which implements a new 'gc.repackFilter' config\n option so that `git gc` can perform filtering repacks.\n\nThanks to Junio and Taylor for discussing the v1, v2 and v3, to John\nCai, who worked on the previous versions, to Jonathan Nieder, Jonathan\nTan and Taylor, who discussed this with me at the Git Merge and\nContributor Summit, and to Stolee, Taylor, Robert Coup and Junio who\ndiscussed the versions John sent.\n\nRange diff with v3:\n\n1: 1e64cac782 < -: ---------- pack-objects: allow --filter without --stdout\n-: ---------- > 1: c2dca82dee pack-objects: allow --filter without --stdout\n2: 7216a7bc05 ! 2: 1dcdba4b1d repack: add --filter= option\n @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix\n + write_promisor_file_1(line.buf);\n item->util = populate_pack_exts(item->string);\n }\n - fclose(out);\n + strbuf_release(&line);\n \n ## t/t7700-repack.sh ##\n @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavailable' '\n @@ t/t7700-repack.sh: test_expect_success 'auto-bitmaps do not complain if unavaila\n + git clone --bare --no-local server client &&\n + git -C client config remote.origin.promisor true &&\n + git -C client rev-list --objects --all --missing=print >objects &&\n -+ test $(grep \"^?\" objects | wc -l) = 0 &&\n ++ test $(grep -c \"^?\" objects) = 0 &&\n + git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n + git -C client rev-list --objects --all --missing=print >objects &&\n -+ test $(grep \"^?\" objects | wc -l) = 1\n ++ test $(grep -c \"^?\" objects) = 1\n +'\n +\n objdir=.git/objects\n-: ---------- > 3: 6bb98b4b00 gc: add gc.repackFilter config option\n\n\nChristian Couder (3):\n pack-objects: allow --filter without --stdout\n repack: add --filter= option\n gc: add gc.repackFilter config option\n\n Documentation/config/gc.txt | 9 +++++++++\n Documentation/git-repack.txt | 8 ++++++++\n builtin/gc.c | 6 ++++++\n builtin/pack-objects.c | 8 ++------\n builtin/repack.c | 28 +++++++++++++++++++++-------\n t/t6500-gc.sh | 19 +++++++++++++++++++\n t/t7700-repack.sh | 15 +++++++++++++++\n 7 files changed, 80 insertions(+), 13 deletions(-)\n\n-- \n2.39.0.59.g395bcb85bc.dirty\n\n"},{"id":"469413","messageId":"20221221040446.2860985-2-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221221040446.2860985-1-christian.couder@gmail.com","subject":"[PATCH v4 1/3] pack-objects: allow --filter without --stdout","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-12-21T04:04:44Z","receivedAt":"2022-12-21T04:05:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\n9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)\ntaught pack-objects to use --filter, but required the use of\n--stdout since a partial clone mechanism was not yet in place to\nhandle missing objects. Since then, changes like 9e27beaa23\n(promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)\nand others added support to dynamically fetch objects that were missing.\n\nRemove the --stdout requirement so that in a follow-up commit, repack\ncan pass --filter to pack-objects to omit certain objects from the\nresulting packfile.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n builtin/pack-objects.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 2193f80b89..aa0b13d015 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4371,12 +4371,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n \t\tunpack_unreachable_expiration = 0;\n \n-\tif (filter_options.choice) {\n-\t\tif (!pack_to_stdout)\n-\t\t\tdie(_(\"cannot use --filter without --stdout\"));\n-\t\tif (stdin_packs)\n-\t\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n-\t}\n+\tif (stdin_packs && filter_options.choice)\n+\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n \n \tif (stdin_packs && use_internal_rev_list)\n \t\tdie(_(\"cannot use internal rev list with --stdin-packs\"));\n-- \n2.39.0.59.g395bcb85bc.dirty\n\n"},{"id":"469414","messageId":"20221221040446.2860985-3-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221221040446.2860985-1-christian.couder@gmail.com","subject":"[PATCH v4 2/3] repack: add --filter= option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-12-21T04:04:45Z","receivedAt":"2022-12-21T04:05:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nAfter cloning with --filter=, for example to avoid\ngetting unneeded large files on a user machine, it's possible\nthat some of these large files still get fetched for some reasons\n(like checking out old branches) over time.\n\nIn this case the repo size could grow too much for no good reason\nand `git repack --filter=` would be useful to remove\nthe unneeded large files.\n\nThis command could be dangerous to use though, as it might remove\nlocal objects that haven't been pushed which would lose data and\ncorrupt the repo. On a server, this command could also corrupt a\nrepo unless ALL the removed objects aren't already available in\nanother remote that clients can access.\n\nTo mitigate that risk, we check that a promisor remote has at\nleast been configured.\n\nSigned-off-by: John Cai \nSigned-off-by: Christian Couder \n---\n Documentation/git-repack.txt | 8 ++++++++\n builtin/repack.c | 28 +++++++++++++++++++++-------\n t/t7700-repack.sh | 15 +++++++++++++++\n 3 files changed, 44 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 4017157949..2539ee0a02 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -143,6 +143,14 @@ depth is 4095.\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n \n+--filter=::\n+\tOmits certain objects (usually blobs) from the resulting\n+\tpackfile. WARNING: this could easily corrupt the current repo\n+\tand lose data if ANY of the omitted objects hasn't been already\n+\tpushed to a remote. Be very careful about objects that might\n+\thave been created locally! See linkgit:git-rev-list[1] for valid\n+\t`` forms.\n+\n -b::\n --write-bitmap-index::\n \tWrite a reachability bitmap index as part of the repack. This\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex c1402ad038..8e5ac9c171 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -49,6 +49,7 @@ struct pack_objects_args {\n \tconst char *depth;\n \tconst char *threads;\n \tconst char *max_pack_size;\n+\tconst char *filter;\n \tint no_reuse_delta;\n \tint no_reuse_object;\n \tint quiet;\n@@ -163,6 +164,8 @@ static void prepare_pack_objects(struct child_process *cmd,\n \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n \tif (args->max_pack_size)\n \t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\tif (args->filter)\n+\t\tstrvec_pushf(&cmd->args, \"--filter=%s\", args->filter);\n \tif (args->no_reuse_delta)\n \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n \tif (args->no_reuse_object)\n@@ -234,6 +237,13 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n \treturn data;\n }\n \n+static void write_promisor_file_1(char *p)\n+{\n+\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp, p);\n+\twrite_promisor_file(promisor_name, NULL, 0);\n+\tfree(promisor_name);\n+}\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t struct string_list *names)\n {\n@@ -265,7 +275,6 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tstruct string_list_item *item;\n-\t\tchar *promisor_name;\n \n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n@@ -282,13 +291,8 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t * concatenate the contents of all .promisor files instead of\n \t\t * just creating a new empty file.\n \t\t */\n-\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n-\t\t\t\t\t line.buf);\n-\t\twrite_promisor_file(promisor_name, NULL, 0);\n-\n+\t\twrite_promisor_file_1(line.buf);\n \t\titem->util = populate_pack_exts(item->string);\n-\n-\t\tfree(promisor_name);\n \t}\n \tfclose(out);\n \tif (finish_command(&cmd))\n@@ -800,6 +804,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_STRING(0, \"filter\", &po_args.filter, N_(\"args\"),\n+\t\t\t\tN_(\"object filtering\")),\n \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n@@ -834,6 +840,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n \t}\n \n+\tif (po_args.filter && !has_promisor_remote())\n+\t\tdie(\"a promisor remote must be setup\\n\"\n+\t\t \"Also please push all the objects \"\n+\t\t \"that might be filtered to that remote!\\n\"\n+\t\t \"Otherwise they will be lost!\");\n+\n \tif (write_bitmaps < 0) {\n \t\tif (!write_midx &&\n \t\t (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n@@ -971,6 +983,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n \t\titem = string_list_append(&names, line.buf);\n+\t\tif (po_args.filter)\n+\t\t\twrite_promisor_file_1(line.buf);\n \t\titem->util = populate_pack_exts(item->string);\n \t}\n \tstrbuf_release(&line);\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 4aabe98139..3a6ad9f623 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -253,6 +253,21 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'repacking with a filter works' '\n+\ttest_when_finished \"rm -rf server client\" &&\n+\ttest_create_repo server &&\n+\tgit -C server config uploadpack.allowFilter true &&\n+\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n+\ttest_commit -C server 1 &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep -c \"^?\" objects) = 0 &&\n+\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep -c \"^?\" objects) = 1\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \n2.39.0.59.g395bcb85bc.dirty\n\n"},{"id":"469415","messageId":"20221221040446.2860985-4-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221221040446.2860985-1-christian.couder@gmail.com","subject":"[PATCH v4 3/3] gc: add gc.repackFilter config option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-12-21T04:04:46Z","receivedAt":"2022-12-21T04:05:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"A previous commit has implemented `git repack --filter=` to\nallow users to remove objects that are available on a promisor remote\nbut that they don't want locally.\n\nUsers might want to perform such a cleanup regularly at the same time as\nthey perform other repacks and cleanups, so as part of `git gc`.\n\nLet's allow them to configure a for that purpose using a\nnew gc.repackFilter config option.\n\nNow when `git gc` will perform a repack with a configured\nthrough this option and not empty, the repack process will be passed a\ncorresponding `--filter=` argument.\n\nSigned-off-by: Christian Couder \n---\n Documentation/config/gc.txt | 9 +++++++++\n builtin/gc.c | 6 ++++++\n t/t6500-gc.sh | 19 +++++++++++++++++++\n 3 files changed, 34 insertions(+)\n\ndiff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\nindex 38fea076a2..9359136f14 100644\n--- a/Documentation/config/gc.txt\n+++ b/Documentation/config/gc.txt\n@@ -130,6 +130,15 @@ or rebase occurring. Since these changes are not part of the current\n project most users will want to expire them sooner, which is why the\n default is more aggressive than `gc.reflogExpire`.\n \n+gc.repackFilter::\n+\tWhen repacking, use the specified filter to omit certain\n+\tobjects from the resulting packfile. WARNING: this could\n+\teasily corrupt the current repo and lose data if ANY of the\n+\tomitted objects hasn't been already pushed to a remote. Be\n+\tvery careful about objects that might have been created\n+\tlocally! See the `--filter=` option of\n+\tlinkgit:git-repack[1].\n+\n gc.rerereResolved::\n \tRecords of conflicted merge you resolved earlier are\n \tkept for this many days when 'git rerere gc' is run.\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 02455fdcd7..bf28619723 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -52,6 +52,7 @@ static timestamp_t gc_log_expire_time;\n static const char *gc_log_expire = \"1.day.ago\";\n static const char *prune_expire = \"2.weeks.ago\";\n static const char *prune_worktrees_expire = \"3.months.ago\";\n+static char *repack_filter;\n static unsigned long big_pack_threshold;\n static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;\n \n@@ -161,6 +162,8 @@ static void gc_config(void)\n \tgit_config_get_ulong(\"gc.bigpackthreshold\", &big_pack_threshold);\n \tgit_config_get_ulong(\"pack.deltacachesize\", &max_delta_cache_size);\n \n+\tgit_config_get_string(\"gc.repackfilter\", &repack_filter);\n+\n \tgit_config(git_default_config, NULL);\n }\n \n@@ -346,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack)\n \n \tif (keep_pack)\n \t\tfor_each_string_list(keep_pack, keep_one_pack, NULL);\n+\n+\tif (repack_filter && *repack_filter)\n+\t\tstrvec_pushf(&repack, \"--filter=%s\", repack_filter);\n }\n \n static void add_repack_incremental_option(void)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex d9acb63951..b1492b521a 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -56,6 +56,7 @@ test_expect_success 'gc -h with invalid configuration' '\n '\n \n test_expect_success 'gc is not aborted due to a stale symref' '\n+\ttest_when_finished \"rm -rf remote client\" &&\n \tgit init remote &&\n \t(\n \t\tcd remote &&\n@@ -202,6 +203,24 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n '\n \n+test_expect_success 'gc.repackFilter launches repack with a filter' '\n+\ttest_when_finished \"rm -rf server client\" &&\n+\ttest_create_repo server &&\n+\tgit -C server config uploadpack.allowFilter true &&\n+\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n+\ttest_commit -C server 1 &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep -c \"^?\" objects) = 0 &&\n+\n+\tGIT_TRACE=$(pwd)/trace.out git -C client -c gc.repackFilter=blob:none -c repack.writeBitmaps=false -c gc.pruneExpire=now gc &&\n+\n+\tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\ttest $(grep -c \"^?\" objects) = 1\n+'\n+\n prepare_cruft_history () {\n \ttest_commit base &&\n \n-- \n2.39.0.59.g395bcb85bc.dirty\n\n"},{"id":"469785","messageId":"Y7WTrHO1bWNFCRrp@ncase","threadId":"58609","inReplyTo":"20221221040446.2860985-2-christian.couder@gmail.com","subject":"Re: [PATCH v4 1/3] pack-objects: allow --filter without --stdout","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-01-04T14:56:44Z","receivedAt":"2023-01-04T14:57:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 21, 2022 at 05:04:44AM +0100, Christian Couder wrote:\n> From: Christian Couder \n> \n> 9535ce7337 (pack-objects: add list-objects filtering, 2017-11-21)\n> taught pack-objects to use --filter, but required the use of\n> --stdout since a partial clone mechanism was not yet in place to\n> handle missing objects. Since then, changes like 9e27beaa23\n> (promisor-remote: implement promisor_remote_get_direct(), 2019-06-25)\n> and others added support to dynamically fetch objects that were missing.\n> \n> Remove the --stdout requirement so that in a follow-up commit, repack\n> can pass --filter to pack-objects to omit certain objects from the\n> resulting packfile.\n> \n> Signed-off-by: John Cai \n> Signed-off-by: Christian Couder \n> ---\n> builtin/pack-objects.c | 8 ++------\n> 1 file changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 2193f80b89..aa0b13d015 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -4371,12 +4371,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> \tif (!rev_list_all || !rev_list_reflog || !rev_list_index)\n> \t\tunpack_unreachable_expiration = 0;\n> \n> -\tif (filter_options.choice) {\n> -\t\tif (!pack_to_stdout)\n> -\t\t\tdie(_(\"cannot use --filter without --stdout\"));\n> -\t\tif (stdin_packs)\n> -\t\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n> -\t}\n> +\tif (stdin_packs && filter_options.choice)\n> +\t\tdie(_(\"cannot use --filter with --stdin-packs\"));\n> \n> \tif (stdin_packs && use_internal_rev_list)\n> \t\tdie(_(\"cannot use internal rev list with --stdin-packs\"));\n\nCan we add a test to exercise this now-allowed combination of options?\n\nPatrick\n"},{"id":"469786","messageId":"Y7WTv19aqiFCU8au@ncase","threadId":"58609","inReplyTo":"20221221040446.2860985-4-christian.couder@gmail.com","subject":"Re: [PATCH v4 3/3] gc: add gc.repackFilter config option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-01-04T14:57:03Z","receivedAt":"2023-01-04T14:57:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 21, 2022 at 05:04:46AM +0100, Christian Couder wrote:\n> A previous commit has implemented `git repack --filter=` to\n> allow users to remove objects that are available on a promisor remote\n> but that they don't want locally.\n> \n> Users might want to perform such a cleanup regularly at the same time as\n> they perform other repacks and cleanups, so as part of `git gc`.\n> \n> Let's allow them to configure a for that purpose using a\n> new gc.repackFilter config option.\n> \n> Now when `git gc` will perform a repack with a configured\n> through this option and not empty, the repack process will be passed a\n> corresponding `--filter=` argument.\n> \n> Signed-off-by: Christian Couder \n> ---\n> Documentation/config/gc.txt | 9 +++++++++\n> builtin/gc.c | 6 ++++++\n> t/t6500-gc.sh | 19 +++++++++++++++++++\n> 3 files changed, 34 insertions(+)\n> \n> diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt\n> index 38fea076a2..9359136f14 100644\n> --- a/Documentation/config/gc.txt\n> +++ b/Documentation/config/gc.txt\n> @@ -130,6 +130,15 @@ or rebase occurring. Since these changes are not part of the current\n> project most users will want to expire them sooner, which is why the\n> default is more aggressive than `gc.reflogExpire`.\n> \n> +gc.repackFilter::\n> +\tWhen repacking, use the specified filter to omit certain\n> +\tobjects from the resulting packfile. WARNING: this could\n> +\teasily corrupt the current repo and lose data if ANY of the\n> +\tomitted objects hasn't been already pushed to a remote. Be\n> +\tvery careful about objects that might have been created\n> +\tlocally! See the `--filter=` option of\n> +\tlinkgit:git-repack[1].\n\nThis limitation means that no user can ever configure this unless they\nalso set `gc.auto=0`, or otherwise Git might periodically delete their\nlocal objects without any way to restore them.\n\nI see though that this might be a useful thing to have for partial clone\nrepositories: right now they only ever grow in size, so it would be nice\nif Git could gracefully cull that size every now and then. But for this\nto be automatic I'd expect a few safeguards:\n\n - Given a remote with a partial clone filter we only ever delete\n objects that seem to exist on the remote. This might for example\n be determined via a graph-walk of the remote's references. This\n protects against deleting objects that only exist locally.\n\n - Objects that are referenced by any of the currently checked-out\n HEADs should not get puned. This protects against deleting objects\n that you'd have to re-download anyway.\n\n - It would be great to have a grace period after which objects get\n pruned. Given that we have no access times for packed objects\n though this doesn't seem that easy to implement though. Still,\n this would protect the user from objects getting deleted when they\n frequently switch between different commits.\n\nFrom my point of view we should not expose a high-level interface around\ngit-gc(1) at this point in time because of these missing safeguards.\nOtherwise it is only a footgun waiting to go off.\n\nUntil then I don't see much of a problem with exposing the low-level\ninterface via git-repack(1) though.\n\nPatrick\n\n> gc.rerereResolved::\n> \tRecords of conflicted merge you resolved earlier are\n> \tkept for this many days when 'git rerere gc' is run.\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index 02455fdcd7..bf28619723 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -52,6 +52,7 @@ static timestamp_t gc_log_expire_time;\n> static const char *gc_log_expire = \"1.day.ago\";\n> static const char *prune_expire = \"2.weeks.ago\";\n> static const char *prune_worktrees_expire = \"3.months.ago\";\n> +static char *repack_filter;\n> static unsigned long big_pack_threshold;\n> static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;\n> \n> @@ -161,6 +162,8 @@ static void gc_config(void)\n> \tgit_config_get_ulong(\"gc.bigpackthreshold\", &big_pack_threshold);\n> \tgit_config_get_ulong(\"pack.deltacachesize\", &max_delta_cache_size);\n> \n> +\tgit_config_get_string(\"gc.repackfilter\", &repack_filter);\n> +\n> \tgit_config(git_default_config, NULL);\n> }\n> \n> @@ -346,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack)\n> \n> \tif (keep_pack)\n> \t\tfor_each_string_list(keep_pack, keep_one_pack, NULL);\n> +\n> +\tif (repack_filter && *repack_filter)\n> +\t\tstrvec_pushf(&repack, \"--filter=%s\", repack_filter);\n> }\n> \n> static void add_repack_incremental_option(void)\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index d9acb63951..b1492b521a 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -56,6 +56,7 @@ test_expect_success 'gc -h with invalid configuration' '\n> '\n> \n> test_expect_success 'gc is not aborted due to a stale symref' '\n> +\ttest_when_finished \"rm -rf remote client\" &&\n> \tgit init remote &&\n> \t(\n> \t\tcd remote &&\n> @@ -202,6 +203,24 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip \"e\n> \tgrep -E \"^trace: (built-in|exec|run_command): git reflog expire --\" trace.out\n> '\n> \n> +test_expect_success 'gc.repackFilter launches repack with a filter' '\n> +\ttest_when_finished \"rm -rf server client\" &&\n> +\ttest_create_repo server &&\n> +\tgit -C server config uploadpack.allowFilter true &&\n> +\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n> +\ttest_commit -C server 1 &&\n> +\tgit clone --bare --no-local server client &&\n> +\tgit -C client config remote.origin.promisor true &&\n> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n> +\ttest $(grep -c \"^?\" objects) = 0 &&\n> +\n> +\tGIT_TRACE=$(pwd)/trace.out git -C client -c gc.repackFilter=blob:none -c repack.writeBitmaps=false -c gc.pruneExpire=now gc &&\n> +\n> +\tgrep -E \"^trace: (built-in|exec|run_command): git repack .* --filter=blob:none ?.*\" trace.out &&\n> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n> +\ttest $(grep -c \"^?\" objects) = 1\n> +'\n> +\n> prepare_cruft_history () {\n> \ttest_commit base &&\n> \n> -- \n> 2.39.0.59.g395bcb85bc.dirty\n> \n"},{"id":"469787","messageId":"Y7WTuQvoHEWRlEA4@ncase","threadId":"58609","inReplyTo":"20221221040446.2860985-3-christian.couder@gmail.com","subject":"Re: [PATCH v4 2/3] repack: add --filter= option","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-01-04T14:56:57Z","receivedAt":"2023-01-04T14:57:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 21, 2022 at 05:04:45AM +0100, Christian Couder wrote:\n> From: Christian Couder \n> \n> After cloning with --filter=, for example to avoid\n> getting unneeded large files on a user machine, it's possible\n> that some of these large files still get fetched for some reasons\n> (like checking out old branches) over time.\n> \n> In this case the repo size could grow too much for no good reason\n> and `git repack --filter=` would be useful to remove\n> the unneeded large files.\n> \n> This command could be dangerous to use though, as it might remove\n> local objects that haven't been pushed which would lose data and\n> corrupt the repo. On a server, this command could also corrupt a\n> repo unless ALL the removed objects aren't already available in\n> another remote that clients can access.\n> \n> To mitigate that risk, we check that a promisor remote has at\n> least been configured.\n\nWhile this is a nice safeguard, I wonder whether it is sufficient.\nSuppose you for example have a non-bare repository that already has\nblobs checked out that would become removed by the filtering repack --\ndoes Git handle this situation gracefully?\n\nA quick check seems to indicate that it does. But not quite as well as\nI'd have hoped: when I switch to a detached HEAD with an arbitrary\ncommit and then execute `git repack --filter=blob:none` then it also\nremoves blobs that are referenced by the currently checked-out commit.\nThis may or may not be what the user is asking for, but I'd rather lean\ntowards this behaviour being surprising.\n\nPatrick\n\n> Signed-off-by: John Cai \n> Signed-off-by: Christian Couder \n> ---\n> Documentation/git-repack.txt | 8 ++++++++\n> builtin/repack.c | 28 +++++++++++++++++++++-------\n> t/t7700-repack.sh | 15 +++++++++++++++\n> 3 files changed, 44 insertions(+), 7 deletions(-)\n> \n> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\n> index 4017157949..2539ee0a02 100644\n> --- a/Documentation/git-repack.txt\n> +++ b/Documentation/git-repack.txt\n> @@ -143,6 +143,14 @@ depth is 4095.\n> \ta larger and slower repository; see the discussion in\n> \t`pack.packSizeLimit`.\n> \n> +--filter=::\n> +\tOmits certain objects (usually blobs) from the resulting\n> +\tpackfile. WARNING: this could easily corrupt the current repo\n> +\tand lose data if ANY of the omitted objects hasn't been already\n> +\tpushed to a remote. Be very careful about objects that might\n> +\thave been created locally! See linkgit:git-rev-list[1] for valid\n> +\t`` forms.\n> +\n> -b::\n> --write-bitmap-index::\n> \tWrite a reachability bitmap index as part of the repack. This\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index c1402ad038..8e5ac9c171 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -49,6 +49,7 @@ struct pack_objects_args {\n> \tconst char *depth;\n> \tconst char *threads;\n> \tconst char *max_pack_size;\n> +\tconst char *filter;\n> \tint no_reuse_delta;\n> \tint no_reuse_object;\n> \tint quiet;\n> @@ -163,6 +164,8 @@ static void prepare_pack_objects(struct child_process *cmd,\n> \t\tstrvec_pushf(&cmd->args, \"--threads=%s\", args->threads);\n> \tif (args->max_pack_size)\n> \t\tstrvec_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n> +\tif (args->filter)\n> +\t\tstrvec_pushf(&cmd->args, \"--filter=%s\", args->filter);\n> \tif (args->no_reuse_delta)\n> \t\tstrvec_pushf(&cmd->args, \"--no-reuse-delta\");\n> \tif (args->no_reuse_object)\n> @@ -234,6 +237,13 @@ static struct generated_pack_data *populate_pack_exts(const char *name)\n> \treturn data;\n> }\n> \n> +static void write_promisor_file_1(char *p)\n> +{\n> +\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp, p);\n> +\twrite_promisor_file(promisor_name, NULL, 0);\n> +\tfree(promisor_name);\n> +}\n> +\n> static void repack_promisor_objects(const struct pack_objects_args *args,\n> \t\t\t\t struct string_list *names)\n> {\n> @@ -265,7 +275,6 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> \tout = xfdopen(cmd.out, \"r\");\n> \twhile (strbuf_getline_lf(&line, out) != EOF) {\n> \t\tstruct string_list_item *item;\n> -\t\tchar *promisor_name;\n> \n> \t\tif (line.len != the_hash_algo->hexsz)\n> \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n> @@ -282,13 +291,8 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n> \t\t * concatenate the contents of all .promisor files instead of\n> \t\t * just creating a new empty file.\n> \t\t */\n> -\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n> -\t\t\t\t\t line.buf);\n> -\t\twrite_promisor_file(promisor_name, NULL, 0);\n> -\n> +\t\twrite_promisor_file_1(line.buf);\n> \t\titem->util = populate_pack_exts(item->string);\n> -\n> -\t\tfree(promisor_name);\n> \t}\n> \tfclose(out);\n> \tif (finish_command(&cmd))\n> @@ -800,6 +804,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\t\tN_(\"limits the maximum number of threads\")),\n> \t\tOPT_STRING(0, \"max-pack-size\", &po_args.max_pack_size, N_(\"bytes\"),\n> \t\t\t\tN_(\"maximum size of each packfile\")),\n> +\t\tOPT_STRING(0, \"filter\", &po_args.filter, N_(\"args\"),\n> +\t\t\t\tN_(\"object filtering\")),\n> \t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n> \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n> \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n> @@ -834,6 +840,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--cruft\", \"-k\");\n> \t}\n> \n> +\tif (po_args.filter && !has_promisor_remote())\n> +\t\tdie(\"a promisor remote must be setup\\n\"\n> +\t\t \"Also please push all the objects \"\n> +\t\t \"that might be filtered to that remote!\\n\"\n> +\t\t \"Otherwise they will be lost!\");\n> +\n\n> \tif (write_bitmaps < 0) {\n> \t\tif (!write_midx &&\n> \t\t (!(pack_everything & ALL_INTO_ONE) || !is_bare_repository()))\n> @@ -971,6 +983,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\tif (line.len != the_hash_algo->hexsz)\n> \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n> \t\titem = string_list_append(&names, line.buf);\n> +\t\tif (po_args.filter)\n> +\t\t\twrite_promisor_file_1(line.buf);\n> \t\titem->util = populate_pack_exts(item->string);\n> \t}\n> \tstrbuf_release(&line);\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 4aabe98139..3a6ad9f623 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -253,6 +253,21 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success 'repacking with a filter works' '\n> +\ttest_when_finished \"rm -rf server client\" &&\n> +\ttest_create_repo server &&\n> +\tgit -C server config uploadpack.allowFilter true &&\n> +\tgit -C server config uploadpack.allowAnySHA1InWant true &&\n> +\ttest_commit -C server 1 &&\n> +\tgit clone --bare --no-local server client &&\n> +\tgit -C client config remote.origin.promisor true &&\n> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n> +\ttest $(grep -c \"^?\" objects) = 0 &&\n> +\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n> +\ttest $(grep -c \"^?\" objects) = 1\n> +'\n> +\n> objdir=.git/objects\n> midx=$objdir/pack/multi-pack-index\n> \n> -- \n> 2.39.0.59.g395bcb85bc.dirty\n> \n"},{"id":"469802","messageId":"xmqqa62xu5dj.fsf@gitster.g","threadId":"58609","inReplyTo":"Y7WTuQvoHEWRlEA4@ncase","subject":"Re: [PATCH v4 2/3] repack: add --filter= option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-05T01:39:20Z","receivedAt":"2023-01-05T01:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> While this is a nice safeguard, I wonder whether it is sufficient.\n> Suppose you for example have a non-bare repository that already has\n> blobs checked out that would become removed by the filtering repack --\n> does Git handle this situation gracefully?\n>\n> A quick check seems to indicate that it does. But not quite as well as\n> I'd have hoped: when I switch to a detached HEAD with an arbitrary\n> commit and then execute `git repack --filter=blob:none` then it also\n> removes blobs that are referenced by the currently checked-out commit.\n> This may or may not be what the user is asking for, but I'd rather lean\n> towards this behaviour being surprising.\n\nHmph, the user asked not to have blobs that came from remote locally\nand instead refetch them from the promisor on-demand, so I would\nexpect some pruning to happen (I am not a lazy-clone user, though).\n\nAs long as we do the pruning sensibly, that is.\n\nUnless you are always following somebody else without doing any work\non your own, you are likely to have objects that exist only locally\nand nowhere else. It would be unexpected and surprising, if we lost\nthem only because they are of type 'blob' and because there is a\npromisor remote configured.\n\nEven if that is documented, that would be an unacceptable foot-gun\nmisfeature. It is not just a local repository corruption that can\nbe recovered by cloning from elsewhere. We are looking at lost\nwork that cannot be recovered.\n\nI wonder if this topic can be salvaged by making it less aggressive\nin pruning, perhaps by traversing from the tips of remote-tracking\nbranches of the promisor remote to identify which blobs can safely\nbe pruned (by definition, promisor remote cannot lose objects that\nit once published, or its cloners will immediately have corrupt\nrepositories). That may turn this from a misfeature into a feature.\n\nThanks.\n"},{"id":"494810","messageId":"20240515132543.851987-2-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20240515132543.851987-1-christian.couder@gmail.com","subject":"[PATCH v2 1/3] rev-list: refactor --missing=","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-15T13:25:41Z","receivedAt":"2024-05-15T13:25:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Both `git rev-list` and `git pack-objects` support a\n`--missing=` feature, but they currently don't share\nany code for that.\n\nRefactor the support for `--missing=` in\n\"builtin/rev-list.c\" into new \"missing.{c,h}\" files. In a following\ncommit, that refactored code will be used in \"builtin/pack-objects.c\"\ntoo.\n\nIn yet a following commit, we are going to add support for a similar\n'missing-action' feature to another command, and we are also going to\nreuse code from the new \"missing.{c,h}\" files.\n\nAs `enum missing_action` and parse_missing_action_value() are moved to\n\"missing.{c,h}\", we need to modify the latter a bit, so that it stops\nupdating any global variable, but instead returns the parsed value or\n-1 on error.\n\nSigned-off-by: Christian Couder \n---\n Makefile | 1 +\n builtin/rev-list.c | 43 ++++++++-----------------------------------\n missing.c | 20 ++++++++++++++++++++\n missing.h | 17 +++++++++++++++++\n 4 files changed, 46 insertions(+), 35 deletions(-)\n create mode 100644 missing.c\n create mode 100644 missing.h\n\ndiff --git a/Makefile b/Makefile\nindex 0285db5630..e0ddcc2cbd 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1062,6 +1062,7 @@ LIB_OBJS += merge-recursive.o\n LIB_OBJS += merge.o\n LIB_OBJS += midx.o\n LIB_OBJS += midx-write.o\n+LIB_OBJS += missing.o\n LIB_OBJS += name-hash.o\n LIB_OBJS += negotiator/default.o\n LIB_OBJS += negotiator/noop.o\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 77803727e0..40aa770c47 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -20,6 +20,7 @@\n #include \"reflog-walk.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n+#include \"missing.h\"\n \n static const char rev_list_usage[] =\n \"git rev-list [] ... [--] [...]\\n\"\n@@ -71,12 +72,6 @@ static struct oidset omitted_objects;\n static int arg_print_omitted; /* print objects omitted by filter */\n \n static struct oidset missing_objects;\n-enum missing_action {\n-\tMA_ERROR = 0, /* fail if any missing objects are encountered */\n-\tMA_ALLOW_ANY, /* silently allow ALL missing objects */\n-\tMA_PRINT, /* print ALL missing objects in special section */\n-\tMA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */\n-};\n static enum missing_action arg_missing_action;\n \n /* display only the oid of each object encountered */\n@@ -392,34 +387,6 @@ static void print_disk_usage(off_t size)\n \tstrbuf_release(&sb);\n }\n \n-static inline int parse_missing_action_value(const char *value)\n-{\n-\tif (!strcmp(value, \"error\")) {\n-\t\targ_missing_action = MA_ERROR;\n-\t\treturn 1;\n-\t}\n-\n-\tif (!strcmp(value, \"allow-any\")) {\n-\t\targ_missing_action = MA_ALLOW_ANY;\n-\t\tfetch_if_missing = 0;\n-\t\treturn 1;\n-\t}\n-\n-\tif (!strcmp(value, \"print\")) {\n-\t\targ_missing_action = MA_PRINT;\n-\t\tfetch_if_missing = 0;\n-\t\treturn 1;\n-\t}\n-\n-\tif (!strcmp(value, \"allow-promisor\")) {\n-\t\targ_missing_action = MA_ALLOW_PROMISOR;\n-\t\tfetch_if_missing = 0;\n-\t\treturn 1;\n-\t}\n-\n-\treturn 0;\n-}\n-\n static int try_bitmap_count(struct rev_info *revs,\n \t\t\t int filter_provided_objects)\n {\n@@ -569,10 +536,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \t\tif (skip_prefix(arg, \"--missing=\", &arg)) {\n+\t\t\tint res;\n \t\t\tif (revs.exclude_promisor_objects)\n \t\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--exclude-promisor-objects\", \"--missing\");\n-\t\t\tif (parse_missing_action_value(arg))\n+\t\t\tres = parse_missing_action_value(arg);\n+\t\t\tif (res >= 0) {\n+\t\t\t\tif (res != MA_ERROR)\n+\t\t\t\t\tfetch_if_missing = 0;\n+\t\t\t\targ_missing_action = res;\n \t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/missing.c b/missing.c\nnew file mode 100644\nindex 0000000000..ce3cf734a8\n--- /dev/null\n+++ b/missing.c\n@@ -0,0 +1,20 @@\n+#include \"git-compat-util.h\"\n+#include \"missing.h\"\n+#include \"object-file.h\"\n+\n+int parse_missing_action_value(const char *value)\n+{\n+\tif (!strcmp(value, \"error\"))\n+\t\treturn MA_ERROR;\n+\n+\tif (!strcmp(value, \"allow-any\"))\n+\t\treturn MA_ALLOW_ANY;\n+\n+\tif (!strcmp(value, \"print\"))\n+\t\treturn MA_PRINT;\n+\n+\tif (!strcmp(value, \"allow-promisor\"))\n+\t\treturn MA_ALLOW_PROMISOR;\n+\n+\treturn -1;\n+}\ndiff --git a/missing.h b/missing.h\nnew file mode 100644\nindex 0000000000..1e378d6215\n--- /dev/null\n+++ b/missing.h\n@@ -0,0 +1,17 @@\n+#ifndef MISSING_H\n+#define MISSING_H\n+\n+enum missing_action {\n+\tMA_ERROR = 0, /* fail if any missing objects are encountered */\n+\tMA_ALLOW_ANY, /* silently allow ALL missing objects */\n+\tMA_PRINT, /* print ALL missing objects in special section */\n+\tMA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */\n+};\n+\n+/*\n+ Return an `enum missing_action` in case parsing is successful or -1\n+ if parsing failed.\n+*/\n+int parse_missing_action_value(const char *value);\n+\n+#endif /* MISSING_H */\n-- \n2.45.1.148.g0f5efb064b\n\n"},{"id":"494813","messageId":"20240515132543.851987-1-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20221012135114.294680-1-christian.couder@gmail.com","subject":"[PATCH v2 0/3] upload-pack: support a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-15T13:25:40Z","receivedAt":"2024-05-15T13:25:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"`git pack-objects` already supports a `--missing=`\noption, so that it can avoid erroring out if some objects aren't\navailable.\n\nIt is interesting to have `git upload-pack` support a similar way to\navoid sending some objects in case they aren't available on the\nserver.\n\nFor example, in case both the server and the client are using a\nseparate promisor remote that contain some objects, it can be better\nif the server doesn't try to send such objects back to the client, but\ninstead let the client get those objects separately from the promisor\nremote. (The client needs to have the separate promisor remote\nconfigured, for that to work.)\n\nThis could work better if there was something, like perhaps a\ncapability, for the client to tell the server something like:\n\n \"I know how to fetch missing objects from this and that\n promisor remotes, so if you choose to, you may omit objects\n that you know are available from these promisor remotes when\n sending objects to me.\"\n\nBut that capability could be added later as other similar capabilities\nin this area could be very useful. For example in case of a client\ncloning, something like the following is currently needed:\n\n GIT_NO_LAZY_FETCH=0 git clone\n -c remote.my_promisor.promisor=true \\\n -c remote.my_promisor.fetch=\"+refs/heads/*:refs/remotes/my_promisor/*\" \\\n -c remote.my_promisor.url= \\\n --filter=\"blob:limit=5k\" server\n\nBut it would be nice if there was a capability for the client to say\nthat it would like the server to give it information about the\npromisor that it could use, so that the user doesn't have to pass all\nthe \"remote.my_promisor.XXX\" config options on the command like. (It\nwould then be a bit similar to the bundle-uri feature where all the\nbundle related information comes from the server.)\n\nAnother example use of this feature could be a server where some\nobjects have been corrupted or deleted. It could still be useful for\nclients who could get those objects from another source, like perhaps\na different client, to be able to fetch or clone from the server.\n\nThe fact that the new `uploadpack.missingAction` configuration\nvariable has to be set to a non default value on the server means that\nregular client users cannot hurt themselves with this feature.\n\nAs `git rev-list` also supports a `--missing=` option,\nthe first 2 patches in this series are about refactoring related code\nfrom both `git rev-list` and `git pack-objects` into new\n\"missing.{c,h}\" files. Patch 3/3 then adds a new\n`uploadpack.missingAction` configuration variable.\n\nThe changes since v1 are the following:\n\n - In patch 1/3 the refactored parse_missing_action_value() function\n doesn't change the fetch_if_missing global variable anymore. The\n clients of this function change it by themselves instead.\n\n - In patch 1/3 a minor typo was also fixed.\n\n - Old patch 2/4 that added a 'int print_ok' argument to\n parse_missing_action_value() was removed, and now callers deal\n with only the values they recognise.\n\nThanks to Junio for his review and for suggesting of the above\nchanges.\n\nChristian Couder (3):\n rev-list: refactor --missing=\n pack-objects: use the missing action API\n upload-pack: allow configuring a missing-action\n\n Documentation/config/uploadpack.txt | 9 ++\n Makefile | 1 +\n builtin/pack-objects.c | 48 ++++++-----\n builtin/rev-list.c | 43 ++--------\n missing.c | 36 ++++++++\n missing.h | 19 +++++\n t/t5706-upload-pack-missing.sh | 125 ++++++++++++++++++++++++++++\n upload-pack.c | 19 +++++\n 8 files changed, 242 insertions(+), 58 deletions(-)\n create mode 100644 missing.c\n create mode 100644 missing.h\n create mode 100755 t/t5706-upload-pack-missing.sh\n\n-- \n2.45.1.148.g0f5efb064b\n\n"},{"id":"494811","messageId":"20240515132543.851987-3-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20240515132543.851987-1-christian.couder@gmail.com","subject":"[PATCH v2 2/3] pack-objects: use the missing action API","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-15T13:25:42Z","receivedAt":"2024-05-15T13:26:01Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Both `git rev-list` and `git pack-objects` support a\n`--missing=` option. Previous commits created an API\nin \"missing.{c,h}\" to help supporting that option, but only\n`git rev-list` has been using that API so far.\n\nLet's make `git pack-objects` use it too.\n\nThis involves creating a new show_object_fn_from_action() function to\nset the `fn_show_object` variable independently from parsing the\nmissing action, which is now performed by the\nparse_missing_action_value() API function.\n\nSigned-off-by: Christian Couder \n---\n builtin/pack-objects.c | 48 ++++++++++++++++++++++--------------------\n 1 file changed, 25 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex baf0090fc8..55d08c686d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -39,6 +39,7 @@\n #include \"promisor-remote.h\"\n #include \"pack-mtimes.h\"\n #include \"parse-options.h\"\n+#include \"missing.h\"\n \n /*\n * Objects we are going to pack are collected in the `to_pack` structure.\n@@ -250,11 +251,6 @@ static unsigned long window_memory_limit = 0;\n \n static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;\n \n-enum missing_action {\n-\tMA_ERROR = 0, /* fail if any missing objects are encountered */\n-\tMA_ALLOW_ANY, /* silently allow ALL missing objects */\n-\tMA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */\n-};\n static enum missing_action arg_missing_action;\n static show_object_fn fn_show_object;\n \n@@ -3826,33 +3822,39 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\n \tshow_object(obj, name, data);\n }\n \n+static show_object_fn show_object_fn_from_action(enum missing_action action)\n+{\n+\tswitch (action) {\n+\tcase MA_ERROR:\n+\t\treturn show_object;\n+\tcase MA_ALLOW_ANY:\n+\t\treturn show_object__ma_allow_any;\n+\tcase MA_ALLOW_PROMISOR:\n+\t\treturn show_object__ma_allow_promisor;\n+\tdefault:\n+\t\tBUG(\"invalid missing action %d\", action);\n+\t}\n+}\n+\n static int option_parse_missing_action(const struct option *opt UNUSED,\n \t\t\t\t const char *arg, int unset)\n {\n+\tint res;\n+\n \tassert(arg);\n \tassert(!unset);\n \n-\tif (!strcmp(arg, \"error\")) {\n-\t\targ_missing_action = MA_ERROR;\n-\t\tfn_show_object = show_object;\n-\t\treturn 0;\n-\t}\n+\tres = parse_missing_action_value(arg);\n+\tif (res < 0 || (res != MA_ERROR &&\n+\t\t\tres != MA_ALLOW_ANY &&\n+\t\t\tres != MA_ALLOW_PROMISOR))\n+\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n \n-\tif (!strcmp(arg, \"allow-any\")) {\n-\t\targ_missing_action = MA_ALLOW_ANY;\n+\tif (res != MA_ERROR)\n \t\tfetch_if_missing = 0;\n-\t\tfn_show_object = show_object__ma_allow_any;\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(arg, \"allow-promisor\")) {\n-\t\targ_missing_action = MA_ALLOW_PROMISOR;\n-\t\tfetch_if_missing = 0;\n-\t\tfn_show_object = show_object__ma_allow_promisor;\n-\t\treturn 0;\n-\t}\n+\targ_missing_action = res;\n+\tfn_show_object = show_object_fn_from_action(arg_missing_action);\n \n-\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n \treturn 0;\n }\n \n-- \n2.45.1.148.g0f5efb064b\n\n"},{"id":"494812","messageId":"20240515132543.851987-4-christian.couder@gmail.com","threadId":"58609","inReplyTo":"20240515132543.851987-1-christian.couder@gmail.com","subject":"[PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-15T13:25:43Z","receivedAt":"2024-05-15T13:26:02Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder \n\nIn case some objects are missing from a server, it might still be\nuseful to be able to fetch or clone from it if the client already has\nthe missing objects or can get them in some way.\n\nFor example, in case both the server and the client are using a\nseparate promisor remote that contain some objects, it can be better\nif the server doesn't try to send such objects back to the client, but\ninstead let the client get those objects separately from the promisor\nremote. (The client needs to have the separate promisor remote\nconfigured, for that to work.)\n\nAnother example could be a server where some objects have been\ncorrupted or deleted. It could still be useful for clients who could\nget those objects from another source, like perhaps a different\nclient, to be able to fetch or clone from the server.\n\nTo configure what a server should do if there are such missing\nobjects, let's teach `git upload-pack` a new\n`uploadpack.missingAction` configuration variable.\n\nThis new missing-action works in a similar way as what `git rev-list`\nand `git pack-objects` already support with their\n`--missing=` command line options. In fact when\n`uploadpack.missingAction` is set to something different than \"error\",\n`git upload-pack` will just pass the corresponding\n`--missing=` to `git pack-objects`.\n\nSo this new missing-action has the same limitations as\n`git pack-objects --missing=`. Especially when not\nusing promisor remotes, 'allow-any' works only for blobs.\n\nAnother limitation comes from `git upload-pack` itself which requires\nsetting `GIT_NO_LAZY_FETCH` to 0 since 7b70e9efb1 (upload-pack:\ndisable lazy-fetching by default, 2024-04-16) for lazy fetching from\na promisor remote to work on the server side.\n\nSigned-off-by: Christian Couder \n---\n Documentation/config/uploadpack.txt | 9 ++\n missing.c | 16 ++++\n missing.h | 2 +\n t/t5706-upload-pack-missing.sh | 125 ++++++++++++++++++++++++++++\n upload-pack.c | 19 +++++\n 5 files changed, 171 insertions(+)\n create mode 100755 t/t5706-upload-pack-missing.sh\n\ndiff --git a/Documentation/config/uploadpack.txt b/Documentation/config/uploadpack.txt\nindex 16264d82a7..cd70e853b3 100644\n--- a/Documentation/config/uploadpack.txt\n+++ b/Documentation/config/uploadpack.txt\n@@ -82,3 +82,12 @@ uploadpack.allowRefInWant::\n \tis intended for the benefit of load-balanced servers which may\n \tnot have the same view of what OIDs their refs point to due to\n \treplication delay.\n+\n+uploadpack.missingAction::\n+\tIf this option is set, `upload-pack` will call\n+\tlinkgit:git-pack-objects[1] passing it the corresponding\n+\t`--missing=` command line option. See the\n+\tdocumentation for that option, to see the valid values and\n+\ttheir meaning. This could be useful if some objects are\n+\tmissing on a server, but a client already has them or could\n+\tstill get them from somewhere else.\ndiff --git a/missing.c b/missing.c\nindex ce3cf734a8..75036d2089 100644\n--- a/missing.c\n+++ b/missing.c\n@@ -18,3 +18,19 @@ int parse_missing_action_value(const char *value)\n \n \treturn -1;\n }\n+\n+const char *missing_action_to_string(enum missing_action action)\n+{\n+\tswitch (action) {\n+\tcase MA_ERROR:\n+\t\treturn \"error\";\n+\tcase MA_ALLOW_ANY:\n+\t\treturn \"allow-any\";\n+\tcase MA_PRINT:\n+\t\treturn \"print\";\n+\tcase MA_ALLOW_PROMISOR:\n+\t\treturn \"allow-promisor\";\n+\tdefault:\n+\t\tBUG(\"invalid missing action %d\", action);\n+\t}\n+}\ndiff --git a/missing.h b/missing.h\nindex 1e378d6215..74753d7887 100644\n--- a/missing.h\n+++ b/missing.h\n@@ -14,4 +14,6 @@ enum missing_action {\n */\n int parse_missing_action_value(const char *value);\n \n+const char *missing_action_to_string(enum missing_action action);\n+\n #endif /* MISSING_H */\ndiff --git a/t/t5706-upload-pack-missing.sh b/t/t5706-upload-pack-missing.sh\nnew file mode 100755\nindex 0000000000..0f39c0ddd0\n--- /dev/null\n+++ b/t/t5706-upload-pack-missing.sh\n@@ -0,0 +1,125 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in upload-pack'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./test-lib.sh\n+\n+# Setup the repository with three commits, this way HEAD is always\n+# available and we can hide commit 1 or 2.\n+test_expect_success 'setup: create \"template\" repository' '\n+\tgit init template &&\n+\ttest_commit -C template 1 &&\n+\ttest_commit -C template 2 &&\n+\ttest_commit -C template 3 &&\n+\ttest-tool genrandom foo 10240 >template/foo &&\n+\tgit -C template add foo &&\n+\tgit -C template commit -m foo\n+'\n+\n+# A bare repo will act as a server repo with unpacked objects.\n+test_expect_success 'setup: create bare \"server\" repository' '\n+\tgit clone --bare --no-local template server &&\n+\tmv server/objects/pack/pack-* . &&\n+\tpackfile=$(ls pack-*.pack) &&\n+\tgit -C server unpack-objects --strict <\"$packfile\"\n+'\n+\n+# Fetching with 'uploadpack.missingAction=allow-any' only works with\n+# blobs, as `git pack-objects --missing=allow-any` fails if a missing\n+# object is not a blob.\n+test_expect_success \"fetch with uploadpack.missingAction=allow-any\" '\n+\toid=\"$(git -C server rev-parse HEAD:1.t)\" &&\n+\toid_path=\"$(test_oid_to_path $oid)\" &&\n+\tpath=\"server/objects/$oid_path\" &&\n+\n+\tmv \"$path\" \"$path.hidden\" &&\n+\ttest_when_finished \"mv $path.hidden $path\" &&\n+\n+\tgit init client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\t# Client needs the missing objects to be available somehow\n+\tclient_path=\"client/.git/objects/$oid_path\" &&\n+\tmkdir -p $(dirname \"$client_path\") &&\n+\tcp \"$path.hidden\" \"$client_path\" &&\n+\n+\ttest_must_fail git -C client fetch ../server &&\n+\tgit -C server config uploadpack.missingAction error &&\n+\ttest_must_fail git -C client fetch ../server &&\n+\tgit -C server config uploadpack.missingAction allow-any &&\n+\tgit -C client fetch ../server &&\n+\tgit -C server config --unset uploadpack.missingAction\n+'\n+\n+check_missing_objects () {\n+\tgit -C \"$1\" rev-list --objects --all --missing=print > all.txt &&\n+\tsed -n \"s/^\\?\\(.*\\)/\\1/p\" missing.txt &&\n+\ttest_line_count = \"$2\" missing.txt &&\n+\ttest \"$3\" = \"$(cat missing.txt)\"\n+}\n+\n+test_expect_success \"setup for testing uploadpack.missingAction=allow-promisor\" '\n+\t# Create another bare repo called \"server2\"\n+\tgit init --bare server2 &&\n+\n+\t# Copy the largest object from server to server2\n+\tobj=\"HEAD:foo\" &&\n+\toid=\"$(git -C server rev-parse $obj)\" &&\n+\toid_path=\"$(test_oid_to_path $oid)\" &&\n+\tpath=\"server/objects/$oid_path\" &&\n+\tpath2=\"server2/objects/$oid_path\" &&\n+\tmkdir -p $(dirname \"$path2\") &&\n+\tcp \"$path\" \"$path2\" &&\n+\n+\t# Repack everything first\n+\tgit -C server -c repack.writebitmaps=false repack -a -d &&\n+\n+\t# Repack without the largest object and create a promisor pack on server\n+\tgit -C server -c repack.writebitmaps=false repack -a -d \\\n+\t --filter=blob:limit=5k --filter-to=\"$(pwd)\" &&\n+\tpromisor_file=$(ls server/objects/pack/*.pack | sed \"s/\\.pack/.promisor/\") &&\n+\t> \"$promisor_file\" &&\n+\n+\t# Check that only one object is missing on the server\n+\tcheck_missing_objects server 1 \"$oid\" &&\n+\n+\t# Configure server2 as promisor remote for server\n+\tgit -C server remote add server2 \"file://$(pwd)/server2\" &&\n+\tgit -C server config remote.server2.promisor true &&\n+\n+\tgit -C server2 config uploadpack.allowFilter true &&\n+\tgit -C server2 config uploadpack.allowAnySHA1InWant true &&\n+\tgit -C server config uploadpack.allowFilter true &&\n+\tgit -C server config uploadpack.allowAnySHA1InWant true\n+'\n+\n+test_expect_success \"fetch with uploadpack.missingAction=allow-promisor\" '\n+\tgit -C server config uploadpack.missingAction allow-promisor &&\n+\n+\t# Clone from server to create a client\n+\tGIT_NO_LAZY_FETCH=0 git clone -c remote.server2.promisor=true \\\n+\t\t-c remote.server2.fetch=\"+refs/heads/*:refs/remotes/server2/*\" \\\n+\t\t-c remote.server2.url=\"file://$(pwd)/server2\" \\\n+\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\t# Check that the largest object is still missing on the server\n+\tcheck_missing_objects server 1 \"$oid\"\n+'\n+\n+test_expect_success \"fetch without uploadpack.missingAction=allow-promisor\" '\n+\tgit -C server config --unset uploadpack.missingAction &&\n+\n+\t# Clone from server to create a client\n+\tGIT_NO_LAZY_FETCH=0 git clone -c remote.server2.promisor=true \\\n+\t\t-c remote.server2.fetch=\"+refs/heads/*:refs/remotes/server2/*\" \\\n+\t\t-c remote.server2.url=\"file://$(pwd)/server2\" \\\n+\t\t--no-local --filter=\"blob:limit=5k\" server client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\t# Check that the largest object is not missing on the server anymore\n+\tcheck_missing_objects server 0 \"\"\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 902144b9d3..f114f92197 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -29,6 +29,8 @@\n #include \"write-or-die.h\"\n #include \"json-writer.h\"\n #include \"strmap.h\"\n+#include \"missing.h\"\n+#include \"object-file.h\"\n \n /* Remember to update object flag allocation in object.h */\n #define THEY_HAVE\t(1u << 11)\n@@ -96,6 +98,8 @@ struct upload_pack_data {\n \n \tconst char *pack_objects_hook;\n \n+\tenum missing_action missing_action;\n+\n \tunsigned stateless_rpc : 1;\t\t\t\t/* v0 only */\n \tunsigned no_done : 1;\t\t\t\t\t/* v0 only */\n \tunsigned daemon_mode : 1;\t\t\t\t/* v0 only */\n@@ -315,6 +319,9 @@ static void create_pack_file(struct upload_pack_data *pack_data,\n \t\tstrvec_push(&pack_objects.args, \"--delta-base-offset\");\n \tif (pack_data->use_include_tag)\n \t\tstrvec_push(&pack_objects.args, \"--include-tag\");\n+\tif (pack_data->missing_action)\n+\t\tstrvec_pushf(&pack_objects.args, \"--missing=%s\",\n+\t\t\t missing_action_to_string(pack_data->missing_action));\n \tif (pack_data->filter_options.choice) {\n \t\tconst char *spec =\n \t\t\texpand_list_objects_filter_spec(&pack_data->filter_options);\n@@ -1371,6 +1378,18 @@ static int upload_pack_config(const char *var, const char *value,\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n \t\tdata->advertise_sid = git_config_bool(var, value);\n+\t} else if (!strcmp(\"uploadpack.missingaction\", var)) {\n+\t\tint res = parse_missing_action_value(value);\n+\t\tif (res < 0 || (res != MA_ERROR &&\n+\t\t\t\tres != MA_ALLOW_ANY &&\n+\t\t\t\tres != MA_ALLOW_PROMISOR))\n+\t\t\tdie(_(\"invalid value for '%s': '%s'\"), var, value);\n+\t\t/* Allow fetching only from promisor remotes */\n+\t\tif (res == MA_ALLOW_PROMISOR)\n+\t\t\tfetch_if_missing = 1;\n+\t\tif (res == MA_ALLOW_ANY)\n+\t\t\tfetch_if_missing = 0;\n+\t\tdata->missing_action = res;\n \t}\n \n \tif (parse_object_filter_config(var, value, ctx->kvi, data) < 0)\n-- \n2.45.1.148.g0f5efb064b\n\n"},{"id":"494815","messageId":"CAP8UFD0hGoU_BKDq8+o6J9AT9jQOSRKrRw532MeMXTYebd3rHw@mail.gmail.com","threadId":"58609","inReplyTo":"20240515132543.851987-1-christian.couder@gmail.com","subject":"Re: [PATCH v2 0/3] upload-pack: support a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-15T13:59:59Z","receivedAt":"2024-05-15T14:00:13Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Sorry everyone, it looks like I put the wrong message Id in the\n--in-reply-to= option of my `git send-email` command and this appears\nto be part of a different thread. The right message ID should have\nbeen:\n\n20240418184043.2900955-1-christian.couder@gmail.com\n\nSorry again.\n\nOn Wed, May 15, 2024 at 3:25 PM Christian Couder\n wrote:\n>\n> `git pack-objects` already supports a `--missing=`\n> option, so that it can avoid erroring out if some objects aren't\n> available.\n>\n> It is interesting to have `git upload-pack` support a similar way to\n> avoid sending some objects in case they aren't available on the\n> server.\n>\n> For example, in case both the server and the client are using a\n> separate promisor remote that contain some objects, it can be better\n> if the server doesn't try to send such objects back to the client, but\n> instead let the client get those objects separately from the promisor\n> remote. (The client needs to have the separate promisor remote\n> configured, for that to work.)\n>\n> This could work better if there was something, like perhaps a\n> capability, for the client to tell the server something like:\n>\n> \"I know how to fetch missing objects from this and that\n> promisor remotes, so if you choose to, you may omit objects\n> that you know are available from these promisor remotes when\n> sending objects to me.\"\n>\n> But that capability could be added later as other similar capabilities\n> in this area could be very useful. For example in case of a client\n> cloning, something like the following is currently needed:\n>\n> GIT_NO_LAZY_FETCH=0 git clone\n> -c remote.my_promisor.promisor=true \\\n> -c remote.my_promisor.fetch=\"+refs/heads/*:refs/remotes/my_promisor/*\" \\\n> -c remote.my_promisor.url= \\\n> --filter=\"blob:limit=5k\" server\n>\n> But it would be nice if there was a capability for the client to say\n> that it would like the server to give it information about the\n> promisor that it could use, so that the user doesn't have to pass all\n> the \"remote.my_promisor.XXX\" config options on the command like. (It\n> would then be a bit similar to the bundle-uri feature where all the\n> bundle related information comes from the server.)\n>\n> Another example use of this feature could be a server where some\n> objects have been corrupted or deleted. It could still be useful for\n> clients who could get those objects from another source, like perhaps\n> a different client, to be able to fetch or clone from the server.\n>\n> The fact that the new `uploadpack.missingAction` configuration\n> variable has to be set to a non default value on the server means that\n> regular client users cannot hurt themselves with this feature.\n>\n> As `git rev-list` also supports a `--missing=` option,\n> the first 2 patches in this series are about refactoring related code\n> from both `git rev-list` and `git pack-objects` into new\n> \"missing.{c,h}\" files. Patch 3/3 then adds a new\n> `uploadpack.missingAction` configuration variable.\n>\n> The changes since v1 are the following:\n>\n> - In patch 1/3 the refactored parse_missing_action_value() function\n> doesn't change the fetch_if_missing global variable anymore. The\n> clients of this function change it by themselves instead.\n>\n> - In patch 1/3 a minor typo was also fixed.\n>\n> - Old patch 2/4 that added a 'int print_ok' argument to\n> parse_missing_action_value() was removed, and now callers deal\n> with only the values they recognise.\n>\n> Thanks to Junio for his review and for suggesting of the above\n> changes.\n>\n> Christian Couder (3):\n> rev-list: refactor --missing=\n> pack-objects: use the missing action API\n> upload-pack: allow configuring a missing-action\n>\n> Documentation/config/uploadpack.txt | 9 ++\n> Makefile | 1 +\n> builtin/pack-objects.c | 48 ++++++-----\n> builtin/rev-list.c | 43 ++--------\n> missing.c | 36 ++++++++\n> missing.h | 19 +++++\n> t/t5706-upload-pack-missing.sh | 125 ++++++++++++++++++++++++++++\n> upload-pack.c | 19 +++++\n> 8 files changed, 242 insertions(+), 58 deletions(-)\n> create mode 100644 missing.c\n> create mode 100644 missing.h\n> create mode 100755 t/t5706-upload-pack-missing.sh\n>\n> --\n> 2.45.1.148.g0f5efb064b\n>\n"},{"id":"494825","messageId":"xmqq4jaz1263.fsf@gitster.g","threadId":"58609","inReplyTo":"20240515132543.851987-2-christian.couder@gmail.com","subject":"Re: [PATCH v2 1/3] rev-list: refactor --missing=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-15T16:16:04Z","receivedAt":"2024-05-15T16:16:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Both `git rev-list` and `git pack-objects` support a\n> `--missing=` feature, but they currently don't share\n> any code for that.\n>\n> Refactor the support for `--missing=` in\n> \"builtin/rev-list.c\" into new \"missing.{c,h}\" files. In a following\n> commit, that refactored code will be used in \"builtin/pack-objects.c\"\n> too.\n>\n> In yet a following commit, we are going to add support for a similar\n> 'missing-action' feature to another command, and we are also going to\n> reuse code from the new \"missing.{c,h}\" files.\n>\n> As `enum missing_action` and parse_missing_action_value() are moved to\n> \"missing.{c,h}\", we need to modify the latter a bit, so that it stops\n> updating any global variable, but instead returns the parsed value or\n> -1 on error.\n>\n> Signed-off-by: Christian Couder \n> ---\n> Makefile | 1 +\n> builtin/rev-list.c | 43 ++++++++-----------------------------------\n> missing.c | 20 ++++++++++++++++++++\n> missing.h | 17 +++++++++++++++++\n> 4 files changed, 46 insertions(+), 35 deletions(-)\n> create mode 100644 missing.c\n> create mode 100644 missing.h\n\nAll makes sense. Will queue. Thanks.\n\n> diff --git a/Makefile b/Makefile\n> index 0285db5630..e0ddcc2cbd 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1062,6 +1062,7 @@ LIB_OBJS += merge-recursive.o\n> LIB_OBJS += merge.o\n> LIB_OBJS += midx.o\n> LIB_OBJS += midx-write.o\n> +LIB_OBJS += missing.o\n> LIB_OBJS += name-hash.o\n> LIB_OBJS += negotiator/default.o\n> LIB_OBJS += negotiator/noop.o\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 77803727e0..40aa770c47 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -20,6 +20,7 @@\n> #include \"reflog-walk.h\"\n> #include \"oidset.h\"\n> #include \"packfile.h\"\n> +#include \"missing.h\"\n> \n> static const char rev_list_usage[] =\n> \"git rev-list [] ... [--] [...]\\n\"\n> @@ -71,12 +72,6 @@ static struct oidset omitted_objects;\n> static int arg_print_omitted; /* print objects omitted by filter */\n> \n> static struct oidset missing_objects;\n> -enum missing_action {\n> -\tMA_ERROR = 0, /* fail if any missing objects are encountered */\n> -\tMA_ALLOW_ANY, /* silently allow ALL missing objects */\n> -\tMA_PRINT, /* print ALL missing objects in special section */\n> -\tMA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */\n> -};\n> static enum missing_action arg_missing_action;\n> \n> /* display only the oid of each object encountered */\n> @@ -392,34 +387,6 @@ static void print_disk_usage(off_t size)\n> \tstrbuf_release(&sb);\n> }\n> \n> -static inline int parse_missing_action_value(const char *value)\n> -{\n> -\tif (!strcmp(value, \"error\")) {\n> -\t\targ_missing_action = MA_ERROR;\n> -\t\treturn 1;\n> -\t}\n> -\n> -\tif (!strcmp(value, \"allow-any\")) {\n> -\t\targ_missing_action = MA_ALLOW_ANY;\n> -\t\tfetch_if_missing = 0;\n> -\t\treturn 1;\n> -\t}\n> -\n> -\tif (!strcmp(value, \"print\")) {\n> -\t\targ_missing_action = MA_PRINT;\n> -\t\tfetch_if_missing = 0;\n> -\t\treturn 1;\n> -\t}\n> -\n> -\tif (!strcmp(value, \"allow-promisor\")) {\n> -\t\targ_missing_action = MA_ALLOW_PROMISOR;\n> -\t\tfetch_if_missing = 0;\n> -\t\treturn 1;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> static int try_bitmap_count(struct rev_info *revs,\n> \t\t\t int filter_provided_objects)\n> {\n> @@ -569,10 +536,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n> \tfor (i = 1; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> \t\tif (skip_prefix(arg, \"--missing=\", &arg)) {\n> +\t\t\tint res;\n> \t\t\tif (revs.exclude_promisor_objects)\n> \t\t\t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--exclude-promisor-objects\", \"--missing\");\n> -\t\t\tif (parse_missing_action_value(arg))\n> +\t\t\tres = parse_missing_action_value(arg);\n> +\t\t\tif (res >= 0) {\n> +\t\t\t\tif (res != MA_ERROR)\n> +\t\t\t\t\tfetch_if_missing = 0;\n> +\t\t\t\targ_missing_action = res;\n> \t\t\t\tbreak;\n> +\t\t\t}\n> \t\t}\n> \t}\n> \n> diff --git a/missing.c b/missing.c\n> new file mode 100644\n> index 0000000000..ce3cf734a8\n> --- /dev/null\n> +++ b/missing.c\n> @@ -0,0 +1,20 @@\n> +#include \"git-compat-util.h\"\n> +#include \"missing.h\"\n> +#include \"object-file.h\"\n> +\n> +int parse_missing_action_value(const char *value)\n> +{\n> +\tif (!strcmp(value, \"error\"))\n> +\t\treturn MA_ERROR;\n> +\n> +\tif (!strcmp(value, \"allow-any\"))\n> +\t\treturn MA_ALLOW_ANY;\n> +\n> +\tif (!strcmp(value, \"print\"))\n> +\t\treturn MA_PRINT;\n> +\n> +\tif (!strcmp(value, \"allow-promisor\"))\n> +\t\treturn MA_ALLOW_PROMISOR;\n> +\n> +\treturn -1;\n> +}\n> diff --git a/missing.h b/missing.h\n> new file mode 100644\n> index 0000000000..1e378d6215\n> --- /dev/null\n> +++ b/missing.h\n> @@ -0,0 +1,17 @@\n> +#ifndef MISSING_H\n> +#define MISSING_H\n> +\n> +enum missing_action {\n> +\tMA_ERROR = 0, /* fail if any missing objects are encountered */\n> +\tMA_ALLOW_ANY, /* silently allow ALL missing objects */\n> +\tMA_PRINT, /* print ALL missing objects in special section */\n> +\tMA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */\n> +};\n> +\n> +/*\n> + Return an `enum missing_action` in case parsing is successful or -1\n> + if parsing failed.\n> +*/\n> +int parse_missing_action_value(const char *value);\n> +\n> +#endif /* MISSING_H */\n"},{"id":"494827","messageId":"xmqqo797xbt9.fsf@gitster.g","threadId":"58609","inReplyTo":"20240515132543.851987-3-christian.couder@gmail.com","subject":"Re: [PATCH v2 2/3] pack-objects: use the missing action API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-15T16:46:42Z","receivedAt":"2024-05-15T16:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index baf0090fc8..55d08c686d 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -39,6 +39,7 @@\n> #include \"promisor-remote.h\"\n> #include \"pack-mtimes.h\"\n> #include \"parse-options.h\"\n> +#include \"missing.h\"\n> \n> /*\n> * Objects we are going to pack are collected in the `to_pack` structure.\n> @@ -250,11 +251,6 @@ static unsigned long window_memory_limit = 0;\n> \n> static struct string_list uri_protocols = STRING_LIST_INIT_NODUP;\n> \n> -enum missing_action {\n> -\tMA_ERROR = 0, /* fail if any missing objects are encountered */\n> -\tMA_ALLOW_ANY, /* silently allow ALL missing objects */\n> -\tMA_ALLOW_PROMISOR, /* silently allow all missing PROMISOR objects */\n> -};\n\nInteresting. This used to be private to this file, shared the same\nname and most of the values with the one used in rev-list, but not\nidentical (i.e. the new \"missing\" API knows about MA_PRINT but this\nside has been unaware of that value).\n\n> @@ -3826,33 +3822,39 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\n> \tshow_object(obj, name, data);\n> }\n> \n> +static show_object_fn show_object_fn_from_action(enum missing_action action)\n> +{\n> +\tswitch (action) {\n> +\tcase MA_ERROR:\n> +\t\treturn show_object;\n> +\tcase MA_ALLOW_ANY:\n> +\t\treturn show_object__ma_allow_any;\n> +\tcase MA_ALLOW_PROMISOR:\n> +\t\treturn show_object__ma_allow_promisor;\n> +\tdefault:\n> +\t\tBUG(\"invalid missing action %d\", action);\n\nAs this is BUG() to catch programming error, (\"%d\" % action) is OK;\nif this were end-user facint, we would also want to pass the \"arg\"\nstring the caller had only for error reporting.\n\n> static int option_parse_missing_action(const struct option *opt UNUSED,\n> \t\t\t\t const char *arg, int unset)\n> {\n> +\tint res;\n> +\n> \tassert(arg);\n> \tassert(!unset);\n> \n> +\tres = parse_missing_action_value(arg);\n> +\tif (res < 0 || (res != MA_ERROR &&\n> +\t\t\tres != MA_ALLOW_ANY &&\n> +\t\t\tres != MA_ALLOW_PROMISOR))\n> +\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n\nWhat is our expectation for how API would evolve over\ntime? I think it is a given that it will always be a superset of\nthe need of rev-list and the need of pack-objects, but if we were\nto add a new value of MA_FOO, do we expect that all of the new ones\nare not handled by pack-objects, Some but not all? Or none of the\nnew ones are handled by pack-objects?\n\nRegardless of the answer to that question, I think a simple helper\nis warranted here, which will also help the [3/3] which adds exactly\nthe same code to upload-pack.c:upload_pack_config(), so that the\ncallers can do\n\n\tres = parse_missing_action_value_for_packing(arg);\n\tif (res < 0)\n \tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n\nsomething like\n\n\tint parse_missing_action_value_for_packing(const char *arg)\n\t{\n\t\tint res = parse_missing_action_value(arg);\n\n if (res < 0)\n \treturn res;\n\n\t\tswitch (res) {\n\t\tcase MA_ERROR:\n\t\tcase MA_ALLOW_ANY:\n\t\tcase MA_ALLOW_PROMISOR:\n\t\t\treturn res;\n\t\tdefault:\n \treturn -2 - res;\n\t\t}\n\t}\n\nhere, and also in the other place [3/3] adds. This thin wrapper\nreturns:\n\n\t0 <= res : MA_FOO values that are OK for packing\n\t-1 = res : parse_missing_action_value() failed\n\t-1 > res : (2 - res) is the MA_FOO which is unsuitable for packing\n\nto allow the caller to recover which value the user gave us that is\nunsuitable for packing, if it wanted to.\n\n> +\tif (res != MA_ERROR)\n> \t\tfetch_if_missing = 0;\n> +\targ_missing_action = res;\n> +\tfn_show_object = show_object_fn_from_action(arg_missing_action);\n>\n> -\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n> \treturn 0;\n> }\n\nHmph, wouldn't a small array of show_object_fn suffice, making the\nwhole thing more like:\n\n\tstatic show_object_fn const fn[] = {\n\t\t[MA_ERROR] = show_object,\n\t\t[MA_ALLOW_ANY] = show_object__ma_allow_any,\n\t\t[MA_ALLOW_PROMISOR] = show_object__ma_allow_promisor,\n\t};\n\n\tres = parse_missing_action_value_for_packing(arg);\n\tif (res < 0 || ARRAY_SIZE[fn] <= res)\n \tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n\tfn_show_object = fn[res];\n\treturn 0;\n\nwithout the need for show_object_fn_from_action() helper function?\n\nOther than that, the intention of the code is very clear.\n\nWill queue. Thanks.\n"},{"id":"494830","messageId":"xmqqv83fvw7p.fsf@gitster.g","threadId":"58609","inReplyTo":"20240515132543.851987-4-christian.couder@gmail.com","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-15T17:08:58Z","receivedAt":"2024-05-15T17:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> From: Christian Couder \n>\n> In case some objects are missing from a server, it might still be\n> useful to be able to fetch or clone from it if the client already has\n> the missing objects or can get them in some way.\n\nBe more assertive. We do not want to add a new feature randomly\nonly because it _might_ be useful to somebody in a strange and\nnarrow use case that _might_ exist.\n\n> For example, in case both the server and the client are using a\n> separate promisor remote that contain some objects, it can be better\n> if the server doesn't try to send such objects back to the client, but\n> instead let the client get those objects separately from the promisor\n> remote. (The client needs to have the separate promisor remote\n> configured, for that to work.)\n\nIs it \"it can be better\", or is it \"it is always better\"? Pick an\nexample that you can say the latter to make your example more\nconvincing.\n\nRepository S borrows from its \"promisor\" X, and repository C which\ninitially cloned from S borrows from its \"promisor\" S. Even if C\nwants an object in order to fill in the gap in its object graph, S\nmay not have it (and S itself may have no need for that object), and\nin such a case, bypassing S and let C go directly to X would make\nsense.\n\n> Another example could be a server where some objects have been\n> corrupted or deleted. It could still be useful for clients who could\n> get those objects from another source, like perhaps a different\n> client, to be able to fetch or clone from the server.\n>\n> To configure what a server should do if there are such missing\n> objects, let's teach `git upload-pack` a new\n> `uploadpack.missingAction` configuration variable.\n>\n> This new missing-action works in a similar way as what `git rev-list`\n> and `git pack-objects` already support with their\n> `--missing=` command line options. In fact when\n> `uploadpack.missingAction` is set to something different than \"error\",\n> `git upload-pack` will just pass the corresponding\n> `--missing=` to `git pack-objects`.\n>\n> So this new missing-action has the same limitations as\n> `git pack-objects --missing=`. Especially when not\n> using promisor remotes, 'allow-any' works only for blobs.\n>\n> Another limitation comes from `git upload-pack` itself which requires\n> setting `GIT_NO_LAZY_FETCH` to 0 since 7b70e9efb1 (upload-pack:\n> disable lazy-fetching by default, 2024-04-16) for lazy fetching from\n> a promisor remote to work on the server side.\n\nI am puzzled by this new option.\n\nIt feels utterly irresponsible to give an option to set up a server\nthat essentially declares: I'll serve objects you ask me as best\nefforts basis, the pack stream I'll give you may not have all\nobjects you asked for and missing some objects, and when I do so, I\nam not telling you which objects I omitted.\n\nHow do you ensure that a response with an incomplete pack data would\nnot corrupt the repository when the sending side configures\nupload-pack with this option? How does the receiving end know which\nobjects it needs to ask from elsewhere?\n\nOr is the data integrity of the receiving repository is the\nresponsibility of the receiving user that talks with such a server?\nIf that is the case, I am not sure if I want to touch such a feature\neven with 10ft pole.\n\nIs there anything the sender can do but does not do to help the\nreceiver locate where to fetch these missing objects to fill the\n\"unfilled promises\"?\n\nFor example, the sending side _could_ say that \"Sorry, I do not have\nall objects that you asked me to---but you could try these other\nrepositories\".\n"},{"id":"495576","messageId":"CAP8UFD17JiXDENmjERgB0TW=-S=byT-jzyCcXVp35SUU2M9Fpw@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqo797xbt9.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] pack-objects: use the missing action API","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-24T16:40:30Z","receivedAt":"2024-05-24T16:40:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 15, 2024 at 6:46 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n\n> > static int option_parse_missing_action(const struct option *opt UNUSED,\n> > const char *arg, int unset)\n> > {\n> > + int res;\n> > +\n> > assert(arg);\n> > assert(!unset);\n> >\n> > + res = parse_missing_action_value(arg);\n> > + if (res < 0 || (res != MA_ERROR &&\n> > + res != MA_ALLOW_ANY &&\n> > + res != MA_ALLOW_PROMISOR))\n> > + die(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n>\n> What is our expectation for how API would evolve over\n> time? I think it is a given that it will always be a superset of\n> the need of rev-list and the need of pack-objects, but if we were\n> to add a new value of MA_FOO, do we expect that all of the new ones\n> are not handled by pack-objects, Some but not all? Or none of the\n> new ones are handled by pack-objects?\n\nI don't think the API would evolve much over time. At\nleast I don't think we have plans to make it evolve. Perhaps other\noptions similar to MA_PRINT could be added though, maybe MA_TRACE or\nMA_LOG. Maybe such a new option could be handled by pack-object, maybe\nnot. I think for now it's better to be flexible and not guess too\nmuch.\n\n> Regardless of the answer to that question, I think a simple helper\n> is warranted here, which will also help the [3/3] which adds exactly\n> the same code to upload-pack.c:upload_pack_config(), so that the\n> callers can do\n>\n> res = parse_missing_action_value_for_packing(arg);\n> if (res < 0)\n> die(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n\nOk I implemented this in the v3 I just sent.\n\n> > + if (res != MA_ERROR)\n> > fetch_if_missing = 0;\n> > + arg_missing_action = res;\n> > + fn_show_object = show_object_fn_from_action(arg_missing_action);\n> >\n> > - die(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n> > return 0;\n> > }\n>\n> Hmph, wouldn't a small array of show_object_fn suffice, making the\n> whole thing more like:\n>\n> static show_object_fn const fn[] = {\n> [MA_ERROR] = show_object,\n> [MA_ALLOW_ANY] = show_object__ma_allow_any,\n> [MA_ALLOW_PROMISOR] = show_object__ma_allow_promisor,\n> };\n>\n> res = parse_missing_action_value_for_packing(arg);\n> if (res < 0 || ARRAY_SIZE[fn] <= res)\n> die(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n> fn_show_object = fn[res];\n> return 0;\n>\n> without the need for show_object_fn_from_action() helper function?\n\nOk with the small array. I implemented it in v3 too.\n\nThanks for the suggestions.\n"},{"id":"495577","messageId":"CAP8UFD1JZ7eJzrTk1YPrfy1XLAnnLvukABOJE69-JbqVWtL1AQ@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqv83fvw7p.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-24T16:41:56Z","receivedAt":"2024-05-24T16:42:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, May 15, 2024 at 7:09 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > From: Christian Couder \n> >\n> > In case some objects are missing from a server, it might still be\n> > useful to be able to fetch or clone from it if the client already has\n> > the missing objects or can get them in some way.\n>\n> Be more assertive. We do not want to add a new feature randomly\n> only because it _might_ be useful to somebody in a strange and\n> narrow use case that _might_ exist.\n\nOk, I have changed \"it might still be useful\" to \"it is sometimes\nuseful\" in the v3 I just sent.\n\n> > For example, in case both the server and the client are using a\n> > separate promisor remote that contain some objects, it can be better\n> > if the server doesn't try to send such objects back to the client, but\n> > instead let the client get those objects separately from the promisor\n> > remote. (The client needs to have the separate promisor remote\n> > configured, for that to work.)\n>\n> Is it \"it can be better\", or is it \"it is always better\"? Pick an\n> example that you can say the latter to make your example more\n> convincing.\n>\n> Repository S borrows from its \"promisor\" X, and repository C which\n> initially cloned from S borrows from its \"promisor\" S. Even if C\n> wants an object in order to fill in the gap in its object graph, S\n> may not have it (and S itself may have no need for that object), and\n> in such a case, bypassing S and let C go directly to X would make\n> sense.\n\nOk, I use your example above in v3.\n\n> I am puzzled by this new option.\n>\n> It feels utterly irresponsible to give an option to set up a server\n> that essentially declares: I'll serve objects you ask me as best\n> efforts basis, the pack stream I'll give you may not have all\n> objects you asked for and missing some objects, and when I do so, I\n> am not telling you which objects I omitted.\n\nI don't think it's irresponsible. The client anyways checks that it\ngot something usable in the same way as it does when it performs a\npartial fetch or clone. The fetch or clone fails if that's not the\ncase. For example if the checkout part of a clone needs some objects\nbut cannot get them, the whole clone fails.\n\n> How do you ensure that a response with an incomplete pack data would\n> not corrupt the repository when the sending side configures\n> upload-pack with this option? How does the receiving end know which\n> objects it needs to ask from elsewhere?\n\nGit already has support for multiple promisor remotes. When a repo is\nconfigured with 2 promisor remotes, let's call them A and B, then A\ncannot guarantee that B has all the objects that are missing in A. And\nit's the same for B, it cannot guarantee A has all the objects missing\nin B. Also when fetching or cloning from A for example, then no list\nof missing objects is transfered.\n\n> Or is the data integrity of the receiving repository is the\n> responsibility of the receiving user that talks with such a server?\n\nYes, it's the same as when a repo is configured with multiple promisor remotes.\n\nWhen using bundle-uri, it's also the responsibility of the receiving\nuser to check that the bundle it gets from a separate endpoint is\ncorrect.\n\nAlso when only large blobs are on remotes except for the main remote,\nthen, after cloning or fetching from the main remote, the client knows\nthe hashes of the objects it should get from the other remotes. So\nthere is still data integrity.\n\n> If that is the case, I am not sure if I want to touch such a feature\n> even with 10ft pole.\n\nTo give you some context, at GitLab we have a very large part of the\ndisk space on our servers taken by large blobs which often don't\ncompress well (see\nhttps://gitlab.com/gitlab-org/gitaly/-/issues/5699#note_1794464340).\nIt makes a lot of sense for us at GitLab to try to move those large\nblobs to some cheaper separate storage.\n\nWith repack --filter=... we can put large blobs on a separate path,\nbut it has some drawbacks:\n - they are still part of a packfile, which means that storing and\naccessing the objects is expensive in terms of CPU and memory (large\nbinary files are often already compressed and might not delta well\nwith other objects),\n - mounting object storage on a machine might not be easy or might\nnot perform as well as using an object storage API.\n\nSo using a separate remote along with a remote helper for large blobs\nmakes sense. And when a client is cloning, it makes sense for a\nregular Git server, to let the client fetch the large blobs directly\nfrom a remote that has them.\n\n> Is there anything the sender can do but does not do to help the\n> receiver locate where to fetch these missing objects to fill the\n> \"unfilled promises\"?\n>\n> For example, the sending side _could_ say that \"Sorry, I do not have\n> all objects that you asked me to---but you could try these other\n> repositories\".\n\nIn the cover letter of this v2 and the v3, I suggested the following:\n\n---\nFor example in case of a client\ncloning, something like the following is currently needed:\n\n GIT_NO_LAZY_FETCH=0 git clone\n -c remote.my_promisor.promisor=true \\\n -c remote.my_promisor.fetch=\"+refs/heads/*:refs/remotes/my_promisor/*\" \\\n -c remote.my_promisor.url= \\\n --filter=\"blob:limit=5k\" server\n\nBut it would be nice if there was a capability for the client to say\nthat it would like the server to give it information about the\npromisor that it could use, so that the user doesn't have to pass all\nthe \"remote.my_promisor.XXX\" config options on the command like. (It\nwould then be a bit similar to the bundle-uri feature where all the\nbundle related information comes from the server.)\n---\n\nSo yes we are thinking about adding such a way to \"help the receiver\nlocate where to fetch these missing objects\" soon. We needed to start\nsomewhere and we decided to start with this series, because this patch\nseries is quite small and let us already experiment with offloading\nblobs to object storage (see\nhttps://gitlab.com/gitlab-org/gitaly/-/issues/5987).\n\nAlso the client will anyway very likely store the information about\nwhere it can get the missing objects as a promisor remote\nconfiguration in its config. So after the clone, the resulting repo\nwill very likely be very similar as what it would be with the clone\ncommand above.\n"},{"id":"495606","messageId":"xmqqjzjikhdz.fsf@gitster.g","threadId":"58609","inReplyTo":"CAP8UFD1JZ7eJzrTk1YPrfy1XLAnnLvukABOJE69-JbqVWtL1AQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T21:51:52Z","receivedAt":"2024-05-24T21:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> Repository S borrows from its \"promisor\" X, and repository C which\n>> initially cloned from S borrows from its \"promisor\" S. Even if C\n>> wants an object in order to fill in the gap in its object graph, S\n>> may not have it (and S itself may have no need for that object), and\n>> in such a case, bypassing S and let C go directly to X would make\n>> sense.\n> ...\n>>\n>> It feels utterly irresponsible to give an option to set up a server\n>> that essentially declares: I'll serve objects you ask me as best\n>> efforts basis, the pack stream I'll give you may not have all\n>> objects you asked for and missing some objects, and when I do so, I\n>> am not telling you which objects I omitted.\n>\n> I don't think it's irresponsible. The client anyways checks that it\n> got something usable in the same way as it does when it performs a\n> partial fetch or clone. The fetch or clone fails if that's not the\n> case. For example if the checkout part of a clone needs some objects\n> but cannot get them, the whole clone fails.\n\nBut then what can the repository C do after seeing such a failure?\n\nWith the design, S does not even consult C to see if C knows about\nX. Without knowing that, it cannot safely decide that it does not\nhave to send objects that can be obtained from X to C. Instead, S\nsimply say \"if C requests an object that I do not have, just ignore\nit and let C grab it from somewhere else\". How would it not be an\nirresponsible design?\n\n\n"},{"id":"495738","messageId":"CAP8UFD1_aHwbhF12v-miCTWEbbgjtpjTCmkRmFHu4Vusezq6dA@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqjzjikhdz.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-28T10:10:31Z","receivedAt":"2024-05-28T10:10:46Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 24, 2024 at 11:51 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> >> Repository S borrows from its \"promisor\" X, and repository C which\n> >> initially cloned from S borrows from its \"promisor\" S. Even if C\n> >> wants an object in order to fill in the gap in its object graph, S\n> >> may not have it (and S itself may have no need for that object), and\n> >> in such a case, bypassing S and let C go directly to X would make\n> >> sense.\n> > ...\n> >>\n> >> It feels utterly irresponsible to give an option to set up a server\n> >> that essentially declares: I'll serve objects you ask me as best\n> >> efforts basis, the pack stream I'll give you may not have all\n> >> objects you asked for and missing some objects, and when I do so, I\n> >> am not telling you which objects I omitted.\n> >\n> > I don't think it's irresponsible. The client anyways checks that it\n> > got something usable in the same way as it does when it performs a\n> > partial fetch or clone. The fetch or clone fails if that's not the\n> > case. For example if the checkout part of a clone needs some objects\n> > but cannot get them, the whole clone fails.\n>\n> But then what can the repository C do after seeing such a failure?\n\nIt's basically the same as when a regular clone or a partial clone or\na clone using bundle-uri fails or when using a regular bundle fails.\nIf it failed because the remote was not properly configured, then that\nconfig can be fixed. If it fails because the remote doesn't have some\nobjects, then maybe the missing objects can be transferred to the\nremote. And so on.\n\nThe feature doesn't create any new kind of failure. In particular,\nwhen you use a partial clone, even a very simple one with a single\nremote, there is always the risk of not being able to get some missing\nobjects as there is the risk of the remote being unreachable for some\nreason (like if you take a plane and don't have an internet\nconnection, or if there is an outage on the server side). There are\nsome added risks because the feature requires added configuration and\nit can be wrong like any configuration, and because there are 2\nremotes instead of just one. But these are not new kinds of risks.\nThese risks already exist if one uses multiple promisor remotes.\n\n> With the design, S does not even consult C to see if C knows about\n> X.\n\nIf S is managed by a company like GitLab or GitHub, then S will\ncertainly advertise, for example by showing a command that can easily\nbe copy-pasted from the web page of the project onto the user's\ncommand line, some way for C to use X.\n\nIn the cover letter I give the example of the following command that\ncan be used (and advertised by S):\n\n GIT_NO_LAZY_FETCH=0 git clone\n -c remote.my_promisor.promisor=true \\\n -c remote.my_promisor.fetch=\"+refs/heads/*:refs/remotes/my_promisor/*\" \\\n -c remote.my_promisor.url= \\\n --filter=\"blob:limit=5k\" server\n\nI also agree in the cover letter that this is not the most user\nfriendly clone command and I suggest that I could work on improving on\nthat by saying:\n\n\"it would be nice if there was a capability for the client to say\nthat it would like the server to give it information about the\npromisor that it could use, so that the user doesn't have to pass all\nthe \"remote.my_promisor.XXX\" config options on the command like.\"\n\nand by saying that this could be added later.\n\nIf you think that such a capability should definitely be part of this\nwork, for example because it wouldn't be sane to require users to use\nsuch a long and complex command and it could avoid difficult to debug\nfailures, then I would be willing to work on this and add it to this\nseries.\n\n> Without knowing that, it cannot safely decide that it does not\n> have to send objects that can be obtained from X to C.\n\nIn the above command C is asking for a partial clone, as it uses a\n--filter option. This means that C knows very well that it might not\nget from S all the objects needed for a complete object graph. So why\ncan't S safely decide not to send some objects to C? Why would it be\nOk if C wanted a partial clone but didn't want to get some objects\nfrom X at the same time, but would not be Ok if C wants the same\npartial clone but also with the possibility to get some of the objects\nfrom X right away? To me it seems less risky to ask for some objects\nfrom X right away.\n\n> Instead, S\n> simply say \"if C requests an object that I do not have, just ignore\n> it and let C grab it from somewhere else\". How would it not be an\n> irresponsible design?\n\nAgain when using a regular partial clone omitting the same set of\nobjects, C also requests some objects that S doesn't have. And this is\nnot considered an issue or something irresponsible. It already works\nlike this. And then C still has the possibility to configure X as a\npromisor remote and get missing objects from there. So why is it Ok\nwhen it's done in several steps but not in one?\n"},{"id":"495756","messageId":"xmqq34q27wzg.fsf@gitster.g","threadId":"58609","inReplyTo":"CAP8UFD1_aHwbhF12v-miCTWEbbgjtpjTCmkRmFHu4Vusezq6dA@mail.gmail.com","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T15:54:43Z","receivedAt":"2024-05-28T15:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> It's basically the same as when a regular clone or a partial clone or\n> a clone using bundle-uri fails or when using a regular bundle fails.\n> If it failed because the remote was not properly configured, then that\n> config can be fixed. If it fails because the remote doesn't have some\n> objects, then maybe the missing objects can be transferred to the\n> remote. And so on.\n\n> The feature doesn't create any new kind of failure.\n\n> \"it would be nice if there was a capability for the client to say\n> that it would like the server to give it information about the\n> promisor that it could use, so that the user doesn't have to pass all\n> the \"remote.my_promisor.XXX\" config options on the command like.\"\n>\n> and by saying that this could be added later.\n\nCan such an update happen transparently, or does it need changes to\nend-user experience? It is of dubious value to leave the initial\nseries be incomplete if the latter is the case.\n\n>> Without knowing that, it cannot safely decide that it does not\n>> have to send objects that can be obtained from X to C.\n>\n> In the above command C is asking for a partial clone, as it uses a\n> --filter option. This means that C knows very well that it might not\n> get from S all the objects needed for a complete object graph.\n\nHmph. If C asks a partial clone and S is willing to be the promisor\nfor C, S is essentially saying that it will serve C any objects on\ndemand that are reachable from any object it served C in the past,\nforever, no? It might not get from S initially all the objects, but\nif it wants later, S promises to let C have them.\n\n\n> Again when using a regular partial clone omitting the same set of\n> objects, C also requests some objects that S doesn't have. And\n> this is not considered an issue or something irresponsible. It\n> already works like this.\n\n\"S doesn't have\" is because S has pruned objects that it shouldn't\nhave in order to keep the promise it made earlier to C, right? If\nthat is the case, I would very much say S is being irresponsible in\nthat case.\n\n> And then C still has the possibility to configure X as a\n> promisor remote and get missing objects from there. So why is it Ok\n> when it's done in several steps but not in one?\n\nYou are right that S can decide to unilaterally break the promise it\nmade C, so this update is not making it any worse than giving users\na broken implementation of promisor remotes. I wouldn't call it OK,\nthough.\n\nIf somebody identifies that even without this series, S can lead to\nrepository corruption at C by pruning objects it does need to keep\nits promise to C, the next action I expect from healthy project is\nto try coming up with a mechanism to make it less likely that such a\npruning happens by accident (e.g., by noticing allowAnySHA1InWant as\na sign that the repository has promised others to serve anything\nthat used to be reachable from anything it historically served,\ndisabling repack \"-d\" and instead send the currently unreachable\nobjects to an archived pack, and something equally silly like that).\nIt certainly is not to add a new mechanism to make it even easier to\nconfigure S to break promised it made to C.\n\nSo, I dunno.\n"},{"id":"496028","messageId":"CAP8UFD18Y=NhnnzcHoBOiO7bu_VrxnHOeLgUeqiADQPcgtMW=A@mail.gmail.com","threadId":"58609","inReplyTo":"xmqq34q27wzg.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-31T20:43:33Z","receivedAt":"2024-05-31T20:43:48Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, May 28, 2024 at 5:54 PM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n\n> > \"it would be nice if there was a capability for the client to say\n> > that it would like the server to give it information about the\n> > promisor that it could use, so that the user doesn't have to pass all\n> > the \"remote.my_promisor.XXX\" config options on the command like.\"\n> >\n> > and by saying that this could be added later.\n>\n> Can such an update happen transparently, or does it need changes to\n> end-user experience? It is of dubious value to leave the initial\n> series be incomplete if the latter is the case.\n\nI think adding the capability could happen transparently. I think the\ncapability should be a generic one about the server suggesting some\nconfig options and perhaps some command line options, and the client\nagreeing (or not) to setting the config options and using the command\nline options that the server suggests. Of course the important thing\nwhen implementing this capability is to properly limit the config and\ncommand line options (and perhaps their values) that the client\naccepts to set and use. But I think it could be a generic capability\nthat could be extended to other options in the future. That's why I\nthink it could be a separate patch series.\n\n> >> Without knowing that, it cannot safely decide that it does not\n> >> have to send objects that can be obtained from X to C.\n> >\n> > In the above command C is asking for a partial clone, as it uses a\n> > --filter option. This means that C knows very well that it might not\n> > get from S all the objects needed for a complete object graph.\n>\n> Hmph. If C asks a partial clone and S is willing to be the promisor\n> for C, S is essentially saying that it will serve C any objects on\n> demand that are reachable from any object it served C in the past,\n> forever, no? It might not get from S initially all the objects, but\n> if it wants later, S promises to let C have them.\n\nThis promise is not broken, as S can still get the missing objects\nfrom X and then pass them to C. There is even the following test case\nin the patch that shows that it works when uploadpack.missingAction is\nunset (and thus default to \"error\" which is the same as how things\ncurrently work):\n\n+test_expect_success \"fetch without uploadpack.missingAction=allow\n-promisor\" '\n+ git -C server config --unset uploadpack.missingAction &&\n+\n+ # Clone from server to create a client\n+ GIT_NO_LAZY_FETCH=0 git clone -c remote.server2.promisor=true \\\n+ -c remote.server2.fetch=\"+refs/heads/*:refs/remotes/server2/*\" \\\n+ -c remote.server2.url=\"file://$(pwd)/server2\" \\\n+ --no-local --filter=\"blob:limit=5k\" server client &&\n+ test_when_finished \"rm -rf client\" &&\n+\n+ # Check that the largest object is not missing on the server anymore\n+ check_missing_objects server 0 \"\"\n+'\n\nThe only difference with the case when uploadpack.missingAction is set\nto \"allow-promisor\" is that then C gets the missing objects directly\nfrom X without S getting them first. This is why there is:\n\n+ # Check that the largest object is still missing on the server\n+ check_missing_objects server 1 \"$oid\"\n\nat the end of the test case when uploadpack.missingAction is set to\n\"allow-promisor\". This is to check that S didn't get the missing\nobject (which means that the missing object went directly from X to\nC).\n\nSo S keeps its promise to let C have any object through S if C wants.\nIt's just that having large objects through S (instead of directly\nfrom X) is not a good idea because it copies the large objects to S\nfirst which uses up space (and memory and CPU when creating packfiles)\non S while the objects are already on X where disk space is probably\ncheaper (and where less CPU and memory will be used to send the\nobjects).\n\n> > Again when using a regular partial clone omitting the same set of\n> > objects, C also requests some objects that S doesn't have. And\n> > this is not considered an issue or something irresponsible. It\n> > already works like this.\n>\n> \"S doesn't have\" is because S has pruned objects that it shouldn't\n> have in order to keep the promise it made earlier to C, right? If\n> that is the case, I would very much say S is being irresponsible in\n> that case.\n\nYes, S has pruned some objects, but these objects have been moved to X\nfirst and S has access to X, so S can still get the objects. So I\ndon't understand why you say S shouldn't have pruned them and S is\nirresponsible. S is still capable of fulfilling the promise it made.\n\nAnd even if S was not capable of directly fulfilling the promise it\nmade, it would still not be clear to me that S is irresponsible, as S\nand X can be seen as a single entity from the client point of view,\nand S and X together would still be capable of fulfilling the promise\nthat was made to C.\n\nWhen bundle-uri is used, for example, the server is not fulfilling the\npromise alone. It needs the bundle server to work properly for the\nclient to get everything. When using Git LFS, the server is not\nfulfilling the promise alone either. For things to work properly both\nthe Git server and the LFS server have to work together.\n\nSo if it's Ok for other features to require an additional server to\nfulfill the promise, why is it not Ok in the case of S + X?\n\n> > And then C still has the possibility to configure X as a\n> > promisor remote and get missing objects from there. So why is it Ok\n> > when it's done in several steps but not in one?\n>\n> You are right that S can decide to unilaterally break the promise it\n> made C, so this update is not making it any worse than giving users\n> a broken implementation of promisor remotes. I wouldn't call it OK,\n> though.\n\nI don't understand why you compare this to a \"broken\" implementation\nof promisor remotes. What could then be a non-broken one that would\nstore large blobs on a separate server in your opinion? I am really\ninterested in answers to this question. It's not a rhetorical one.\n\n> If somebody identifies that even without this series, S can lead to\n> repository corruption at C by pruning objects it does need to keep\n> its promise to C,\n\nObjects are not pruned if you consider S + X instead of just S. It's\nthe same as when people are using Git LFS or bundle-uri. Nothing is\nreally pruned on the server side, no promise is broken. Some objects\nare just moved to a separate server that is sometimes called to\nprovide some objects on top of those the Git server still sends.\n\n> the next action I expect from healthy project is\n> to try coming up with a mechanism to make it less likely that such a\n> pruning happens by accident\n\nWhen people managing server S decide to move some objects to X and make S\naccess X, it's not accidental pruning. But maybe you are talking\nabout the case when uploadpack.missingAction is set to \"allow-any\"\ninstead of \"allow-promisor\"?\n\n> (e.g., by noticing allowAnySHA1InWant as\n> a sign that the repository has promised others to serve anything\n> that used to be reachable from anything it historically served,\n> disabling repack \"-d\" and instead send the currently unreachable\n> objects to an archived pack, and something equally silly like that).\n> It certainly is not to add a new mechanism to make it even easier to\n> configure S to break promised it made to C.\n\nThis new mechanism has some important benefits. Yes, there are risks,\nbut it's our opinion at GitLab, with some data from GitLab servers\n(see the link I previously sent to a GitLab issue) to back it up, that\nthe rewards are well worth the added risks.\n\nI agree that ideally a Git server should be able to handle every kind\nof object very well, but that's unfortunately not the case. Git LFS\nhas some drawbacks, bundle-uri also has some drawbacks, but there are\nreasons why they exist. Multiple promisor remote is a solution that\nalready exists in Git too. Yes, it also has some drawbacks, but they\nare in many ways similar to those of bundle-uri and Git LFS. Yes, this\npatch is making multiple promisor remotes easier to use, but it\ndoesn't fundamentally change the risks associated with using multiple\npromisor remotes.\n"},{"id":"496037","messageId":"xmqqwmn9athc.fsf@gitster.g","threadId":"58609","inReplyTo":"CAP8UFD18Y=NhnnzcHoBOiO7bu_VrxnHOeLgUeqiADQPcgtMW=A@mail.gmail.com","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-01T09:43:27Z","receivedAt":"2024-06-01T09:43:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>> Hmph. If C asks a partial clone and S is willing to be the promisor\n>> for C, S is essentially saying that it will serve C any objects on\n>> demand that are reachable from any object it served C in the past,\n>> forever, no? It might not get from S initially all the objects, but\n>> if it wants later, S promises to let C have them.\n>\n> This promise is not broken, as S can still get the missing objects\n> from X and then pass them to C. There is even the following test case\n> in the patch that shows that it works when uploadpack.missingAction is\n> unset (and thus default to \"error\" which is the same as how things\n> currently work):\n\nAnd the whole point of that configuration is to make it easier for S\nto break that promise, no? uploadPack.missingAction is set at S and\nis not under control of C, right?\n\n> So S keeps its promise to let C have any object through S if C wants.\n> It's just that having large objects through S (instead of directly\n> from X) is not a good idea ...\n> ...\n> So if it's Ok for other features to require an additional server to\n> fulfill the promise, why is it not Ok in the case of S + X?\n\nI am questioning the design that does not give C any say in the\ndecision if it is a good idea or not to ask S relay large objects.\nS unilaterally decides that it does not want to and does not serve\nsuch large objects to C, and without even checking with C if it can\nreach X to fetch directly, silently assuming that C will do so,\nright? It is quite different from the contract between C and S in\nthe simpler world.\n\n> I don't understand why you compare this to a \"broken\" implementation\n> of promisor remotes. What could then be a non-broken one that would\n> store large blobs on a separate server in your opinion? I am really\n> interested in answers to this question. It's not a rhetorical one.\n\nYou as S would tell C \"I want you to go to X because I am not\nserving objects X and Y\". Or at least make sure that C knows about\nX before deciding to omit what X ought to have. Not doing anything\nand silently assuming that C will get them from elsewhere is simply\nirresponsible, especially if C is not even told about X over the\nprotocol, no?\n\n"},{"id":"496153","messageId":"CAP8UFD0aiBY27_8HqjyMtze1ZcME2gZVTMyB=botNErmu_CJDg@mail.gmail.com","threadId":"58609","inReplyTo":"xmqqwmn9athc.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-06-03T15:01:10Z","receivedAt":"2024-06-03T15:01:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Jun 1, 2024 at 11:43 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n\n> > I don't understand why you compare this to a \"broken\" implementation\n> > of promisor remotes. What could then be a non-broken one that would\n> > store large blobs on a separate server in your opinion? I am really\n> > interested in answers to this question. It's not a rhetorical one.\n>\n> You as S would tell C \"I want you to go to X because I am not\n> serving objects X and Y\". Or at least make sure that C knows about\n> X before deciding to omit what X ought to have.\n\nOk, so if there was a way for S to suggest config options and perhaps\nalso command line options to C (like I mentioned in a previous email),\nand if S would suggest C to configure X as a promisor remote and C\nwould accept to do that, then it would be fine for the feature to\nwork, right?\n\nAlternatively, if C would pass a new option called for example\n--known-promisor=X on top of all other options, then that could be Ok\ntoo?\n"},{"id":"496165","messageId":"xmqq1q5e3pf0.fsf@gitster.g","threadId":"58609","inReplyTo":"CAP8UFD0aiBY27_8HqjyMtze1ZcME2gZVTMyB=botNErmu_CJDg@mail.gmail.com","subject":"Re: [PATCH v2 3/3] upload-pack: allow configuring a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-03T17:29:55Z","receivedAt":"2024-06-03T17:30:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Alternatively, if C would pass a new option called for example\n> --known-promisor=X on top of all other options, then that could be Ok\n> too?\n\nWhy do you need a new option \"--known-promisor=X\" in the first\nplace? Doesn't it known by the repository already and you do not\nhave to bother the end user to supply it from the command line, no?\n\nIn any case, a protocol extension that lets S tell C that S wants C\nto fetch missing objects from X (which means that if C knows about X\nin its \".git/config\" then there is no need for end-user interaction\nat all), or a protocol extension that C tells S that C is willing to\nsee objects available from X omitted when S does not have them\n(again, this could be done by looking at \".git/config\" at C, but\nthere may be security implications???). Either would work OK and I\noffhand do not see much preference between the two. \n\nWhat is important is to ensure that such deliberate omission by S is\nsafe and acceptable by C.\n\nThanks.\n\n\n[Footnote]\n\nBy the way, I briefly wondered: would \"X\" that is valid for \"C\" be\nalways valid for \"S\" these days?\n\nI as \"S\" may prefer you as \"C\" to go ssh://gitlab.com/x.git but you\nmay only be able to go https://gitlab.com/x.git for networking\nreasons. Silly situation like that may make communicating \"X\"\nbetween \"S\" and \"C\" a bit harder than we would like. Of course this\nis not a high priority issue as the user with such networking need\nare probably already using url.X.insteadof to rewrite the submodule\npaths recommended in .gitmodules after they clone a superproject,\niow, even if it were a problem, it is a solved one.\n\nSo, in short, yes, exchanging the known promisor \"X\" between \"S\" and\n\"C\", regardless of which side speaks first, as long as the negotiation\nmakes sure both sides agree, would be a good direction to go, I\nwould think.\n\n\n"}]}