{"thread":{"id":"49064","subject":"[PATCH 0/2] Repacking of promisor packfiles","startedAt":"2018-08-07T20:12:39Z","lastAt":"2018-08-18T16:05:45Z","messageCount":18,"participants":["Jonathan Tan","Junio C Hamano","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"354767","messageId":"cover.1533672584.git.jonathantanmy@google.com","threadId":"49064","inReplyTo":null,"subject":"[PATCH 0/2] Repacking of promisor packfiles","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-07T20:12:30Z","receivedAt":"2018-08-07T20:12:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"These patches teach Git to also repack promisor packfiles upon GC, which\nreduces one of the pain points of current partial clone usage (many\npromisor packfiles in the objects/pack/ directory, generated upon each\nfetch).\n\nIn the t/ tests, I strived to verify that repack doesn't accidentally\ndelete any objects. Let me know if you can think of better ways to do\nthat.\n\nJonathan Tan (2):\n  repack: refactor setup of pack-objects cmd\n  repack: repack promisor objects if -a or -A is set\n\n Documentation/git-repack.txt |   5 ++\n builtin/repack.c             | 163 ++++++++++++++++++++++++-----------\n t/t0410-partial-clone.sh     |  71 ++++++++++++---\n 3 files changed, 180 insertions(+), 59 deletions(-)\n\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354768","messageId":"bff8576b1ec9168499dec3164a93344d6d36fa9c.1533672584.git.jonathantanmy@google.com","threadId":"49064","inReplyTo":"cover.1533672584.git.jonathantanmy@google.com","subject":"[PATCH 1/2] repack: refactor setup of pack-objects cmd","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-07T20:12:31Z","receivedAt":"2018-08-07T20:12:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent patch will teach repack to run pack-objects with some same\nand some different arguments if repacking of promisor objects is\nrequired. Refactor the setup of the pack-objects cmd so that setting up\nthe arguments common to both is done in a function.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/repack.c | 99 +++++++++++++++++++++++++++---------------------\n 1 file changed, 55 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6c636e159..ca695fa10 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -138,6 +138,47 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)\n \tstrbuf_release(&buf);\n }\n \n+struct packed_objects_args {\n+\tconst char *window;\n+\tconst char *window_memory;\n+\tconst char *depth;\n+\tconst char *threads;\n+\tconst char *max_pack_size;\n+\tint no_reuse_delta;\n+\tint no_reuse_object;\n+\tint quiet;\n+\tint local;\n+};\n+\n+static void prepare_packed_objects(struct child_process *cmd,\n+\t\t\t\t   const struct packed_objects_args *args)\n+{\n+\targv_array_push(&cmd->args, \"pack-objects\");\n+\tif (args->window)\n+\t\targv_array_pushf(&cmd->args, \"--window=%s\", args->window);\n+\tif (args->window_memory)\n+\t\targv_array_pushf(&cmd->args, \"--window-memory=%s\", args->window_memory);\n+\tif (args->depth)\n+\t\targv_array_pushf(&cmd->args, \"--depth=%s\", args->depth);\n+\tif (args->threads)\n+\t\targv_array_pushf(&cmd->args, \"--threads=%s\", args->threads);\n+\tif (args->max_pack_size)\n+\t\targv_array_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\tif (args->no_reuse_delta)\n+\t\targv_array_pushf(&cmd->args, \"--no-reuse-delta\");\n+\tif (args->no_reuse_object)\n+\t\targv_array_pushf(&cmd->args, \"--no-reuse-object\");\n+\tif (args->local)\n+\t\targv_array_push(&cmd->args,  \"--local\");\n+\tif (args->quiet)\n+\t\targv_array_push(&cmd->args,  \"--quiet\");\n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd->args,  \"--delta-base-offset\");\n+\targv_array_push(&cmd->args, packtmp);\n+\tcmd->git_cmd = 1;\n+\tcmd->out = -1;\n+}\n+\n #define ALL_INTO_ONE 1\n #define LOOSEN_UNREACHABLE 2\n \n@@ -165,15 +206,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint delete_redundant = 0;\n \tconst char *unpack_unreachable = NULL;\n \tint keep_unreachable = 0;\n-\tconst char *window = NULL, *window_memory = NULL;\n-\tconst char *depth = NULL;\n-\tconst char *threads = NULL;\n-\tconst char *max_pack_size = NULL;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tint no_reuse_delta = 0, no_reuse_object = 0;\n \tint no_update_server_info = 0;\n-\tint quiet = 0;\n-\tint local = 0;\n+\tstruct packed_objects_args po_args = {NULL};\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -183,14 +218,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t   LOOSEN_UNREACHABLE | ALL_INTO_ONE),\n \t\tOPT_BOOL('d', NULL, &delete_redundant,\n \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n-\t\tOPT_BOOL('f', NULL, &no_reuse_delta,\n+\t\tOPT_BOOL('f', NULL, &po_args.no_reuse_delta,\n \t\t\t\tN_(\"pass --no-reuse-delta to git-pack-objects\")),\n-\t\tOPT_BOOL('F', NULL, &no_reuse_object,\n+\t\tOPT_BOOL('F', NULL, &po_args.no_reuse_object,\n \t\t\t\tN_(\"pass --no-reuse-object to git-pack-objects\")),\n \t\tOPT_BOOL('n', NULL, &no_update_server_info,\n \t\t\t\tN_(\"do not run git-update-server-info\")),\n-\t\tOPT__QUIET(&quiet, N_(\"be quiet\")),\n-\t\tOPT_BOOL('l', \"local\", &local,\n+\t\tOPT__QUIET(&po_args.quiet, N_(\"be quiet\")),\n+\t\tOPT_BOOL('l', \"local\", &po_args.local,\n \t\t\t\tN_(\"pass --local to git-pack-objects\")),\n \t\tOPT_BOOL('b', \"write-bitmap-index\", &write_bitmaps,\n \t\t\t\tN_(\"write bitmap index\")),\n@@ -198,15 +233,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n \t\tOPT_BOOL('k', \"keep-unreachable\", &keep_unreachable,\n \t\t\t\tN_(\"with -a, repack unreachable objects\")),\n-\t\tOPT_STRING(0, \"window\", &window, N_(\"n\"),\n+\t\tOPT_STRING(0, \"window\", &po_args.window, N_(\"n\"),\n \t\t\t\tN_(\"size of the window used for delta compression\")),\n-\t\tOPT_STRING(0, \"window-memory\", &window_memory, N_(\"bytes\"),\n+\t\tOPT_STRING(0, \"window-memory\", &po_args.window_memory, N_(\"bytes\"),\n \t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n-\t\tOPT_STRING(0, \"depth\", &depth, N_(\"n\"),\n+\t\tOPT_STRING(0, \"depth\", &po_args.depth, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n-\t\tOPT_STRING(0, \"threads\", &threads, N_(\"n\"),\n+\t\tOPT_STRING(0, \"threads\", &po_args.threads, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n-\t\tOPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\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_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n@@ -238,7 +273,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n-\targv_array_push(&cmd.args, \"pack-objects\");\n+\tprepare_packed_objects(&cmd, &po_args);\n+\n \targv_array_push(&cmd.args, \"--keep-true-parents\");\n \tif (!pack_kept_objects)\n \t\targv_array_push(&cmd.args, \"--honor-pack-keep\");\n@@ -251,20 +287,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n-\tif (window)\n-\t\targv_array_pushf(&cmd.args, \"--window=%s\", window);\n-\tif (window_memory)\n-\t\targv_array_pushf(&cmd.args, \"--window-memory=%s\", window_memory);\n-\tif (depth)\n-\t\targv_array_pushf(&cmd.args, \"--depth=%s\", depth);\n-\tif (threads)\n-\t\targv_array_pushf(&cmd.args, \"--threads=%s\", threads);\n-\tif (max_pack_size)\n-\t\targv_array_pushf(&cmd.args, \"--max-pack-size=%s\", max_pack_size);\n-\tif (no_reuse_delta)\n-\t\targv_array_pushf(&cmd.args, \"--no-reuse-delta\");\n-\tif (no_reuse_object)\n-\t\targv_array_pushf(&cmd.args, \"--no-reuse-object\");\n \tif (write_bitmaps)\n \t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n \n@@ -292,17 +314,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\targv_array_push(&cmd.args, \"--incremental\");\n \t}\n \n-\tif (local)\n-\t\targv_array_push(&cmd.args,  \"--local\");\n-\tif (quiet)\n-\t\targv_array_push(&cmd.args,  \"--quiet\");\n-\tif (delta_base_offset)\n-\t\targv_array_push(&cmd.args,  \"--delta-base-offset\");\n-\n-\targv_array_push(&cmd.args, packtmp);\n-\n-\tcmd.git_cmd = 1;\n-\tcmd.out = -1;\n \tcmd.no_stdin = 1;\n \n \tret = start_command(&cmd);\n@@ -320,7 +331,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (ret)\n \t\treturn ret;\n \n-\tif (!names.nr && !quiet)\n+\tif (!names.nr && !po_args.quiet)\n \t\tprintf(\"Nothing new to pack.\\n\");\n \n \t/*\n@@ -441,7 +452,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tif (!string_list_has_string(&names, sha1))\n \t\t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n-\t\tif (!quiet && isatty(2))\n+\t\tif (!po_args.quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n \t}\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354769","messageId":"61396b2a73a801f62818ad3b691f75170d2e3919.1533672584.git.jonathantanmy@google.com","threadId":"49064","inReplyTo":"cover.1533672584.git.jonathantanmy@google.com","subject":"[PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-07T20:12:32Z","receivedAt":"2018-08-07T20:12:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, repack does not touch promisor packfiles at all, potentially\ncausing the performance of repositories that have many such packfiles to\ndrop. Therefore, repack all promisor objects if invoked with -a or -A.\n\nThis is done by an additional invocation of pack-objects on all promisor\nobjects individually given, which takes care of deduplication and allows\nthe resulting packfiles to respect flags such as --max-pack-size.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git-repack.txt |  5 +++\n builtin/repack.c             | 64 ++++++++++++++++++++++++++++++--\n t/t0410-partial-clone.sh     | 71 ++++++++++++++++++++++++++++++------\n 3 files changed, 125 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex d90e7907f..d05625096 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -40,6 +40,11 @@ OPTIONS\n Note that users fetching over dumb protocols will have to fetch the\n whole new pack in order to get any contained object, no matter how many\n other objects in that pack they already have locally.\n++\n+Promisor packfiles are repacked separately: if there are packfiles that\n+have an associated \".promisor\" file, these packfiles will be repacked\n+into another separate pack, and an empty \".promisor\" file corresponding\n+to the new separate pack will be written.\n \n -A::\n \tSame as `-a`, unless `-d` is used.  Then any unreachable\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ca695fa10..a78d2f0aa 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -8,6 +8,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"argv-array.h\"\n+#include \"packfile.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -83,7 +84,7 @@ static void remove_pack_on_signal(int signo)\n \n /*\n  * Adds all packs hex strings to the fname list, which do not\n- * have a corresponding .keep or .promisor file. These packs are not to\n+ * have a corresponding .keep file. These packs are not to\n  * be kept if we are going to pack everything into one file.\n  */\n static void get_non_kept_pack_filenames(struct string_list *fname_list,\n@@ -111,8 +112,7 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n \n \t\tfname = xmemdupz(e->d_name, len);\n \n-\t\tif (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)) &&\n-\t\t    !file_exists(mkpath(\"%s/%s.promisor\", packdir, fname)))\n+\t\tif (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))\n \t\t\tstring_list_append_nodup(fname_list, fname);\n \t\telse\n \t\t\tfree(fname);\n@@ -122,7 +122,7 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n {\n-\tconst char *exts[] = {\".pack\", \".idx\", \".keep\", \".bitmap\"};\n+\tconst char *exts[] = {\".pack\", \".idx\", \".keep\", \".bitmap\", \".promisor\"};\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t plen;\n@@ -179,6 +179,58 @@ static void prepare_packed_objects(struct child_process *cmd,\n \tcmd->out = -1;\n }\n \n+static int write_oid(const struct object_id *oid, struct packed_git *pack,\n+\t\t     uint32_t pos, void *data)\n+{\n+\tint fd = *(int *)data;\n+\n+\txwrite(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ);\n+\txwrite(fd, \"\\n\", 1);\n+\treturn 0;\n+}\n+\n+static void repack_promisor_objects(const struct packed_objects_args *args,\n+\t\t\t\t    struct string_list *names)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *out;\n+\tstruct strbuf line = STRBUF_INIT;\n+\n+\tprepare_packed_objects(&cmd, args);\n+\tcmd.in = -1;\n+\n+\tif (start_command(&cmd))\n+\t\tdie(\"Could not start pack-objects to repack promisor objects\");\n+\n+\tfor_each_packed_object(write_oid, &cmd.in,\n+\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\tclose(cmd.in);\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tchar *promisor_name;\n+\t\tint fd;\n+\t\tif (line.len != 40)\n+\t\t\tdie(\"repack: Expecting 40 character sha1 lines only from pack-objects.\");\n+\t\tstring_list_append(names, line.buf);\n+\n+\t\t/*\n+\t\t * pack-objects creates the .pack and .idx files, but not the\n+\t\t * .promisor file. Create the .promisor file, which is empty.\n+\t\t */\n+\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n+\t\t\t\t\t  line.buf);\n+\t\tfd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\tif (fd < 0)\n+\t\t\tdie_errno(\"unable to create '%s'\", promisor_name);\n+\t\tclose(fd);\n+\t\tfree(promisor_name);\n+\t}\n+\tfclose(out);\n+\tif (finish_command(&cmd))\n+\t\tdie(\"Could not finish pack-objects to repack promisor objects\");\n+}\n+\n #define ALL_INTO_ONE 1\n #define LOOSEN_UNREACHABLE 2\n \n@@ -191,6 +243,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t{\".pack\"},\n \t\t{\".idx\"},\n \t\t{\".bitmap\", 1},\n+\t\t{\".promisor\", 1},\n \t};\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n@@ -293,6 +346,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\tget_non_kept_pack_filenames(&existing_packs, &keep_pack_list);\n \n+\t\tif (repository_format_partial_clone)\n+\t\t\trepack_promisor_objects(&po_args, &names);\n+\n \t\tif (existing_packs.nr && delete_redundant) {\n \t\t\tif (unpack_unreachable) {\n \t\t\t\targv_array_pushf(&cmd.args,\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 4984ca583..27ba7d201 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -271,28 +271,77 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n \tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n '\n \n-test_expect_success 'gc does not repack promisor objects' '\n+test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-\ttest_commit -C repo my_commit &&\n+\ttest_commit -C repo one &&\n+\ttest_commit -C repo two &&\n \n-\tTREE_HASH=$(git -C repo rev-parse HEAD^{tree}) &&\n-\tHASH=$(printf \"$TREE_HASH\\n\" | pack_as_from_promisor) &&\n+\tTREE_ONE=$(git -C repo rev-parse one^{tree}) &&\n+\tprintf \"$TREE_ONE\\n\" | pack_as_from_promisor &&\n+\tTREE_TWO=$(git -C repo rev-parse two^{tree}) &&\n+\tprintf \"$TREE_TWO\\n\" | pack_as_from_promisor &&\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n \tgit -C repo gc &&\n \n-\t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n-\trm repo/.git/objects/pack/pack-$HASH.* &&\n-\n-\t# Ensure that the single other pack contains the commit, but not the tree\n+\t# Ensure that exactly one promisor packfile exists, and that it\n+\t# contains the trees but not the commits\n+\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n+\ttest_line_count = 1 promisorlist &&\n+\tPROMISOR_PACKFILE=$(sed \"s/.promisor/.pack/\" <promisorlist) &&\n+\tgit verify-pack $PROMISOR_PACKFILE -v >out &&\n+\tgrep \"$TREE_ONE\" out &&\n+\tgrep \"$TREE_TWO\" out &&\n+\t! grep \"$(git -C repo rev-parse one)\" out &&\n+\t! grep \"$(git -C repo rev-parse two)\" out &&\n+\n+\t# Remove the promisor packfile and associated files\n+\trm $(sed \"s/.promisor//\" <promisorlist).* &&\n+\n+\t# Ensure that the single other pack contains the commits, but not the\n+\t# trees\n \tls repo/.git/objects/pack/pack-*.pack >packlist &&\n \ttest_line_count = 1 packlist &&\n \tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse HEAD)\" out &&\n-\t! grep \"$TREE_HASH\" out\n+\tgrep \"$(git -C repo rev-parse one)\" out &&\n+\tgrep \"$(git -C repo rev-parse two)\" out &&\n+\t! grep \"$TREE_ONE\" out &&\n+\t! grep \"$TREE_TWO\" out\n+'\n+\n+repack_and_check () {\n+\trm -rf repo2 &&\n+\tcp -r repo repo2 &&\n+\tgit -C repo2 repack $1 -d &&\n+\tgit -C repo2 fsck &&\n+\n+\tgit -C repo2 cat-file -e $2 &&\n+\tgit -C repo2 cat-file -e $3\n+}\n+\n+test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n+\n+\tgit -C repo commit --allow-empty -m one &&\n+\tgit -C repo commit --allow-empty -m two &&\n+\tgit -C repo commit --allow-empty -m three &&\n+\tgit -C repo commit --allow-empty -m four &&\n+\tONE=$(git -C repo rev-parse HEAD^^^) &&\n+\tTWO=$(git -C repo rev-parse HEAD^^) &&\n+\tTHREE=$(git -C repo rev-parse HEAD^) &&\n+\n+\tprintf \"$TWO\\n\" | pack_as_from_promisor &&\n+\tprintf \"$THREE\\n\" | pack_as_from_promisor &&\n+\tdelete_object repo \"$ONE\" &&\n+\n+\trepack_and_check -a \"$TWO\" \"$THREE\" &&\n+\trepack_and_check -A \"$TWO\" \"$THREE\" &&\n+\trepack_and_check -l \"$TWO\" \"$THREE\"\n '\n \n test_expect_success 'gc stops traversal when a missing but promised object is reached' '\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354770","messageId":"xmqqzhxxki84.fsf@gitster-ct.c.googlers.com","threadId":"49064","inReplyTo":"61396b2a73a801f62818ad3b691f75170d2e3919.1533672584.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T20:57:31Z","receivedAt":"2018-08-07T20:57:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> +static int write_oid(const struct object_id *oid, struct packed_git *pack,\n> +\t\t     uint32_t pos, void *data)\n> +{\n> +\tint fd = *(int *)data;\n> +\n> +\txwrite(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ);\n> +\txwrite(fd, \"\\n\", 1);\n> +\treturn 0;\n> +}\n> +\n> +static void repack_promisor_objects(const struct packed_objects_args *args,\n> +\t\t\t\t    struct string_list *names)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tFILE *out;\n> +\tstruct strbuf line = STRBUF_INIT;\n> +\n> +\tprepare_packed_objects(&cmd, args);\n> +\tcmd.in = -1;\n> +\n> +\tif (start_command(&cmd))\n> +\t\tdie(\"Could not start pack-objects to repack promisor objects\");\n> +\n> +\tfor_each_packed_object(write_oid, &cmd.in,\n> +\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n> +\tclose(cmd.in);\n\nfor_each_object_in_pack() is a fine way to make sure that you list\neverythning in a pack, but I suspect it is a horrible way to feed a\nlist of objects to pack-objects, as it goes by the .idx order, which\nis by definition a way to enumerate objects in a randomly useless\norder.\n\nDo we already have an access to the in-core reverse index for the\npack at this point in the code?  If so, we can enumerate the objects\nin the offset order without incurring a lot of cost (building the\nin-core revindex is the more expensive part).  When writing a pack,\nwe try to make sure that related objects are written out close to\neach other [*1*], so listing them in the offset order (with made-up\npathname information to _force_ that objects that live close\ntogether in the original pack are grouped together by getting\nsimilar names) might give us a better than horrible deltification.\nI dunno.\n\n\tSide note *1*: \"related\" has two axis, and one is relevant\n\tfor better deltification, while the other is not useful.\n\tThe one I have in mind here is that we write set of blobs\n\tthat belong to the same \"delta family\" together.\n\nI do not think such a \"make it a bit better than horrible\" is necessary\nin an initial version, but it deserves an in-code NEEDSWORK comment\nto remind us that we need to measure and experiment.\n\nThanks.\n"},{"id":"354777","messageId":"xmqq7el1kdlt.fsf@gitster-ct.c.googlers.com","threadId":"49064","inReplyTo":"61396b2a73a801f62818ad3b691f75170d2e3919.1533672584.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-07T22:37:18Z","receivedAt":"2018-08-07T22:37:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> @@ -293,6 +346,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \tif (pack_everything & ALL_INTO_ONE) {\n>  \t\tget_non_kept_pack_filenames(&existing_packs, &keep_pack_list);\n>  \n> +\t\tif (repository_format_partial_clone)\n> +\t\t\trepack_promisor_objects(&po_args, &names);\n> +\n>  \t\tif (existing_packs.nr && delete_redundant) {\n>  \t\t\tif (unpack_unreachable) {\n>  \t\t\t\targv_array_pushf(&cmd.args,\n\nJust a note (and a request-for-sanity-check) and not meant to be a\nrequest to update the code, but with a still-in-pu 4b757a40 (\"Use\nremote_odb_get_direct() and has_remote_odb()\", 2018-08-02) in\nflight, repository_format_partial_clone is now gone.\n\nI've tentatively resolved the above to read like so:\n\n\t\tif (has_remote_odb())\n\t\t\trepack_promisor_objects(&po_args, &names);\n\nbut I am not sure if it makes sense to always require odb helper to\nbe present for any promisor.  As long as you do not have need to\nactually access these missing objects, you do not need any remote\nodb access at all, in which case requiring has_remote_odb() as a\nprecondition to concatenate the promisor packs to coalesce them into\none pack does not make sense---you only want to know if there are\nany .promisor packs.\n\nIn other words, I suspect that the world is not black (i.e. partial\nclone, which always has remote-odb) and white (i.e. full repository,\nwithout remote-odb).  4b757a40 makes it impossible to have a gray\n(i.e. partial clone, but no access to remote-odb), which I am not\nsure if it is a good thing.\n"},{"id":"354779","messageId":"20180807232304.181764-1-jonathantanmy@google.com","threadId":"49064","inReplyTo":"xmqqzhxxki84.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-07T23:23:04Z","receivedAt":"2018-08-07T23:23:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> for_each_object_in_pack() is a fine way to make sure that you list\n> everythning in a pack, but I suspect it is a horrible way to feed a\n> list of objects to pack-objects, as it goes by the .idx order, which\n> is by definition a way to enumerate objects in a randomly useless\n> order.\n\nThat's true.\n\n> Do we already have an access to the in-core reverse index for the\n> pack at this point in the code?\n\nAs far as I can tell, no. These patches construct the list of promisor\nobjects in repack.c (which does not use the revindex at all), to be\nprocessed by pack-objects in a different process (which does use the\nrevindex in reuse-delta mode, which is the default). I guess that we\ncould have access to the revindex if we were to libify pack-objects and\nrun it in the same process as repack.c or if we were to add a special\nmode to pack-objects that reads for itself the list of all the promisor\nobjects.\n\n> If so, we can enumerate the objects\n> in the offset order without incurring a lot of cost (building the\n> in-core revindex is the more expensive part).  When writing a pack,\n> we try to make sure that related objects are written out close to\n> each other [*1*], so listing them in the offset order (with made-up\n> pathname information to _force_ that objects that live close\n> together in the original pack are grouped together by getting\n> similar names) might give us a better than horrible deltification.\n> I dunno.\n\nBefore I write the NEEDSWORK comment, just to clarify - do you mean\nbetter than horrible object locality? I think deltification still occurs\nbecause pack-objects sorts the input when doing the windowed\ndeltification, for example:\n\n  git rev-parse HEAD:fetch-pack.c HEAD HEAD^ HEAD^^ \\\n\tHEAD^^^ v2.17.0:fetch-pack.c | git pack-objects --window=2 abc\n\nproduces a packfile with a delta, as checked by `verify-pack -v`.\n\n> \tSide note *1*: \"related\" has two axis, and one is relevant\n> \tfor better deltification, while the other is not useful.\n> \tThe one I have in mind here is that we write set of blobs\n> \tthat belong to the same \"delta family\" together.\n\nAs far as I can see, they do not need to be adjacent in the packfile to\nhave one be a delta against the other.\n\n> I do not think such a \"make it a bit better than horrible\" is necessary\n> in an initial version, but it deserves an in-code NEEDSWORK comment\n> to remind us that we need to measure and experiment.\n\nOK, I'll do this in the next reroll.\n"},{"id":"354781","messageId":"20180807232528.182492-1-jonathantanmy@google.com","threadId":"49064","inReplyTo":"xmqq7el1kdlt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-07T23:25:28Z","receivedAt":"2018-08-07T23:25:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Just a note (and a request-for-sanity-check) and not meant to be a\n> request to update the code, but with a still-in-pu 4b757a40 (\"Use\n> remote_odb_get_direct() and has_remote_odb()\", 2018-08-02) in\n> flight, repository_format_partial_clone is now gone.\n> \n> I've tentatively resolved the above to read like so:\n> \n> \t\tif (has_remote_odb())\n> \t\t\trepack_promisor_objects(&po_args, &names);\n> \n> but I am not sure if it makes sense to always require odb helper to\n> be present for any promisor.  As long as you do not have need to\n> actually access these missing objects, you do not need any remote\n> odb access at all, in which case requiring has_remote_odb() as a\n> precondition to concatenate the promisor packs to coalesce them into\n> one pack does not make sense---you only want to know if there are\n> any .promisor packs.\n> \n> In other words, I suspect that the world is not black (i.e. partial\n> clone, which always has remote-odb) and white (i.e. full repository,\n> without remote-odb).  4b757a40 makes it impossible to have a gray\n> (i.e. partial clone, but no access to remote-odb), which I am not\n> sure if it is a good thing.\n\nThanks for the heads-up. This makes me realize that even the current\nprecondition (repository_format_partial_clone) is not an exact one - I\nshould only be doing this if I know that there is at least one promisor\nobject (if not, in the rare case that a repo is configured to be partial\nbut does not have any promisor objects, repack will generate an empty\npackfile). In the next reroll, I'll take care of this, which should\navoid this merge issue too.\n"},{"id":"354784","messageId":"xmqqy3dhiu08.fsf@gitster-ct.c.googlers.com","threadId":"49064","inReplyTo":"20180807232304.181764-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-08T00:25:59Z","receivedAt":"2018-08-08T00:26:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> each other [*1*], so listing them in the offset order (with made-up\n>> pathname information to _force_ that objects that live close\n>> together in the original pack are grouped together by getting\n>> similar names) might give us a better than horrible deltification.\n>> I dunno.\n>\n> Before I write the NEEDSWORK comment, just to clarify - do you mean\n> better than horrible object locality? I think deltification still occurs\n> because pack-objects sorts the input when doing the windowed\n> deltification, for example:\n\nBut what to delta against what else is determined by the pathname\ninfo, which is now lost by enumerating objects without tree/history\nwalking.  By giving phoney pathnames to objects while enumerating\nthem in offset order and giving similar pathnames to objects closer\nto each other, I was hoping that better bases will likely to be in\nthe same window.  The order in which objects are prepared and fed to\ntry_delta() is \"group by type, and then sort by path-hash (major\nkey) and size (descending order, used as minor key)\", so that\nthe largest among similarly named blobs is stored as base and other\nblobs with similar name try to become delta against that one.\n\n>   git rev-parse HEAD:fetch-pack.c HEAD HEAD^ HEAD^^ \\\n> \tHEAD^^^ v2.17.0:fetch-pack.c | git pack-objects --window=2 abc\n>\n> produces a packfile with a delta, as checked by `verify-pack -v`.\n\nIs that interesting?  You can make the same argument that\nfast-import produces a packfile with a delta.\n\nIt is known to produce horrible delta---its delta base selection\nessentially is \"you are giving me a new object?  let me see if it\ndeltas well with the object you gave me immediately before that (and\nnothing else)\".\n\n"},{"id":"354851","messageId":"20180808155045.GB1607@sigill.intra.peff.net","threadId":"49064","inReplyTo":"20180807232304.181764-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-08T15:50:46Z","receivedAt":"2018-08-08T15:50:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 07, 2018 at 04:23:04PM -0700, Jonathan Tan wrote:\n\n> > Do we already have an access to the in-core reverse index for the\n> > pack at this point in the code?\n> \n> As far as I can tell, no. These patches construct the list of promisor\n> objects in repack.c (which does not use the revindex at all), to be\n> processed by pack-objects in a different process (which does use the\n> revindex in reuse-delta mode, which is the default). I guess that we\n> could have access to the revindex if we were to libify pack-objects and\n> run it in the same process as repack.c or if we were to add a special\n> mode to pack-objects that reads for itself the list of all the promisor\n> objects.\n\nIt's not the end of the world to access the revindex here. It requires\neffort proportional to the size of the packfile, which makes it a bad\nchoice for operations that want to touch only a few objects. But here\nyou're iterating over the whole packfile anyway.\n\nI think that the for_each_packed_object() interface should visit items\nin pack order. Something like this (I'd probably change the name of the\ninner \"i\" variable for clarity, but I didn't here to minimize the diff):\n\ndiff --git a/packfile.c b/packfile.c\nindex 04d387f312..334f47e4cd 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1887,10 +1887,13 @@ int has_pack_index(const unsigned char *sha1)\n \n int for_each_object_in_pack(struct packed_git *p, each_packed_object_fn cb, void *data)\n {\n-\tuint32_t i;\n+\tuint32_t in_pack;\n \tint r = 0;\n \n-\tfor (i = 0; i < p->num_objects; i++) {\n+\tload_pack_revindex(p);\n+\n+\tfor (in_pack = 0; in_pack < p->num_objects; in_pack++) {\n+\t\tuint32_t i = p->revindex[in_pack].nr;\n \t\tstruct object_id oid;\n \n \t\tif (!nth_packed_object_oid(&oid, p, i))\n\nPossibly it should be an optional flag, in case the caller is going to\neventually sort by sha1 anyway. But the major caller that sorts by sha1\nis cat-file's --batch-all-objects option.  And IMHO that is actually a\nbad default. Try timing this in a fully-packed repository:\n\n  time git cat-file --batch-all-objects --buffer --batch | wc -c\n\nversus this:\n\n  time git show-index <.git/objects/pack/*.idx |\n  sort -n |\n  awk '{print $2}' |\n  git cat-file --buffer --batch | wc -c\n\nOn my git.git repo, it's literally 7x faster to do the latter, because\nof locality in resolving deltas.\n\nSo I think we should consider at least making it an option for cat-file\nto do the iteration in pack-ish order (it can't be perfect, of course,\nbecause there may be multiple packs, loose objects, etc, but\napproximating it is a good idea).\n\nI guess it has to be an option, since we advertise in the manpage that\nthe output is sorted by hash.\n\n> > If so, we can enumerate the objects\n> > in the offset order without incurring a lot of cost (building the\n> > in-core revindex is the more expensive part).  When writing a pack,\n> > we try to make sure that related objects are written out close to\n> > each other [*1*], so listing them in the offset order (with made-up\n> > pathname information to _force_ that objects that live close\n> > together in the original pack are grouped together by getting\n> > similar names) might give us a better than horrible deltification.\n> > I dunno.\n> \n> Before I write the NEEDSWORK comment, just to clarify - do you mean\n> better than horrible object locality? I think deltification still occurs\n> because pack-objects sorts the input when doing the windowed\n> deltification, for example:\n> \n>   git rev-parse HEAD:fetch-pack.c HEAD HEAD^ HEAD^^ \\\n> \tHEAD^^^ v2.17.0:fetch-pack.c | git pack-objects --window=2 abc\n> \n> produces a packfile with a delta, as checked by `verify-pack -v`.\n\nYou're going to get a lot of reused deltas from the old pack, but you'll\nprobably miss opportunities to delta objects between packs. And\npack-object's compute_write_order() will fix some of the issues:\n\n - it will group objects of each type together\n\n - it will group delta \"families\" together\n\nSo the resulting packfile may not actually be _too_ bad with respect to\ndelta locality. Where it will fail is:\n\n - there's no locality with respect to history, so a traversal will have\n   bad I/O patterns as it skips all over the packfile\n\n - the delta heuristics do use the incoming order (which usually is from\n   a traversal) as a hint. But IMHO that is way less important than\n   lacking the names. Even if you have an actual traversal, two versions\n   of the same file may be separated by hundreds of other blob changes\n   in the middle. Traversal order doesn't help you; putting things with\n   similar names next to each other does.\n\n   For traversals using bitmaps, we have a \"name hash cache\" in the\n   .bitmap file, which stores the 32-bit hash of the name. That lets us\n   put similar names next to each other without actually knowing the\n   names. If we absolutely can't traverse as part of the strategy for\n   repacking a promisor pack, then it may be worth trying to figure out\n   how to carry that hash around.\n\n-Peff\n"},{"id":"354861","messageId":"xmqqin4kj0j0.fsf@gitster-ct.c.googlers.com","threadId":"49064","inReplyTo":"20180808155045.GB1607@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-08T16:17:23Z","receivedAt":"2018-08-08T16:17:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think that the for_each_packed_object() interface should visit items\n> in pack order.\n\nYeah, that may make more sense.  Any order is more logical than the\nobject name order (which by definition is random, so useful only\nwhen you want them in a random order), and pack order will give us\nlocality of accesses.\n\n>    For traversals using bitmaps, we have a \"name hash cache\" in the\n>    .bitmap file, which stores the 32-bit hash of the name. That lets us\n>    put similar names next to each other without actually knowing the\n>    names.\n\nI forgot about that one; it is a neat trick.  As .primisor packs are\nintentionally kept apart from the remainder of the repository, and\nbecause .bitmap wants to have a single pack, we cannot just read\nthe .bitmap file for this particular application, though.\n\n"},{"id":"354869","messageId":"xmqqd0usizpx.fsf@gitster-ct.c.googlers.com","threadId":"49064","inReplyTo":"20180807232528.182492-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-08T16:34:50Z","receivedAt":"2018-08-08T16:34:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n>> Just a note (and a request-for-sanity-check) and not meant to be a\n>> request to update the code, but with a still-in-pu 4b757a40 (\"Use\n>> remote_odb_get_direct() and has_remote_odb()\", 2018-08-02) in\n>> flight, repository_format_partial_clone is now gone.\n>> ...\n> Thanks for the heads-up. This makes me realize that even the current\n> precondition (repository_format_partial_clone) is not an exact one - I\n> should only be doing this if I know that there is at least one promisor\n> object (if not, in the rare case that a repo is configured to be partial\n> but does not have any promisor objects, repack will generate an empty\n> packfile). In the next reroll, I'll take care of this, which should\n> avoid this merge issue too.\n\nI think that it is a good change, regardless of the semantic\nconflict with any other topic, to look at the .promisor packs and\nnot look at the partial clone extension for this \"do we want to\ncoalesce .promisor packs into one?\" application.  You only want to\ndo so when you have two or more such packs; being in partial clone\nmight be a preconditon for having such packs, but it is overly loose.\n\nIn any case, remote-odb topic is turning out to be more trouble than\nI originally thought; in the same change that removes the variable,\nit actually removes the support for a repository configured with a\npartial clone extension, and causes a hard error in t0410, breaking\ntest of 'pu'.  I do not know how many people actually use partial\nclone right now, but their repositories would be broken the same way\nwhen they update Git, if we merge that topic in its current shape.\n\nI am dropping it from 'pu' for today's integration cycle.\n"},{"id":"354882","messageId":"20180808184544.120498-1-jonathantanmy@google.com","threadId":"49064","inReplyTo":"xmqqy3dhiu08.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-08T18:45:44Z","receivedAt":"2018-08-08T18:45:50Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> But what to delta against what else is determined by the pathname\n> info, which is now lost by enumerating objects without tree/history\n> walking.  By giving phoney pathnames to objects while enumerating\n> them in offset order and giving similar pathnames to objects closer\n> to each other, I was hoping that better bases will likely to be in\n> the same window.  The order in which objects are prepared and fed to\n> try_delta() is \"group by type, and then sort by path-hash (major\n> key) and size (descending order, used as minor key)\", so that\n> the largest among similarly named blobs is stored as base and other\n> blobs with similar name try to become delta against that one.\n\nThanks for the patient explanation - I think I see it now. If we\nenumerate in pack order and pass \"<sha-1> <fake-name>\" instead of\n\"<sha-1>\" to pack-objects in such a way that we make pack-objects sort\nby {type -> pack-order -> size} instead of {type -> size}, we can\nhopefully get better deltas. I'll write a NEEDSWORK similar to this.\n\nI'll send a reroll later today.\n"},{"id":"354948","messageId":"cover.1533767314.git.jonathantanmy@google.com","threadId":"49064","inReplyTo":"cover.1533672584.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] Repacking of promisor packfiles","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-08T22:34:04Z","receivedAt":"2018-08-08T22:34:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Changes from v1:\n - add NEEDSWORK stating that input to pack-objects could be removed\n - run pack-objects to repack promisor objects only if there is at least\n   one of them - this exposed a possible bug where the later part of\n   cmd_repack() requires a correct packed_git (this was mostly noticed\n   by tests that deal with corrupt packfiles), so insert a call to\n   reprepare_packed_git() after all packfile manipulation is done\n - rename prepare_packed_objects() to prepare_pack_objects() to better\n   reflect that we are preparing a call to pack-objects, not preparing\n   existing packed objects\n\nPeff raised the possibility of making for_each_packed_object() use pack\norder instead of index order - I'll also take a look at that, separately\nfrom this patch series.\n\nJonathan Tan (2):\n  repack: refactor setup of pack-objects cmd\n  repack: repack promisor objects if -a or -A is set\n\n Documentation/git-repack.txt |   5 +\n builtin/repack.c             | 182 ++++++++++++++++++++++++++---------\n t/t0410-partial-clone.sh     |  85 +++++++++++++---\n 3 files changed, 213 insertions(+), 59 deletions(-)\n\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354949","messageId":"7dba07a3037c8134f755b4116788211dd5d6ef7f.1533767314.git.jonathantanmy@google.com","threadId":"49064","inReplyTo":"cover.1533767314.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] repack: refactor setup of pack-objects cmd","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-08T22:34:05Z","receivedAt":"2018-08-08T22:34:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A subsequent patch will teach repack to run pack-objects with some same\nand some different arguments if repacking of promisor objects is\nrequired. Refactor the setup of the pack-objects cmd so that setting up\nthe arguments common to both is done in a function.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/repack.c | 99 +++++++++++++++++++++++++++---------------------\n 1 file changed, 55 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 6c636e159..f8cae7d66 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -138,6 +138,47 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name)\n \tstrbuf_release(&buf);\n }\n \n+struct pack_objects_args {\n+\tconst char *window;\n+\tconst char *window_memory;\n+\tconst char *depth;\n+\tconst char *threads;\n+\tconst char *max_pack_size;\n+\tint no_reuse_delta;\n+\tint no_reuse_object;\n+\tint quiet;\n+\tint local;\n+};\n+\n+static void prepare_pack_objects(struct child_process *cmd,\n+\t\t\t\t const struct pack_objects_args *args)\n+{\n+\targv_array_push(&cmd->args, \"pack-objects\");\n+\tif (args->window)\n+\t\targv_array_pushf(&cmd->args, \"--window=%s\", args->window);\n+\tif (args->window_memory)\n+\t\targv_array_pushf(&cmd->args, \"--window-memory=%s\", args->window_memory);\n+\tif (args->depth)\n+\t\targv_array_pushf(&cmd->args, \"--depth=%s\", args->depth);\n+\tif (args->threads)\n+\t\targv_array_pushf(&cmd->args, \"--threads=%s\", args->threads);\n+\tif (args->max_pack_size)\n+\t\targv_array_pushf(&cmd->args, \"--max-pack-size=%s\", args->max_pack_size);\n+\tif (args->no_reuse_delta)\n+\t\targv_array_pushf(&cmd->args, \"--no-reuse-delta\");\n+\tif (args->no_reuse_object)\n+\t\targv_array_pushf(&cmd->args, \"--no-reuse-object\");\n+\tif (args->local)\n+\t\targv_array_push(&cmd->args,  \"--local\");\n+\tif (args->quiet)\n+\t\targv_array_push(&cmd->args,  \"--quiet\");\n+\tif (delta_base_offset)\n+\t\targv_array_push(&cmd->args,  \"--delta-base-offset\");\n+\targv_array_push(&cmd->args, packtmp);\n+\tcmd->git_cmd = 1;\n+\tcmd->out = -1;\n+}\n+\n #define ALL_INTO_ONE 1\n #define LOOSEN_UNREACHABLE 2\n \n@@ -165,15 +206,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint delete_redundant = 0;\n \tconst char *unpack_unreachable = NULL;\n \tint keep_unreachable = 0;\n-\tconst char *window = NULL, *window_memory = NULL;\n-\tconst char *depth = NULL;\n-\tconst char *threads = NULL;\n-\tconst char *max_pack_size = NULL;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n-\tint no_reuse_delta = 0, no_reuse_object = 0;\n \tint no_update_server_info = 0;\n-\tint quiet = 0;\n-\tint local = 0;\n+\tstruct pack_objects_args po_args = {NULL};\n \n \tstruct option builtin_repack_options[] = {\n \t\tOPT_BIT('a', NULL, &pack_everything,\n@@ -183,14 +218,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t   LOOSEN_UNREACHABLE | ALL_INTO_ONE),\n \t\tOPT_BOOL('d', NULL, &delete_redundant,\n \t\t\t\tN_(\"remove redundant packs, and run git-prune-packed\")),\n-\t\tOPT_BOOL('f', NULL, &no_reuse_delta,\n+\t\tOPT_BOOL('f', NULL, &po_args.no_reuse_delta,\n \t\t\t\tN_(\"pass --no-reuse-delta to git-pack-objects\")),\n-\t\tOPT_BOOL('F', NULL, &no_reuse_object,\n+\t\tOPT_BOOL('F', NULL, &po_args.no_reuse_object,\n \t\t\t\tN_(\"pass --no-reuse-object to git-pack-objects\")),\n \t\tOPT_BOOL('n', NULL, &no_update_server_info,\n \t\t\t\tN_(\"do not run git-update-server-info\")),\n-\t\tOPT__QUIET(&quiet, N_(\"be quiet\")),\n-\t\tOPT_BOOL('l', \"local\", &local,\n+\t\tOPT__QUIET(&po_args.quiet, N_(\"be quiet\")),\n+\t\tOPT_BOOL('l', \"local\", &po_args.local,\n \t\t\t\tN_(\"pass --local to git-pack-objects\")),\n \t\tOPT_BOOL('b', \"write-bitmap-index\", &write_bitmaps,\n \t\t\t\tN_(\"write bitmap index\")),\n@@ -198,15 +233,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"with -A, do not loosen objects older than this\")),\n \t\tOPT_BOOL('k', \"keep-unreachable\", &keep_unreachable,\n \t\t\t\tN_(\"with -a, repack unreachable objects\")),\n-\t\tOPT_STRING(0, \"window\", &window, N_(\"n\"),\n+\t\tOPT_STRING(0, \"window\", &po_args.window, N_(\"n\"),\n \t\t\t\tN_(\"size of the window used for delta compression\")),\n-\t\tOPT_STRING(0, \"window-memory\", &window_memory, N_(\"bytes\"),\n+\t\tOPT_STRING(0, \"window-memory\", &po_args.window_memory, N_(\"bytes\"),\n \t\t\t\tN_(\"same as the above, but limit memory size instead of entries count\")),\n-\t\tOPT_STRING(0, \"depth\", &depth, N_(\"n\"),\n+\t\tOPT_STRING(0, \"depth\", &po_args.depth, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n-\t\tOPT_STRING(0, \"threads\", &threads, N_(\"n\"),\n+\t\tOPT_STRING(0, \"threads\", &po_args.threads, N_(\"n\"),\n \t\t\t\tN_(\"limits the maximum number of threads\")),\n-\t\tOPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\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_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n@@ -238,7 +273,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tsigchain_push_common(remove_pack_on_signal);\n \n-\targv_array_push(&cmd.args, \"pack-objects\");\n+\tprepare_pack_objects(&cmd, &po_args);\n+\n \targv_array_push(&cmd.args, \"--keep-true-parents\");\n \tif (!pack_kept_objects)\n \t\targv_array_push(&cmd.args, \"--honor-pack-keep\");\n@@ -251,20 +287,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n-\tif (window)\n-\t\targv_array_pushf(&cmd.args, \"--window=%s\", window);\n-\tif (window_memory)\n-\t\targv_array_pushf(&cmd.args, \"--window-memory=%s\", window_memory);\n-\tif (depth)\n-\t\targv_array_pushf(&cmd.args, \"--depth=%s\", depth);\n-\tif (threads)\n-\t\targv_array_pushf(&cmd.args, \"--threads=%s\", threads);\n-\tif (max_pack_size)\n-\t\targv_array_pushf(&cmd.args, \"--max-pack-size=%s\", max_pack_size);\n-\tif (no_reuse_delta)\n-\t\targv_array_pushf(&cmd.args, \"--no-reuse-delta\");\n-\tif (no_reuse_object)\n-\t\targv_array_pushf(&cmd.args, \"--no-reuse-object\");\n \tif (write_bitmaps)\n \t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n \n@@ -292,17 +314,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\targv_array_push(&cmd.args, \"--incremental\");\n \t}\n \n-\tif (local)\n-\t\targv_array_push(&cmd.args,  \"--local\");\n-\tif (quiet)\n-\t\targv_array_push(&cmd.args,  \"--quiet\");\n-\tif (delta_base_offset)\n-\t\targv_array_push(&cmd.args,  \"--delta-base-offset\");\n-\n-\targv_array_push(&cmd.args, packtmp);\n-\n-\tcmd.git_cmd = 1;\n-\tcmd.out = -1;\n \tcmd.no_stdin = 1;\n \n \tret = start_command(&cmd);\n@@ -320,7 +331,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (ret)\n \t\treturn ret;\n \n-\tif (!names.nr && !quiet)\n+\tif (!names.nr && !po_args.quiet)\n \t\tprintf(\"Nothing new to pack.\\n\");\n \n \t/*\n@@ -441,7 +452,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\tif (!string_list_has_string(&names, sha1))\n \t\t\t\tremove_redundant_pack(packdir, item->string);\n \t\t}\n-\t\tif (!quiet && isatty(2))\n+\t\tif (!po_args.quiet && isatty(2))\n \t\t\topts |= PRUNE_PACKED_VERBOSE;\n \t\tprune_packed_objects(opts);\n \t}\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354950","messageId":"3dab08e467c23f2c0785e34c3a8703d809b6479a.1533767314.git.jonathantanmy@google.com","threadId":"49064","inReplyTo":"cover.1533767314.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-08T22:34:06Z","receivedAt":"2018-08-08T22:34:17Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, repack does not touch promisor packfiles at all, potentially\ncausing the performance of repositories that have many such packfiles to\ndrop. Therefore, repack all promisor objects if invoked with -a or -A.\n\nThis is done by an additional invocation of pack-objects on all promisor\nobjects individually given, which takes care of deduplication and allows\nthe resulting packfiles to respect flags such as --max-pack-size.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git-repack.txt |  5 +++\n builtin/repack.c             | 83 +++++++++++++++++++++++++++++++++--\n t/t0410-partial-clone.sh     | 85 +++++++++++++++++++++++++++++++-----\n 3 files changed, 158 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex d90e7907f..d05625096 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -40,6 +40,11 @@ OPTIONS\n Note that users fetching over dumb protocols will have to fetch the\n whole new pack in order to get any contained object, no matter how many\n other objects in that pack they already have locally.\n++\n+Promisor packfiles are repacked separately: if there are packfiles that\n+have an associated \".promisor\" file, these packfiles will be repacked\n+into another separate pack, and an empty \".promisor\" file corresponding\n+to the new separate pack will be written.\n \n -A::\n \tSame as `-a`, unless `-d` is used.  Then any unreachable\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex f8cae7d66..5c97dec3d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -8,6 +8,7 @@\n #include \"strbuf.h\"\n #include \"string-list.h\"\n #include \"argv-array.h\"\n+#include \"packfile.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -83,7 +84,7 @@ static void remove_pack_on_signal(int signo)\n \n /*\n  * Adds all packs hex strings to the fname list, which do not\n- * have a corresponding .keep or .promisor file. These packs are not to\n+ * have a corresponding .keep file. These packs are not to\n  * be kept if we are going to pack everything into one file.\n  */\n static void get_non_kept_pack_filenames(struct string_list *fname_list,\n@@ -111,8 +112,7 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n \n \t\tfname = xmemdupz(e->d_name, len);\n \n-\t\tif (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)) &&\n-\t\t    !file_exists(mkpath(\"%s/%s.promisor\", packdir, fname)))\n+\t\tif (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))\n \t\t\tstring_list_append_nodup(fname_list, fname);\n \t\telse\n \t\t\tfree(fname);\n@@ -122,7 +122,7 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n \n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n {\n-\tconst char *exts[] = {\".pack\", \".idx\", \".keep\", \".bitmap\"};\n+\tconst char *exts[] = {\".pack\", \".idx\", \".keep\", \".bitmap\", \".promisor\"};\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \tsize_t plen;\n@@ -179,6 +179,76 @@ static void prepare_pack_objects(struct child_process *cmd,\n \tcmd->out = -1;\n }\n \n+/*\n+ * Write oid to the given struct child_process's stdin, starting it first if\n+ * necessary.\n+ */\n+static int write_oid(const struct object_id *oid, struct packed_git *pack,\n+\t\t     uint32_t pos, void *data)\n+{\n+\tstruct child_process *cmd = data;\n+\n+\tif (cmd->in == -1) {\n+\t\tif (start_command(cmd))\n+\t\t\tdie(\"Could not start pack-objects to repack promisor objects\");\n+\t}\n+\n+\txwrite(cmd->in, oid_to_hex(oid), GIT_SHA1_HEXSZ);\n+\txwrite(cmd->in, \"\\n\", 1);\n+\treturn 0;\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+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tFILE *out;\n+\tstruct strbuf line = STRBUF_INIT;\n+\n+\tprepare_pack_objects(&cmd, args);\n+\tcmd.in = -1;\n+\n+\t/*\n+\t * NEEDSWORK: Giving pack-objects only the OIDs without any ordering\n+\t * hints may result in suboptimal deltas in the resulting pack. See if\n+\t * the OIDs can be sent with fake paths such that pack-objects can use a\n+\t * {type -> existing pack order} ordering when computing deltas instead\n+\t * of a {type -> size} ordering, which may produce better deltas.\n+\t */\n+\tfor_each_packed_object(write_oid, &cmd,\n+\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n+\n+\tif (cmd.in == -1)\n+\t\t/* No packed objects; cmd was never started */\n+\t\treturn;\n+\n+\tclose(cmd.in);\n+\n+\tout = xfdopen(cmd.out, \"r\");\n+\twhile (strbuf_getline_lf(&line, out) != EOF) {\n+\t\tchar *promisor_name;\n+\t\tint fd;\n+\t\tif (line.len != 40)\n+\t\t\tdie(\"repack: Expecting 40 character sha1 lines only from pack-objects.\");\n+\t\tstring_list_append(names, line.buf);\n+\n+\t\t/*\n+\t\t * pack-objects creates the .pack and .idx files, but not the\n+\t\t * .promisor file. Create the .promisor file, which is empty.\n+\t\t */\n+\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n+\t\t\t\t\t  line.buf);\n+\t\tfd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n+\t\tif (fd < 0)\n+\t\t\tdie_errno(\"unable to create '%s'\", promisor_name);\n+\t\tclose(fd);\n+\t\tfree(promisor_name);\n+\t}\n+\tfclose(out);\n+\tif (finish_command(&cmd))\n+\t\tdie(\"Could not finish pack-objects to repack promisor objects\");\n+}\n+\n #define ALL_INTO_ONE 1\n #define LOOSEN_UNREACHABLE 2\n \n@@ -191,6 +261,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t{\".pack\"},\n \t\t{\".idx\"},\n \t\t{\".bitmap\", 1},\n+\t\t{\".promisor\", 1},\n \t};\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n@@ -293,6 +364,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (pack_everything & ALL_INTO_ONE) {\n \t\tget_non_kept_pack_filenames(&existing_packs, &keep_pack_list);\n \n+\t\trepack_promisor_objects(&po_args, &names);\n+\n \t\tif (existing_packs.nr && delete_redundant) {\n \t\t\tif (unpack_unreachable) {\n \t\t\t\targv_array_pushf(&cmd.args,\n@@ -440,6 +513,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t/* End of pack replacement. */\n \n+\treprepare_packed_git(the_repository);\n+\n \tif (delete_redundant) {\n \t\tint opts = 0;\n \t\tstring_list_sort(&names);\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 4984ca583..128130066 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -271,28 +271,91 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n \tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n '\n \n-test_expect_success 'gc does not repack promisor objects' '\n+test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-\ttest_commit -C repo my_commit &&\n+\ttest_commit -C repo one &&\n+\ttest_commit -C repo two &&\n \n-\tTREE_HASH=$(git -C repo rev-parse HEAD^{tree}) &&\n-\tHASH=$(printf \"$TREE_HASH\\n\" | pack_as_from_promisor) &&\n+\tTREE_ONE=$(git -C repo rev-parse one^{tree}) &&\n+\tprintf \"$TREE_ONE\\n\" | pack_as_from_promisor &&\n+\tTREE_TWO=$(git -C repo rev-parse two^{tree}) &&\n+\tprintf \"$TREE_TWO\\n\" | pack_as_from_promisor &&\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n \tgit -C repo gc &&\n \n-\t# Ensure that the promisor packfile still exists, and remove it\n-\ttest -e repo/.git/objects/pack/pack-$HASH.pack &&\n-\trm repo/.git/objects/pack/pack-$HASH.* &&\n-\n-\t# Ensure that the single other pack contains the commit, but not the tree\n+\t# Ensure that exactly one promisor packfile exists, and that it\n+\t# contains the trees but not the commits\n+\tls repo/.git/objects/pack/pack-*.promisor >promisorlist &&\n+\ttest_line_count = 1 promisorlist &&\n+\tPROMISOR_PACKFILE=$(sed \"s/.promisor/.pack/\" <promisorlist) &&\n+\tgit verify-pack $PROMISOR_PACKFILE -v >out &&\n+\tgrep \"$TREE_ONE\" out &&\n+\tgrep \"$TREE_TWO\" out &&\n+\t! grep \"$(git -C repo rev-parse one)\" out &&\n+\t! grep \"$(git -C repo rev-parse two)\" out &&\n+\n+\t# Remove the promisor packfile and associated files\n+\trm $(sed \"s/.promisor//\" <promisorlist).* &&\n+\n+\t# Ensure that the single other pack contains the commits, but not the\n+\t# trees\n \tls repo/.git/objects/pack/pack-*.pack >packlist &&\n \ttest_line_count = 1 packlist &&\n \tgit verify-pack repo/.git/objects/pack/pack-*.pack -v >out &&\n-\tgrep \"$(git -C repo rev-parse HEAD)\" out &&\n-\t! grep \"$TREE_HASH\" out\n+\tgrep \"$(git -C repo rev-parse one)\" out &&\n+\tgrep \"$(git -C repo rev-parse two)\" out &&\n+\t! grep \"$TREE_ONE\" out &&\n+\t! grep \"$TREE_TWO\" out\n+'\n+\n+test_expect_success 'gc does not repack promisor objects if there are none' '\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\ttest_commit -C repo one &&\n+\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n+\tgit -C repo gc &&\n+\n+\t# Ensure that only one pack exists\n+\tls repo/.git/objects/pack/pack-*.pack >packlist &&\n+\ttest_line_count = 1 packlist\n+'\n+\n+repack_and_check () {\n+\trm -rf repo2 &&\n+\tcp -r repo repo2 &&\n+\tgit -C repo2 repack $1 -d &&\n+\tgit -C repo2 fsck &&\n+\n+\tgit -C repo2 cat-file -e $2 &&\n+\tgit -C repo2 cat-file -e $3\n+}\n+\n+test_expect_success 'repack -d does not irreversibly delete promisor objects' '\n+\trm -rf repo &&\n+\ttest_create_repo repo &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n+\n+\tgit -C repo commit --allow-empty -m one &&\n+\tgit -C repo commit --allow-empty -m two &&\n+\tgit -C repo commit --allow-empty -m three &&\n+\tgit -C repo commit --allow-empty -m four &&\n+\tONE=$(git -C repo rev-parse HEAD^^^) &&\n+\tTWO=$(git -C repo rev-parse HEAD^^) &&\n+\tTHREE=$(git -C repo rev-parse HEAD^) &&\n+\n+\tprintf \"$TWO\\n\" | pack_as_from_promisor &&\n+\tprintf \"$THREE\\n\" | pack_as_from_promisor &&\n+\tdelete_object repo \"$ONE\" &&\n+\n+\trepack_and_check -a \"$TWO\" \"$THREE\" &&\n+\trepack_and_check -A \"$TWO\" \"$THREE\" &&\n+\trepack_and_check -l \"$TWO\" \"$THREE\"\n '\n \n test_expect_success 'gc stops traversal when a missing but promised object is reached' '\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"355009","messageId":"xmqqftzncvxv.fsf@gitster-ct.c.googlers.com","threadId":"49064","inReplyTo":"3dab08e467c23f2c0785e34c3a8703d809b6479a.1533767314.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T17:05:16Z","receivedAt":"2018-08-09T17:05:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> +/*\n> + * Write oid to the given struct child_process's stdin, starting it first if\n> + * necessary.\n> + */\n> +static int write_oid(const struct object_id *oid, struct packed_git *pack,\n> +\t\t     uint32_t pos, void *data)\n> +{\n> +\tstruct child_process *cmd = data;\n> +\n> +\tif (cmd->in == -1) {\n> +\t\tif (start_command(cmd))\n> +\t\t\tdie(\"Could not start pack-objects to repack promisor objects\");\n> +\t}\n> +\n> +\txwrite(cmd->in, oid_to_hex(oid), GIT_SHA1_HEXSZ);\n> +\txwrite(cmd->in, \"\\n\", 1);\n> +\treturn 0;\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> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\tFILE *out;\n> +\tstruct strbuf line = STRBUF_INIT;\n> +\n> +\tprepare_pack_objects(&cmd, args);\n> +\tcmd.in = -1;\n> +\n> +\t/*\n> +\t * NEEDSWORK: Giving pack-objects only the OIDs without any ordering\n> +\t * hints may result in suboptimal deltas in the resulting pack. See if\n> +\t * the OIDs can be sent with fake paths such that pack-objects can use a\n> +\t * {type -> existing pack order} ordering when computing deltas instead\n> +\t * of a {type -> size} ordering, which may produce better deltas.\n> +\t */\n> +\tfor_each_packed_object(write_oid, &cmd,\n> +\t\t\t       FOR_EACH_OBJECT_PROMISOR_ONLY);\n> +\n> +\tif (cmd.in == -1)\n> +\t\t/* No packed objects; cmd was never started */\n> +\t\treturn;\n> +\tclose(cmd.in);\n> +\n> +\tout = xfdopen(cmd.out, \"r\");\n\nHmm, it is clever to auto-start the pack-objects (and notice there\nwasn't anything needing to pack).  Two things that are worth noting\nare:\n\n - The code takes advantage of the fact that cmd.in being left as -1\n   is a sign that start_command() was never called (well, it could\n   be that it was called but failed to open pipe---but then we would\n   have died inside write_oid(), so we can ignore taht case).  This\n   is not relying on its implementation detail---cmd.in would be\n   replaced by a real fd to the pipe if start_command() was called.\n\n - We know that pack-objects reads all its standard input through to\n   the EOF before emitting a single byte to its standard output, and\n   that is why we can use bidi pipe and not worry about deadlocking\n   caused by not reading from cmd.out at all (before closing cmd.in,\n   that is).\n\nSo I kind of like the cleverness of the design of this code.\n\n> +\twhile (strbuf_getline_lf(&line, out) != EOF) {\n> +\t\tchar *promisor_name;\n> +\t\tint fd;\n> +\t\tif (line.len != 40)\n> +\t\t\tdie(\"repack: Expecting 40 character sha1 lines only from pack-objects.\");\n> +\t\tstring_list_append(names, line.buf);\n> +\n> +\t\t/*\n> +\t\t * pack-objects creates the .pack and .idx files, but not the\n> +\t\t * .promisor file. Create the .promisor file, which is empty.\n> +\t\t */\n> +\t\tpromisor_name = mkpathdup(\"%s-%s.promisor\", packtmp,\n> +\t\t\t\t\t  line.buf);\n> +\t\tfd = open(promisor_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n> +\t\tif (fd < 0)\n> +\t\t\tdie_errno(\"unable to create '%s'\", promisor_name);\n> +\t\tclose(fd);\n> +\t\tfree(promisor_name);\n\nFor now, as we do not know what is the appropriate thing to leave in\nthe file, empty is fine, but perhaps in the future we may want to\ncarry forward the info from the existing .promisor file?  What does\nit initially contain?  Transport invoked with from_promisor bit\nseems to call index-pack with \"--promisor\" option with no argument,\nso this is probably in line with that.  Do we in the future need to\nteach \"--promisor\" option to pack-objects we invoke here, which will\nbe passed to the index-pack it internally performs, so that we do\nnot have to do this by hand here?\n\nIn any case, don't we need to update the doc for the \"--promisor\"\noption of \"index-pack\"?  The same comment for the new options added\nin the same topic, e.g. \"--from-promisor\" and \"--no-dependents\"\noptions added to \"fetch-pack\".  It seems that the topic that ended\nat 0c16cd499d was done rather hastily and under-documented?\n\n> @@ -440,6 +513,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \t/* End of pack replacement. */\n>  \n> +\treprepare_packed_git(the_repository);\n> +\n\nI do not think this call hurts, but why does this become suddenly\nnecessary with _this_ patch?  Is it an unrelated bugfix?\n\n>  \tif (delete_redundant) {\n>  \t\tint opts = 0;\n>  \t\tstring_list_sort(&names);\n\nThanks.\n"},{"id":"355021","messageId":"CAGf8dg+5ywyQVfuPfbRrKFdAatst25307ctQvkWqCDKHka7z4g@mail.gmail.com","threadId":"49064","inReplyTo":"xmqqftzncvxv.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-09T18:12:23Z","receivedAt":"2018-08-09T18:12:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, Aug 9, 2018 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Hmm, it is clever to auto-start the pack-objects (and notice there\n> wasn't anything needing to pack).  Two things that are worth noting\n> are:\n>\n>  - The code takes advantage of the fact that cmd.in being left as -1\n>    is a sign that start_command() was never called (well, it could\n>    be that it was called but failed to open pipe---but then we would\n>    have died inside write_oid(), so we can ignore taht case).  This\n>    is not relying on its implementation detail---cmd.in would be\n>    replaced by a real fd to the pipe if start_command() was called.\n>\n>  - We know that pack-objects reads all its standard input through to\n>    the EOF before emitting a single byte to its standard output, and\n>    that is why we can use bidi pipe and not worry about deadlocking\n>    caused by not reading from cmd.out at all (before closing cmd.in,\n>    that is).\n>\n> So I kind of like the cleverness of the design of this code.\n\nThanks for checking this.\n\n> For now, as we do not know what is the appropriate thing to leave in\n> the file, empty is fine, but perhaps in the future we may want to\n> carry forward the info from the existing .promisor file?  What does\n> it initially contain?  Transport invoked with from_promisor bit\n> seems to call index-pack with \"--promisor\" option with no argument,\n> so this is probably in line with that.  Do we in the future need to\n> teach \"--promisor\" option to pack-objects we invoke here, which will\n> be passed to the index-pack it internally performs, so that we do\n> not have to do this by hand here?\n\nThere might be more than one .promisor file that we are repacking, so\nwe would also have to deal with the order. It might be better to\ndocument that the contents of .promisor files are lost upon repack.\n\n> In any case, don't we need to update the doc for the \"--promisor\"\n> option of \"index-pack\"?  The same comment for the new options added\n> in the same topic, e.g. \"--from-promisor\" and \"--no-dependents\"\n> options added to \"fetch-pack\".  It seems that the topic that ended\n> at 0c16cd499d was done rather hastily and under-documented?\n\nYes, you're right - I'll make a patch documenting these.\n\n>> @@ -440,6 +513,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>>\n>>       /* End of pack replacement. */\n>>\n>> +     reprepare_packed_git(the_repository);\n>> +\n>\n> I do not think this call hurts, but why does this become suddenly\n> necessary with _this_ patch?  Is it an unrelated bugfix?\n\nHmm...I thought I mentioned somewhere that we need\nreprepare_packed_git now because for_each_packed_object calls\nprepare_packed_git, but apparently I didn't. I would add the following\nto the commit message (if you'd rather not do it yourself, let me know\nand I'll send a v3 with the new commit message):\n\nBecause for_each_packed_object() calls prepare_packed_git(), a call to\nreprepare_packed_git() now has to be inserted after all the packfile\nmanipulation - if not, old information about packed objects would\nremain in the_repository->objects->packed_git.\n"},{"id":"355993","messageId":"CACsJy8D63N3UPspm9-+ns0mv0OhEKDtxC25oZHR-d6SB6Hf2Lg@mail.gmail.com","threadId":"49064","inReplyTo":"3dab08e467c23f2c0785e34c3a8703d809b6479a.1533767314.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] repack: repack promisor objects if -a or -A is set","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-08-18T16:05:17Z","receivedAt":"2018-08-18T16:05:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Aug 9, 2018 at 12:35 AM Jonathan Tan <jonathantanmy@google.com> wrote:\n> @@ -179,6 +179,76 @@ static void prepare_pack_objects(struct child_process *cmd,\n>         cmd->out = -1;\n>  }\n>\n> +/*\n> + * Write oid to the given struct child_process's stdin, starting it first if\n> + * necessary.\n> + */\n> +static int write_oid(const struct object_id *oid, struct packed_git *pack,\n> +                    uint32_t pos, void *data)\n> +{\n> +       struct child_process *cmd = data;\n> +\n> +       if (cmd->in == -1) {\n> +               if (start_command(cmd))\n> +                       die(\"Could not start pack-objects to repack promisor objects\");\n\n_() ? (also other messages)\n-- \nDuy\n"}]}