{"thread":{"id":"57319","subject":"[PATCH 0/2] repack: add --filter=","startedAt":"2022-01-27T01:49:53Z","lastAt":"2022-02-26T21:44:11Z","messageCount":34,"participants":["John Cai via GitGitGadget","Derrick Stolee","John Cai","Christian Couder","Robert Coup","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"447025","messageId":"pull.1206.git.git.1643248180.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":null,"subject":"[PATCH 0/2] repack: add --filter=","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T01:49:38Z","receivedAt":"2022-01-27T01:49:53Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This patch series aims to make partial clones more useful by allowing repack\nto create packfiles with promisor objects. The longer vision is to be able\nto use partial clones on a git server to offload large blobs to an http\nserver. We can then store large blobs on said http server, and use a remote\nhelper to grab these objects when necessary.\n\nThis is the first step in allowing a repack to honor a filter spec.\n\nJohn Cai (2):\n  pack-objects: allow --filter without --stdout\n  repack: add --filter=<filter-spec> option\n\n Documentation/git-repack.txt   |   5 +\n builtin/pack-objects.c         |   2 -\n builtin/repack.c               |  10 ++\n t/lib-httpd.sh                 |   2 +\n t/lib-httpd/apache.conf        |   8 ++\n t/lib-httpd/list.sh            |  43 +++++++++\n t/lib-httpd/upload.sh          |  46 +++++++++\n t/t0410-partial-clone.sh       |  52 ++++++++++\n t/t0410/git-remote-testhttpgit | 170 +++++++++++++++++++++++++++++++++\n t/t7700-repack.sh              |  20 ++++\n 10 files changed, 356 insertions(+), 2 deletions(-)\n create mode 100644 t/lib-httpd/list.sh\n create mode 100644 t/lib-httpd/upload.sh\n create mode 100755 t/t0410/git-remote-testhttpgit\n\n\nbase-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1206%2Fjohn-cai%2Fjc-repack-filter-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1206/john-cai/jc-repack-filter-v1\nPull-Request: https://github.com/git/git/pull/1206\n-- \ngitgitgadget\n"},{"id":"447026","messageId":"0eec9b117dad5e3cfddf8a17ea74af9a4e23e102.1643248180.git.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.git.git.1643248180.gitgitgadget@gmail.com","subject":"[PATCH 1/2] pack-objects: allow --filter without --stdout","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T01:49:39Z","receivedAt":"2022-01-27T01:49:57Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\n9535ce7 taught pack-objects to use filtering, but added a requirement of\nthe --stdout since a partial clone mechanism was not yet in place to\nhandle missing objects. Since then, changes like 9e27beaa and others\nadded support to dynamically fetch objects that were missing.\n\nRemove the --stdout requirement so that in the next commit, repack can\npass --filter to pack-objects to omit certain objects from the packfile.\n\nBased-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n builtin/pack-objects.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ba2006f2212..2d1ecb18784 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4075,8 +4075,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\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-- \ngitgitgadget\n\n"},{"id":"447027","messageId":"a3166381572481f2ed159740eb8a1d88d4f9dc0f.1643248180.git.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.git.git.1643248180.gitgitgadget@gmail.com","subject":"[PATCH 2/2] repack: add --filter=<filter-spec> option","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-27T01:49:40Z","receivedAt":"2022-01-27T01:49:58Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nCurrently, repack does not work with partial clones. When repack is run\non a partially cloned repository, it grabs all missing objects from\npromisor remotes. This also means that when gc is run for repository\nmaintenance on a partially cloned repository, it will end up getting\nmissing objects, which is not what we want.\n\nIn order to make repack work with partial clone, teach repack a new\noption --filter, which takes a <filter-spec> argument. repack will skip\nany objects that are matched by <filter-spec> similar to how the clone\ncommand will skip fetching certain objects.\n\nThe final goal of this feature, is to be able to store objects on a\nserver other than the regular git server itself.\n\nThere are several scripts added so we can test the process of using a\nremote helper to upload blobs to an http server:\n\n- t/lib-httpd/list.sh lists blobs uploaded to the http server.\n- t/lib-httpd/upload.sh uploads blobs to the http server.\n- t/t0410/git-remote-testhttpgit a remote helper that can access blobs\n  onto from an http server. Copied over from t/t5801/git-remote-testhttpgit\n  and modified to upload blobs to an http server.\n- t/t0410/lib-http-promisor.sh convenience functions for uploading\n  blobs\n\nBased-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n Documentation/git-repack.txt   |   5 +\n builtin/repack.c               |  10 ++\n t/lib-httpd.sh                 |   2 +\n t/lib-httpd/apache.conf        |   8 ++\n t/lib-httpd/list.sh            |  43 +++++++++\n t/lib-httpd/upload.sh          |  46 +++++++++\n t/t0410-partial-clone.sh       |  52 ++++++++++\n t/t0410/git-remote-testhttpgit | 170 +++++++++++++++++++++++++++++++++\n t/t7700-repack.sh              |  20 ++++\n 9 files changed, 356 insertions(+)\n create mode 100644 t/lib-httpd/list.sh\n create mode 100644 t/lib-httpd/upload.sh\n create mode 100755 t/t0410/git-remote-testhttpgit\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex ee30edc178a..e394ec52ab1 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -126,6 +126,11 @@ depth is 4095.\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n \n+--filter=<filter-spec>::\n+\tOmits certain objects (usually blobs) from the resulting\n+\tpackfile. See linkgit:git-rev-list[1] for valid\n+\t`<filter-spec>` 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 da1e364a756..9c2e5bcfe3b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -152,6 +152,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@@ -172,6 +173,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@@ -660,6 +663,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@@ -819,6 +824,11 @@ 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\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n+\t\t\t\t\t\t\tline.buf);\n+\t\t\twrite_promisor_file(promisor_name, NULL, 0);\n+\t\t}\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 782891908d7..fc6587c6d39 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -136,6 +136,8 @@ prepare_httpd() {\n \tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-perl.sh\n+\tinstall_script upload.sh\n+\tinstall_script list.sh\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 497b9b9d927..1ea382750f0 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -129,6 +129,8 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/\n ScriptAlias /error_smart/ error-smart-http.sh/\n ScriptAlias /error/ error.sh/\n ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n+ScriptAlias /upload/ upload.sh/\n+ScriptAlias /list/ list.sh/\n <Directory ${GIT_EXEC_PATH}>\n \tOptions FollowSymlinks\n </Directory>\n@@ -156,6 +158,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n <Files ${GIT_EXEC_PATH}/git-http-backend>\n \tOptions ExecCGI\n </Files>\n+<Files upload.sh>\n+  Options ExecCGI\n+</Files>\n+<Files list.sh>\n+  Options ExecCGI\n+</Files>\n \n RewriteEngine on\n RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]\ndiff --git a/t/lib-httpd/list.sh b/t/lib-httpd/list.sh\nnew file mode 100644\nindex 00000000000..e63406be3b2\n--- /dev/null\n+++ b/t/lib-httpd/list.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+# Used in the httpd test server to be called by a remote helper to list objects.\n+\n+FILES_DIR=\"www/files\"\n+\n+OLDIFS=\"$IFS\"\n+IFS='&'\n+set -- $QUERY_STRING\n+IFS=\"$OLDIFS\"\n+\n+while test $# -gt 0\n+do\n+\tkey=${1%%=*}\n+\tval=${1#*=}\n+\n+\tcase \"$key\" in\n+\t\"sha1\") sha1=\"$val\" ;;\n+\t*) echo >&2 \"unknown key '$key'\" ;;\n+\tesac\n+\n+\tshift\n+done\n+\n+if test -d \"$FILES_DIR\"\n+then\n+\tif test -z \"$sha1\"\n+\tthen\n+\t\techo 'Status: 200 OK'\n+\t\techo\n+\t\tls \"$FILES_DIR\" | tr '-' ' '\n+\telse\n+\t\tif test -f \"$FILES_DIR/$sha1\"-*\n+\t\tthen\n+\t\t\techo 'Status: 200 OK'\n+\t\t\techo\n+\t\t\tcat \"$FILES_DIR/$sha1\"-*\n+\t\telse\n+\t\t\techo 'Status: 404 Not Found'\n+\t\t\techo\n+\t\tfi\n+\tfi\n+fi\ndiff --git a/t/lib-httpd/upload.sh b/t/lib-httpd/upload.sh\nnew file mode 100644\nindex 00000000000..202de63b2dc\n--- /dev/null\n+++ b/t/lib-httpd/upload.sh\n@@ -0,0 +1,46 @@\n+#!/bin/sh\n+\n+# In part from http://codereview.stackexchange.com/questions/79549/bash-cgi-upload-file\n+# Used in the httpd test server to for a remote helper to call to upload blobs.\n+\n+FILES_DIR=\"www/files\"\n+\n+OLDIFS=\"$IFS\"\n+IFS='&'\n+set -- $QUERY_STRING\n+IFS=\"$OLDIFS\"\n+\n+while test $# -gt 0\n+do\n+\tkey=${1%%=*}\n+\tval=${1#*=}\n+\n+\tcase \"$key\" in\n+\t\"sha1\") sha1=\"$val\" ;;\n+\t\"type\") type=\"$val\" ;;\n+\t\"size\") size=\"$val\" ;;\n+\t\"delete\") delete=1 ;;\n+\t*) echo >&2 \"unknown key '$key'\" ;;\n+\tesac\n+\n+\tshift\n+done\n+\n+case \"$REQUEST_METHOD\" in\n+POST)\n+\tif test \"$delete\" = \"1\"\n+\tthen\n+\t\trm -f \"$FILES_DIR/$sha1-$size-$type\"\n+\telse\n+\t\tmkdir -p \"$FILES_DIR\"\n+\t\tcat >\"$FILES_DIR/$sha1-$size-$type\"\n+\tfi\n+\n+\techo 'Status: 204 No Content'\n+\techo\n+\t;;\n+\n+*)\n+\techo 'Status: 405 Method Not Allowed'\n+\techo\n+esac\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex f17abd298c8..731f6bebc64 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,31 @@ promise_and_delete () {\n \tdelete_object repo \"$HASH\"\n }\n \n+upload_blob() {\n+\tSERVER_REPO=\"$1\"\n+\tHASH=\"$2\"\n+\n+\ttest -n \"$HASH\" || die \"Invalid argument '$HASH'\"\n+\tHASH_SIZE=$(git -C \"$SERVER_REPO\" cat-file -s \"$HASH\") || {\n+\t\techo >&2 \"Cannot get blob size of '$HASH'\"\n+\t\treturn 1\n+\t}\n+\n+\tUPLOAD_URL=\"http://127.0.0.1:$LIB_HTTPD_PORT/upload/?sha1=$HASH&size=$HASH_SIZE&type=blob\"\n+\n+\tgit -C \"$SERVER_REPO\" cat-file blob \"$HASH\" >object &&\n+\tcurl --data-binary @object --include \"$UPLOAD_URL\"\n+}\n+\n+upload_blobs_from_stdin() {\n+\tSERVER_REPO=\"$1\"\n+\twhile read -r blob\n+\tdo\n+\t\techo \"uploading $blob\"\n+\t\tupload_blob \"$SERVER_REPO\" \"$blob\" || return\n+\tdone\n+}\n+\n test_expect_success 'extensions.partialclone without filter' '\n \ttest_create_repo server &&\n \tgit clone --filter=\"blob:none\" \"file://$(pwd)/server\" client &&\n@@ -668,6 +693,33 @@ test_expect_success 'fetching of missing objects from an HTTP server' '\n \tgrep \"$HASH\" out\n '\n \n+PATH=\"$TEST_DIRECTORY/t0410:$PATH\"\n+\n+test_expect_success 'fetch of missing objects through remote helper' '\n+\trm -rf origin server &&\n+\ttest_create_repo origin &&\n+\tdd if=/dev/zero of=origin/file1 bs=801k count=1 &&\n+\tgit -C origin add file1 &&\n+\tgit -C origin commit -m \"large blob\" &&\n+\tsha=\"$(git -C origin rev-parse :file1)\" &&\n+\texpected=\"?$(git -C origin rev-parse :file1)\" &&\n+\tgit clone --bare --no-local origin server &&\n+\tgit -C server remote add httpremote \"testhttpgit::${PWD}/server\" &&\n+\tgit -C server config remote.httpremote.promisor true &&\n+\tgit -C server config --remove-section remote.origin &&\n+\tgit -C server rev-list --all --objects --filter-print-omitted \\\n+\t\t--filter=blob:limit=800k | perl -ne \"print if s/^[~]//\" \\\n+\t\t>large_blobs.txt &&\n+\tupload_blobs_from_stdin server <large_blobs.txt &&\n+\tgit -C server -c repack.writebitmaps=false repack -a -d \\\n+\t\t--filter=blob:limit=800k &&\n+\tgit -C server rev-list --objects --all --missing=print >objects &&\n+\tgrep \"$expected\" objects &&\n+\tHTTPD_URL=$HTTPD_URL git -C server show $sha &&\n+\tgit -C server rev-list --objects --all --missing=print >objects &&\n+\tgrep \"$sha\" objects\n+'\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \ndiff --git a/t/t0410/git-remote-testhttpgit b/t/t0410/git-remote-testhttpgit\nnew file mode 100755\nindex 00000000000..e5e187243ed\n--- /dev/null\n+++ b/t/t0410/git-remote-testhttpgit\n@@ -0,0 +1,170 @@\n+#!/bin/sh\n+# Copyright (c) 2012 Felipe Contreras\n+# Copyright (c) 2020 Christian Couder\n+\n+# This is a git remote helper that can be used to store blobs on an http server\n+\n+# The first argument can be a url when the fetch/push command was a url\n+# instead of a configured remote. In this case, use a generic alias.\n+if test \"$1\" = \"testhttpgit::$2\"; then\n+\talias=_\n+else\n+\talias=$1\n+fi\n+url=$2\n+\n+unset GIT_DIR\n+\n+h_refspec=\"refs/heads/*:refs/testhttpgit/$alias/heads/*\"\n+t_refspec=\"refs/tags/*:refs/testhttpgit/$alias/tags/*\"\n+\n+if test -n \"$GIT_REMOTE_TESTHTTPGIT_NOREFSPEC\"\n+then\n+\th_refspec=\"\"\n+\tt_refspec=\"\"\n+fi\n+\n+die () {\n+\techo >&2 \"fatal: $*\"\n+\techo \"fatal: $*\" >>/tmp/t0430.txt\n+\techo >>/tmp/t0430.txt\n+\texit 1\n+}\n+\n+force=\n+\n+mark_count_tmp=$(mktemp -t git-remote-http-mark-count_XXXXXX) || die \"Failed to create temp file\"\n+echo \"1\" >\"$mark_count_tmp\"\n+\n+get_mark_count() {\n+\tmark=$(cat \"$mark_count_tmp\")\n+\techo \"$mark\"\n+\tmark=$((mark+1))\n+\techo \"$mark\" >\"$mark_count_tmp\"\t\n+}\n+\n+export_blob_from_file() {\n+\tfile=\"$1\"\n+\techo \"blob\"\n+\techo \"mark :$(get_mark_count)\"\n+\tsize=$(wc -c <\"$file\") || return\n+\techo \"data $size\"\n+\tcat \"$file\" || return\n+\techo\n+}\n+\n+while read line\n+do\n+\tcase $line in\n+\tcapabilities)\n+\t\techo 'import'\n+\t\techo 'export'\n+\t\ttest -n \"$h_refspec\" && echo \"refspec $h_refspec\"\n+\t\ttest -n \"$t_refspec\" && echo \"refspec $t_refspec\"\n+\t\ttest -n \"$GIT_REMOTE_TESTHTTPGIT_SIGNED_TAGS\" && echo \"signed-tags\"\n+\t\ttest -n \"$GIT_REMOTE_TESTHTTPGIT_NO_PRIVATE_UPDATE\" && echo \"no-private-update\"\n+\t\techo 'option'\n+\t\techo\n+\t\t;;\n+\tlist)\n+\t\tgit -C \"$url\" for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'\n+\t\thead=$(git -C \"$url\" symbolic-ref HEAD)\n+\t\techo \"@$head HEAD\"\n+\t\techo\n+\t\t;;\n+\timport*)\n+\t\t# read all import lines\n+\t\twhile true\n+\t\tdo\n+\t\t\tref=\"${line#* }\"\n+\t\t\trefs=\"$refs $ref\"\n+\t\t\tread line\n+\t\t\ttest \"${line%% *}\" != \"import\" && break\n+\t\tdone\n+\n+\t\techo \"refs: $refs\" >>/tmp/t0430.txt\n+\n+\t\tif test -n \"$GIT_REMOTE_TESTHTTPGIT_FAILURE\"\n+\t\tthen\n+\t\t\techo \"feature done\"\n+\t\t\texit 1\n+\t\tfi\n+\n+\t\techo \"feature done\"\n+\n+\t\ttmpdir=$(mktemp -d -t git-remote-http-import_XXXXXX) || die \"Failed to create temp directory\"\n+\n+\t\tfor ref in $refs\n+\t\tdo\n+\t\t\tget_url=\"$HTTPD_URL/list/?sha1=$ref\"\n+\t\t\techo \"curl url: $get_url\" >>/tmp/t0430.txt\n+\t\t\techo \"curl output: $tmpdir/$ref\" >>/tmp/t0430.txt\n+\t\t\tcurl -s -o \"$tmpdir/$ref\" \"$get_url\" ||\n+\t\t\t\tdie \"curl '$get_url' failed\"\n+\t\t\techo \"exporting from: $tmpdir/$ref\" >>/tmp/t0430.txt\n+\t\t\texport_blob_from_file \"$tmpdir/$ref\" ||\n+\t\t\t\tdie \"failed to export blob from '$tmpdir/$ref'\"\n+\t\t\techo \"done exporting\" >>/tmp/t0430.txt\n+\t\tdone\n+\n+\t\techo \"done\"\n+\t\t;;\n+\texport)\n+\t\tif test -n \"$GIT_REMOTE_TESTHTTPGIT_FAILURE\"\n+\t\tthen\n+\t\t\t# consume input so fast-export doesn't get SIGPIPE;\n+\t\t\t# git would also notice that case, but we want\n+\t\t\t# to make sure we are exercising the later\n+\t\t\t# error checks\n+\t\t\twhile read line; do\n+\t\t\t\ttest \"done\" = \"$line\" && break\n+\t\t\tdone\n+\t\t\texit 1\n+\t\tfi\n+\n+\t\tbefore=$(git -C \"$url\" for-each-ref --format=' %(refname) %(objectname) ')\n+\n+\t\tgit -C \"$url\" fast-import \\\n+\t\t\t${force:+--force} \\\n+\t\t\t${testhttpgitmarks:+\"--import-marks=$testhttpgitmarks\"} \\\n+\t\t\t${testhttpgitmarks:+\"--export-marks=$testhttpgitmarks\"} \\\n+\t\t\t--quiet\n+\n+\t\t# figure out which refs were updated\n+\t\tgit -C \"$url\" for-each-ref --format='%(refname) %(objectname)' |\n+\t\twhile read ref a\n+\t\tdo\n+\t\t\tcase \"$before\" in\n+\t\t\t*\" $ref $a \"*)\n+\t\t\t\tcontinue ;;\t# unchanged\n+\t\t\tesac\n+\t\t\tif test -z \"$GIT_REMOTE_TESTHTTPGIT_PUSH_ERROR\"\n+\t\t\tthen\n+\t\t\t\techo \"ok $ref\"\n+\t\t\telse\n+\t\t\t\techo \"error $ref $GIT_REMOTE_TESTHTTPGIT_PUSH_ERROR\"\n+\t\t\tfi\n+\t\tdone\n+\n+\t\techo\n+\t\t;;\n+\toption\\ *)\n+\t\tread cmd opt val <<-EOF\n+\t\t$line\n+\t\tEOF\n+\t\tcase $opt in\n+\t\tforce)\n+\t\t\ttest $val = \"true\" && force=\"true\" || force=\n+\t\t\techo \"ok\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"unsupported\"\n+\t\t\t;;\n+\t\tesac\n+\t\t;;\n+\t'')\n+\t\texit\n+\t\t;;\n+\tesac\n+done\n+\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex e489869dd94..78cc1858cb6 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'repack with filter does not fetch from remote' '\n+\trm -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+\techo content1 >server/file1 &&\n+\tgit -C server add file1 &&\n+\tgit -C server commit -m initial_commit &&\n+\texpected=\"?$(git -C server rev-parse :file1)\" &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\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+\tgrep \"$expected\" objects &&\n+\tgit -C client repack -a -d &&\n+\texpected=\"$(git -C server rev-parse :file1)\" &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\tgrep \"$expected\" objects\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \ngitgitgadget\n"},{"id":"447083","messageId":"a62a007f-7c61-68eb-c0e6-548dc9b6f671@gmail.com","threadId":"57319","inReplyTo":"a3166381572481f2ed159740eb8a1d88d4f9dc0f.1643248180.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] repack: add --filter=<filter-spec> option","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2022-01-27T15:03:08Z","receivedAt":"2022-01-27T15:03:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/26/2022 8:49 PM, John Cai via GitGitGadget wrote:\n> From: John Cai <johncai86@gmail.com>\n> \n> Currently, repack does not work with partial clones. When repack is run\n> on a partially cloned repository, it grabs all missing objects from\n> promisor remotes. This also means that when gc is run for repository\n> maintenance on a partially cloned repository, it will end up getting\n> missing objects, which is not what we want.\n\nThis shouldn't be what is happening. Do you have a demonstration of\nthis happening? repack_promisor_objects() should be avoiding following\nlinks outside of promisor packs so we can safely 'git gc' in a partial\nclone without downloading all reachable blobs.\n\n> In order to make repack work with partial clone, teach repack a new\n> option --filter, which takes a <filter-spec> argument. repack will skip\n> any objects that are matched by <filter-spec> similar to how the clone\n> command will skip fetching certain objects.\n\nThis is a bit misleading, since 'git clone' doesn't \"skip fetching\",\nbut instead requests a filter and the server can choose to write a\npack-file using that filter. I'm not sure if it's worth how pedantic\nI'm being here.\n\nThe thing that I find confusing here is that you are adding an option\nthat could be run on a _full_ repository. If I have a set of packs\nand none of them are promisor (I have every reachable object), then\nwhat is the end result after 'git repack -adf --filter=blob:none'?\nThose existing pack-files shouldn't be deleted because they have\nobjects that are not in the newly-created pack-file.\n\nI'd like to see some additional clarity on this before continuing\nto review this series.\n\n> The final goal of this feature, is to be able to store objects on a\n> server other than the regular git server itself.\n> \n> There are several scripts added so we can test the process of using a\n> remote helper to upload blobs to an http server:\n> \n> - t/lib-httpd/list.sh lists blobs uploaded to the http server.\n> - t/lib-httpd/upload.sh uploads blobs to the http server.\n> - t/t0410/git-remote-testhttpgit a remote helper that can access blobs\n>   onto from an http server. Copied over from t/t5801/git-remote-testhttpgit\n>   and modified to upload blobs to an http server.\n> - t/t0410/lib-http-promisor.sh convenience functions for uploading\n>   blobs\n\nI think these changes to the tests should be extracted to a new\npatch where this can be discussed in more detail. I didn't look\ntoo closely at them because I want to focus on whether this\n--filter option is a good direction for 'git repack'.\n\n>  \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n> @@ -819,6 +824,11 @@ 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\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n> +\t\t\t\t\t\t\tline.buf);\n> +\t\t\twrite_promisor_file(promisor_name, NULL, 0);\n\nThis code is duplicated in repack_promisor_objects(), so it would be\ngood to extract that logic into a helper method called by both places.\n\n> +\t\t}\n>  \t}\n>  \tfclose(out);\n>  \tret = finish_command(&cmd);\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index e489869dd94..78cc1858cb6 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n>  \ttest_must_be_empty actual\n>  '\n>  \n> +test_expect_success 'repack with filter does not fetch from remote' '\n> +\trm -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> +\techo content1 >server/file1 &&\n> +\tgit -C server add file1 &&\n> +\tgit -C server commit -m initial_commit &&\n> +\texpected=\"?$(git -C server rev-parse :file1)\" &&\n> +\tgit clone --bare --no-local server client &&\n\nYou could use \"file:://$(pwd)/server\" here instead of \"server\".\n\n> +\tgit -C client config remote.origin.promisor true &&\n> +\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\nThis isn't testing what you want it to test, because your initial\nclone doesn't use --filter=blob:none, so you already have all of\nthe objects in the client. You would never trigger a need for a\nfetch from the remote.\n\n> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n> +\tgrep \"$expected\" objects &&\n> +\tgit -C client repack -a -d &&\n> +\texpected=\"$(git -C server rev-parse :file1)\" &&\n\nThis is signalling to me that you are looking for a remote fetch\nnow that you are repacking everything, and that can only happen\nif you deleted objects from the client during your first repack.\nThat seems incorrect.\n\n> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n> +\tgrep \"$expected\" objects\n> +'\n\nBased on my current understanding, this patch seems unnecessary (repacks\nshould already be doing the right thing when in the presence of a partial\nclone) and incorrect (we should not delete existing reachable objects\nwhen repacking with a filter).\n\nI look forward to hearing more about your intended use of this feature so\nwe can land on a better way to solve the problems you are having.\n\nThanks,\n-Stolee\n"},{"id":"447304","messageId":"A4BAD509-FA1F-49C3-87AF-CF4B73C559F1@gmail.com","threadId":"57319","inReplyTo":"a62a007f-7c61-68eb-c0e6-548dc9b6f671@gmail.com","subject":"Re: [PATCH 2/2] repack: add --filter=<filter-spec> option","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-29T19:14:19Z","receivedAt":"2022-01-29T19:14:22Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Stolee,\n\nThanks for taking the time to review this patch! I added some points of clarification\ndown below.\n\nOn 27 Jan 2022, at 10:03, Derrick Stolee wrote:\n\n> On 1/26/2022 8:49 PM, John Cai via GitGitGadget wrote:\n>> From: John Cai <johncai86@gmail.com>\n>>\n>> Currently, repack does not work with partial clones. When repack is run\n>> on a partially cloned repository, it grabs all missing objects from\n>> promisor remotes. This also means that when gc is run for repository\n>> maintenance on a partially cloned repository, it will end up getting\n>> missing objects, which is not what we want.\n>\n> This shouldn't be what is happening. Do you have a demonstration of\n> this happening? repack_promisor_objects() should be avoiding following\n> links outside of promisor packs so we can safely 'git gc' in a partial\n> clone without downloading all reachable blobs.\n\nYou're right, sorry I was mistaken about this detail of how partial clones work.\n>\n>> In order to make repack work with partial clone, teach repack a new\n>> option --filter, which takes a <filter-spec> argument. repack will skip\n>> any objects that are matched by <filter-spec> similar to how the clone\n>> command will skip fetching certain objects.\n>\n> This is a bit misleading, since 'git clone' doesn't \"skip fetching\",\n> but instead requests a filter and the server can choose to write a\n> pack-file using that filter. I'm not sure if it's worth how pedantic\n> I'm being here.\n\nThanks for the more precise description of the mechanics of partial clone.\nI'll improve the wording in the next version of this patch series.\n\n>\n> The thing that I find confusing here is that you are adding an option\n> that could be run on a _full_ repository. If I have a set of packs\n> and none of them are promisor (I have every reachable object), then\n> what is the end result after 'git repack -adf --filter=blob:none'?\n> Those existing pack-files shouldn't be deleted because they have\n> objects that are not in the newly-created pack-file.\n>\n> I'd like to see some additional clarity on this before continuing\n> to review this series.\n\nApologies for the lack of clarity. Indeed, I can see why this is the most important\ndetail of this patch to provide enough context on, as it involves deleting\nobjects from a full repository as you said.\n\nTo back up a little, the goal is to be able to offload large\nblobs to a separate http server. Christian Couder has a demo [1] that shows\nthis in detail.\n\nIf we had the following:\nA. an http server to use as a generalized object store\nB. a server update hook that uploads large blobs to 1.\nC. a git server\nD. a regular job that runs `git repack --filter` to remove large\nblobs from C.\n\nClients would need to configure both C) and A) as promisor remotes to\nbe able to get everything. When they push new large blobs, they can\nstill push them to C), as B) will upload them to A), and D) will\nregularly remove those large blobs from C).\n\nThis way with a little bit of client and server configuration, we can have\na native way to support offloading large files without git LFS.\nIt would be more flexible as you can easily tweak which blobs are considered large\nfiles by tweaking B) and D).\n\n>\n>> The final goal of this feature, is to be able to store objects on a\n>> server other than the regular git server itself.\n>>\n>> There are several scripts added so we can test the process of using a\n>> remote helper to upload blobs to an http server:\n>>\n>> - t/lib-httpd/list.sh lists blobs uploaded to the http server.\n>> - t/lib-httpd/upload.sh uploads blobs to the http server.\n>> - t/t0410/git-remote-testhttpgit a remote helper that can access blobs\n>>   onto from an http server. Copied over from t/t5801/git-remote-testhttpgit\n>>   and modified to upload blobs to an http server.\n>> - t/t0410/lib-http-promisor.sh convenience functions for uploading\n>>   blobs\n>\n> I think these changes to the tests should be extracted to a new\n> patch where this can be discussed in more detail. I didn't look\n> too closely at them because I want to focus on whether this\n> --filter option is a good direction for 'git repack'.\n>\n>>  \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n>> @@ -819,6 +824,11 @@ 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\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n>> +\t\t\t\t\t\t\tline.buf);\n>> +\t\t\twrite_promisor_file(promisor_name, NULL, 0);\n>\n> This code is duplicated in repack_promisor_objects(), so it would be\n> good to extract that logic into a helper method called by both places.\n\nThanks for pointing this out. I'll incorporate this into the next version.\n>\n>> +\t\t}\n>>  \t}\n>>  \tfclose(out);\n>>  \tret = finish_command(&cmd);\n>\n>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>> index e489869dd94..78cc1858cb6 100755\n>> --- a/t/t7700-repack.sh\n>> +++ b/t/t7700-repack.sh\n>> @@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n>>  \ttest_must_be_empty actual\n>>  '\n>>\n>> +test_expect_success 'repack with filter does not fetch from remote' '\n>> +\trm -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>> +\techo content1 >server/file1 &&\n>> +\tgit -C server add file1 &&\n>> +\tgit -C server commit -m initial_commit &&\n>> +\texpected=\"?$(git -C server rev-parse :file1)\" &&\n>> +\tgit clone --bare --no-local server client &&\n>\n> You could use \"file:://$(pwd)/server\" here instead of \"server\".\n\ngood point, thanks\n\n>\n>> +\tgit -C client config remote.origin.promisor true &&\n>> +\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n> This isn't testing what you want it to test, because your initial\n> clone doesn't use --filter=blob:none, so you already have all of\n> the objects in the client. You would never trigger a need for a\n> fetch from the remote.\n\nright, so this test is actually testing that repack --filter would shed objects to show\nthat it can be used as D) as a regular cleanup job for git servers that utilize another\nhttp server to host large blobs.\n\n>\n>> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n>> +\tgrep \"$expected\" objects &&\n>> +\tgit -C client repack -a -d &&\n>> +\texpected=\"$(git -C server rev-parse :file1)\" &&\n>\n> This is signalling to me that you are looking for a remote fetch\n> now that you are repacking everything, and that can only happen\n> if you deleted objects from the client during your first repack.\n> That seems incorrect.\n>\n>> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n>> +\tgrep \"$expected\" objects\n>> +'\n>\n> Based on my current understanding, this patch seems unnecessary (repacks\n> should already be doing the right thing when in the presence of a partial\n> clone) and incorrect (we should not delete existing reachable objects\n> when repacking with a filter).\n>\n> I look forward to hearing more about your intended use of this feature so\n> we can land on a better way to solve the problems you are having.\n\nThanks for the callouts on the big picture of this proposed change. Looking\nforward to getting your thoughts on this!\n>\n> Thanks,\n> -Stolee\n"},{"id":"447308","messageId":"CAP8UFD2z1P2-7zhyBEoSpV=KBri9qEQpho_q6RZ1+7tUNLiyHQ@mail.gmail.com","threadId":"57319","inReplyTo":"A4BAD509-FA1F-49C3-87AF-CF4B73C559F1@gmail.com","subject":"Re: [PATCH 2/2] repack: add --filter=<filter-spec> option","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-01-30T08:16:07Z","receivedAt":"2022-01-30T08:16:22Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Jan 29, 2022 at 8:14 PM John Cai <johncai86@gmail.com> wrote:\n\n> Apologies for the lack of clarity. Indeed, I can see why this is the most important\n> detail of this patch to provide enough context on, as it involves deleting\n> objects from a full repository as you said.\n>\n> To back up a little, the goal is to be able to offload large\n> blobs to a separate http server. Christian Couder has a demo [1] that shows\n> this in detail.\n\nYou might have forgotten to provide a link for [1], also I am not sure\nif you wanted to link to the repo:\n\nhttps://gitlab.com/chriscool/partial-clone-demo/\n\nor the demo itself in the repo:\n\nhttps://gitlab.com/chriscool/partial-clone-demo/-/blob/master/http-promisor/server_demo.txt\n\n> If we had the following:\n> A. an http server to use as a generalized object store\n> B. a server update hook that uploads large blobs to 1.\n\ns/1./A./\n\n> C. a git server\n> D. a regular job that runs `git repack --filter` to remove large\n> blobs from C.\n>\n> Clients would need to configure both C) and A) as promisor remotes to\n\nMaybe s/C)/C./ and s/A)/A./\n\nAlso note that configuring A. as a promisor remote requires a remote helper.\n\n> be able to get everything. When they push new large blobs, they can\n> still push them to C), as B) will upload them to A), and D) will\n> regularly remove those large blobs from C).\n>\n> This way with a little bit of client and server configuration, we can have\n> a native way to support offloading large files without git LFS.\n> It would be more flexible as you can easily tweak which blobs are considered large\n> files by tweaking B) and D).\n\nYeah, that's the idea of the demo.\n\nThanks for working on this!\n"},{"id":"447313","messageId":"8D4655AA-8D2A-4D4C-A7CD-B79A8A9E66D7@gmail.com","threadId":"57319","inReplyTo":"A4BAD509-FA1F-49C3-87AF-CF4B73C559F1@gmail.com","subject":"Re: [PATCH 2/2] repack: add --filter=<filter-spec> option","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-30T13:02:07Z","receivedAt":"2022-01-30T13:02:13Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Sorry forgot to include the link to Christian's demo. included below\n\nOn 29 Jan 2022, at 14:14, John Cai wrote:\n\n> Hi Stolee,\n>\n> Thanks for taking the time to review this patch! I added some points of clarification\n> down below.\n>\n> On 27 Jan 2022, at 10:03, Derrick Stolee wrote:\n>\n>> On 1/26/2022 8:49 PM, John Cai via GitGitGadget wrote:\n>>> From: John Cai <johncai86@gmail.com>\n>>>\n>>> Currently, repack does not work with partial clones. When repack is run\n>>> on a partially cloned repository, it grabs all missing objects from\n>>> promisor remotes. This also means that when gc is run for repository\n>>> maintenance on a partially cloned repository, it will end up getting\n>>> missing objects, which is not what we want.\n>>\n>> This shouldn't be what is happening. Do you have a demonstration of\n>> this happening? repack_promisor_objects() should be avoiding following\n>> links outside of promisor packs so we can safely 'git gc' in a partial\n>> clone without downloading all reachable blobs.\n>\n> You're right, sorry I was mistaken about this detail of how partial clones work.\n>>\n>>> In order to make repack work with partial clone, teach repack a new\n>>> option --filter, which takes a <filter-spec> argument. repack will skip\n>>> any objects that are matched by <filter-spec> similar to how the clone\n>>> command will skip fetching certain objects.\n>>\n>> This is a bit misleading, since 'git clone' doesn't \"skip fetching\",\n>> but instead requests a filter and the server can choose to write a\n>> pack-file using that filter. I'm not sure if it's worth how pedantic\n>> I'm being here.\n>\n> Thanks for the more precise description of the mechanics of partial clone.\n> I'll improve the wording in the next version of this patch series.\n>\n>>\n>> The thing that I find confusing here is that you are adding an option\n>> that could be run on a _full_ repository. If I have a set of packs\n>> and none of them are promisor (I have every reachable object), then\n>> what is the end result after 'git repack -adf --filter=blob:none'?\n>> Those existing pack-files shouldn't be deleted because they have\n>> objects that are not in the newly-created pack-file.\n>>\n>> I'd like to see some additional clarity on this before continuing\n>> to review this series.\n>\n> Apologies for the lack of clarity. Indeed, I can see why this is the most important\n> detail of this patch to provide enough context on, as it involves deleting\n> objects from a full repository as you said.\n>\n> To back up a little, the goal is to be able to offload large\n> blobs to a separate http server. Christian Couder has a demo [1] that shows\n> this in detail.\n>\n> If we had the following:\n> A. an http server to use as a generalized object store\n> B. a server update hook that uploads large blobs to 1.\n> C. a git server\n> D. a regular job that runs `git repack --filter` to remove large\n> blobs from C.\n>\n> Clients would need to configure both C) and A) as promisor remotes to\n> be able to get everything. When they push new large blobs, they can\n> still push them to C), as B) will upload them to A), and D) will\n> regularly remove those large blobs from C).\n>\n> This way with a little bit of client and server configuration, we can have\n> a native way to support offloading large files without git LFS.\n> It would be more flexible as you can easily tweak which blobs are considered large\n> files by tweaking B) and D).\n>\n\n[1] https://gitlab.com/chriscool/partial-clone-demo/-/blob/master/http-promisor/server_demo.txt\n\n>>\n>>> The final goal of this feature, is to be able to store objects on a\n>>> server other than the regular git server itself.\n>>>\n>>> There are several scripts added so we can test the process of using a\n>>> remote helper to upload blobs to an http server:\n>>>\n>>> - t/lib-httpd/list.sh lists blobs uploaded to the http server.\n>>> - t/lib-httpd/upload.sh uploads blobs to the http server.\n>>> - t/t0410/git-remote-testhttpgit a remote helper that can access blobs\n>>>   onto from an http server. Copied over from t/t5801/git-remote-testhttpgit\n>>>   and modified to upload blobs to an http server.\n>>> - t/t0410/lib-http-promisor.sh convenience functions for uploading\n>>>   blobs\n>>\n>> I think these changes to the tests should be extracted to a new\n>> patch where this can be discussed in more detail. I didn't look\n>> too closely at them because I want to focus on whether this\n>> --filter option is a good direction for 'git repack'.\n>>\n>>>  \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n>>> @@ -819,6 +824,11 @@ 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\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n>>> +\t\t\t\t\t\t\tline.buf);\n>>> +\t\t\twrite_promisor_file(promisor_name, NULL, 0);\n>>\n>> This code is duplicated in repack_promisor_objects(), so it would be\n>> good to extract that logic into a helper method called by both places.\n>\n> Thanks for pointing this out. I'll incorporate this into the next version.\n>>\n>>> +\t\t}\n>>>  \t}\n>>>  \tfclose(out);\n>>>  \tret = finish_command(&cmd);\n>>\n>>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>>> index e489869dd94..78cc1858cb6 100755\n>>> --- a/t/t7700-repack.sh\n>>> +++ b/t/t7700-repack.sh\n>>> @@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n>>>  \ttest_must_be_empty actual\n>>>  '\n>>>\n>>> +test_expect_success 'repack with filter does not fetch from remote' '\n>>> +\trm -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>>> +\techo content1 >server/file1 &&\n>>> +\tgit -C server add file1 &&\n>>> +\tgit -C server commit -m initial_commit &&\n>>> +\texpected=\"?$(git -C server rev-parse :file1)\" &&\n>>> +\tgit clone --bare --no-local server client &&\n>>\n>> You could use \"file:://$(pwd)/server\" here instead of \"server\".\n>\n> good point, thanks\n>\n>>\n>>> +\tgit -C client config remote.origin.promisor true &&\n>>> +\tgit -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n>> This isn't testing what you want it to test, because your initial\n>> clone doesn't use --filter=blob:none, so you already have all of\n>> the objects in the client. You would never trigger a need for a\n>> fetch from the remote.\n>\n> right, so this test is actually testing that repack --filter would shed objects to show\n> that it can be used as D) as a regular cleanup job for git servers that utilize another\n> http server to host large blobs.\n>\n>>\n>>> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n>>> +\tgrep \"$expected\" objects &&\n>>> +\tgit -C client repack -a -d &&\n>>> +\texpected=\"$(git -C server rev-parse :file1)\" &&\n>>\n>> This is signalling to me that you are looking for a remote fetch\n>> now that you are repacking everything, and that can only happen\n>> if you deleted objects from the client during your first repack.\n>> That seems incorrect.\n>>\n>>> +\tgit -C client rev-list --objects --all --missing=print >objects &&\n>>> +\tgrep \"$expected\" objects\n>>> +'\n>>\n>> Based on my current understanding, this patch seems unnecessary (repacks\n>> should already be doing the right thing when in the presence of a partial\n>> clone) and incorrect (we should not delete existing reachable objects\n>> when repacking with a filter).\n>>\n>> I look forward to hearing more about your intended use of this feature so\n>> we can land on a better way to solve the problems you are having.\n>\n> Thanks for the callouts on the big picture of this proposed change. Looking\n> forward to getting your thoughts on this!\n>>\n>> Thanks,\n>> -Stolee\n"},{"id":"448019","messageId":"6e7c8410b8dcd2f4a7e188eb5b55ae8eecb54e40.1644372606.git.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] repack: add --filter=<filter-spec> option","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T02:10:04Z","receivedAt":"2022-02-09T02:40:48Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn order to use a separate http server as a remote to offload large\nblobs, imagine the following:\n\nA. an http server to use as a generalized object store.\nB. a server update hook that uploads large blobs to (A).\nC. a git server\nD. a remote helper that knows how to download objects from the http\nserver\nE. a regular job that runs `git repack --filter` to remove large\nblobs from (C).\n\nClients would need to configure both (C) and (A) as promisor remotes to\nbe able to get everything. When they push new large blobs, they can\nstill push them to (C), as (B) will upload them to (A), and (E) will\nregularly remove those large blobs from (C).\n\nThis way with a little bit of client and server configuration, we can\nhave a native way to support offloading large files without git LFS.\nIt would be more flexible as you can easily tweak which blobs are\nconsidered large files by tweaking (B) and (E).\n\nA fuller demo can be found at http://tiny.cc/object_storage_demo\n\nBased-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n Documentation/git-repack.txt |  5 +++++\n builtin/repack.c             | 22 +++++++++++++++-------\n 2 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex ee30edc178a..e394ec52ab1 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -126,6 +126,11 @@ depth is 4095.\n \ta larger and slower repository; see the discussion in\n \t`pack.packSizeLimit`.\n \n+--filter=<filter-spec>::\n+\tOmits certain objects (usually blobs) from the resulting\n+\tpackfile. See linkgit:git-rev-list[1] for valid\n+\t`<filter-spec>` 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 da1e364a756..3f1e8a39a2b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -152,6 +152,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@@ -172,6 +173,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@@ -238,6 +241,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@@ -269,7 +279,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@@ -286,13 +295,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@@ -660,6 +664,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@@ -819,6 +825,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);\n-- \ngitgitgadget\n\n"},{"id":"448020","messageId":"21ED346B-A906-4905-B061-EDE53691C586@gmail.com","threadId":"57319","inReplyTo":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-09T02:27:31Z","receivedAt":"2022-02-09T02:40:58Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Johannes\n\nI'm not sure where I went wrong on GGG. Somehow the cc list didn't get translated into\ncc fields. Here's the PR: https://github.com/git/git/pull/1206. Thanks!\n\ncc'ing folks I meant to cc for this patch series\n\nOn 8 Feb 2022, at 21:10, John Cai via GitGitGadget wrote:\n\n> This patch series makes partial clone more useful by making it possible to\n> run repack to remove objects from a repository (replacing it with promisor\n> objects). This is useful when we want to offload large blobs from a git\n> server onto another git server, or even use an http server through a remote\n> helper.\n>\n> In [A], a --refilter option on fetch and fetch-pack is being discussed where\n> either a less restrictive or more restrictive filter can be used. In the\n> more restrictive case, the objects that already exist will not be deleted.\n> But, one can imagine that users might want the ability to delete objects\n> when they apply a more restrictive filter in order to save space, and this\n> patch series would also allow that.\n>\n> There are a couple of things we need to adjust to make this possible. This\n> patch has three parts.\n>\n>  1. Allow --filter in pack-objects without --stdout\n>  2. Add a --filter flag for repack\n>  3. Allow missing promisor objects in upload-pack\n>  4. Tests that demonstrate the ability to offload objects onto an http\n>     remote\n>\n> cc: Christian Couder christian.couder@gmail.com cc: Derrick Stolee\n> stolee@gmail.com cc: Robert Coup robert@coup.net.nz\n>\n> A.\n> https://lore.kernel.org/git/pull.1138.git.1643730593.gitgitgadget@gmail.com/\n>\n> John Cai (4):\n>   pack-objects: allow --filter without --stdout\n>   repack: add --filter=<filter-spec> option\n>   upload-pack: allow missing promisor objects\n>   tests for repack --filter mode\n>\n>  Documentation/git-repack.txt   |   5 +\n>  builtin/pack-objects.c         |   2 -\n>  builtin/repack.c               |  22 +++--\n>  t/lib-httpd.sh                 |   2 +\n>  t/lib-httpd/apache.conf        |   8 ++\n>  t/lib-httpd/list.sh            |  43 +++++++++\n>  t/lib-httpd/upload.sh          |  46 +++++++++\n>  t/t0410-partial-clone.sh       |  81 ++++++++++++++++\n>  t/t0410/git-remote-testhttpgit | 170 +++++++++++++++++++++++++++++++++\n>  t/t7700-repack.sh              |  20 ++++\n>  upload-pack.c                  |   5 +\n>  11 files changed, 395 insertions(+), 9 deletions(-)\n>  create mode 100644 t/lib-httpd/list.sh\n>  create mode 100644 t/lib-httpd/upload.sh\n>  create mode 100755 t/t0410/git-remote-testhttpgit\n>\n>\n> base-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1206%2Fjohn-cai%2Fjc-repack-filter-v2\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1206/john-cai/jc-repack-filter-v2\n> Pull-Request: https://github.com/git/git/pull/1206\n>\n> Range-diff vs v1:\n>\n>  1:  0eec9b117da = 1:  f43b76ca650 pack-objects: allow --filter without --stdout\n>  -:  ----------- > 2:  6e7c8410b8d repack: add --filter=<filter-spec> option\n>  -:  ----------- > 3:  40612b9663b upload-pack: allow missing promisor objects\n>  2:  a3166381572 ! 4:  d76faa1f16e repack: add --filter=<filter-spec> option\n>      @@ Metadata\n>       Author: John Cai <johncai86@gmail.com>\n>\n>        ## Commit message ##\n>      -    repack: add --filter=<filter-spec> option\n>      +    tests for repack --filter mode\n>\n>      -    Currently, repack does not work with partial clones. When repack is run\n>      -    on a partially cloned repository, it grabs all missing objects from\n>      -    promisor remotes. This also means that when gc is run for repository\n>      -    maintenance on a partially cloned repository, it will end up getting\n>      -    missing objects, which is not what we want.\n>      -\n>      -    In order to make repack work with partial clone, teach repack a new\n>      -    option --filter, which takes a <filter-spec> argument. repack will skip\n>      -    any objects that are matched by <filter-spec> similar to how the clone\n>      -    command will skip fetching certain objects.\n>      -\n>      -    The final goal of this feature, is to be able to store objects on a\n>      -    server other than the regular git server itself.\n>      +    This patch adds tests to test both repack --filter functionality in\n>      +    isolation (in t7700-repack.sh) as well as how it can be used to offload\n>      +    large blobs (in t0410-partial-clone.sh)\n>\n>           There are several scripts added so we can test the process of using a\n>      -    remote helper to upload blobs to an http server:\n>      +    remote helper to upload blobs to an http server.\n>\n>           - t/lib-httpd/list.sh lists blobs uploaded to the http server.\n>           - t/lib-httpd/upload.sh uploads blobs to the http server.\n>      @@ Commit message\n>           Based-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\n>           Signed-off-by: John Cai <johncai86@gmail.com>\n>\n>      - ## Documentation/git-repack.txt ##\n>      -@@ Documentation/git-repack.txt: depth is 4095.\n>      - \ta larger and slower repository; see the discussion in\n>      - \t`pack.packSizeLimit`.\n>      -\n>      -+--filter=<filter-spec>::\n>      -+\tOmits certain objects (usually blobs) from the resulting\n>      -+\tpackfile. See linkgit:git-rev-list[1] for valid\n>      -+\t`<filter-spec>` forms.\n>      -+\n>      - -b::\n>      - --write-bitmap-index::\n>      - \tWrite a reachability bitmap index as part of the repack. This\n>      -\n>      - ## builtin/repack.c ##\n>      -@@ builtin/repack.c: 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>      -@@ builtin/repack.c: 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>      -@@ builtin/repack.c: 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>      -@@ builtin/repack.c: 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\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n>      -+\t\t\t\t\t\t\tline.buf);\n>      -+\t\t\twrite_promisor_file(promisor_name, NULL, 0);\n>      -+\t\t}\n>      - \t}\n>      - \tfclose(out);\n>      - \tret = finish_command(&cmd);\n>      -\n>        ## t/lib-httpd.sh ##\n>       @@ t/lib-httpd.sh: prepare_httpd() {\n>        \tinstall_script error-smart-http.sh\n>      @@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects from\n>       +\tgit -C server rev-list --objects --all --missing=print >objects &&\n>       +\tgrep \"$sha\" objects\n>       +'\n>      ++\n>      ++test_expect_success 'fetch does not cause server to fetch missing objects' '\n>      ++\trm -rf origin server client &&\n>      ++\ttest_create_repo origin &&\n>      ++\tdd if=/dev/zero of=origin/file1 bs=801k count=1 &&\n>      ++\tgit -C origin add file1 &&\n>      ++\tgit -C origin commit -m \"large blob\" &&\n>      ++\tsha=\"$(git -C origin rev-parse :file1)\" &&\n>      ++\texpected=\"?$(git -C origin rev-parse :file1)\" &&\n>      ++\tgit clone --bare --no-local origin server &&\n>      ++\tgit -C server remote add httpremote \"testhttpgit::${PWD}/server\" &&\n>      ++\tgit -C server config remote.httpremote.promisor true &&\n>      ++\tgit -C server config --remove-section remote.origin &&\n>      ++\tgit -C server rev-list --all --objects --filter-print-omitted \\\n>      ++\t\t--filter=blob:limit=800k | perl -ne \"print if s/^[~]//\" \\\n>      ++\t\t>large_blobs.txt &&\n>      ++\tupload_blobs_from_stdin server <large_blobs.txt &&\n>      ++\tgit -C server -c repack.writebitmaps=false repack -a -d \\\n>      ++\t\t--filter=blob:limit=800k &&\n>      ++\tgit -C server config uploadpack.allowmissingpromisor true &&\n>      ++\tgit clone -c remote.httpremote.url=\"testhttpgit::${PWD}/server\" \\\n>      ++\t-c remote.httpremote.fetch='+refs/heads/*:refs/remotes/httpremote/*' \\\n>      ++\t-c remote.httpremote.promisor=true --bare --no-local \\\n>      ++\t--filter=blob:limit=800k server client &&\n>      ++\tgit -C client rev-list --objects --all --missing=print >client_objects &&\n>      ++\tgrep \"$expected\" client_objects &&\n>      ++\tgit -C server rev-list --objects --all --missing=print >server_objects &&\n>      ++\tgrep \"$expected\" server_objects\n>      ++'\n>       +\n>        # DO NOT add non-httpd-specific tests here, because the last part of this\n>        # test script is only executed when httpd is available and enabled.\n>\n> -- \n> gitgitgadget\n"},{"id":"448024","messageId":"40612b9663b8d20e8cfa25ccfce76c7f97e4934d.1644372606.git.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] upload-pack: allow missing promisor objects","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T02:10:05Z","receivedAt":"2022-02-09T02:41:16Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nWhen a git server (A) is being used alongside an http server (B) remote\nthat stores large blobs, and a client fetches objects from both (A) as\nwell as (B), we do not want (A) to fetch missing objects during object\ntraversal.\n\nAdd a config value uploadpack.allowmissingpromisor that, when set to\ntrue, will allow (A) to skip fetching missing objects.\n\nBased-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n upload-pack.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 8acc98741bb..39b56650b77 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -112,6 +112,7 @@ struct upload_pack_data {\n \tunsigned allow_ref_in_want : 1;\t\t\t\t/* v2 only */\n \tunsigned allow_sideband_all : 1;\t\t\t/* v2 only */\n \tunsigned advertise_sid : 1;\n+\tunsigned allow_missing_promisor : 1;\n };\n \n static void upload_pack_data_init(struct upload_pack_data *data)\n@@ -309,6 +310,8 @@ 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->allow_missing_promisor)\n+\t\tstrvec_push(&pack_objects.args, \"--missing=allow-promisor\");\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@@ -1315,6 +1318,8 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)\n \t\tdata->allow_ref_in_want = git_config_bool(var, value);\n \t} else if (!strcmp(\"uploadpack.allowsidebandall\", var)) {\n \t\tdata->allow_sideband_all = git_config_bool(var, value);\n+\t} else if (!strcmp(\"uploadpack.allowmissingpromisor\", var)) {\n+\t\tdata->allow_missing_promisor = git_config_bool(var, value);\n \t} else if (!strcmp(\"core.precomposeunicode\", var)) {\n \t\tprecomposed_unicode = git_config_bool(var, value);\n \t} else if (!strcmp(\"transfer.advertisesid\", var)) {\n-- \ngitgitgadget\n\n"},{"id":"448025","messageId":"f43b76ca650b626751880db373982d30f5d8e15d.1644372606.git.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] pack-objects: allow --filter without --stdout","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T02:10:03Z","receivedAt":"2022-02-09T02:41:18Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\n9535ce7 taught pack-objects to use filtering, but added a requirement of\nthe --stdout since a partial clone mechanism was not yet in place to\nhandle missing objects. Since then, changes like 9e27beaa and others\nadded support to dynamically fetch objects that were missing.\n\nRemove the --stdout requirement so that in the next commit, repack can\npass --filter to pack-objects to omit certain objects from the packfile.\n\nBased-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n builtin/pack-objects.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex ba2006f2212..2d1ecb18784 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4075,8 +4075,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\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-- \ngitgitgadget\n\n"},{"id":"448026","messageId":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.git.git.1643248180.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T02:10:02Z","receivedAt":"2022-02-09T02:41:34Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This patch series makes partial clone more useful by making it possible to\nrun repack to remove objects from a repository (replacing it with promisor\nobjects). This is useful when we want to offload large blobs from a git\nserver onto another git server, or even use an http server through a remote\nhelper.\n\nIn [A], a --refilter option on fetch and fetch-pack is being discussed where\neither a less restrictive or more restrictive filter can be used. In the\nmore restrictive case, the objects that already exist will not be deleted.\nBut, one can imagine that users might want the ability to delete objects\nwhen they apply a more restrictive filter in order to save space, and this\npatch series would also allow that.\n\nThere are a couple of things we need to adjust to make this possible. This\npatch has three parts.\n\n 1. Allow --filter in pack-objects without --stdout\n 2. Add a --filter flag for repack\n 3. Allow missing promisor objects in upload-pack\n 4. Tests that demonstrate the ability to offload objects onto an http\n    remote\n\ncc: Christian Couder christian.couder@gmail.com cc: Derrick Stolee\nstolee@gmail.com cc: Robert Coup robert@coup.net.nz\n\nA.\nhttps://lore.kernel.org/git/pull.1138.git.1643730593.gitgitgadget@gmail.com/\n\nJohn Cai (4):\n  pack-objects: allow --filter without --stdout\n  repack: add --filter=<filter-spec> option\n  upload-pack: allow missing promisor objects\n  tests for repack --filter mode\n\n Documentation/git-repack.txt   |   5 +\n builtin/pack-objects.c         |   2 -\n builtin/repack.c               |  22 +++--\n t/lib-httpd.sh                 |   2 +\n t/lib-httpd/apache.conf        |   8 ++\n t/lib-httpd/list.sh            |  43 +++++++++\n t/lib-httpd/upload.sh          |  46 +++++++++\n t/t0410-partial-clone.sh       |  81 ++++++++++++++++\n t/t0410/git-remote-testhttpgit | 170 +++++++++++++++++++++++++++++++++\n t/t7700-repack.sh              |  20 ++++\n upload-pack.c                  |   5 +\n 11 files changed, 395 insertions(+), 9 deletions(-)\n create mode 100644 t/lib-httpd/list.sh\n create mode 100644 t/lib-httpd/upload.sh\n create mode 100755 t/t0410/git-remote-testhttpgit\n\n\nbase-commit: 38062e73e009f27ea192d50481fcb5e7b0e9d6eb\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1206%2Fjohn-cai%2Fjc-repack-filter-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1206/john-cai/jc-repack-filter-v2\nPull-Request: https://github.com/git/git/pull/1206\n\nRange-diff vs v1:\n\n 1:  0eec9b117da = 1:  f43b76ca650 pack-objects: allow --filter without --stdout\n -:  ----------- > 2:  6e7c8410b8d repack: add --filter=<filter-spec> option\n -:  ----------- > 3:  40612b9663b upload-pack: allow missing promisor objects\n 2:  a3166381572 ! 4:  d76faa1f16e repack: add --filter=<filter-spec> option\n     @@ Metadata\n      Author: John Cai <johncai86@gmail.com>\n      \n       ## Commit message ##\n     -    repack: add --filter=<filter-spec> option\n     +    tests for repack --filter mode\n      \n     -    Currently, repack does not work with partial clones. When repack is run\n     -    on a partially cloned repository, it grabs all missing objects from\n     -    promisor remotes. This also means that when gc is run for repository\n     -    maintenance on a partially cloned repository, it will end up getting\n     -    missing objects, which is not what we want.\n     -\n     -    In order to make repack work with partial clone, teach repack a new\n     -    option --filter, which takes a <filter-spec> argument. repack will skip\n     -    any objects that are matched by <filter-spec> similar to how the clone\n     -    command will skip fetching certain objects.\n     -\n     -    The final goal of this feature, is to be able to store objects on a\n     -    server other than the regular git server itself.\n     +    This patch adds tests to test both repack --filter functionality in\n     +    isolation (in t7700-repack.sh) as well as how it can be used to offload\n     +    large blobs (in t0410-partial-clone.sh)\n      \n          There are several scripts added so we can test the process of using a\n     -    remote helper to upload blobs to an http server:\n     +    remote helper to upload blobs to an http server.\n      \n          - t/lib-httpd/list.sh lists blobs uploaded to the http server.\n          - t/lib-httpd/upload.sh uploads blobs to the http server.\n     @@ Commit message\n          Based-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\n          Signed-off-by: John Cai <johncai86@gmail.com>\n      \n     - ## Documentation/git-repack.txt ##\n     -@@ Documentation/git-repack.txt: depth is 4095.\n     - \ta larger and slower repository; see the discussion in\n     - \t`pack.packSizeLimit`.\n     - \n     -+--filter=<filter-spec>::\n     -+\tOmits certain objects (usually blobs) from the resulting\n     -+\tpackfile. See linkgit:git-rev-list[1] for valid\n     -+\t`<filter-spec>` forms.\n     -+\n     - -b::\n     - --write-bitmap-index::\n     - \tWrite a reachability bitmap index as part of the repack. This\n     -\n     - ## builtin/repack.c ##\n     -@@ builtin/repack.c: 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     -@@ builtin/repack.c: 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     -@@ builtin/repack.c: 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     -@@ builtin/repack.c: 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\tchar *promisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n     -+\t\t\t\t\t\t\tline.buf);\n     -+\t\t\twrite_promisor_file(promisor_name, NULL, 0);\n     -+\t\t}\n     - \t}\n     - \tfclose(out);\n     - \tret = finish_command(&cmd);\n     -\n       ## t/lib-httpd.sh ##\n      @@ t/lib-httpd.sh: prepare_httpd() {\n       \tinstall_script error-smart-http.sh\n     @@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects from\n      +\tgit -C server rev-list --objects --all --missing=print >objects &&\n      +\tgrep \"$sha\" objects\n      +'\n     ++\n     ++test_expect_success 'fetch does not cause server to fetch missing objects' '\n     ++\trm -rf origin server client &&\n     ++\ttest_create_repo origin &&\n     ++\tdd if=/dev/zero of=origin/file1 bs=801k count=1 &&\n     ++\tgit -C origin add file1 &&\n     ++\tgit -C origin commit -m \"large blob\" &&\n     ++\tsha=\"$(git -C origin rev-parse :file1)\" &&\n     ++\texpected=\"?$(git -C origin rev-parse :file1)\" &&\n     ++\tgit clone --bare --no-local origin server &&\n     ++\tgit -C server remote add httpremote \"testhttpgit::${PWD}/server\" &&\n     ++\tgit -C server config remote.httpremote.promisor true &&\n     ++\tgit -C server config --remove-section remote.origin &&\n     ++\tgit -C server rev-list --all --objects --filter-print-omitted \\\n     ++\t\t--filter=blob:limit=800k | perl -ne \"print if s/^[~]//\" \\\n     ++\t\t>large_blobs.txt &&\n     ++\tupload_blobs_from_stdin server <large_blobs.txt &&\n     ++\tgit -C server -c repack.writebitmaps=false repack -a -d \\\n     ++\t\t--filter=blob:limit=800k &&\n     ++\tgit -C server config uploadpack.allowmissingpromisor true &&\n     ++\tgit clone -c remote.httpremote.url=\"testhttpgit::${PWD}/server\" \\\n     ++\t-c remote.httpremote.fetch='+refs/heads/*:refs/remotes/httpremote/*' \\\n     ++\t-c remote.httpremote.promisor=true --bare --no-local \\\n     ++\t--filter=blob:limit=800k server client &&\n     ++\tgit -C client rev-list --objects --all --missing=print >client_objects &&\n     ++\tgrep \"$expected\" client_objects &&\n     ++\tgit -C server rev-list --objects --all --missing=print >server_objects &&\n     ++\tgrep \"$expected\" server_objects\n     ++'\n      +\n       # DO NOT add non-httpd-specific tests here, because the last part of this\n       # test script is only executed when httpd is available and enabled.\n\n-- \ngitgitgadget\n"},{"id":"448027","messageId":"d76faa1f16e8b5f8eb13284fdb162525fcbcb22e.1644372606.git.gitgitgadget@gmail.com","threadId":"57319","inReplyTo":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] tests for repack --filter mode","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-09T02:10:06Z","receivedAt":"2022-02-09T02:41:35Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nThis patch adds tests to test both repack --filter functionality in\nisolation (in t7700-repack.sh) as well as how it can be used to offload\nlarge blobs (in t0410-partial-clone.sh)\n\nThere are several scripts added so we can test the process of using a\nremote helper to upload blobs to an http server.\n\n- t/lib-httpd/list.sh lists blobs uploaded to the http server.\n- t/lib-httpd/upload.sh uploads blobs to the http server.\n- t/t0410/git-remote-testhttpgit a remote helper that can access blobs\n  onto from an http server. Copied over from t/t5801/git-remote-testhttpgit\n  and modified to upload blobs to an http server.\n- t/t0410/lib-http-promisor.sh convenience functions for uploading\n  blobs\n\nBased-on-patch-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n t/lib-httpd.sh                 |   2 +\n t/lib-httpd/apache.conf        |   8 ++\n t/lib-httpd/list.sh            |  43 +++++++++\n t/lib-httpd/upload.sh          |  46 +++++++++\n t/t0410-partial-clone.sh       |  81 ++++++++++++++++\n t/t0410/git-remote-testhttpgit | 170 +++++++++++++++++++++++++++++++++\n t/t7700-repack.sh              |  20 ++++\n 7 files changed, 370 insertions(+)\n create mode 100644 t/lib-httpd/list.sh\n create mode 100644 t/lib-httpd/upload.sh\n create mode 100755 t/t0410/git-remote-testhttpgit\n\ndiff --git a/t/lib-httpd.sh b/t/lib-httpd.sh\nindex 782891908d7..fc6587c6d39 100644\n--- a/t/lib-httpd.sh\n+++ b/t/lib-httpd.sh\n@@ -136,6 +136,8 @@ prepare_httpd() {\n \tinstall_script error-smart-http.sh\n \tinstall_script error.sh\n \tinstall_script apply-one-time-perl.sh\n+\tinstall_script upload.sh\n+\tinstall_script list.sh\n \n \tln -s \"$LIB_HTTPD_MODULE_PATH\" \"$HTTPD_ROOT_PATH/modules\"\n \ndiff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf\nindex 497b9b9d927..1ea382750f0 100644\n--- a/t/lib-httpd/apache.conf\n+++ b/t/lib-httpd/apache.conf\n@@ -129,6 +129,8 @@ ScriptAlias /broken_smart/ broken-smart-http.sh/\n ScriptAlias /error_smart/ error-smart-http.sh/\n ScriptAlias /error/ error.sh/\n ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n+ScriptAlias /upload/ upload.sh/\n+ScriptAlias /list/ list.sh/\n <Directory ${GIT_EXEC_PATH}>\n \tOptions FollowSymlinks\n </Directory>\n@@ -156,6 +158,12 @@ ScriptAliasMatch /one_time_perl/(.*) apply-one-time-perl.sh/$1\n <Files ${GIT_EXEC_PATH}/git-http-backend>\n \tOptions ExecCGI\n </Files>\n+<Files upload.sh>\n+  Options ExecCGI\n+</Files>\n+<Files list.sh>\n+  Options ExecCGI\n+</Files>\n \n RewriteEngine on\n RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]\ndiff --git a/t/lib-httpd/list.sh b/t/lib-httpd/list.sh\nnew file mode 100644\nindex 00000000000..e63406be3b2\n--- /dev/null\n+++ b/t/lib-httpd/list.sh\n@@ -0,0 +1,43 @@\n+#!/bin/sh\n+\n+# Used in the httpd test server to be called by a remote helper to list objects.\n+\n+FILES_DIR=\"www/files\"\n+\n+OLDIFS=\"$IFS\"\n+IFS='&'\n+set -- $QUERY_STRING\n+IFS=\"$OLDIFS\"\n+\n+while test $# -gt 0\n+do\n+\tkey=${1%%=*}\n+\tval=${1#*=}\n+\n+\tcase \"$key\" in\n+\t\"sha1\") sha1=\"$val\" ;;\n+\t*) echo >&2 \"unknown key '$key'\" ;;\n+\tesac\n+\n+\tshift\n+done\n+\n+if test -d \"$FILES_DIR\"\n+then\n+\tif test -z \"$sha1\"\n+\tthen\n+\t\techo 'Status: 200 OK'\n+\t\techo\n+\t\tls \"$FILES_DIR\" | tr '-' ' '\n+\telse\n+\t\tif test -f \"$FILES_DIR/$sha1\"-*\n+\t\tthen\n+\t\t\techo 'Status: 200 OK'\n+\t\t\techo\n+\t\t\tcat \"$FILES_DIR/$sha1\"-*\n+\t\telse\n+\t\t\techo 'Status: 404 Not Found'\n+\t\t\techo\n+\t\tfi\n+\tfi\n+fi\ndiff --git a/t/lib-httpd/upload.sh b/t/lib-httpd/upload.sh\nnew file mode 100644\nindex 00000000000..202de63b2dc\n--- /dev/null\n+++ b/t/lib-httpd/upload.sh\n@@ -0,0 +1,46 @@\n+#!/bin/sh\n+\n+# In part from http://codereview.stackexchange.com/questions/79549/bash-cgi-upload-file\n+# Used in the httpd test server to for a remote helper to call to upload blobs.\n+\n+FILES_DIR=\"www/files\"\n+\n+OLDIFS=\"$IFS\"\n+IFS='&'\n+set -- $QUERY_STRING\n+IFS=\"$OLDIFS\"\n+\n+while test $# -gt 0\n+do\n+\tkey=${1%%=*}\n+\tval=${1#*=}\n+\n+\tcase \"$key\" in\n+\t\"sha1\") sha1=\"$val\" ;;\n+\t\"type\") type=\"$val\" ;;\n+\t\"size\") size=\"$val\" ;;\n+\t\"delete\") delete=1 ;;\n+\t*) echo >&2 \"unknown key '$key'\" ;;\n+\tesac\n+\n+\tshift\n+done\n+\n+case \"$REQUEST_METHOD\" in\n+POST)\n+\tif test \"$delete\" = \"1\"\n+\tthen\n+\t\trm -f \"$FILES_DIR/$sha1-$size-$type\"\n+\telse\n+\t\tmkdir -p \"$FILES_DIR\"\n+\t\tcat >\"$FILES_DIR/$sha1-$size-$type\"\n+\tfi\n+\n+\techo 'Status: 204 No Content'\n+\techo\n+\t;;\n+\n+*)\n+\techo 'Status: 405 Method Not Allowed'\n+\techo\n+esac\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex f17abd298c8..0724043ffb7 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -30,6 +30,31 @@ promise_and_delete () {\n \tdelete_object repo \"$HASH\"\n }\n \n+upload_blob() {\n+\tSERVER_REPO=\"$1\"\n+\tHASH=\"$2\"\n+\n+\ttest -n \"$HASH\" || die \"Invalid argument '$HASH'\"\n+\tHASH_SIZE=$(git -C \"$SERVER_REPO\" cat-file -s \"$HASH\") || {\n+\t\techo >&2 \"Cannot get blob size of '$HASH'\"\n+\t\treturn 1\n+\t}\n+\n+\tUPLOAD_URL=\"http://127.0.0.1:$LIB_HTTPD_PORT/upload/?sha1=$HASH&size=$HASH_SIZE&type=blob\"\n+\n+\tgit -C \"$SERVER_REPO\" cat-file blob \"$HASH\" >object &&\n+\tcurl --data-binary @object --include \"$UPLOAD_URL\"\n+}\n+\n+upload_blobs_from_stdin() {\n+\tSERVER_REPO=\"$1\"\n+\twhile read -r blob\n+\tdo\n+\t\techo \"uploading $blob\"\n+\t\tupload_blob \"$SERVER_REPO\" \"$blob\" || return\n+\tdone\n+}\n+\n test_expect_success 'extensions.partialclone without filter' '\n \ttest_create_repo server &&\n \tgit clone --filter=\"blob:none\" \"file://$(pwd)/server\" client &&\n@@ -668,6 +693,62 @@ test_expect_success 'fetching of missing objects from an HTTP server' '\n \tgrep \"$HASH\" out\n '\n \n+PATH=\"$TEST_DIRECTORY/t0410:$PATH\"\n+\n+test_expect_success 'fetch of missing objects through remote helper' '\n+\trm -rf origin server &&\n+\ttest_create_repo origin &&\n+\tdd if=/dev/zero of=origin/file1 bs=801k count=1 &&\n+\tgit -C origin add file1 &&\n+\tgit -C origin commit -m \"large blob\" &&\n+\tsha=\"$(git -C origin rev-parse :file1)\" &&\n+\texpected=\"?$(git -C origin rev-parse :file1)\" &&\n+\tgit clone --bare --no-local origin server &&\n+\tgit -C server remote add httpremote \"testhttpgit::${PWD}/server\" &&\n+\tgit -C server config remote.httpremote.promisor true &&\n+\tgit -C server config --remove-section remote.origin &&\n+\tgit -C server rev-list --all --objects --filter-print-omitted \\\n+\t\t--filter=blob:limit=800k | perl -ne \"print if s/^[~]//\" \\\n+\t\t>large_blobs.txt &&\n+\tupload_blobs_from_stdin server <large_blobs.txt &&\n+\tgit -C server -c repack.writebitmaps=false repack -a -d \\\n+\t\t--filter=blob:limit=800k &&\n+\tgit -C server rev-list --objects --all --missing=print >objects &&\n+\tgrep \"$expected\" objects &&\n+\tHTTPD_URL=$HTTPD_URL git -C server show $sha &&\n+\tgit -C server rev-list --objects --all --missing=print >objects &&\n+\tgrep \"$sha\" objects\n+'\n+\n+test_expect_success 'fetch does not cause server to fetch missing objects' '\n+\trm -rf origin server client &&\n+\ttest_create_repo origin &&\n+\tdd if=/dev/zero of=origin/file1 bs=801k count=1 &&\n+\tgit -C origin add file1 &&\n+\tgit -C origin commit -m \"large blob\" &&\n+\tsha=\"$(git -C origin rev-parse :file1)\" &&\n+\texpected=\"?$(git -C origin rev-parse :file1)\" &&\n+\tgit clone --bare --no-local origin server &&\n+\tgit -C server remote add httpremote \"testhttpgit::${PWD}/server\" &&\n+\tgit -C server config remote.httpremote.promisor true &&\n+\tgit -C server config --remove-section remote.origin &&\n+\tgit -C server rev-list --all --objects --filter-print-omitted \\\n+\t\t--filter=blob:limit=800k | perl -ne \"print if s/^[~]//\" \\\n+\t\t>large_blobs.txt &&\n+\tupload_blobs_from_stdin server <large_blobs.txt &&\n+\tgit -C server -c repack.writebitmaps=false repack -a -d \\\n+\t\t--filter=blob:limit=800k &&\n+\tgit -C server config uploadpack.allowmissingpromisor true &&\n+\tgit clone -c remote.httpremote.url=\"testhttpgit::${PWD}/server\" \\\n+\t-c remote.httpremote.fetch='+refs/heads/*:refs/remotes/httpremote/*' \\\n+\t-c remote.httpremote.promisor=true --bare --no-local \\\n+\t--filter=blob:limit=800k server client &&\n+\tgit -C client rev-list --objects --all --missing=print >client_objects &&\n+\tgrep \"$expected\" client_objects &&\n+\tgit -C server rev-list --objects --all --missing=print >server_objects &&\n+\tgrep \"$expected\" server_objects\n+'\n+\n # DO NOT add non-httpd-specific tests here, because the last part of this\n # test script is only executed when httpd is available and enabled.\n \ndiff --git a/t/t0410/git-remote-testhttpgit b/t/t0410/git-remote-testhttpgit\nnew file mode 100755\nindex 00000000000..e5e187243ed\n--- /dev/null\n+++ b/t/t0410/git-remote-testhttpgit\n@@ -0,0 +1,170 @@\n+#!/bin/sh\n+# Copyright (c) 2012 Felipe Contreras\n+# Copyright (c) 2020 Christian Couder\n+\n+# This is a git remote helper that can be used to store blobs on an http server\n+\n+# The first argument can be a url when the fetch/push command was a url\n+# instead of a configured remote. In this case, use a generic alias.\n+if test \"$1\" = \"testhttpgit::$2\"; then\n+\talias=_\n+else\n+\talias=$1\n+fi\n+url=$2\n+\n+unset GIT_DIR\n+\n+h_refspec=\"refs/heads/*:refs/testhttpgit/$alias/heads/*\"\n+t_refspec=\"refs/tags/*:refs/testhttpgit/$alias/tags/*\"\n+\n+if test -n \"$GIT_REMOTE_TESTHTTPGIT_NOREFSPEC\"\n+then\n+\th_refspec=\"\"\n+\tt_refspec=\"\"\n+fi\n+\n+die () {\n+\techo >&2 \"fatal: $*\"\n+\techo \"fatal: $*\" >>/tmp/t0430.txt\n+\techo >>/tmp/t0430.txt\n+\texit 1\n+}\n+\n+force=\n+\n+mark_count_tmp=$(mktemp -t git-remote-http-mark-count_XXXXXX) || die \"Failed to create temp file\"\n+echo \"1\" >\"$mark_count_tmp\"\n+\n+get_mark_count() {\n+\tmark=$(cat \"$mark_count_tmp\")\n+\techo \"$mark\"\n+\tmark=$((mark+1))\n+\techo \"$mark\" >\"$mark_count_tmp\"\t\n+}\n+\n+export_blob_from_file() {\n+\tfile=\"$1\"\n+\techo \"blob\"\n+\techo \"mark :$(get_mark_count)\"\n+\tsize=$(wc -c <\"$file\") || return\n+\techo \"data $size\"\n+\tcat \"$file\" || return\n+\techo\n+}\n+\n+while read line\n+do\n+\tcase $line in\n+\tcapabilities)\n+\t\techo 'import'\n+\t\techo 'export'\n+\t\ttest -n \"$h_refspec\" && echo \"refspec $h_refspec\"\n+\t\ttest -n \"$t_refspec\" && echo \"refspec $t_refspec\"\n+\t\ttest -n \"$GIT_REMOTE_TESTHTTPGIT_SIGNED_TAGS\" && echo \"signed-tags\"\n+\t\ttest -n \"$GIT_REMOTE_TESTHTTPGIT_NO_PRIVATE_UPDATE\" && echo \"no-private-update\"\n+\t\techo 'option'\n+\t\techo\n+\t\t;;\n+\tlist)\n+\t\tgit -C \"$url\" for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'\n+\t\thead=$(git -C \"$url\" symbolic-ref HEAD)\n+\t\techo \"@$head HEAD\"\n+\t\techo\n+\t\t;;\n+\timport*)\n+\t\t# read all import lines\n+\t\twhile true\n+\t\tdo\n+\t\t\tref=\"${line#* }\"\n+\t\t\trefs=\"$refs $ref\"\n+\t\t\tread line\n+\t\t\ttest \"${line%% *}\" != \"import\" && break\n+\t\tdone\n+\n+\t\techo \"refs: $refs\" >>/tmp/t0430.txt\n+\n+\t\tif test -n \"$GIT_REMOTE_TESTHTTPGIT_FAILURE\"\n+\t\tthen\n+\t\t\techo \"feature done\"\n+\t\t\texit 1\n+\t\tfi\n+\n+\t\techo \"feature done\"\n+\n+\t\ttmpdir=$(mktemp -d -t git-remote-http-import_XXXXXX) || die \"Failed to create temp directory\"\n+\n+\t\tfor ref in $refs\n+\t\tdo\n+\t\t\tget_url=\"$HTTPD_URL/list/?sha1=$ref\"\n+\t\t\techo \"curl url: $get_url\" >>/tmp/t0430.txt\n+\t\t\techo \"curl output: $tmpdir/$ref\" >>/tmp/t0430.txt\n+\t\t\tcurl -s -o \"$tmpdir/$ref\" \"$get_url\" ||\n+\t\t\t\tdie \"curl '$get_url' failed\"\n+\t\t\techo \"exporting from: $tmpdir/$ref\" >>/tmp/t0430.txt\n+\t\t\texport_blob_from_file \"$tmpdir/$ref\" ||\n+\t\t\t\tdie \"failed to export blob from '$tmpdir/$ref'\"\n+\t\t\techo \"done exporting\" >>/tmp/t0430.txt\n+\t\tdone\n+\n+\t\techo \"done\"\n+\t\t;;\n+\texport)\n+\t\tif test -n \"$GIT_REMOTE_TESTHTTPGIT_FAILURE\"\n+\t\tthen\n+\t\t\t# consume input so fast-export doesn't get SIGPIPE;\n+\t\t\t# git would also notice that case, but we want\n+\t\t\t# to make sure we are exercising the later\n+\t\t\t# error checks\n+\t\t\twhile read line; do\n+\t\t\t\ttest \"done\" = \"$line\" && break\n+\t\t\tdone\n+\t\t\texit 1\n+\t\tfi\n+\n+\t\tbefore=$(git -C \"$url\" for-each-ref --format=' %(refname) %(objectname) ')\n+\n+\t\tgit -C \"$url\" fast-import \\\n+\t\t\t${force:+--force} \\\n+\t\t\t${testhttpgitmarks:+\"--import-marks=$testhttpgitmarks\"} \\\n+\t\t\t${testhttpgitmarks:+\"--export-marks=$testhttpgitmarks\"} \\\n+\t\t\t--quiet\n+\n+\t\t# figure out which refs were updated\n+\t\tgit -C \"$url\" for-each-ref --format='%(refname) %(objectname)' |\n+\t\twhile read ref a\n+\t\tdo\n+\t\t\tcase \"$before\" in\n+\t\t\t*\" $ref $a \"*)\n+\t\t\t\tcontinue ;;\t# unchanged\n+\t\t\tesac\n+\t\t\tif test -z \"$GIT_REMOTE_TESTHTTPGIT_PUSH_ERROR\"\n+\t\t\tthen\n+\t\t\t\techo \"ok $ref\"\n+\t\t\telse\n+\t\t\t\techo \"error $ref $GIT_REMOTE_TESTHTTPGIT_PUSH_ERROR\"\n+\t\t\tfi\n+\t\tdone\n+\n+\t\techo\n+\t\t;;\n+\toption\\ *)\n+\t\tread cmd opt val <<-EOF\n+\t\t$line\n+\t\tEOF\n+\t\tcase $opt in\n+\t\tforce)\n+\t\t\ttest $val = \"true\" && force=\"true\" || force=\n+\t\t\techo \"ok\"\n+\t\t\t;;\n+\t\t*)\n+\t\t\techo \"unsupported\"\n+\t\t\t;;\n+\t\tesac\n+\t\t;;\n+\t'')\n+\t\texit\n+\t\t;;\n+\tesac\n+done\n+\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex e489869dd94..78cc1858cb6 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'repack with filter does not fetch from remote' '\n+\trm -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+\techo content1 >server/file1 &&\n+\tgit -C server add file1 &&\n+\tgit -C server commit -m initial_commit &&\n+\texpected=\"?$(git -C server rev-parse :file1)\" &&\n+\tgit clone --bare --no-local server client &&\n+\tgit -C client config remote.origin.promisor true &&\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+\tgrep \"$expected\" objects &&\n+\tgit -C client repack -a -d &&\n+\texpected=\"$(git -C server rev-parse :file1)\" &&\n+\tgit -C client rev-list --objects --all --missing=print >objects &&\n+\tgrep \"$expected\" objects\n+'\n+\n objdir=.git/objects\n midx=$objdir/pack/multi-pack-index\n \n-- \ngitgitgadget\n"},{"id":"448603","messageId":"CAFLLRpJ1aDyLb4qAoQwYDyGdP1_PH8kzLAQCKJpQwiYiapZ5Aw@mail.gmail.com","threadId":"57319","inReplyTo":"pull.1206.v2.git.git.1644372606.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2022-02-16T15:39:47Z","receivedAt":"2022-02-16T15:40:06Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi John,\n\nOn Wed, 9 Feb 2022 at 02:41, John Cai via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> This patch series makes partial clone more useful by making it possible to\n> run repack to remove objects from a repository (replacing it with promisor\n> objects). This is useful when we want to offload large blobs from a git\n> server onto another git server, or even use an http server through a remote\n> helper.\n>\n> In [A], a --refilter option on fetch and fetch-pack is being discussed where\n> either a less restrictive or more restrictive filter can be used. In the\n> more restrictive case, the objects that already exist will not be deleted.\n> But, one can imagine that users might want the ability to delete objects\n> when they apply a more restrictive filter in order to save space, and this\n> patch series would also allow that.\n\nThis all makes sense to me, and the implementation is remarkably short -\ngluing together capabilities that are already there, and writing tests.\n\n*But*, running `repack --filter` drops objects from the object db.\nThat seems like\na capability Git shouldn't idly expose without people understanding the\nconsequences - mostly that they really have another copy elsewhere or they\nwill lose data, and it won't necessarily be obvious for a long time. Otherwise\nit is a footgun.\n\nI don't know whether that is just around naming (--delete-filter /\n--drop-filter /\n--expire-filter ?), and/or making the documentation very explicit that\nthis isn't so\nmuch \"omitting certain objects from a packfile\" as irretrievably\ndeleting objects.\n\nRob :)\n"},{"id":"448637","messageId":"CB2ACEF7-76A9-4253-AD43-7BC842F9576D@gmail.com","threadId":"57319","inReplyTo":"CAFLLRpJ1aDyLb4qAoQwYDyGdP1_PH8kzLAQCKJpQwiYiapZ5Aw@mail.gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-16T21:07:14Z","receivedAt":"2022-02-16T21:07:23Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Rob,\n\nglad these two efforts dovetail nicely!\n\nOn 16 Feb 2022, at 10:39, Robert Coup wrote:\n\n> Hi John,\n>\n> On Wed, 9 Feb 2022 at 02:41, John Cai via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> This patch series makes partial clone more useful by making it possible to\n>> run repack to remove objects from a repository (replacing it with promisor\n>> objects). This is useful when we want to offload large blobs from a git\n>> server onto another git server, or even use an http server through a remote\n>> helper.\n>>\n>> In [A], a --refilter option on fetch and fetch-pack is being discussed where\n>> either a less restrictive or more restrictive filter can be used. In the\n>> more restrictive case, the objects that already exist will not be deleted.\n>> But, one can imagine that users might want the ability to delete objects\n>> when they apply a more restrictive filter in order to save space, and this\n>> patch series would also allow that.\n>\n> This all makes sense to me, and the implementation is remarkably short -\n> gluing together capabilities that are already there, and writing tests.\n>\n> *But*, running `repack --filter` drops objects from the object db.\n> That seems like\n> a capability Git shouldn't idly expose without people understanding the\n> consequences - mostly that they really have another copy elsewhere or they\n> will lose data, and it won't necessarily be obvious for a long time. Otherwise\n> it is a footgun.\n\nYes, great point. I think there was concern from Stolee around this as well.\n>\n> I don't know whether that is just around naming (--delete-filter /\n> --drop-filter /\n> --expire-filter ?), and/or making the documentation very explicit that\n> this isn't so\n> much \"omitting certain objects from a packfile\" as irretrievably\n> deleting objects.\n\nYeah, making the name very clear (I kind of like --delete-filter) would certainly help.\nAlso, to have more protection we can either\n\n1. add a config value that needs to be set to true for repack to remove\nobjects (repack.allowDestroyFilter).\n\n2. --filter is dry-run by default and prints out objects that would have been removed,\nand it has to be combined with another flag --destroy in order for it to actually remove\nobjects from the odb.\n\n>\n> Rob :)\n"},{"id":"448712","messageId":"CAFLLRp+9TmMKu5UpaN4sUr+o_9AGAVvtis0e87VMJsCva67q3w@mail.gmail.com","threadId":"57319","inReplyTo":"d76faa1f16e8b5f8eb13284fdb162525fcbcb22e.1644372606.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/4] tests for repack --filter mode","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2022-02-17T16:14:02Z","receivedAt":"2022-02-17T16:14:23Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi John,\n\nMinor, but should we use oid rather than sha1 in the list.sh/upload.sh\nscripts? wrt sha256 slowly coming along the pipe.\n\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index e489869dd94..78cc1858cb6 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n>         test_must_be_empty actual\n>  '\n>\n> +test_expect_success 'repack with filter does not fetch from remote' '\n> +       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> +       echo content1 >server/file1 &&\n> +       git -C server add file1 &&\n> +       git -C server commit -m initial_commit &&\n> +       expected=\"?$(git -C server rev-parse :file1)\" &&\n> +       git clone --bare --no-local server client &&\n> +       git -C client config remote.origin.promisor true &&\n> +       git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n\nDoes writing bitmaps have any effect/interaction here?\n\n> +       git -C client rev-list --objects --all --missing=print >objects &&\n> +       grep \"$expected\" objects &&\n\nThis is testing the object that was cloned initially is gone after the\nrepack, ok.\n\n> +       git -C client repack -a -d &&\n> +       expected=\"$(git -C server rev-parse :file1)\" &&\n> +       git -C client rev-list --objects --all --missing=print >objects &&\n> +       grep \"$expected\" objects\n\nBut I'm not sure what you're testing here? A repack wouldn't fetch\nmissing objects for a promisor pack anyway... and because there's no\n'^' in the pattern the grep will succeed regardless of whether the\nobject is missing/present.\n\nRob :)\n"},{"id":"448731","messageId":"6BD2011A-9CD7-488C-8F17-F78FE59E93C7@gmail.com","threadId":"57319","inReplyTo":"CAFLLRp+9TmMKu5UpaN4sUr+o_9AGAVvtis0e87VMJsCva67q3w@mail.gmail.com","subject":"Re: [PATCH v2 4/4] tests for repack --filter mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-17T20:36:57Z","receivedAt":"2022-02-17T20:37:04Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Rob,\n\nOn 17 Feb 2022, at 11:14, Robert Coup wrote:\n\n> Hi John,\n>\n> Minor, but should we use oid rather than sha1 in the list.sh/upload.sh\n> scripts? wrt sha256 slowly coming along the pipe.\n\ngood point, I'll make those adjustments.\n\n>\n>> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n>> index e489869dd94..78cc1858cb6 100755\n>> --- a/t/t7700-repack.sh\n>> +++ b/t/t7700-repack.sh\n>> @@ -237,6 +237,26 @@ test_expect_success 'auto-bitmaps do not complain if unavailable' '\n>>         test_must_be_empty actual\n>>  '\n>>\n>> +test_expect_success 'repack with filter does not fetch from remote' '\n>> +       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>> +       echo content1 >server/file1 &&\n>> +       git -C server add file1 &&\n>> +       git -C server commit -m initial_commit &&\n>> +       expected=\"?$(git -C server rev-parse :file1)\" &&\n>> +       git clone --bare --no-local server client &&\n>> +       git -C client config remote.origin.promisor true &&\n>> +       git -C client -c repack.writebitmaps=false repack -a -d --filter=blob:none &&\n>\n> Does writing bitmaps have any effect/interaction here?\n\nCurrently writing bitmaps don't play well with promisor objects. If I'm reading\nthe code correctly, it seems that when we build a bitmap with\nbitmap_writer_build(), find_object_pos() gets called and will complain if an\nobject is missing from the pack.\n\nWe probably need to do the work to allow bitmaps to play well with promisor\nobjects.\n\n>\n>> +       git -C client rev-list --objects --all --missing=print >objects &&\n>> +       grep \"$expected\" objects &&\n>\n> This is testing the object that was cloned initially is gone after the\n> repack, ok.\n>\n>> +       git -C client repack -a -d &&\n>> +       expected=\"$(git -C server rev-parse :file1)\" &&\n>> +       git -C client rev-list --objects --all --missing=print >objects &&\n>> +       grep \"$expected\" objects\n>\n> But I'm not sure what you're testing here? A repack wouldn't fetch\n> missing objects for a promisor pack anyway... and because there's no\n> '^' in the pattern the grep will succeed regardless of whether the\n> object is missing/present.\n\nGood point. I overlooked the fact that by this point in the test, repack has\nalready written a promisor file. I think I'll just remove these last couple of\nlines.\n\n>\n> Rob :)\n"},{"id":"448946","messageId":"YhMC+3FdSEZz22qX@nand.local","threadId":"57319","inReplyTo":"CB2ACEF7-76A9-4253-AD43-7BC842F9576D@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T03:11:55Z","receivedAt":"2022-02-21T03:11:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Feb 16, 2022 at 04:07:14PM -0500, John Cai wrote:\n> > I don't know whether that is just around naming (--delete-filter /\n> > --drop-filter /\n> > --expire-filter ?), and/or making the documentation very explicit that\n> > this isn't so\n> > much \"omitting certain objects from a packfile\" as irretrievably\n> > deleting objects.\n>\n> Yeah, making the name very clear (I kind of like --delete-filter) would certainly help.\n> Also, to have more protection we can either\n>\n> 1. add a config value that needs to be set to true for repack to remove\n> objects (repack.allowDestroyFilter).\n>\n> 2. --filter is dry-run by default and prints out objects that would have been removed,\n> and it has to be combined with another flag --destroy in order for it to actually remove\n> objects from the odb.\n\nI share the same concern as Robert and Stolee do. But I think this issue\ngoes deeper than just naming.\n\nEven if we called this `git repack --delete-filter` and only ran it with\n`--i-know-what-im-doing` flag, we would still be leaving repository\ncorruption on the table, just making it marginally more difficult to\nachieve.\n\nI'm not familiar enough with the proposal to comment authoritatively,\nbut it seems like we should be verifying that there is a promisor remote\nwhich promises any objects that we are about to filter out of the\nrepository.\n\nI think that this is basically what `pack-objects`'s\n`--missing=allow-promisor` does, though I don't think that's the right\ntool for this job, either. Because we pack-objects also knows the object\nfilter, by the time we are ready to construct a pack, we're traversing\nthe filtered list of objects.\n\nSo we don't even bother to call show_object (or, in this case,\nbuiltin/pack-objects.c::show_objecT__ma_allow_promisor) on them.\n\nSo I wonder what your thoughts are on having pack-objects only allow an\nobject to get \"filtered out\" if a copy of it is promised by some\npromisor remote. Alternatively, and perhaps a more straight-forward\noption might be to have `git repack` look at any objects that exist in a\npack we're about to delete, but don't exist in any of the packs we are\ngoing to leave around, and make sure that any of those objects are\neither unreachable or exist on a promisor remote.\n\nBut as it stands right now, I worry that this feature is too easily\nmisused and could result in unintended repository corruption.\n\nI think verifying that that any objects we're about to delete exist\nsomewhere should make this safer to use, though even then, I think we're\nstill open to a TOCTOU race whereby the promisor has the objects when\nwe're about to delete them (convincing Git that deleting those objects\nis OK to do) but gets rid of them after objects have been deleted from\nthe local copy (leaving no copies of the object around).\n\nSo, I don't know exactly what the right path forward is. But I'm curious\nto get your thoughts on the above.\n\nThanks,\nTaylor\n"},{"id":"449007","messageId":"CAFLLRp+JHi6B-RTeaWVPy2bZVHJ-y7EyMpymQy2LBynbZ8RzNA@mail.gmail.com","threadId":"57319","inReplyTo":"YhMC+3FdSEZz22qX@nand.local","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2022-02-21T15:38:03Z","receivedAt":"2022-02-21T15:38:22Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi,\n\nOn Mon, 21 Feb 2022 at 03:11, Taylor Blau <me@ttaylorr.com> wrote:\n>\n> we would still be leaving repository\n> corruption on the table, just making it marginally more difficult to\n> achieve.\n\nWhile reviewing John's patch I initially wondered if a better approach\nmight be something like `git repack -a -d --exclude-stdin`, passing a\nlist of specific objects to exclude from the new pack (sourced from\nrev-list via a filter, etc). To me this seems like a less dangerous\napproach, but my concern was it doesn't use the existing filter\ncapabilities of pack-objects, and we end up generating and passing\naround a huge list of oids. And of course any mistakes in the list\ngeneration aren't visible until it's too late.\n\nI also wonder whether there's a race condition if the repository gets\nupdated? If you're moving large objects out in advance, then filtering\nthe remainder there's nothing to stop a new large object being pushed\nbetween those two steps and getting dropped.\n\nMy other idea, which is growing on me, is whether repack could\ngenerate two valid packs: one for the included objects via the filter\n(as John's change does now), and one containing the filtered-out\nobjects. `git repack -a -d --split-filter=<filter>` Then a user could\nthen move/extract the second packfile to object storage, but there'd\nbe no way to *accidentally* corrupt the repository by using a bad\noption. With this approach the race condition above goes away too.\n\n    $ git repack -a -d -q --split-filter=blob:limit=1m\n    pack-37b7443e3123549a2ddee31f616ae272c51cae90\n    pack-10789d94fcd99ffe1403b63b167c181a9df493cd\n\nFirst pack identifier being the objects that match the filter (ie:\ncommits/trees/blobs <1m), and the second pack identifier being the\nobjects that are excluded by the filter (blobs >1m).\n\nAn astute --i-know-what-im-doing reader could spot that you could just\ndelete the second packfile and achieve the same outcome as the current\nproposed patch, subject to being confident the race condition hadn't\nhappened to you.\n\nThanks,\nRob :)\n"},{"id":"449025","messageId":"YhPSb74x7davprsz@nand.local","threadId":"57319","inReplyTo":"CAFLLRp+JHi6B-RTeaWVPy2bZVHJ-y7EyMpymQy2LBynbZ8RzNA@mail.gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T17:57:03Z","receivedAt":"2022-02-21T18:09:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 21, 2022 at 03:38:03PM +0000, Robert Coup wrote:\n> Hi,\n>\n> On Mon, 21 Feb 2022 at 03:11, Taylor Blau <me@ttaylorr.com> wrote:\n> >\n> > we would still be leaving repository\n> > corruption on the table, just making it marginally more difficult to\n> > achieve.\n>\n> While reviewing John's patch I initially wondered if a better approach\n> might be something like `git repack -a -d --exclude-stdin`, passing a\n> list of specific objects to exclude from the new pack (sourced from\n> rev-list via a filter, etc). To me this seems like a less dangerous\n> approach, but my concern was it doesn't use the existing filter\n> capabilities of pack-objects, and we end up generating and passing\n> around a huge list of oids. And of course any mistakes in the list\n> generation aren't visible until it's too late.\n\nYeah; I think the most elegant approach would have pack-objects do as\nmuch work as possible, and have repack be in charge of coordinating what\nall the pack-objects invocation(s) have to do.\n\n> I also wonder whether there's a race condition if the repository gets\n> updated? If you're moving large objects out in advance, then filtering\n> the remainder there's nothing to stop a new large object being pushed\n> between those two steps and getting dropped.\n\nYeah, we will want to make sure that we're operating on a consistent\nview of the repository. If this is all done in-process, it won't be a\nproblem since we'll capture an atomic snapshot of the reference states\nonce. If this is done across multiple processes, we'll need to make sure\nwe're passing around that snapshot where appropriate.\n\nSee the `--refs-snapshot`-related code in git-repack for when we write a\nmulti-pack bitmap for an example of the latter.\n\n> My other idea, which is growing on me, is whether repack could\n> generate two valid packs: one for the included objects via the filter\n> (as John's change does now), and one containing the filtered-out\n> objects. `git repack -a -d --split-filter=<filter>` Then a user could\n> then move/extract the second packfile to object storage, but there'd\n> be no way to *accidentally* corrupt the repository by using a bad\n> option. With this approach the race condition above goes away too.\n>\n>     $ git repack -a -d -q --split-filter=blob:limit=1m\n>     pack-37b7443e3123549a2ddee31f616ae272c51cae90\n>     pack-10789d94fcd99ffe1403b63b167c181a9df493cd\n>\n> First pack identifier being the objects that match the filter (ie:\n> commits/trees/blobs <1m), and the second pack identifier being the\n> objects that are excluded by the filter (blobs >1m).\n\nI like this idea quite a bit. We also have a lot of existing tools that\nwould make an implementation fairly lightweight, namely pack-objects'\n`--stdin-packs` mode.\n\nUsing that would look something like first having `repack` generate the\nfiltered pack first, remembering its name [1]. After that, we would run\n`pack-objects` again, this time with `--stdin-packs`, where the positive\npacks are the ones we're going to delete, and the negative pack(s)\nis/are the filtered one generated in the last step.\n\nThe second invocation would leave us with a single pack which represents\nall of the objects in packs we are about to delete, skipping any objects\nthat are in the filtered pack we just generated. In other words, it\nwould leave the repository with two packs: one with all of the objects\nthat met the filter criteria, and one with all objects that don't meet\nthe filter criteria.\n\nA client could then upload the \"doesn't meet the filter criteria\" pack\noff elsewhere, and then delete it locally. (I'm assuming this last part\nin particular is orchestrated by some other command, and we aren't\nencouraging users to run \"rm\" inside of .git/objects/pack!)\n\n> An astute --i-know-what-im-doing reader could spot that you could just\n> delete the second packfile and achieve the same outcome as the current\n> proposed patch, subject to being confident the race condition hadn't\n> happened to you.\n\nYeah, and I think this goes to my joking remark in the last paragraph.\nIf we allow users to delete packs at will, all bets are off regardless\nof how safely we generate those packs. But I think splitting the\nrepository into two packs and _then_ dealing with one of them separately\nas opposed to deleting objects which don't meet the filter criteria\nimmediately is moving in a safer direction.\n\n> Thanks,\n> Rob :)\n\nThanks,\nTaylor\n\n[1]: Optionally \"name_s_\", if we passed the `--max-pack-size` option to\n`git pack-objects`, which we can trigger via a `git repack` option of\nthe same name.\n"},{"id":"449059","messageId":"CAP8UFD2dpicW64eqBK47g43xDWA1qv2BMBEOSqj_My5PUs8TSg@mail.gmail.com","threadId":"57319","inReplyTo":"YhMC+3FdSEZz22qX@nand.local","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-21T21:10:15Z","receivedAt":"2022-02-21T21:10:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 21, 2022 at 4:11 AM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Wed, Feb 16, 2022 at 04:07:14PM -0500, John Cai wrote:\n> > > I don't know whether that is just around naming (--delete-filter /\n> > > --drop-filter /\n> > > --expire-filter ?), and/or making the documentation very explicit that\n> > > this isn't so\n> > > much \"omitting certain objects from a packfile\" as irretrievably\n> > > deleting objects.\n> >\n> > Yeah, making the name very clear (I kind of like --delete-filter) would certainly help.\n\nI am ok with making the name and doc very clear that it deletes\nobjects and they might be lost if they haven't been saved elsewhere\nfirst.\n\n> > Also, to have more protection we can either\n> >\n> > 1. add a config value that needs to be set to true for repack to remove\n> > objects (repack.allowDestroyFilter).\n\nI don't think it's of much value. We don't have such config values for\nother possibly destructive operations.\n\n> > 2. --filter is dry-run by default and prints out objects that would have been removed,\n> > and it has to be combined with another flag --destroy in order for it to actually remove\n> > objects from the odb.\n\nI am not sure it's of much value either compared to naming it\n--filter-destroy. It's likely to just make things more difficult for\nusers to understand.\n\n> I share the same concern as Robert and Stolee do. But I think this issue\n> goes deeper than just naming.\n>\n> Even if we called this `git repack --delete-filter` and only ran it with\n> `--i-know-what-im-doing` flag, we would still be leaving repository\n> corruption on the table, just making it marginally more difficult to\n> achieve.\n\nMy opinion on this is that the promisor object mechanism assumes by\ndesign that some objects are outside a repo, and that this repo\nshouldn't care much about these objects possibly being corrupted.\n\nIt's the same for git LFS. As only a pointer file is stored in Git and\nthe real file is stored elsewhere, the Git repo doesn't care by design\nabout possible corruption of the real file.\n\nI am not against a name and some docs that strongly state that users\nshould be very careful when using such a command, but otherwise I\nthink such a command is perfectly ok. We have other commands that by\ndesign could lead to some objects or data being lost.\n\n> I'm not familiar enough with the proposal to comment authoritatively,\n> but it seems like we should be verifying that there is a promisor remote\n> which promises any objects that we are about to filter out of the\n> repository.\n\nI think it could be a follow up mode that could be useful and safe,\nbut there should be no requirement for such a mode. In some cases you\nknow very much what you want and you don't want checks. For example if\nyou have taken proper care to transfer large objects to another\nremote, you might just not need other possibly expansive checks.\n\n[...]\n\n> But as it stands right now, I worry that this feature is too easily\n> misused and could result in unintended repository corruption.\n\nAre you worrying about the UI or about what it does?\n\nI am ok with improving the UI, but I think what it does is reasonable.\n\n> I think verifying that that any objects we're about to delete exist\n> somewhere should make this safer to use, though even then, I think we're\n> still open to a TOCTOU race whereby the promisor has the objects when\n> we're about to delete them (convincing Git that deleting those objects\n> is OK to do) but gets rid of them after objects have been deleted from\n> the local copy (leaving no copies of the object around).\n\nPossible TOCTOU races are a good reason why something with no check is\nperhaps a better goal for now.\n"},{"id":"449060","messageId":"YhQHYQ9b9bYYv10r@nand.local","threadId":"57319","inReplyTo":"CAP8UFD2dpicW64eqBK47g43xDWA1qv2BMBEOSqj_My5PUs8TSg@mail.gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T21:42:57Z","receivedAt":"2022-02-21T21:43:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Feb 21, 2022 at 10:10:15PM +0100, Christian Couder wrote:\n> > > Also, to have more protection we can either\n> > >\n> > > 1. add a config value that needs to be set to true for repack to remove\n> > > objects (repack.allowDestroyFilter).\n>\n> I don't think it's of much value. We don't have such config values for\n> other possibly destructive operations.\n>\n> > > 2. --filter is dry-run by default and prints out objects that would have been removed,\n> > > and it has to be combined with another flag --destroy in order for it to actually remove\n> > > objects from the odb.\n>\n> I am not sure it's of much value either compared to naming it\n> --filter-destroy. It's likely to just make things more difficult for\n> users to understand.\n\nOn this and the above, I agree with Christian.\n\n> > I share the same concern as Robert and Stolee do. But I think this issue\n> > goes deeper than just naming.\n> >\n> > Even if we called this `git repack --delete-filter` and only ran it with\n> > `--i-know-what-im-doing` flag, we would still be leaving repository\n> > corruption on the table, just making it marginally more difficult to\n> > achieve.\n>\n> My opinion on this is that the promisor object mechanism assumes by\n> design that some objects are outside a repo, and that this repo\n> shouldn't care much about these objects possibly being corrupted.\n\nFor what it's worth, I am fine having a mode of repack which allows us\nto remove objects that we know are stored by a promisor remote. But this\nseries doesn't do that, so users could easily run `git repack -d\n--filter=...` and find that they have irrecoverably corrupted their\nrepository.\n\nI think that there are some other reasonable directions, though. One\nwhich Robert and I discussed was making it possible to split a\nrepository into two packs, one which holds objects that match some\n`--filter` criteria, and one which holds the objects that don't match\nthat filter.\n\nAnother option would be to prune the repository according to objects\nthat are already made available by a promisor remote.\n\nAn appealing quality about the above two directions is that the first\ndoesn't actually remove any objects, just makes it easier to push a\nwhole pack of unwanted objects off to a promsior remote. The second\nprunes the repository according to objects that are already made\navailable by the promisor remote. (Yes, there is a TOCTOU race there,\ntoo, but it's the same prune-while-pushing race that Git already has\ntoday).\n\n> I am not against a name and some docs that strongly state that users\n> should be very careful when using such a command, but otherwise I\n> think such a command is perfectly ok. We have other commands that by\n> design could lead to some objects or data being lost.\n\nI can think of a handful of ways to remove objects which are unreachable\nfrom a repository, but I am not sure we have any ways to remove objects\nwhich are reachable.\n\n> > But as it stands right now, I worry that this feature is too easily\n> > misused and could result in unintended repository corruption.\n>\n> Are you worrying about the UI or about what it does?\n>\n> I am ok with improving the UI, but I think what it does is reasonable.\n\nI am more worried about the proposal's functionality than its UI,\nhopefully my concerns there are summarized above.\n\nThanks,\nTaylor\n"},{"id":"449171","messageId":"CAP8UFD3U4t-inWC5mZYhybWpjVwkqA7v4hYZ5voBOEJ=+_Y1kQ@mail.gmail.com","threadId":"57319","inReplyTo":"YhQHYQ9b9bYYv10r@nand.local","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-22T17:11:11Z","receivedAt":"2022-02-22T17:11:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Feb 21, 2022 at 10:42 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Mon, Feb 21, 2022 at 10:10:15PM +0100, Christian Couder wrote:\n> > > > Also, to have more protection we can either\n> > > >\n> > > > 1. add a config value that needs to be set to true for repack to remove\n> > > > objects (repack.allowDestroyFilter).\n> >\n> > I don't think it's of much value. We don't have such config values for\n> > other possibly destructive operations.\n> >\n> > > > 2. --filter is dry-run by default and prints out objects that would have been removed,\n> > > > and it has to be combined with another flag --destroy in order for it to actually remove\n> > > > objects from the odb.\n> >\n> > I am not sure it's of much value either compared to naming it\n> > --filter-destroy. It's likely to just make things more difficult for\n> > users to understand.\n>\n> On this and the above, I agree with Christian.\n>\n> > > I share the same concern as Robert and Stolee do. But I think this issue\n> > > goes deeper than just naming.\n> > >\n> > > Even if we called this `git repack --delete-filter` and only ran it with\n> > > `--i-know-what-im-doing` flag, we would still be leaving repository\n> > > corruption on the table, just making it marginally more difficult to\n> > > achieve.\n> >\n> > My opinion on this is that the promisor object mechanism assumes by\n> > design that some objects are outside a repo, and that this repo\n> > shouldn't care much about these objects possibly being corrupted.\n>\n> For what it's worth, I am fine having a mode of repack which allows us\n> to remove objects that we know are stored by a promisor remote. But this\n> series doesn't do that, so users could easily run `git repack -d\n> --filter=...` and find that they have irrecoverably corrupted their\n> repository.\n\nIn some cases we just know the objects we are removing are stored by a\npromisor remote or are replicated on different physical machines or\nboth, so you should be fine with this.\n\nIf you are not fine with this because sometimes a user might use it\nwithout knowing, then why are you ok with commands deleting refs not\nchecking that there isn't a regular repack removing dangling objects?\n\nAlso note that people who want to remove objects using a filter can\nalready do it by cloning with a filter and then replacing the original\npacks with the packs from the clone. So refusing this new feature is\njust making things more cumbersome.\n\n> I think that there are some other reasonable directions, though. One\n> which Robert and I discussed was making it possible to split a\n> repository into two packs, one which holds objects that match some\n> `--filter` criteria, and one which holds the objects that don't match\n> that filter.\n\nI am ok with someone implementing this feature, but if an option that\nactually deletes the filtered objects is rejected then such a feature\nwill be used with some people just deleting one of the resulting packs\n(and they might get it wrong), so I don't think any real safety will\nbe gained.\n\n> Another option would be to prune the repository according to objects\n> that are already made available by a promisor remote.\n\nIf the objects have just been properly transferred to the promisor\nremote, the check will just waste resources.\n\n> An appealing quality about the above two directions is that the first\n> doesn't actually remove any objects, just makes it easier to push a\n> whole pack of unwanted objects off to a promsior remote. The second\n> prunes the repository according to objects that are already made\n> available by the promisor remote. (Yes, there is a TOCTOU race there,\n> too, but it's the same prune-while-pushing race that Git already has\n> today).\n>\n> > I am not against a name and some docs that strongly state that users\n> > should be very careful when using such a command, but otherwise I\n> > think such a command is perfectly ok. We have other commands that by\n> > design could lead to some objects or data being lost.\n>\n> I can think of a handful of ways to remove objects which are unreachable\n> from a repository, but I am not sure we have any ways to remove objects\n> which are reachable.\n\nCloning with a filter already does that. It's by design in the\npromisor object and partial clone mechanisms that reachable objects\nare removed. Having more than one promisor remote, which is an\nexisting mechanism, means that it's just wasteful to require all the\nremotes to have all the reachable objects, so how could people easily\nset up such remotes? Why make it unnecessarily hard and forbid a\nstraightforward way?\n"},{"id":"449173","messageId":"YhUeUCIetu/aOu6k@nand.local","threadId":"57319","inReplyTo":"CAP8UFD3U4t-inWC5mZYhybWpjVwkqA7v4hYZ5voBOEJ=+_Y1kQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-22T17:33:04Z","receivedAt":"2022-02-22T17:33:09Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Christian,\n\nI think my objections may be based on a misunderstanding of John and\nyour original proposal. From reading [1], it seemed to me like a\nrequired step of this proposal was to upload the objects you want to\nfilter out ahead of time, and then run `git repack -ad --filter=...`.\n\nSo my concerns thus far have been around the lack of cohesion between\n(1) the filter which describes the set of objects uploaded to the HTTP\nserver, and (2) the filter used when re-filtering the repository.\n\nIf (1) and (2) aren't inverses of each other, then in the case where (2)\nleaves behind an object which wasn't caught by (1), we have lost that\nobject.\n\nIf instead the server used in your script at [1] is a stand-in for an\nordinary Git remote, that changes my thinking significantly. See below\nfor more details:\n\nOn Tue, Feb 22, 2022 at 06:11:11PM +0100, Christian Couder wrote:\n> > > > I share the same concern as Robert and Stolee do. But I think this issue\n> > > > goes deeper than just naming.\n> > > >\n> > > > Even if we called this `git repack --delete-filter` and only ran it with\n> > > > `--i-know-what-im-doing` flag, we would still be leaving repository\n> > > > corruption on the table, just making it marginally more difficult to\n> > > > achieve.\n> > >\n> > > My opinion on this is that the promisor object mechanism assumes by\n> > > design that some objects are outside a repo, and that this repo\n> > > shouldn't care much about these objects possibly being corrupted.\n> >\n> > For what it's worth, I am fine having a mode of repack which allows us\n> > to remove objects that we know are stored by a promisor remote. But this\n> > series doesn't do that, so users could easily run `git repack -d\n> > --filter=...` and find that they have irrecoverably corrupted their\n> > repository.\n>\n> In some cases we just know the objects we are removing are stored by a\n> promisor remote or are replicated on different physical machines or\n> both, so you should be fine with this.\n\nI am definitely OK with a convenient way to re-filter your repository\nlocally so long as you know that the objects you are filtering out are\navailable via some promisor remote.\n\nBut perhaps I have misunderstood what this proposal is for. Reading\nthrough John's original cover letter and the link to your demo script, I\nunderstood that a key part of this was being able to upload the pack of\nobjects you were about to filter out of your local copy to some server\n(not necessarily Git) over HTTP.\n\nMy hesitation so far has been based on that understanding. Reading these\npatches, I don't see a mechanism to upload objects we're about to\nexpunge to a promisor remote.\n\nBut perhaps I'm misunderstanding: if you are instead assuming that the\nexisting set of remotes can serve any objects that we deleted, and this\nis the way to delete them, then I am OK with that approach. But I think\neither way, I am missing some details in the original proposal that\nwould have perhaps made it easier for me to understand what your goals\nare.\n\nIn any case, this patch series doesn't seem to correctly set up a\npromisor remote for me, since doing the following on a fresh clone of\ngit.git (after running \"make\"):\n\n    $ bin-wrappers/git repack -ad --filter=blob:none\n    $ bin-wrappers/git fsck\n\nresults in many \"missing blob\" and \"missing link\" lines out of output.\n\n(FWIW, I think what's missing here is correctly setting up the affected\nremote(s) as promisors and indicating that we're now in a filtered\nsetting when going from a full clone down to a partial one.)\n\n> If you are not fine with this because sometimes a user might use it\n> without knowing, then why are you ok with commands deleting refs not\n> checking that there isn't a regular repack removing dangling objects?\n\nI'm not sure I totally understand your question, but my general sense\nhas been \"because we typically make it difficult / impossible to remove\nreachable objects\".\n\nThanks,\nTaylor\n\n[1]: https://gitlab.com/chriscool/partial-clone-demo/-/blob/master/http-promisor/server_demo.txt#L47-52\n"},{"id":"449180","messageId":"CFECF0B1-A94F-4372-AFC9-C0469A17E9A5@gmail.com","threadId":"57319","inReplyTo":"YhQHYQ9b9bYYv10r@nand.local","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-22T18:52:09Z","receivedAt":"2022-02-22T18:52:16Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Taylor,\n\nOn 21 Feb 2022, at 16:42, Taylor Blau wrote:\n\n> On Mon, Feb 21, 2022 at 10:10:15PM +0100, Christian Couder wrote:\n>>>> Also, to have more protection we can either\n>>>>\n>>>> 1. add a config value that needs to be set to true for repack to remove\n>>>> objects (repack.allowDestroyFilter).\n>>\n>> I don't think it's of much value. We don't have such config values for\n>> other possibly destructive operations.\n>>\n>>>> 2. --filter is dry-run by default and prints out objects that would have been removed,\n>>>> and it has to be combined with another flag --destroy in order for it to actually remove\n>>>> objects from the odb.\n>>\n>> I am not sure it's of much value either compared to naming it\n>> --filter-destroy. It's likely to just make things more difficult for\n>> users to understand.\n>\n> On this and the above, I agree with Christian.\n>\n>>> I share the same concern as Robert and Stolee do. But I think this issue\n>>> goes deeper than just naming.\n>>>\n>>> Even if we called this `git repack --delete-filter` and only ran it with\n>>> `--i-know-what-im-doing` flag, we would still be leaving repository\n>>> corruption on the table, just making it marginally more difficult to\n>>> achieve.\n>>\n>> My opinion on this is that the promisor object mechanism assumes by\n>> design that some objects are outside a repo, and that this repo\n>> shouldn't care much about these objects possibly being corrupted.\n>\n> For what it's worth, I am fine having a mode of repack which allows us\n> to remove objects that we know are stored by a promisor remote. But this\n> series doesn't do that, so users could easily run `git repack -d\n> --filter=...` and find that they have irrecoverably corrupted their\n> repository.\n>\n> I think that there are some other reasonable directions, though. One\n> which Robert and I discussed was making it possible to split a\n> repository into two packs, one which holds objects that match some\n> `--filter` criteria, and one which holds the objects that don't match\n> that filter.\n>\n> Another option would be to prune the repository according to objects\n> that are already made available by a promisor remote.\n\nThanks for the discussion around the two packfile idea. Definitely interesting.\nHowever, I'm leaning towards the second option here where we ensure that objects that\nare about to be deleted can be retrieved via a promisor remote. That way we have an\neasy path to recovery.\n\n>\n> An appealing quality about the above two directions is that the first\n> doesn't actually remove any objects, just makes it easier to push a\n> whole pack of unwanted objects off to a promsior remote. The second\n> prunes the repository according to objects that are already made\n> available by the promisor remote. (Yes, there is a TOCTOU race there,\n> too, but it's the same prune-while-pushing race that Git already has\n> today).\n>\n>> I am not against a name and some docs that strongly state that users\n>> should be very careful when using such a command, but otherwise I\n>> think such a command is perfectly ok. We have other commands that by\n>> design could lead to some objects or data being lost.\n>\n> I can think of a handful of ways to remove objects which are unreachable\n> from a repository, but I am not sure we have any ways to remove objects\n> which are reachable.\n>\n>>> But as it stands right now, I worry that this feature is too easily\n>>> misused and could result in unintended repository corruption.\n>>\n>> Are you worrying about the UI or about what it does?\n>>\n>> I am ok with improving the UI, but I think what it does is reasonable.\n>\n> I am more worried about the proposal's functionality than its UI,\n> hopefully my concerns there are summarized above.\n>\n> Thanks,\n> Taylor\n"},{"id":"449189","messageId":"YhU7BK5uOS5OK/ZB@nand.local","threadId":"57319","inReplyTo":"CFECF0B1-A94F-4372-AFC9-C0469A17E9A5@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-22T19:35:32Z","receivedAt":"2022-02-22T19:35:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 22, 2022 at 01:52:09PM -0500, John Cai wrote:\n> > Another option would be to prune the repository according to objects\n> > that are already made available by a promisor remote.\n>\n> Thanks for the discussion around the two packfile idea. Definitely\n> interesting.  However, I'm leaning towards the second option here\n> where we ensure that objects that are about to be deleted can be\n> retrieved via a promisor remote. That way we have an easy path to\n> recovery.\n\nYeah, I think this may have all come from a potential misunderstanding I\nhad with the original proposal. More of the details there can be found\nin [1].\n\nBut assuming that this proposal isn't about first offloading some\nobjects to an auxiliary (non-Git) server, then I think refiltering into\na single pack makes sense, because we trust the remote to still have\nany objects we deleted.\n\n(The snag I hit was that it seemed like your+Christian's proposal hinged\non using _two_ filters, one to produce the set of objects you wanted to\nget rid of, and another to produce the set of objects you wanted to\nkeep. The lack of cohesion between the two is what gave me pause, but it\nmay not have been what either of you were thinking in the first place).\n\nAnyway, I'm not sure \"spitting\" a repository along a `--filter` into two\npacks is all that interesting of an idea, but it is something we could\ndo if it became useful to you without writing too much new code (and\ninstead leveraging `git pack-objects --stdin-packs`).\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YhUeUCIetu%2FaOu6k@nand.local/\n"},{"id":"449270","messageId":"CAFLLRpKLSxLV82SCr8x=BBRBybxj1XOxb=Srs5_X2idvvb1YEg@mail.gmail.com","threadId":"57319","inReplyTo":"CAP8UFD3U4t-inWC5mZYhybWpjVwkqA7v4hYZ5voBOEJ=+_Y1kQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Robert Coup","fromEmail":"robert.coup@koordinates.com","sentAt":"2022-02-23T15:40:50Z","receivedAt":"2022-02-23T15:41:20Z","isPatch":true,"sender":{"key":"robert.coup@koordinates.com","avatar":"https://gravatar.com/avatar/d1a87d63ffb562b791992d8a119ebbdd742e703109d23333ca3fca51306ee95c?d=mp&s=160"},"body":"Hi Christian,\n\nOn Tue, 22 Feb 2022 at 17:11, Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> In some cases we just know the objects we are removing are stored by a\n> promisor remote or are replicated on different physical machines or\n> both, so you should be fine with this.\n\nFrom my point of view I think the goal here is great.\n\n> > Another option would be to prune the repository according to objects\n> > that are already made available by a promisor remote.\n>\n> If the objects have just been properly transferred to the promisor\n> remote, the check will just waste resources.\n\nAs far as I can see this patch doesn't know or check that any of the\nfiltered-out objects are held anywhere else... it simply applies a\nfilter during repacking and the excluded objects are dropped. That's\nthe aspect I have concerns about.\n\nMaybe an approach where you build/get/maintain a list of\nobjects-I-definitely-have-elsewhere and pass it as an exclude list to\nrepack would be a cleaner/safer/easier solution? If you're confident\nenough you don't need to check with the promisor remote then you can\nuse a local list, or even something generated with `rev-list\n--filter=`.\n\nThanks,\n\nRob :)\n"},{"id":"449330","messageId":"xmqqv8x5v0qc.fsf@gitster.g","threadId":"57319","inReplyTo":"CAP8UFD3U4t-inWC5mZYhybWpjVwkqA7v4hYZ5voBOEJ=+_Y1kQ@mail.gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T19:31:23Z","receivedAt":"2022-02-23T19:31:33Z","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>> For what it's worth, I am fine having a mode of repack which allows us\n>> to remove objects that we know are stored by a promisor remote. But this\n>> series doesn't do that, so users could easily run `git repack -d\n>> --filter=...` and find that they have irrecoverably corrupted their\n>> repository.\n>\n> In some cases we just know the objects we are removing are stored by a\n> promisor remote or are replicated on different physical machines or\n> both, so you should be fine with this.\n\nSo, we need to decide if an object we have that is outside the\nnarrowed filter definition was (and still is, but let's keep the\nassumption the whole lazy clone mechanism makes: promisor remotes\nwill never shed objects that they once served) available at the\npromisor remote, but I suspect we have too little information to\nreliably do so.  It is OK to assume that objects in existing packs\ntaken from the promisor remotes and everything reachable from them\n(but missing from our object store) will be available to us from\nthere.  But if we see an object that is outside of _new_ filter spec\n(e.g. you fetched with \"max 100MB\", now you are refiltering with\n\"max 50MB\", narrowing the spec, and you need to decide for an object\nthat weigh 70MB), can we tell if that can be retrieved from the\npromisor or is it unique to our repository until we push it out?  I\nam not sure.  For that matter, do we even have a way to compare if\nthe new filter spec is a subset, a superset, or neither, of the\noriginal filter spec?\n\n> If you are not fine with this because sometimes a user might use it\n> without knowing, then why are you ok with commands deleting refs not\n> checking that there isn't a regular repack removing dangling objects?\n\nSorry, I do not follow this argument.  Your user may do \"branch -D\"\nbecause the branch deleted is no longer needed, which may mean that\nobjects only reachable from the deleted branch are no longer needed.\nI do not see what repack has anything to do with that.  As long as\nthe filter spec does not change (in other words, before this series\nis applied), the repack that discards objects that are known to be\nreachable from objects in packs retrieved from promisor remote, the\nobjects that are no longer reachable may be removed and that will\nnot lose objects that we do not know to be retrievable from there\n(which is different from objects that we know are unretrievable).\nBut with filter spec changing after the fact, I am not sure if that\nis safe.  IOW, \"commands deleting refs\" may have been OK without\nthis series, but this series may be what makes it not OK, no?\n\nPuzzled.\n\n"},{"id":"449671","messageId":"36CA51FE-8B7F-4D08-A91D-95D8F76606C9@gmail.com","threadId":"57319","inReplyTo":"xmqqv8x5v0qc.fsf@gitster.g","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-26T16:01:46Z","receivedAt":"2022-02-26T16:01:52Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Thank you for everyone's feedback. Really appreciate the collaboration!\n\nOn 23 Feb 2022, at 14:31, Junio C Hamano wrote:\n\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>>> For what it's worth, I am fine having a mode of repack which allows us\n>>> to remove objects that we know are stored by a promisor remote. But this\n>>> series doesn't do that, so users could easily run `git repack -d\n>>> --filter=...` and find that they have irrecoverably corrupted their\n>>> repository.\n>>\n>> In some cases we just know the objects we are removing are stored by a\n>> promisor remote or are replicated on different physical machines or\n>> both, so you should be fine with this.\n>\n> So, we need to decide if an object we have that is outside the\n> narrowed filter definition was (and still is, but let's keep the\n> assumption the whole lazy clone mechanism makes: promisor remotes\n> will never shed objects that they once served) available at the\n> promisor remote, but I suspect we have too little information to\n> reliably do so.  It is OK to assume that objects in existing packs\n> taken from the promisor remotes and everything reachable from them\n> (but missing from our object store) will be available to us from\n> there.  But if we see an object that is outside of _new_ filter spec\n> (e.g. you fetched with \"max 100MB\", now you are refiltering with\n> \"max 50MB\", narrowing the spec, and you need to decide for an object\n> that weigh 70MB), can we tell if that can be retrieved from the\n> promisor or is it unique to our repository until we push it out?  I\n> am not sure.  For that matter, do we even have a way to compare if\n> the new filter spec is a subset, a superset, or neither, of the\n> original filter spec?\n\nlet me try to summarize (perhaps over simplify) the main concern folks have\nwith this feature, so please correct me if I'm wrong!\n\nAs a user, if I apply a filter that ends up deleting objects that it turns\nout do not exist anywhere else, then I have irrecoverably corrupted my\nrepository.\n\nBefore git allows me to delete objects from my repository, it should be pretty\ncertain that I have path to recover those objects if I need to.\n\nIs that correct? It seems to me that, put another way, we don't want to give\nusers too much rope to hang themselves.\n\nI can see why we would want to do this. In this case, there have been a couple\nof alternative ideas proposed throughout this thread that I think are viable and\nI wanted to get folks thoughts.\n\n1. split pack file - (Rob gave this idea and Taylor provided some more detail on\n   how using pack-objects would make it fairly straightforward to implement)\n\nwhen a user wants to apply a filter that removes objects from their repository,\nsplit the packfile into one containing objects that are filtered out, and\nanother packfile with objects that remain.\n\npros: simple to implement\ncons: does not address the question \"how sure am I that the objects I want to\nfilter out of my repository exist on a promsior remote?\"\n\n2. check the promisor remotes to see if they contain the objects that are about\n   to get deleted. Only delete objects that we find on promisor remotes.\n\npros: provides assurance that I have access to objects I am about to delete from\na promsior remote.\ncons: more complex to implement. [*]\n\nOut of these two, I like 2 more for the aforementioned pros.\n\n* I am beginning to look into how fetches work and am still pretty new to the\ncodebase so I don't know if this is even feasible, but I was thinking perhaps\nwe could write a function that fetches with a --filter and create a promisor\npackfile containing promisor objects (this operaiton would have to somehow\nignore the presence of the actual objects in the repository). Then, we would\nhave a record of objects we have access to. Then, repack --filter can remove\nonly the objects contained in this promisor packfile.\n\n>\n>> If you are not fine with this because sometimes a user might use it\n>> without knowing, then why are you ok with commands deleting refs not\n>> checking that there isn't a regular repack removing dangling objects?\n>\n> Sorry, I do not follow this argument.  Your user may do \"branch -D\"\n> because the branch deleted is no longer needed, which may mean that\n> objects only reachable from the deleted branch are no longer needed.\n> I do not see what repack has anything to do with that.  As long as\n> the filter spec does not change (in other words, before this series\n> is applied), the repack that discards objects that are known to be\n> reachable from objects in packs retrieved from promisor remote, the\n> objects that are no longer reachable may be removed and that will\n> not lose objects that we do not know to be retrievable from there\n> (which is different from objects that we know are unretrievable).\n> But with filter spec changing after the fact, I am not sure if that\n> is safe.  IOW, \"commands deleting refs\" may have been OK without\n> this series, but this series may be what makes it not OK, no?\n>\n> Puzzled.\n"},{"id":"449672","messageId":"YhpjbQeFaMNVnyP9@nand.local","threadId":"57319","inReplyTo":"36CA51FE-8B7F-4D08-A91D-95D8F76606C9@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-26T17:29:17Z","receivedAt":"2022-02-26T17:29:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 26, 2022 at 11:01:46AM -0500, John Cai wrote:\n> let me try to summarize (perhaps over simplify) the main concern folks\n> have with this feature, so please correct me if I'm wrong!\n>\n> As a user, if I apply a filter that ends up deleting objects that it\n> turns out do not exist anywhere else, then I have irrecoverably\n> corrupted my repository.\n>\n> Before git allows me to delete objects from my repository, it should\n> be pretty certain that I have path to recover those objects if I need\n> to.\n>\n> Is that correct? It seems to me that, put another way, we don't want\n> to give users too much rope to hang themselves.\n\nI wrote about my concerns in some more detail in [1], but the thing I\nwas most unclear on was how your demo script[2] was supposed to work.\n\nNamely, I wasn't sure if you had intended to use two separate filters to\n\"re-filter\" a repository, one to filter objects to be uploaded to a\ncontent store, and another to filter objects to be expunged from the\nrepository. I have major concerns with that approach, namely that if\neach of the filters is not exactly the inverse of the other, then we\nwill either upload too few objects, or delete too many.\n\nMy other concern was around what guarantees we currently provide for a\npromisor remote. My understanding is that we expect an object which was\nreceived from the promisor remote to always be fetch-able later on. If\nthat's the case, then I don't mind the idea of refiltering a repository,\nprovided that you only need to specify a filter once.\n\nSo the suggestion about splitting a repository into two packs was a\npotential way to mediate the \"two filter\" problem, since the two packs\nyou get exactly correspond to the set of objects that match the filter,\nand the set of objects that _don't_ match the filter.\n\nIn either case, I tried to use the patches in [1] and was able to\ncorrupt my local repository (even when fetching from a remote that held\nonto the objects I had pruned locally).\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YhUeUCIetu%2FaOu6k@nand.local/\n[2]: https://gitlab.com/chriscool/partial-clone-demo/-/blob/master/http-promisor/server_demo.txt#L47-52\n"},{"id":"449678","messageId":"47AC2D8D-ADB2-4280-86F0-6B1E239C1EBE@gmail.com","threadId":"57319","inReplyTo":"YhpjbQeFaMNVnyP9@nand.local","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-26T20:19:11Z","receivedAt":"2022-02-26T20:19:15Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Taylor,\n\n(resending this because my email client misbehaved and set the mime type to html)\n\nOn 26 Feb 2022, at 12:29, Taylor Blau wrote:\n\n> On Sat, Feb 26, 2022 at 11:01:46AM -0500, John Cai wrote:\n>> let me try to summarize (perhaps over simplify) the main concern folks\n>> have with this feature, so please correct me if I'm wrong!\n>>\n>> As a user, if I apply a filter that ends up deleting objects that it\n>> turns out do not exist anywhere else, then I have irrecoverably\n>> corrupted my repository.\n>>\n>> Before git allows me to delete objects from my repository, it should\n>> be pretty certain that I have path to recover those objects if I need\n>> to.\n>>\n>> Is that correct? It seems to me that, put another way, we don't want\n>> to give users too much rope to hang themselves.\n>\n> I wrote about my concerns in some more detail in [1], but the thing I\n> was most unclear on was how your demo script[2] was supposed to work.\n>\n> Namely, I wasn't sure if you had intended to use two separate filters to\n> \"re-filter\" a repository, one to filter objects to be uploaded to a\n> content store, and another to filter objects to be expunged from the\n> repository. I have major concerns with that approach, namely that if\n> each of the filters is not exactly the inverse of the other, then we\n> will either upload too few objects, or delete too many.\n\nThanks for bringing this up again. I meant to write back regarding what you raised\nin the other part of this thread. I think this is a valid concern. To attain the\ngoal of offloading certain blobs onto another server(B) and saving space on a git\nserver(A), then there will essentially be two steps. One to upload objects to (B),\nand one to remove objects from (A). As you said, these two need to be the inverse of each\nother or else you might end up with missing objects.\n\nThinking about it more, there is also an issue of timing. Even if the filters\nare exact inverses of each other, let's say we have the following order of\nevents:\n\n- (A)'s large blobs get upload to (B)\n- large blob (C) get added to (A)\n- (A) gets repacked with a filter\n\nIn this case we could lose (C) forever. So it does seem like we need some built in guarantee\nthat we only shed objects from the repo if we know we can retrieve them later on.\n\n>\n> My other concern was around what guarantees we currently provide for a\n> promisor remote. My understanding is that we expect an object which was\n> received from the promisor remote to always be fetch-able later on. If\n> that's the case, then I don't mind the idea of refiltering a repository,\n> provided that you only need to specify a filter once.\n\nCould you clarify what you mean by re-filtering a repository? By that I assumed\nit meant specifying a filter eg: 100mb, and then narrowing it by specifying a\n50mb filter.\n>\n> So the suggestion about splitting a repository into two packs was a\n> potential way to mediate the \"two filter\" problem, since the two packs\n> you get exactly correspond to the set of objects that match the filter,\n> and the set of objects that _don't_ match the filter.\n>\n> In either case, I tried to use the patches in [1] and was able to\n> corrupt my local repository (even when fetching from a remote that held\n> onto the objects I had pruned locally).\n>\n> Thanks,\n> Taylor\n\nThanks!\nJohn\n\n>\n> [1]: https://lore.kernel.org/git/YhUeUCIetu%2FaOu6k@nand.local/\n> [2]: https://gitlab.com/chriscool/partial-clone-demo/-/blob/master/http-promisor/server_demo.txt#L47-52\n"},{"id":"449679","messageId":"YhqNy+t5SARNivQ5@nand.local","threadId":"57319","inReplyTo":"47AC2D8D-ADB2-4280-86F0-6B1E239C1EBE@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-26T20:30:03Z","receivedAt":"2022-02-26T20:30:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 26, 2022 at 03:19:11PM -0500, John Cai wrote:\n> Thanks for bringing this up again. I meant to write back regarding what you raised\n> in the other part of this thread. I think this is a valid concern. To attain the\n> goal of offloading certain blobs onto another server(B) and saving space on a git\n> server(A), then there will essentially be two steps. One to upload objects to (B),\n> and one to remove objects from (A). As you said, these two need to be the inverse of each\n> other or else you might end up with missing objects.\n\nDo you mean that you want to offload objects both from a local clone of\nsome repository, _and_ the original remote it was cloned from?\n\nI don't understand what the role of \"another server\" is here. If this\nproposal was about making it easy to remove objects from a local copy of\na repository based on a filter provided that there was a Git server\nelsewhere that could act as a promisor remote, than that makes sense to\nme.\n\nBut I think I'm not quite understanding the rest of what you're\nsuggesting.\n\n> > My other concern was around what guarantees we currently provide for a\n> > promisor remote. My understanding is that we expect an object which was\n> > received from the promisor remote to always be fetch-able later on. If\n> > that's the case, then I don't mind the idea of refiltering a repository,\n> > provided that you only need to specify a filter once.\n>\n> Could you clarify what you mean by re-filtering a repository? By that I assumed\n> it meant specifying a filter eg: 100mb, and then narrowing it by specifying a\n> 50mb filter.\n\nI meant: applying a filter to a local clone (either where there wasn't a\nfilter before, or a filter which matched more objects) and then removing\nobjects that don't match the filter.\n\nBut your response makes me think of another potential issue. What\nhappens if I do the following:\n\n    $ git repack -ad --filter=blob:limit=100k\n    $ git repack -ad --filter=blob:limit=200k\n\nWhat should the second invocation do? I would expect that it needs to do\na fetch from the promisor remote to recover any blobs between (100, 200]\nKB in size, since they would be gone after the first repack.\n\nThis is a problem not just with two consecutive `git repack --filter`s,\nI think, since you could cook up the same situation with:\n\n    $ git clone --filter=blob:limit=100k git@github.com:git\n    $ git -C git repack -ad --filter=blob:limit=200k\n\nI don't think the existing patches handle this situation, so I'm curious\nwhether it's something you have considered or not before.\n\n(Unrelated to the above, but please feel free to trim any quoted parts\nof emails when responding if they get overly long.)\n\nThanks,\nTaylor\n"},{"id":"449681","messageId":"5106811D-2937-49CB-AC93-875D3B3BC241@gmail.com","threadId":"57319","inReplyTo":"YhqNy+t5SARNivQ5@nand.local","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-26T21:05:37Z","receivedAt":"2022-02-26T21:05:42Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Taylor,\n\nOn 26 Feb 2022, at 15:30, Taylor Blau wrote:\n\n> On Sat, Feb 26, 2022 at 03:19:11PM -0500, John Cai wrote:\n>> Thanks for bringing this up again. I meant to write back regarding what you raised\n>> in the other part of this thread. I think this is a valid concern. To attain the\n>> goal of offloading certain blobs onto another server(B) and saving space on a git\n>> server(A), then there will essentially be two steps. One to upload objects to (B),\n>> and one to remove objects from (A). As you said, these two need to be the inverse of each\n>> other or else you might end up with missing objects.\n>\n> Do you mean that you want to offload objects both from a local clone of\n> some repository, _and_ the original remote it was cloned from?\n\nyes, exactly. The \"another server\" would be something like an http server, OR another remote\nwhich hosts a subset of the objects (let's say the large blobs).\n>\n> I don't understand what the role of \"another server\" is here. If this\n> proposal was about making it easy to remove objects from a local copy of\n> a repository based on a filter provided that there was a Git server\n> elsewhere that could act as a promisor remote, than that makes sense to\n> me.\n>\n> But I think I'm not quite understanding the rest of what you're\n> suggesting.\n\nSorry for the lack of clarity here. The goal is to make it easy for a remote to offload a subset\nof its objects to __another__ remote (either a Git server or an http server through a remote helper).\n>\n>>> My other concern was around what guarantees we currently provide for a\n>>> promisor remote. My understanding is that we expect an object which was\n>>> received from the promisor remote to always be fetch-able later on. If\n>>> that's the case, then I don't mind the idea of refiltering a repository,\n>>> provided that you only need to specify a filter once.\n>>\n>> Could you clarify what you mean by re-filtering a repository? By that I assumed\n>> it meant specifying a filter eg: 100mb, and then narrowing it by specifying a\n>> 50mb filter.\n>\n> I meant: applying a filter to a local clone (either where there wasn't a\n> filter before, or a filter which matched more objects) and then removing\n> objects that don't match the filter.\n>\n> But your response makes me think of another potential issue. What\n> happens if I do the following:\n>\n>     $ git repack -ad --filter=blob:limit=100k\n>     $ git repack -ad --filter=blob:limit=200k\n>\n> What should the second invocation do? I would expect that it needs to do\n> a fetch from the promisor remote to recover any blobs between (100, 200]\n> KB in size, since they would be gone after the first repack.\n>\n> This is a problem not just with two consecutive `git repack --filter`s,\n> I think, since you could cook up the same situation with:\n>\n>     $ git clone --filter=blob:limit=100k git@github.com:git\n>     $ git -C git repack -ad --filter=blob:limit=200k\n>\n> I don't think the existing patches handle this situation, so I'm curious\n> whether it's something you have considered or not before.\n\nI have not-will have to think through this case, but this sound similar to\nwhat [1] is about.\nis about.\n\n>\n> (Unrelated to the above, but please feel free to trim any quoted parts\n> of emails when responding if they get overly long.)\n>\n> Thanks,\n> Taylor\n\nThanks\nJohn\n\n1. https://lore.kernel.org/git/pull.1138.v2.git.1645719218.gitgitgadget@gmail.com/\n"},{"id":"449682","messageId":"YhqfJ8lFD9p6BPBx@nand.local","threadId":"57319","inReplyTo":"5106811D-2937-49CB-AC93-875D3B3BC241@gmail.com","subject":"Re: [PATCH v2 0/4] [RFC] repack: add --filter=","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-26T21:44:07Z","receivedAt":"2022-02-26T21:44:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sat, Feb 26, 2022 at 04:05:37PM -0500, John Cai wrote:\n> Hi Taylor,\n>\n> On 26 Feb 2022, at 15:30, Taylor Blau wrote:\n>\n> > On Sat, Feb 26, 2022 at 03:19:11PM -0500, John Cai wrote:\n> >> Thanks for bringing this up again. I meant to write back regarding what you raised\n> >> in the other part of this thread. I think this is a valid concern. To attain the\n> >> goal of offloading certain blobs onto another server(B) and saving space on a git\n> >> server(A), then there will essentially be two steps. One to upload objects to (B),\n> >> and one to remove objects from (A). As you said, these two need to be the inverse of each\n> >> other or else you might end up with missing objects.\n> >\n> > Do you mean that you want to offload objects both from a local clone of\n> > some repository, _and_ the original remote it was cloned from?\n>\n> yes, exactly. The \"another server\" would be something like an http server, OR another remote\n> which hosts a subset of the objects (let's say the large blobs).\n> >\n> > I don't understand what the role of \"another server\" is here. If this\n> > proposal was about making it easy to remove objects from a local copy of\n> > a repository based on a filter provided that there was a Git server\n> > elsewhere that could act as a promisor remote, than that makes sense to\n> > me.\n> >\n> > But I think I'm not quite understanding the rest of what you're\n> > suggesting.\n>\n> Sorry for the lack of clarity here. The goal is to make it easy for a remote to offload a subset\n> of its objects to __another__ remote (either a Git server or an http server through a remote helper).\n\nDoes the other server then act as a promisor remote in conjunction with\nthe Git server? I'm having trouble understanding why the _Git_ remote\nyou originally cloned from needs to offload its objects, too.\n\nSo I think the list would benefit from understanding some more of the\ndetails and motivation there. But it would also benefit us to have some\nunderstanding of how we'll ensure that any objects which are moved out\nof a Git repository make their way to another server.\n\nI am curious to hear Jonathan Tan's thoughts on this all, too.\n\nThanks,\nTaylor\n"}]}