{"thread":{"id":"61334","subject":"[PATCH 0/4] upload-pack: support a missing-action","startedAt":"2024-04-18T18:41:03Z","lastAt":"2024-05-24T18:25:50Z","messageCount":13,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"493148","messageId":"20240418184043.2900955-1-christian.couder@gmail.com","threadId":"61334","inReplyTo":null,"subject":"[PATCH 0/4] upload-pack: support a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-04-18T18:40:39Z","receivedAt":"2024-04-18T18:41:03Z","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=<missing-action>`\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\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\nAs `git rev-list` also supports a `--missing=<missing-action>` option,\nthe first 3 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 4/4 then adds a new\n`uploadpack.missingAction` configuration variable.\n\nChristian Couder (4):\n  rev-list: refactor --missing=<missing-action>\n  missing: support rejecting --missing=print\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              |  46 +++++-----\n builtin/rev-list.c                  |  41 ++-------\n missing.c                           |  42 ++++++++++\n missing.h                           |  20 +++++\n t/t5706-upload-pack-missing.sh      | 125 ++++++++++++++++++++++++++++\n upload-pack.c                       |  19 +++++\n 8 files changed, 244 insertions(+), 59 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.44.0.655.g111bceeb19\n\n"},{"id":"493149","messageId":"20240418184043.2900955-2-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240418184043.2900955-1-christian.couder@gmail.com","subject":"[PATCH 1/4] rev-list: refactor --missing=<missing-action>","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-04-18T18:40:40Z","receivedAt":"2024-04-18T18:41:04Z","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=<missing-action>` feature, but they currently don't share\nany code for that.\n\nIn a following commit, we are going to add support for a similar\n'missing-action' feature to another command. To avoid duplicating\nsimilar code, let's start refactoring the rev-list code for this\nfeature into new \"missing.{c,h}\" files.\n\nFor now, this refactoring is about moving code from\n\"builtin/rev-list.c\" into new \"missing.{c,h}\" files. But in a\nfollowing commit, that refactored code will be used in\n`git pack-objects` too.\n\nAs `enum missing_action` and parse_missing_action_value() are moved to\n\"missing.{c,h}\", we need to modify the later a bit, so that it stops\nupdating a global variable, but instead returns either -1 on error or\nthe parsed value otherwise.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Makefile           |  1 +\n builtin/rev-list.c | 41 ++++++-----------------------------------\n missing.c          | 26 ++++++++++++++++++++++++++\n missing.h          | 18 ++++++++++++++++++\n 4 files changed, 51 insertions(+), 35 deletions(-)\n create mode 100644 missing.c\n create mode 100644 missing.h\n\ndiff --git a/Makefile b/Makefile\nindex 1e31acc72e..75583a71a0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1076,6 +1076,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..f71cc5ebe1 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 [<options>] <commit>... [--] [<path>...]\\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,14 @@ 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\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..83e0c5e584\n--- /dev/null\n+++ b/missing.c\n@@ -0,0 +1,26 @@\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\tfetch_if_missing = 0;\n+\t\treturn MA_ALLOW_ANY;\n+\t}\n+\n+\tif (!strcmp(value, \"print\")) {\n+\t\tfetch_if_missing = 0;\n+\t\treturn MA_PRINT;\n+\t}\n+\n+\tif (!strcmp(value, \"allow-promisor\")) {\n+\t\tfetch_if_missing = 0;\n+\t\treturn MA_ALLOW_PROMISOR;\n+\t}\n+\n+\treturn -1;\n+}\ndiff --git a/missing.h b/missing.h\nnew file mode 100644\nindex 0000000000..c8261dfe55\n--- /dev/null\n+++ b/missing.h\n@@ -0,0 +1,18 @@\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. Also sets the fetch_if_missing global variable\n+  from \"object-file.h\".\n+ */\n+int parse_missing_action_value(const char *value);\n+\n+#endif /* MISSING_H */\n-- \n2.44.0.655.g111bceeb19\n\n"},{"id":"493150","messageId":"20240418184043.2900955-3-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240418184043.2900955-1-christian.couder@gmail.com","subject":"[PATCH 2/4] missing: support rejecting --missing=print","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-04-18T18:40:41Z","receivedAt":"2024-04-18T18:41:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"`git pack-objects` supports the `--missing=<missing-action>` option in\nthe same way as `git rev-list` except when '<missing-action>' is\n\"print\", which `git pack-objects` doesn't support.\n\nAs we want to refactor `git pack-objects` to use the same code from\n\"missing.{c,h}\" as `git rev-list` for the `--missing=...` feature, let's\nmake it possible for that code to reject `--missing=print`.\n\n`git pack-objects` will then use that code in a following commit.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/rev-list.c | 2 +-\n missing.c          | 4 ++--\n missing.h          | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex f71cc5ebe1..a712a6fd62 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -539,7 +539,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\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\tres = parse_missing_action_value(arg);\n+\t\t\tres = parse_missing_action_value(arg, 1);\n \t\t\tif (res >= 0) {\n \t\t\t\targ_missing_action = res;\n \t\t\t\tbreak;\ndiff --git a/missing.c b/missing.c\nindex 83e0c5e584..d306dea2d9 100644\n--- a/missing.c\n+++ b/missing.c\n@@ -2,7 +2,7 @@\n #include \"missing.h\"\n #include \"object-file.h\"\n \n-int parse_missing_action_value(const char *value)\n+int parse_missing_action_value(const char *value, int print_ok)\n {\n \tif (!strcmp(value, \"error\"))\n \t\treturn MA_ERROR;\n@@ -12,7 +12,7 @@ int parse_missing_action_value(const char *value)\n \t\treturn MA_ALLOW_ANY;\n \t}\n \n-\tif (!strcmp(value, \"print\")) {\n+\tif (!strcmp(value, \"print\") && print_ok) {\n \t\tfetch_if_missing = 0;\n \t\treturn MA_PRINT;\n \t}\ndiff --git a/missing.h b/missing.h\nindex c8261dfe55..77906691e7 100644\n--- a/missing.h\n+++ b/missing.h\n@@ -13,6 +13,6 @@ enum missing_action {\n   if parsing failed. Also sets the fetch_if_missing global variable\n   from \"object-file.h\".\n  */\n-int parse_missing_action_value(const char *value);\n+int parse_missing_action_value(const char *value, int print_ok);\n \n #endif /* MISSING_H */\n-- \n2.44.0.655.g111bceeb19\n\n"},{"id":"493151","messageId":"20240418184043.2900955-4-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240418184043.2900955-1-christian.couder@gmail.com","subject":"[PATCH 3/4] pack-objects: use the missing action API","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-04-18T18:40:42Z","receivedAt":"2024-04-18T18:41:08Z","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=<missing-action>` 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 <chriscool@tuxfamily.org>\n---\n builtin/pack-objects.c | 46 ++++++++++++++++++++----------------------\n 1 file changed, 22 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex baf0090fc8..bce586b9a6 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,35 @@ 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-\n-\tif (!strcmp(arg, \"allow-any\")) {\n-\t\targ_missing_action = MA_ALLOW_ANY;\n-\t\tfetch_if_missing = 0;\n-\t\tfn_show_object = show_object__ma_allow_any;\n-\t\treturn 0;\n-\t}\n+\tres = parse_missing_action_value(arg, 0);\n+\tif (res < 0)\n+\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\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.44.0.655.g111bceeb19\n\n"},{"id":"493152","messageId":"20240418184043.2900955-5-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240418184043.2900955-1-christian.couder@gmail.com","subject":"[PATCH 4/4] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-04-18T18:40:43Z","receivedAt":"2024-04-18T18:41:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <chriscool@tuxfamily.org>\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 missing-action works in a similar way as what `git rev-list` and\n`git pack-objects` already support with their\n`--missing=<missing-action>` 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=<missing-action>` to `git pack-objects`.\n\nSo the `uploadpack.missingAction` has the same limitations as\n`git pack-objects --missing=<missing-action>`. Especially when not\nusing promisor remotes, 'allow-any' works only for blobs.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\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=<missing-action>` 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 d306dea2d9..022ebbe4be 100644\n--- a/missing.c\n+++ b/missing.c\n@@ -24,3 +24,19 @@ int parse_missing_action_value(const char *value, int print_ok)\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 77906691e7..ca574f83a0 100644\n--- a/missing.h\n+++ b/missing.h\n@@ -15,4 +15,6 @@ enum missing_action {\n  */\n int parse_missing_action_value(const char *value, int print_ok);\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..21bb6aed2b\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\" <all.txt >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 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 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..c355ebe1e5 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, 0);\n+\t\tif (res < 0)\n+\t\t\tdie(_(\"invalid value for '%s': '%s'\"), var, value);\n+\t\t/*\n+\t\t * parse_missing_action_value() unsets fetch_if_missing\n+\t\t * but if we allow promisor we want to still fetch from\n+\t\t * the promisor remote\n+\t\t */\n+\t\tif (res == MA_ALLOW_PROMISOR)\n+\t\t\tfetch_if_missing =1;\n+\t\tdata->missing_action = res;\n \t}\n \n \tif (parse_object_filter_config(var, value, ctx->kvi, data) < 0)\n-- \n2.44.0.655.g111bceeb19\n\n"},{"id":"493153","messageId":"xmqqo7a6tr1k.fsf@gitster.g","threadId":"61334","inReplyTo":"20240418184043.2900955-1-christian.couder@gmail.com","subject":"Re: [PATCH 0/4] upload-pack: support a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T19:21:59Z","receivedAt":"2024-04-18T19:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> `git pack-objects` already supports a `--missing=<missing-action>`\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\nIs it interesting?  In what way?\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\nIt is unclear what the precondition for such an arrangement to work\nreliably, and a lot more importantly, how we can validate that the\nprecondition holds when \"fetch\" talks to \"upload-pack\".  If you get\nit wrong, you'd have a server that would corrupt repositories that\nfetch from it.\n\nThat is where my \"Is it really interesting?  I do not find your\nexplanation convincing yet.\" above primarily comes from.\n\n\nPresumably \"fetch\" could tell \"upload-pack\" something like:\n\n\tI know how to fetch missing objects from this and that\n\tpromisor remotes, so if you choose to, you may omit objects\n\tthat you know are available from these promisor remotes when\n\tsending objects to me.\n\nusing a new capability, and we can allow upload-pack to omit objects\nonly when such a new capability tells it to?\n"},{"id":"493160","messageId":"xmqqplumpczs.fsf@gitster.g","threadId":"61334","inReplyTo":"20240418184043.2900955-2-christian.couder@gmail.com","subject":"Re: [PATCH 1/4] rev-list: refactor --missing=<missing-action>","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T21:39:03Z","receivedAt":"2024-04-18T21:39:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> Both `git rev-list` and `git pack-objects` support a\n> `--missing=<missing-action>` feature, but they currently don't share\n> any code for that.\n\nSo \"git pack-objects\" will still stay unaware of <missing.h> after\nthis step, which was a bit unexpected.\n\n> For now, this refactoring is about moving code from\n> \"builtin/rev-list.c\" into new \"missing.{c,h}\" files. But in a\n> following commit, that refactored code will be used in\n> `git pack-objects` too.\n\nI think it is easier to grok if you said this in the second\nparagraph, before mentioning \"another command\".  The first paragraph\ntalks about rev-list and pack-objects logically but not phisically\nsharing the feature, so with \"For now, this refactoring is about\nmoving ... into\" -> \"Refactor the support for --missing in rev-list\ninto\", it is enough to explain how this change is a first step to\nmake things better, without bringing the third thing into picture.\n\nIOW ...\n\n> In a following commit, we are going to add support for a similar\n> 'missing-action' feature to another command. To avoid duplicating\n> similar code, let's start refactoring the rev-list code for this\n> feature into new \"missing.{c,h}\" files.\n\n... this paragraph should have much lower weight in explaining this\ncommit.\n\n> As `enum missing_action` and parse_missing_action_value() are moved to\n> \"missing.{c,h}\", we need to modify the later a bit, so that it stops\n\n\"later\" -> \"latter\"?\n\n> updating a global variable, but instead returns either -1 on error or\n> the parsed value otherwise.\n\nOK.  As a shared helper function, I agree that assignment to a global\nshould be outside of its responsibility.  Which means that the caller\nis now responsible for making that assignment.\n\n>  static int try_bitmap_count(struct rev_info *revs,\n>  \t\t\t    int filter_provided_objects)\n>  {\n> @@ -569,10 +536,14 @@ 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\targ_missing_action = res;\n>  \t\t\t\tbreak;\n> +\t\t\t}\n\n... which seems to be missing from here.\n\n>  \t\t}\n>  \t}\n>  \n> diff --git a/missing.c b/missing.c\n> new file mode 100644\n> index 0000000000..83e0c5e584\n> --- /dev/null\n> +++ b/missing.c\n> @@ -0,0 +1,26 @@\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\tfetch_if_missing = 0;\n> +\t\treturn MA_ALLOW_ANY;\n> +\t}\n> +\n> +\tif (!strcmp(value, \"print\")) {\n> +\t\tfetch_if_missing = 0;\n> +\t\treturn MA_PRINT;\n> +\t}\n> +\n> +\tif (!strcmp(value, \"allow-promisor\")) {\n> +\t\tfetch_if_missing = 0;\n> +\t\treturn MA_ALLOW_PROMISOR;\n> +\t}\n\n... and this one still touches the global.\n\n> +\treturn -1;\n> +}\n> diff --git a/missing.h b/missing.h\n> new file mode 100644\n> index 0000000000..c8261dfe55\n> --- /dev/null\n> +++ b/missing.h\n> @@ -0,0 +1,18 @@\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. Also sets the fetch_if_missing global variable\n> +  from \"object-file.h\".\n> + */\n\n... and this also mentions the global.\n\n> +int parse_missing_action_value(const char *value);\n> +\n> +#endif /* MISSING_H */\n\n"},{"id":"493161","messageId":"xmqqfrvipcm9.fsf@gitster.g","threadId":"61334","inReplyTo":"20240418184043.2900955-3-christian.couder@gmail.com","subject":"Re: [PATCH 2/4] missing: support rejecting --missing=print","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-18T21:47:10Z","receivedAt":"2024-04-18T21:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> `git pack-objects` supports the `--missing=<missing-action>` option in\n> the same way as `git rev-list` except when '<missing-action>' is\n> \"print\", which `git pack-objects` doesn't support.\n>\n> As we want to refactor `git pack-objects` to use the same code from\n> \"missing.{c,h}\" as `git rev-list` for the `--missing=...` feature, let's\n> make it possible for that code to reject `--missing=print`.\n>\n> `git pack-objects` will then use that code in a following commit.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  builtin/rev-list.c | 2 +-\n>  missing.c          | 4 ++--\n>  missing.h          | 2 +-\n>  3 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index f71cc5ebe1..a712a6fd62 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -539,7 +539,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\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\tres = parse_missing_action_value(arg);\n> +\t\t\tres = parse_missing_action_value(arg, 1);\n\nHmph, this smells like a horribly unscalable design, as we make the\nvocabulary of missing-action richer, you'd end up piling on \"this\nchoice is allowed in this call\" parameters, wouldn't you?  The first\nperson who adds such an ad-hoc parameter would say \"hey, what's just\none extra parameter print_ok between friends\", but the next person\nwould say the same thing for their new choice and adds frotz_ok, and\nwe'd be in chaos.\n\nRather, shouldn't the _caller_ decide if the parsed value is\nsomething it does not like and barf?\n\nAlternatively, add a _single_ \"reject\" bitset and do something like\n\n\tint parse_missing_action_value(const char *value, unsigned reject)\n\t{\n\t\t...\n\t\tif (!(reject & (1<<MA_ERROR) && !strcmp(value, \"error\")))\n\t\t\treturn MA_ERROR;\n\t\tif (!(reject & (1<<MA_PRINT) && !strcmp(value, \"print\")))\n\t\t\treturn MA_PRINT;\n\t\t...\n\nwhich would scale better (but still my preference is to have the\ncaller deal with only the values it recognises---do not make the\ncaller say \"if (res >= 0 && res != MA_PRINT)\" as that will not scale\nwhen new choices that are accepted elsewhere are added.\n"},{"id":"495572","messageId":"20240524163926.2019648-1-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240418184043.2900955-1-christian.couder@gmail.com","subject":"[PATCH v3 0/3] upload-pack: support a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-24T16:39:23Z","receivedAt":"2024-05-24T16:39:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Special note and links\n======================\n\nThis v3 is sent in reply to the v1 as the v2 was mistakenly sent to an\nunrelated patch series. \n\nv1: https://lore.kernel.org/git/20240418184043.2900955-1-christian.couder@gmail.com/\nv2: https://lore.kernel.org/git/20240515132543.851987-1-christian.couder@gmail.com/\n\nRationale\n=========\n\n`git pack-objects` already supports a `--missing=<missing-action>`\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=<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=<missing-action>` 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\nChanges between v2 and v3\n=========================\n\nThe changes since v2 are the following:\n\n  - In patch 2/3, the show_object_fn_from_action() function was\n    replaced by using a `static show_object_fn const fn[]`.\n\n  - In patch 2/3, a new parse_missing_action_value_for_packing()\n    function was introduced in the missing action API to simplify\n    pack-object's code.\n\n  - In patch 3/3, the new parse_missing_action_value_for_packing()\n    function is used too.\n\n  - In patch 3/3, the commit message has been improved to make it more\n    assertive and clarify how the new feature can be used in the\n    context of promisor remotes.\n\n  - In patch 3/3, `TEST_PASSES_SANITIZE_LEAK=true` was removed in\n    't/t5706-upload-pack-missing.sh' as leak tests don't pass\n    otherwise. Leaks seems to be in existing config and promisor\n    related code.\n\n  - In patch 3/3, `sed -n \"s/^\\?\\(.*\\)/\\1/p\"` was replaced with\n    `perl -ne 'print if s/^[?]//'` in 't/t5706-upload-pack-missing.sh'\n    as the former doesn't seem to work in our \"linux-musl (alpine)\"\n    tests.\n\nThanks to Junio for his reviews of v1 and v3, and for suggesting the\nmost of the above changes.\n\nRange diff between v2 and v3\n============================\n\n(Might not be super useful as changes in patch 2/3 are not seen.)\n\n1:  0a961dd4f5 = 1:  67c761b08a rev-list: refactor --missing=<missing-action>\n2:  410acc6a39 < -:  ---------- pack-objects: use the missing action API\n-:  ---------- > 2:  7bf04f3096 pack-objects: use the missing action API\n3:  0f5efb064b ! 3:  bac909a070 upload-pack: allow configuring a missing-action\n    @@ Metadata\n      ## Commit message ##\n         upload-pack: allow configuring a missing-action\n     \n    -    In case some objects are missing from a server, it might still be\n    +    In case some objects are missing from a server, it is sometimes\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    -    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    +    Suppose repository S borrows from its \"promisor\" X, and repository C\n    +    which initially cloned from S borrows from its \"promisor\" S. If C\n    +    wants an object in order to fill in the gap in its object graph, and\n    +    S does not have it (as S itself has no need for that object), then it\n    +    makes sense to let C go directly to X bypassing S.\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    @@ Documentation/config/uploadpack.txt: uploadpack.allowRefInWant::\n     +  still get them from somewhere else.\n     \n      ## missing.c ##\n    -@@ missing.c: int parse_missing_action_value(const char *value)\n    - \n    -   return -1;\n    +@@ missing.c: int parse_missing_action_value_for_packing(const char *value)\n    +           return -2 - res;\n    +   }\n      }\n     +\n     +const char *missing_action_to_string(enum missing_action action)\n    @@ missing.c: int parse_missing_action_value(const char *value)\n     +}\n     \n      ## missing.h ##\n    -@@ missing.h: enum missing_action {\n    - */\n    - int parse_missing_action_value(const char *value);\n    +@@ missing.h: int parse_missing_action_value(const char *value);\n    +  */\n    + int parse_missing_action_value_for_packing(const char *value);\n      \n    ++/* Return a short string literal describing the action. */\n     +const char *missing_action_to_string(enum missing_action action);\n     +\n      #endif /* MISSING_H */\n    @@ t/t5706-upload-pack-missing.sh (new)\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    @@ t/t5706-upload-pack-missing.sh (new)\n     +\n     +check_missing_objects () {\n     +  git -C \"$1\" rev-list --objects --all --missing=print > all.txt &&\n    -+  sed -n \"s/^\\?\\(.*\\)/\\1/p\" <all.txt >missing.txt &&\n    ++  perl -ne 'print if s/^[?]//' all.txt >missing.txt &&\n     +  test_line_count = \"$2\" missing.txt &&\n     +  test \"$3\" = \"$(cat missing.txt)\"\n     +}\n    @@ upload-pack.c: static int upload_pack_config(const char *var, const char *value,\n        } else if (!strcmp(\"transfer.advertisesid\", var)) {\n                data->advertise_sid = git_config_bool(var, value);\n     +  } else if (!strcmp(\"uploadpack.missingaction\", var)) {\n    -+          int res = parse_missing_action_value(value);\n    -+          if (res < 0 || (res != MA_ERROR &&\n    -+                          res != MA_ALLOW_ANY &&\n    -+                          res != MA_ALLOW_PROMISOR))\n    ++          int res = parse_missing_action_value_for_packing(value);\n    ++          if (res < 0)\n     +                  die(_(\"invalid value for '%s': '%s'\"), var, value);\n     +          /* Allow fetching only from promisor remotes */\n     +          if (res == MA_ALLOW_PROMISOR)\n\n\nChristian Couder (3):\n  rev-list: refactor --missing=<missing-action>\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              |  37 ++++----\n builtin/rev-list.c                  |  43 ++--------\n missing.c                           |  53 ++++++++++++\n missing.h                           |  28 +++++++\n t/t5706-upload-pack-missing.sh      | 125 ++++++++++++++++++++++++++++\n upload-pack.c                       |  17 ++++\n 8 files changed, 255 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.219.g07663fd880\n\n"},{"id":"495573","messageId":"20240524163926.2019648-2-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240524163926.2019648-1-christian.couder@gmail.com","subject":"[PATCH v3 1/3] rev-list: refactor --missing=<missing-action>","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-24T16:39:24Z","receivedAt":"2024-05-24T16:39:45Z","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=<missing-action>` feature, but they currently don't share\nany code for that.\n\nRefactor the support for `--missing=<missing-action>` 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 <chriscool@tuxfamily.org>\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 cf504963c2..870dea2e9d 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1063,6 +1063,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 [<options>] <commit>... [--] [<path>...]\\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.219.gbac909a070\n\n"},{"id":"495574","messageId":"20240524163926.2019648-3-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240524163926.2019648-1-christian.couder@gmail.com","subject":"[PATCH v3 2/3] pack-objects: use the missing action API","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-24T16:39:25Z","receivedAt":"2024-05-24T16:39:47Z","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=<missing-action>` 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 parse_missing_action_value_for_packing()\nfunction in the missing action API as `git pack-objects` doesn't\nsupport the MA_PRINT missing action and other commands using the API\nin the future might not support it either.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/pack-objects.c | 37 ++++++++++++++-----------------------\n missing.c              | 17 +++++++++++++++++\n missing.h              |  8 ++++++++\n 3 files changed, 39 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cd2396896d..fad16e0ce2 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@@ -3830,30 +3826,25 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\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+\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 \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-\n-\tif (!strcmp(arg, \"allow-any\")) {\n-\t\targ_missing_action = MA_ALLOW_ANY;\n-\t\tfetch_if_missing = 0;\n-\t\tfn_show_object = show_object__ma_allow_any;\n-\t\treturn 0;\n-\t}\n+\tres = parse_missing_action_value_for_packing(arg);\n+\tif (res < 0 || ARRAY_SIZE(fn) <= res)\n+\t\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n \n-\tif (!strcmp(arg, \"allow-promisor\")) {\n-\t\targ_missing_action = MA_ALLOW_PROMISOR;\n+\tfn_show_object = fn[res];\n+\targ_missing_action = res;\n+\tif (res != MA_ERROR)\n \t\tfetch_if_missing = 0;\n-\t\tfn_show_object = show_object__ma_allow_promisor;\n-\t\treturn 0;\n-\t}\n \n-\tdie(_(\"invalid value for '%s': '%s'\"), \"--missing\", arg);\n \treturn 0;\n }\n \ndiff --git a/missing.c b/missing.c\nindex ce3cf734a8..a0c2800d30 100644\n--- a/missing.c\n+++ b/missing.c\n@@ -18,3 +18,20 @@ int parse_missing_action_value(const char *value)\n \n \treturn -1;\n }\n+\n+int parse_missing_action_value_for_packing(const char *value)\n+{\n+\tint res = parse_missing_action_value(value);\n+\n+\tif (res < 0)\n+\t\treturn -1;\n+\n+\tswitch (res) {\n+\tcase MA_ERROR:\n+\tcase MA_ALLOW_ANY:\n+\tcase MA_ALLOW_PROMISOR:\n+\t\treturn res;\n+\tdefault:\n+\t\treturn -2 - res;\n+\t}\n+}\ndiff --git a/missing.h b/missing.h\nindex 1e378d6215..cdfd522852 100644\n--- a/missing.h\n+++ b/missing.h\n@@ -14,4 +14,12 @@ enum missing_action {\n */\n int parse_missing_action_value(const char *value);\n \n+/*\n+  Return a 'res' with the following meaning:\n+    0 <= res : an MA_FOO value that is OK for packing\n+    -1 = res : parse_missing_action_value() failed\n+    -1 > res : (2 - res) is an MA_FOO value which is unsuitable for packing\n+ */\n+int parse_missing_action_value_for_packing(const char *value);\n+\n #endif /* MISSING_H */\n-- \n2.45.1.219.gbac909a070\n\n"},{"id":"495575","messageId":"20240524163926.2019648-4-christian.couder@gmail.com","threadId":"61334","inReplyTo":"20240524163926.2019648-1-christian.couder@gmail.com","subject":"[PATCH v3 3/3] upload-pack: allow configuring a missing-action","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-05-24T16:39:26Z","receivedAt":"2024-05-24T16:39:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In case some objects are missing from a server, it is sometimes\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\nSuppose repository S borrows from its \"promisor\" X, and repository C\nwhich initially cloned from S borrows from its \"promisor\" S. If C\nwants an object in order to fill in the gap in its object graph, and\nS does not have it (as S itself has no need for that object), then it\nmakes sense to let C go directly to X bypassing S.\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=<missing-action>` 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=<missing-action>` to `git pack-objects`.\n\nSo this new missing-action has the same limitations as\n`git pack-objects --missing=<missing-action>`. 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 <chriscool@tuxfamily.org>\n---\n Documentation/config/uploadpack.txt |   9 ++\n missing.c                           |  16 ++++\n missing.h                           |   3 +\n t/t5706-upload-pack-missing.sh      | 124 ++++++++++++++++++++++++++++\n upload-pack.c                       |  17 ++++\n 5 files changed, 169 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=<missing-action>` 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 a0c2800d30..814cab5326 100644\n--- a/missing.c\n+++ b/missing.c\n@@ -35,3 +35,19 @@ int parse_missing_action_value_for_packing(const char *value)\n \t\treturn -2 - res;\n \t}\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 cdfd522852..3a4659e546 100644\n--- a/missing.h\n+++ b/missing.h\n@@ -22,4 +22,7 @@ int parse_missing_action_value(const char *value);\n  */\n int parse_missing_action_value_for_packing(const char *value);\n \n+/* Return a short string literal describing the action. */\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..1a9b06d84e\n--- /dev/null\n+++ b/t/t5706-upload-pack-missing.sh\n@@ -0,0 +1,124 @@\n+#!/bin/sh\n+\n+test_description='handling of missing objects in upload-pack'\n+\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+\tperl -ne 'print if s/^[?]//' all.txt >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 8fbd138515..064996d95e 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@@ -1374,6 +1381,16 @@ 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_for_packing(value);\n+\t\tif (res < 0)\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.219.gbac909a070\n\n"},{"id":"495584","messageId":"xmqq5xv3kqxh.fsf@gitster.g","threadId":"61334","inReplyTo":"20240524163926.2019648-1-christian.couder@gmail.com","subject":"Re: [PATCH v3 0/3] upload-pack: support a missing-action","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T18:25:46Z","receivedAt":"2024-05-24T18:25:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> The changes since v2 are the following:\n> ...\n>\n> Thanks to Junio for his reviews of v1 and v3, and for suggesting the\n> most of the above changes.\n> ...\n\n>\n> Range diff between v2 and v3\n> ============================\n>\n> (Might not be super useful as changes in patch 2/3 are not seen.)\n>\n> 1:  0a961dd4f5 = 1:  67c761b08a rev-list: refactor --missing=<missing-action>\n> 2:  410acc6a39 < -:  ---------- pack-objects: use the missing action API\n> -:  ---------- > 2:  7bf04f3096 pack-objects: use the missing action API\n> 3:  0f5efb064b ! 3:  bac909a070 upload-pack: allow configuring a missing-action\n>     @@ Metadata\n>       ## Commit message ##\n>          upload-pack: allow configuring a missing-action\n>      \n>     -    In case some objects are missing from a server, it might still be\n>     +    In case some objects are missing from a server, it is sometimes\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>     -    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>     +    Suppose repository S borrows from its \"promisor\" X, and repository C\n>     +    which initially cloned from S borrows from its \"promisor\" S. If C\n>     +    wants an object in order to fill in the gap in its object graph, and\n>     +    S does not have it (as S itself has no need for that object), then it\n>     +    makes sense to let C go directly to X bypassing S.\n\nMost notably, what is still missing in this iteration, even though I\nalready pointed it out in the earlier reviews, is that the readers\nwould not get a good sense of how much trust they need to place on\nthe other side S, in order to save their repositories from getting\ncorrupted by S sending an incomplete pack, and what mechanism there\nalready is to make sure missing objects after fetching such an\nincomplete pack from S are all available at X.\n\nIn short, I agree with the goal of having \"S is borrowing from X, we\ncloned from S, we can fill our missing objects by lazily fetching\ndirectly from X\" as a feature.  But I want to see it as a safe\nfeature, but from these patches I do not see how the necessary\nsafety is guaranteed.\n\nThanks.\n"}]}