{"thread":{"id":"50716","subject":"[PATCH 3/4] repack: add --sparse and pass to pack-objects","startedAt":"2019-03-12T13:26:11Z","lastAt":"2019-03-12T14:20:07Z","messageCount":8,"participants":["Nathaniel Filardo","Derrick Stolee","Dr N.W. Filardo"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"371247","messageId":"20190312131858.26115-4-nwf20@cl.cam.ac.uk","threadId":"50716","inReplyTo":"20190312131858.26115-1-nwf20@cl.cam.ac.uk","subject":"[PATCH 3/4] repack: add --sparse and pass to pack-objects","fromName":"Nathaniel Filardo","fromEmail":"nwf20@cl.cam.ac.uk","sentAt":"2019-03-12T13:18:57Z","receivedAt":"2019-03-12T13:26:11Z","isPatch":true,"sender":{"key":"nwf20@cl.cam.ac.uk","avatar":null},"body":"The sparse connectivity algorithm saves a whole lot of time when there\nare UNINTERESTING trees around.\n---\n Documentation/git-repack.txt |  4 ++++\n builtin/repack.c             | 10 ++++++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex aa0cc8bd44..836d81457a 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -165,6 +165,10 @@ depth is 4095.\n \tPass the `--delta-islands` option to `git-pack-objects`, see\n \tlinkgit:git-pack-objects[1].\n \n+--sparse::\n+\tPass the `--sparse` option to `git-pack-objects`; see\n+\tlinkgit:git-pack-objects[1].\n+\n Configuration\n -------------\n \ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 67f8978043..71e715b594 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -17,6 +17,7 @@ static int pack_kept_objects = -1;\n static int write_bitmaps;\n static int use_delta_islands;\n static char *packdir, *packtmp;\n+static int sparse;\n \n static const char *const git_repack_usage[] = {\n \tN_(\"git repack [<options>]\"),\n@@ -48,6 +49,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tuse_delta_islands = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"pack.usesparse\")) {\n+\t\tsparse = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -326,11 +331,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\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+\t\tOPT_BOOL(0, \"sparse\", &sparse,\n+\t\t\t N_(\"use the sparse reachability algorithm\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"do not repack this pack\")),\n \t\tOPT_END()\n \t};\n \n+\tsparse = git_env_bool(\"GIT_TEST_PACK_SPARSE\", 0);\n \tgit_config(repack_config, NULL);\n \n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n@@ -366,6 +374,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd.args, \"--all\");\n \targv_array_push(&cmd.args, \"--reflog\");\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n+\tif (sparse)\n+\t\targv_array_push(&cmd.args, \"--sparse\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n \tif (write_bitmaps)\n-- \n2.17.1\n\n"},{"id":"371260","messageId":"20190312131858.26115-2-nwf20@cl.cam.ac.uk","threadId":"50716","inReplyTo":"20190312131858.26115-1-nwf20@cl.cam.ac.uk","subject":"[PATCH 1/4] count-objects: report statistics about kept packs","fromName":"Nathaniel Filardo","fromEmail":"nwf20@cl.cam.ac.uk","sentAt":"2019-03-12T13:18:55Z","receivedAt":"2019-03-12T13:37:01Z","isPatch":true,"sender":{"key":"nwf20@cl.cam.ac.uk","avatar":null},"body":"Specifically: number of kept packs, size of kept packs (and indexes),\nand number of objects in kept packs.\n\nSigned-off-by: Nathaniel Filardo <nwf20@cl.cam.ac.uk>\n---\n builtin/count-objects.c | 17 +++++++++++++++--\n 1 file changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex 3fae474f6f..0309c7907f 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -117,17 +117,24 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \n \tif (verbose) {\n \t\tstruct packed_git *p;\n-\t\tunsigned long num_pack = 0;\n-\t\toff_t size_pack = 0;\n+\t\tunsigned long num_pack = 0, num_pack_keep = 0;\n+\t\tunsigned long packed_keep = 0;\n+\t\toff_t size_pack = 0, size_pack_keep = 0;\n \t\tstruct strbuf loose_buf = STRBUF_INIT;\n \t\tstruct strbuf pack_buf = STRBUF_INIT;\n \t\tstruct strbuf garbage_buf = STRBUF_INIT;\n+\t\tstruct strbuf pack_keep_buf = STRBUF_INIT;\n \n \t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\tif (!p->pack_local)\n \t\t\t\tcontinue;\n \t\t\tif (open_pack_index(p))\n \t\t\t\tcontinue;\n+\t\t\tif (p->pack_keep || p->pack_keep_in_core) {\n+\t\t\t\tpacked_keep += p->num_objects;\n+\t\t\t\tsize_pack_keep += p->pack_size + p->index_size;\n+\t\t\t\tnum_pack_keep++;\n+\t\t\t}\n \t\t\tpacked += p->num_objects;\n \t\t\tsize_pack += p->pack_size + p->index_size;\n \t\t\tnum_pack++;\n@@ -136,12 +143,15 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tif (human_readable) {\n \t\t\tstrbuf_humanise_bytes(&loose_buf, loose_size);\n \t\t\tstrbuf_humanise_bytes(&pack_buf, size_pack);\n+\t\t\tstrbuf_humanise_bytes(&pack_keep_buf, size_pack_keep);\n \t\t\tstrbuf_humanise_bytes(&garbage_buf, size_garbage);\n \t\t} else {\n \t\t\tstrbuf_addf(&loose_buf, \"%lu\",\n \t\t\t\t    (unsigned long)(loose_size / 1024));\n \t\t\tstrbuf_addf(&pack_buf, \"%lu\",\n \t\t\t\t    (unsigned long)(size_pack / 1024));\n+\t\t\tstrbuf_addf(&pack_keep_buf, \"%lu\",\n+\t\t\t\t    (unsigned long)(size_pack_keep / 1024));\n \t\t\tstrbuf_addf(&garbage_buf, \"%lu\",\n \t\t\t\t    (unsigned long)(size_garbage / 1024));\n \t\t}\n@@ -149,8 +159,11 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t\tprintf(\"count: %lu\\n\", loose);\n \t\tprintf(\"size: %s\\n\", loose_buf.buf);\n \t\tprintf(\"in-pack: %lu\\n\", packed);\n+\t\tprintf(\"in-pack-keep: %lu\\n\", packed_keep);\n \t\tprintf(\"packs: %lu\\n\", num_pack);\n+\t\tprintf(\"packs-keep: %lu\\n\", num_pack_keep);\n \t\tprintf(\"size-pack: %s\\n\", pack_buf.buf);\n+\t\tprintf(\"size-pack-keep: %s\\n\", pack_keep_buf.buf);\n \t\tprintf(\"prune-packable: %lu\\n\", packed_loose);\n \t\tprintf(\"garbage: %lu\\n\", garbage);\n \t\tprintf(\"size-garbage: %s\\n\", garbage_buf.buf);\n-- \n2.17.1\n\n"},{"id":"371261","messageId":"20190312131858.26115-1-nwf20@cl.cam.ac.uk","threadId":"50716","inReplyTo":null,"subject":"[PATCH 0/4] Speed up repacking when lots of pack-kept objects","fromName":"Nathaniel Filardo","fromEmail":"nwf20@cl.cam.ac.uk","sentAt":"2019-03-12T13:18:54Z","receivedAt":"2019-03-12T13:37:03Z","isPatch":true,"sender":{"key":"nwf20@cl.cam.ac.uk","avatar":null},"body":"This patch series improves handling of very large repositories, as generated\nby, for example, bup (https://github.com/bup/bup).  Prolonged operation\nthereof creates quite a lot of small pack files; repacking improves\nfilesystem performance of the objects/pack directory, but is quite\nexpensive, in terms of time and memory.  We have adopted a strategy that\nmarks \"large\" (tens of GB) of pack files as \"kept\" and defers repacking\nuntil there are enough un-kept packs or enough bytes of un-kept objects.\n(The first patch in the series will make our accounting easier, replacing\nsome terrible shell scripting with grep.)\n\nWhile this strategy has generally improved our lives relative to either\nextreme (not repacking, or repacking after every bup save operation), it\nstill leaves a good bit to be desired.  Because our packs are marked as\nkept, repacking will leave the objects therein alone, but it still must\ninstantiate in memory and walk the entire object graph.  However, because\nour kept packs are transitively closed, such that an object in one\nnecessarily references only objects in other kept packs, we should like to\navoid reasoning about them more or less altogether.\n\nThis series attempts to do just that.  The middle patches are just some\ngroundwork for the last patch, which carries the punch line.  This last\npatch adds an option to builtin/repack to enumerate commit and tree objects\nwithin kept packs as UNINTERESTING to its spawned builtin/pack-objects\ncommand.  Together with inducing the use of sparse reachability, this speeds\nenumerating candidate objects for repacking and thereby substantially\nreduces the runtime of our repack operations, while producing identical\nresults.\n\nI am, however, rather a novice when it comes to git internals, so any and\nall feedback is quite welcome.\n\nNathaniel Filardo (4):\n  count-objects: report statistics about kept packs\n  revision walk: optionally use sparse reachability\n  repack: add --sparse and pass to pack-objects\n  repack: optionally assume transitive kept packs\n\n Documentation/git-gc.txt         |  5 +++\n Documentation/git-repack.txt     | 25 +++++++++++++\n bisect.c                         |  2 +-\n blame.c                          |  2 +-\n builtin/checkout.c               |  2 +-\n builtin/commit.c                 |  2 +-\n builtin/count-objects.c          | 17 ++++++++-\n builtin/describe.c               |  2 +-\n builtin/fast-export.c            |  2 +-\n builtin/fmt-merge-msg.c          |  2 +-\n builtin/gc.c                     |  5 +++\n builtin/log.c                    | 10 ++---\n builtin/merge.c                  |  2 +-\n builtin/pack-objects.c           |  4 +-\n builtin/repack.c                 | 64 +++++++++++++++++++++++++++++++-\n builtin/rev-list.c               |  2 +-\n builtin/shortlog.c               |  2 +-\n bundle.c                         |  2 +-\n http-push.c                      |  2 +-\n merge-recursive.c                |  2 +-\n pack-bitmap-write.c              |  2 +-\n pack-bitmap.c                    |  4 +-\n reachable.c                      |  4 +-\n ref-filter.c                     |  2 +-\n remote.c                         |  2 +-\n revision.c                       | 10 +++--\n revision.h                       |  2 +-\n sequencer.c                      |  6 +--\n shallow.c                        |  2 +-\n submodule.c                      |  4 +-\n t/helper/test-revision-walking.c |  2 +-\n 31 files changed, 154 insertions(+), 42 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"371262","messageId":"20190312131858.26115-5-nwf20@cl.cam.ac.uk","threadId":"50716","inReplyTo":"20190312131858.26115-1-nwf20@cl.cam.ac.uk","subject":"[PATCH 4/4] repack: optionally assume transitive kept packs","fromName":"Nathaniel Filardo","fromEmail":"nwf20@cl.cam.ac.uk","sentAt":"2019-03-12T13:18:58Z","receivedAt":"2019-03-12T13:37:05Z","isPatch":true,"sender":{"key":"nwf20@cl.cam.ac.uk","avatar":null},"body":"If the user is careful to mark .pack files as kept only when they refer\nto (other) kept packs, then we can rely on this when walking the object\ngraph in subsequent repack operations and reduce the time and memory\nspent building the object graph.\n\nTowards that end, then, teach git repack to enumerate the non-BLOBs\nobjects (the tree and commits and tags) we're skipping because they're\nin kept packs and mark them as UNINTERESTING for pack-object's\noperation.  Because this creates UNINTERESTING marks, we make repack's\n--assume-pack-keep-transitive imply --sparse for pack-object.\n\nIn testing with a 203GB repository with 80M objects, this amounts to\nseveral gigabytes of memory and over ten minutes saved.  This machine\nhas 32G of RAM and the repository is on a fast SSD to speed testing.\n\nAll told, this test repository takes about 33 minutes elapsed (28\nminutes in user code) and 3 GB of RAM to repack 12M objects occupying\n33GB scattered across 477 pack files not marked as kept (the remainder\nof the objects are spread across three kept pack files).  The time and\nRAM usage with --assume-pack-keep-transitive should be dominated by\nfactors proportional to the size and number of un-kept objects.\n\nWithout these optimizations, it takes about 45 minutes (34 minutes in\nuser code) and 7 GB of RAM to compute the same pack file.  The extra\ntime and total memory use are expected to be proportional to the kept\npacks' size, not the working set of the repack.  The time discrepancy\ncan be expected to be more dramatic when the repository is on spinning\nrust rather than SSD.\n\nSigned-off-by: Nathaniel Filardo <nwf20@cl.cam.ac.uk>\n---\n Documentation/git-gc.txt     |  5 ++++\n Documentation/git-repack.txt | 21 ++++++++++++++\n builtin/gc.c                 |  5 ++++\n builtin/repack.c             | 56 ++++++++++++++++++++++++++++++++++--\n 4 files changed, 85 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt\nindex a7c1b0f60e..7115564f7d 100644\n--- a/Documentation/git-gc.txt\n+++ b/Documentation/git-gc.txt\n@@ -96,6 +96,11 @@ be performed as well.\n \t`.keep` files are consolidated into a single pack. When this\n \toption is used, `gc.bigPackThreshold` is ignored.\n \n+--assume-pack-keep-transitive::\n+\tPass the `--assume-pack-keep-transitive` option to `git-repack`;\n+\tsee linkgit:git-repack[1].\n+\n+\n CONFIGURATION\n -------------\n \ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 836d81457a..014812c4bb 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -169,6 +169,27 @@ depth is 4095.\n \tPass the `--sparse` option to `git-pack-objects`; see\n \tlinkgit:git-pack-objects[1].\n \n+--assume-pack-keep-transitive::\n+\tThis flag accelerates the search for objects to pack by assuming\n+\tthat commit objects found in kept packfiles make reference only\n+\tto that or other kept packfiles.  This is very useful if, for\n+\texample, this repository periodically repacks all reachable objects\n+\tand marks the resulting pack file as kept.\n+\n+\tBecause this option may prevent git from descending into kept packs,\n+\tno objects inside kept packs will be available for delta processing.\n+\tOne should therefore restrict the use of this option to situations\n+\twhere delta processing is disabled or when relatively large amounts\n+\tof data are considered and the relative gain of a wider set of\n+\tpossible delta base objects is reduced.\n+\n+\tThe simplest way to ensure transitivity of kept packs is to run `git\n+\trepack` with `-a` (or `-A`) and `-d` and mark all resulting packs as\n+\tkept, never removing the kept marker.  In practice, one may wish to\n+\twait to mark packs as kept until they are sufficiently large\n+\t(reducing the number of pack files necessary for a given set of\n+\tobjects) but not so large as to be unwieldy.\n+\n Configuration\n -------------\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 020f725acc..1346afb3b1 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -49,6 +49,7 @@ static const char *prune_expire = \"2.weeks.ago\";\n static const char *prune_worktrees_expire = \"3.months.ago\";\n static unsigned long big_pack_threshold;\n static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;\n+static int assume_pack_keep_transitive;\n \n static struct argv_array pack_refs_cmd = ARGV_ARRAY_INIT;\n static struct argv_array reflog = ARGV_ARRAY_INIT;\n@@ -526,6 +527,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\t   PARSE_OPT_NOCOMPLETE),\n \t\tOPT_BOOL(0, \"keep-largest-pack\", &keep_base_pack,\n \t\t\t N_(\"repack all other packs except the largest pack\")),\n+\t\tOPT_BOOL(0, \"assume-pack-keep-transitive\", &assume_pack_keep_transitive,\n+\t\t\t N_(\"assume kept packs reference only kept packs\")),\n \t\tOPT_END()\n \t};\n \n@@ -564,6 +567,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t}\n \tif (quiet)\n \t\targv_array_push(&repack, \"-q\");\n+\tif (assume_pack_keep_transitive)\n+\t\targv_array_push(&repack, \"--assume-pack-keep-transitive\");\n \n \tif (auto_gc) {\n \t\t/*\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 71e715b594..b2b407d42c 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -11,6 +11,8 @@\n #include \"midx.h\"\n #include \"packfile.h\"\n #include \"object-store.h\"\n+#include \"revision.h\"\n+#include \"list-objects.h\"\n \n static int delta_base_offset = 1;\n static int pack_kept_objects = -1;\n@@ -261,6 +263,30 @@ static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\tdie(_(\"could not finish pack-objects to repack promisor objects\"));\n }\n \n+static void apkt_show_commit(struct commit *commit, void *data)\n+{\n+\tstruct tree *tree;\n+\tstruct pack_entry e;\n+\n+\tif (!find_pack_entry(the_repository, &commit->object.oid, &e))\n+\t\treturn;\n+\n+\tif (!(e.p->pack_keep || e.p->pack_keep_in_core))\n+\t\treturn;\n+\n+\ttree = get_commit_tree(commit);\n+\tif (tree)\n+\t\ttree->object.flags |= UNINTERESTING;\n+\n+\twrite_oid(&commit->object.oid, e.p, 0, data);\n+\twrite_oid(&tree->object.oid, NULL, 0, data);\n+}\n+\n+static void apkt_show_object(struct object *obj, const char *name, void *data)\n+{\n+\treturn;\n+}\n+\n #define ALL_INTO_ONE 1\n #define LOOSEN_UNREACHABLE 2\n \n@@ -292,6 +318,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tint no_update_server_info = 0;\n \tint midx_cleared = 0;\n+\tint assume_pack_keep_transitive = 0;\n \tstruct pack_objects_args po_args = {NULL};\n \n \tstruct option builtin_repack_options[] = {\n@@ -333,6 +360,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_BOOL(0, \"sparse\", &sparse,\n \t\t\t N_(\"use the sparse reachability algorithm\")),\n+\t\tOPT_BOOL(0, \"assume-pack-keep-transitive\", &assume_pack_keep_transitive,\n+\t\t\t\tN_(\"assume kept packs reference only kept packs\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"do not repack this pack\")),\n \t\tOPT_END()\n@@ -351,6 +380,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t    (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n \n+\tif (assume_pack_keep_transitive) {\n+\t\t/* imply --honor-pack-keep */\n+\t\tpack_kept_objects = 0;\n+\t}\n+\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \n@@ -374,7 +408,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd.args, \"--all\");\n \targv_array_push(&cmd.args, \"--reflog\");\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n-\tif (sparse)\n+\tif (sparse || assume_pack_keep_transitive)\n \t\targv_array_push(&cmd.args, \"--sparse\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n@@ -409,12 +443,30 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\targv_array_push(&cmd.args, \"--incremental\");\n \t}\n \n-\tcmd.no_stdin = 1;\n+\tcmd.in = -1;\n+\tcmd.no_stdin = !assume_pack_keep_transitive;\n \n \tret = start_command(&cmd);\n \tif (ret)\n \t\treturn ret;\n \n+\tif (assume_pack_keep_transitive) {\n+\t\tstruct rev_info revs;\n+\t\tconst char *revargv[] = { \"skip\", \"--all\", \"--reflog\", \"--indexed-objects\", NULL };\n+\n+\t\trepo_init_revisions(the_repository, &revs, NULL);\n+\t\tsetup_revisions(sizeof(revargv)/sizeof(revargv[0]) - 1 , revargv, &revs, NULL);\n+\t\tif (prepare_revision_walk(&revs, sparse))\n+\t\t\tdie(\"revision walk setup failed\");\n+\n+\t\txwrite(cmd.in, \"--not\\n\", 6);\n+\t\ttraverse_commit_list(&revs, apkt_show_commit, apkt_show_object,\n+\t\t\t\t     &cmd);\n+\t\txwrite(cmd.in, \"--not\\n\", 6);\n+\n+\t\tclose(cmd.in);\n+\t}\n+\n \tout = xfdopen(cmd.out, \"r\");\n \twhile (strbuf_getline_lf(&line, out) != EOF) {\n \t\tif (line.len != the_hash_algo->hexsz)\n-- \n2.17.1\n\n"},{"id":"371263","messageId":"20190312131858.26115-3-nwf20@cl.cam.ac.uk","threadId":"50716","inReplyTo":"20190312131858.26115-1-nwf20@cl.cam.ac.uk","subject":"[PATCH 2/4] revision walk: optionally use sparse reachability","fromName":"Nathaniel Filardo","fromEmail":"nwf20@cl.cam.ac.uk","sentAt":"2019-03-12T13:18:56Z","receivedAt":"2019-03-12T13:37:08Z","isPatch":true,"sender":{"key":"nwf20@cl.cam.ac.uk","avatar":null},"body":"The only caller that passes a non-zero value to prepare_revision_walk\nafter this patch is builtin/pack-objects.  Without this, sparsity seems\nto do little good therein, as prepare_revision_walk will densely\npropagate UNINTERESTING flags from trees to tree contents, before\nmark_edges_uninteresting has a chance to be faster by being sparse.\n\nSigned-off-by: Nathaniel Filardo <nwf20@cl.cam.ac.uk>\n---\n bisect.c                         |  2 +-\n blame.c                          |  2 +-\n builtin/checkout.c               |  2 +-\n builtin/commit.c                 |  2 +-\n builtin/describe.c               |  2 +-\n builtin/fast-export.c            |  2 +-\n builtin/fmt-merge-msg.c          |  2 +-\n builtin/log.c                    | 10 +++++-----\n builtin/merge.c                  |  2 +-\n builtin/pack-objects.c           |  4 ++--\n builtin/rev-list.c               |  2 +-\n builtin/shortlog.c               |  2 +-\n bundle.c                         |  2 +-\n http-push.c                      |  2 +-\n merge-recursive.c                |  2 +-\n pack-bitmap-write.c              |  2 +-\n pack-bitmap.c                    |  4 ++--\n reachable.c                      |  4 ++--\n ref-filter.c                     |  2 +-\n remote.c                         |  2 +-\n revision.c                       | 10 ++++++----\n revision.h                       |  2 +-\n sequencer.c                      |  6 +++---\n shallow.c                        |  2 +-\n submodule.c                      |  4 ++--\n t/helper/test-revision-walking.c |  2 +-\n 26 files changed, 41 insertions(+), 39 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 3af955c4bc..47f8e3a7cc 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -655,7 +655,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \n static void bisect_common(struct rev_info *revs)\n {\n-\tif (prepare_revision_walk(revs))\n+\tif (prepare_revision_walk(revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \tif (revs->tree_objects)\n \t\tmark_edges_uninteresting(revs, NULL, 0);\ndiff --git a/blame.c b/blame.c\nindex 5c07dec190..13270ba07b 100644\n--- a/blame.c\n+++ b/blame.c\n@@ -1833,7 +1833,7 @@ void setup_scoreboard(struct blame_scoreboard *sb,\n \t * bottom commits we would reach while traversing as\n \t * uninteresting.\n \t */\n-\tif (prepare_revision_walk(sb->revs))\n+\tif (prepare_revision_walk(sb->revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \tif (sb->reverse && sb->revs->first_parent_only) {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 0e6037b296..8d6773cbaf 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -997,7 +997,7 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *ne\n \tfor_each_ref(add_pending_uninteresting_ref, &revs);\n \tadd_pending_oid(&revs, \"HEAD\", &new_commit->object.oid, UNINTERESTING);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"internal error in revision walk\"));\n \tif (!(old_commit->object.flags & UNINTERESTING))\n \t\tsuggest_reattach(old_commit, &revs);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f17537474a..5072e1c85e 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1001,7 +1001,7 @@ static const char *find_author_by_nickname(const char *name)\n \trevs.mailmap = &mailmap;\n \tread_mailmap(revs.mailmap, NULL);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \tcommit = get_revision(&revs);\n \tif (commit) {\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 1409cedce2..0e0d9c5173 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -493,7 +493,7 @@ static void describe_blob(struct object_id oid, struct strbuf *dst)\n \tif (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)\n \t\tBUG(\"setup_revisions could not handle all args?\");\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \n \ttraverse_commit_list(&revs, process_commit, process_object, &pcd);\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex 9e283482ef..7962b4a96c 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -1148,7 +1148,7 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)\n \n \tget_tags_and_duplicates(&revs.cmdline);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \trevs.diffopt.format_callback = show_filemodify;\n \trevs.diffopt.format_callback_data = &paths_of_changed_objects;\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex a4615587fd..d38af04e0c 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -356,7 +356,7 @@ static void shortlog(const char *name,\n \tadd_pending_object(rev, branch, name);\n \tadd_pending_object(rev, &head->object, \"^HEAD\");\n \thead->object.flags |= UNINTERESTING;\n-\tif (prepare_revision_walk(rev))\n+\tif (prepare_revision_walk(rev, 0))\n \t\tdie(\"revision walk setup failed\");\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tstruct pretty_print_context ctx = {0};\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ab859f5904..e2f12a88bb 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -378,7 +378,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \tif (rev->early_output)\n \t\tsetup_early_output(rev);\n \n-\tif (prepare_revision_walk(rev))\n+\tif (prepare_revision_walk(rev, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \tif (rev->early_output)\n@@ -936,7 +936,7 @@ static void get_patch_ids(struct rev_info *rev, struct patch_ids *ids)\n \to2->flags ^= UNINTERESTING;\n \tadd_pending_object(&check_rev, o1, \"o1\");\n \tadd_pending_object(&check_rev, o2, \"o2\");\n-\tif (prepare_revision_walk(&check_rev))\n+\tif (prepare_revision_walk(&check_rev, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \twhile ((commit = get_revision(&check_rev)) != NULL) {\n@@ -1424,7 +1424,7 @@ static void prepare_bases(struct base_tree_info *bases,\n \tbase->object.flags |= UNINTERESTING;\n \tadd_pending_object(&revs, &base->object, \"base\");\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \t/*\n \t * Traverse the commits list, get prerequisite patch ids\n@@ -1800,7 +1800,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tget_patch_ids(&rev, &ids);\n \t}\n \n-\tif (prepare_revision_walk(&rev))\n+\tif (prepare_revision_walk(&rev, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \trev.boundary = 1;\n \twhile ((commit = get_revision(&rev)) != NULL) {\n@@ -2090,7 +2090,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"Unknown commit %s\"), limit);\n \n \t/* reverse the list of commits */\n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \twhile ((commit = get_revision(&revs)) != NULL) {\n \t\tcommit_list_insert(commit, &list);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5ce8946d39..1fbb4069cf 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -406,7 +406,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \t\tadd_pending_object(&rev, &j->item->object, NULL);\n \n \tsetup_revisions(0, NULL, &rev, NULL);\n-\tif (prepare_revision_walk(&rev))\n+\tif (prepare_revision_walk(&rev, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \tctx.abbrev = rev.abbrev;\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a154fc29f6..c4ff6f79af 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3134,7 +3134,7 @@ static void get_object_list(int ac, const char **av)\n \tif (use_delta_islands)\n \t\tload_delta_islands(the_repository);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, sparse))\n \t\tdie(_(\"revision walk setup failed\"));\n \tmark_edges_uninteresting(&revs, show_edge, sparse);\n \n@@ -3149,7 +3149,7 @@ static void get_object_list(int ac, const char **av)\n \t\tif (add_unseen_recent_objects_to_traversal(&revs,\n \t\t\t\tunpack_unreachable_expiration))\n \t\t\tdie(_(\"unable to add recent objects\"));\n-\t\tif (prepare_revision_walk(&revs))\n+\t\tif (prepare_revision_walk(&revs, sparse))\n \t\t\tdie(_(\"revision walk setup failed\"));\n \t\ttraverse_commit_list(&revs, record_recent_commit,\n \t\t\t\t     record_recent_object, NULL);\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 5b5b6dbb1c..f192bfdce8 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -543,7 +543,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \tif (revs.tree_objects)\n \t\tmark_edges_uninteresting(&revs, show_edge, 0);\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 65cd41392c..8a236aa317 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -188,7 +188,7 @@ static void get_from_rev(struct rev_info *rev, struct shortlog *log)\n {\n \tstruct commit *commit;\n \n-\tif (prepare_revision_walk(rev))\n+\tif (prepare_revision_walk(rev, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \twhile ((commit = get_revision(rev)) != NULL)\n \t\tshortlog_add_commit(log, commit);\ndiff --git a/bundle.c b/bundle.c\nindex b45666c49b..b1ffa02a70 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -160,7 +160,7 @@ int verify_bundle(struct repository *r,\n \treq_nr = revs.pending.nr;\n \tsetup_revisions(2, argv, &revs, NULL);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \ti = req_nr;\ndiff --git a/http-push.c b/http-push.c\nindex b22c7caea0..ba9416f2e0 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1931,7 +1931,7 @@ int cmd_main(int argc, const char **argv)\n \n \t\t/* Generate a list of objects that need to be pushed */\n \t\tpushing = 0;\n-\t\tif (prepare_revision_walk(&revs))\n+\t\tif (prepare_revision_walk(&revs, 0))\n \t\t\tdie(\"revision walk setup failed\");\n \t\tmark_edges_uninteresting(&revs, NULL, 0);\n \t\tobjects_to_send = get_delta(&revs, ref_lock);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 6c40c61c47..adf218f66b 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1141,7 +1141,7 @@ static int find_first_merges(struct repository *repo,\n \tsetup_revisions(ARRAY_SIZE(rev_args)-1, rev_args, &revs, &rev_opts);\n \n \t/* save all revisions from the above list that contain b */\n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \twhile ((commit = get_revision(&revs)) != NULL) {\n \t\tstruct object *o = &(commit->object);\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 5566e94abe..e317550020 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -296,7 +296,7 @@ void bitmap_writer_build(struct packing_data *to_pack)\n \t\t\tadd_pending_object(&revs, object, \"\");\n \t\t\trevs.include_check_data = base;\n \n-\t\t\tif (prepare_revision_walk(&revs))\n+\t\t\tif (prepare_revision_walk(&revs, 0))\n \t\t\t\tdie(\"revision walk setup failed\");\n \n \t\t\ttraverse_commit_list(&revs, show_commit, show_object, base);\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 4695aaf6b4..21bbbab063 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -584,7 +584,7 @@ static struct bitmap *find_objects(struct bitmap_index *bitmap_git,\n \t\trevs->include_check = should_include;\n \t\trevs->include_check_data = &incdata;\n \n-\t\tif (prepare_revision_walk(revs))\n+\t\tif (prepare_revision_walk(revs, 0))\n \t\t\tdie(\"revision walk setup failed\");\n \n \t\tshow_data.bitmap_git = bitmap_git;\n@@ -987,7 +987,7 @@ void test_bitmap_walk(struct rev_info *revs)\n \n \tresult_popcnt = bitmap_popcount(result);\n \n-\tif (prepare_revision_walk(revs))\n+\tif (prepare_revision_walk(revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \n \ttdata.bitmap_git = bitmap_git;\ndiff --git a/reachable.c b/reachable.c\nindex 0d00a91de4..0339adc074 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -234,7 +234,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t * Set up the revision walk - this will move all commits\n \t * from the pending list to the commit walking list.\n \t */\n-\tif (prepare_revision_walk(revs))\n+\tif (prepare_revision_walk(revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n \n@@ -242,7 +242,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,\n \t\trevs->ignore_missing_links = 1;\n \t\tif (add_unseen_recent_objects_to_traversal(revs, mark_recent))\n \t\t\tdie(\"unable to mark recent objects\");\n-\t\tif (prepare_revision_walk(revs))\n+\t\tif (prepare_revision_walk(revs, 0))\n \t\t\tdie(\"revision walk setup failed\");\n \t\ttraverse_commit_list(revs, mark_commit, mark_object, &cp);\n \t}\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3aca105307..a5e16f3b80 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2071,7 +2071,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)\n \tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n \n \trevs.limited = 1;\n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \told_nr = array->nr;\ndiff --git a/remote.c b/remote.c\nindex 9cc3b07d21..4ddd93d5c7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1944,7 +1944,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsetup_revisions(argv.argc, argv.argv, &revs, NULL);\n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(_(\"revision walk setup failed\"));\n \n \t/* ... and count the commits on each side. */\ndiff --git a/revision.c b/revision.c\nindex eb8e51bc63..d108fe49bb 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -389,7 +389,8 @@ void add_pending_oid(struct rev_info *revs, const char *name,\n }\n \n static struct commit *handle_commit(struct rev_info *revs,\n-\t\t\t\t    struct object_array_entry *entry)\n+\t\t\t\t    struct object_array_entry *entry,\n+\t\t\t\t    int sparse)\n {\n \tstruct object *object = entry->item;\n \tconst char *name = entry->name;\n@@ -456,7 +457,8 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\tif (!revs->tree_objects)\n \t\t\treturn NULL;\n \t\tif (flags & UNINTERESTING) {\n-\t\t\tmark_tree_contents_uninteresting(revs->repo, tree);\n+\t\t\tif (!sparse)\n+\t\t\t\tmark_tree_contents_uninteresting(revs->repo, tree);\n \t\t\treturn NULL;\n \t\t}\n \t\tadd_pending_object_with_path(revs, object, name, mode, path);\n@@ -3289,7 +3291,7 @@ static void expand_topo_walk(struct rev_info *revs, struct commit *commit)\n \t}\n }\n \n-int prepare_revision_walk(struct rev_info *revs)\n+int prepare_revision_walk(struct rev_info *revs, int sparse)\n {\n \tint i;\n \tstruct object_array old_pending;\n@@ -3301,7 +3303,7 @@ int prepare_revision_walk(struct rev_info *revs)\n \trevs->pending.objects = NULL;\n \tfor (i = 0; i < old_pending.nr; i++) {\n \t\tstruct object_array_entry *e = old_pending.objects + i;\n-\t\tstruct commit *commit = handle_commit(revs, e);\n+\t\tstruct commit *commit = handle_commit(revs, e, sparse);\n \t\tif (commit) {\n \t\t\tif (!(commit->object.flags & SEEN)) {\n \t\t\t\tcommit->object.flags |= SEEN;\ndiff --git a/revision.h b/revision.h\nindex 4134dc6029..2e921c515a 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -320,7 +320,7 @@ int handle_revision_arg(const char *arg, struct rev_info *revs,\n \t\t\tint flags, unsigned revarg_opt);\n \n void reset_revision_walk(void);\n-int prepare_revision_walk(struct rev_info *revs);\n+int prepare_revision_walk(struct rev_info *revs, int sparse);\n struct commit *get_revision(struct rev_info *revs);\n char *get_revision_mark(const struct rev_info *revs,\n \t\t\tconst struct commit *commit);\ndiff --git a/sequencer.c b/sequencer.c\nindex 95dda23eee..df32dc1e71 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1990,7 +1990,7 @@ static int prepare_revs(struct replay_opts *opts)\n \tif (opts->action == REPLAY_PICK && !opts->revs->no_walk)\n \t\topts->revs->reverse ^= 1;\n \n-\tif (prepare_revision_walk(opts->revs))\n+\tif (prepare_revision_walk(opts->revs, 0))\n \t\treturn error(_(\"revision walk setup failed\"));\n \n \treturn 0;\n@@ -4062,7 +4062,7 @@ int sequencer_pick_revisions(struct repository *r,\n \t    opts->revs->no_walk &&\n \t    !opts->revs->cmdline.rev->flags) {\n \t\tstruct commit *cmit;\n-\t\tif (prepare_revision_walk(opts->revs))\n+\t\tif (prepare_revision_walk(opts->revs, 0))\n \t\t\treturn error(_(\"revision walk setup failed\"));\n \t\tcmit = get_revision(opts->revs);\n \t\tif (!cmit)\n@@ -4529,7 +4529,7 @@ int sequencer_make_script(struct repository *r, FILE *out,\n \tif (setup_revisions(argc, argv, &revs, NULL) > 1)\n \t\treturn error(_(\"make_script: unhandled options\"));\n \n-\tif (prepare_revision_walk(&revs) < 0)\n+\tif (prepare_revision_walk(&revs, 0) < 0)\n \t\treturn error(_(\"make_script: error preparing revisions\"));\n \n \tif (rebase_merges)\ndiff --git a/shallow.c b/shallow.c\nindex ce45297940..e9f3886a59 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -196,7 +196,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n \tsave_commit_buffer = 0;\n \tsetup_revisions(ac, av, &revs, NULL);\n \n-\tif (prepare_revision_walk(&revs))\n+\tif (prepare_revision_walk(&revs, 0))\n \t\tdie(\"revision walk setup failed\");\n \ttraverse_commit_list(&revs, show_commit, NULL, &not_shallow_list);\n \ndiff --git a/submodule.c b/submodule.c\nindex 21cf50ca15..2b18f888ca 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -456,7 +456,7 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,\n \t\tadd_pending_object(rev, &list->item->object,\n \t\t\toid_to_hex(&list->item->object.oid));\n \t}\n-\treturn prepare_revision_walk(rev);\n+\treturn prepare_revision_walk(rev, 0);\n }\n \n static void print_submodule_summary(struct repository *r, struct rev_info *rev, struct diff_options *o)\n@@ -843,7 +843,7 @@ static void collect_changed_submodules(struct repository *r,\n \n \trepo_init_revisions(r, &rev, NULL);\n \tsetup_revisions(argv->argc, argv->argv, &rev, NULL);\n-\tif (prepare_revision_walk(&rev))\n+\tif (prepare_revision_walk(&rev, 0))\n \t\tdie(\"revision walk setup failed\");\n \n \twhile ((commit = get_revision(&rev))) {\ndiff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c\nindex 625b2dbf82..dc1ff31d5c 100644\n--- a/t/helper/test-revision-walking.c\n+++ b/t/helper/test-revision-walking.c\n@@ -34,7 +34,7 @@ static int run_revision_walk(void)\n \n \trepo_init_revisions(the_repository, &rev, NULL);\n \tsetup_revisions(argc, argv, &rev, NULL);\n-\tif (prepare_revision_walk(&rev))\n+\tif (prepare_revision_walk(&rev, 0))\n \t\tdie(\"revision walk setup failed\");\n \n \twhile ((commit = get_revision(&rev)) != NULL) {\n-- \n2.17.1\n\n"},{"id":"371264","messageId":"f26c68df-8895-2a2d-0175-5fa3581ab45a@gmail.com","threadId":"50716","inReplyTo":"20190312131858.26115-4-nwf20@cl.cam.ac.uk","subject":"Re: [PATCH 3/4] repack: add --sparse and pass to pack-objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-03-12T13:47:18Z","receivedAt":"2019-03-12T13:47:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/12/2019 9:18 AM, Nathaniel Filardo wrote:\n> The sparse connectivity algorithm saves a whole lot of time when there\n> are UNINTERESTING trees around.\n\nInteresting! Do you have some performance numbers to include with\nthis statement?\n> @@ -48,6 +49,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n>  \t\tuse_delta_islands = git_config_bool(var, value);\n>  \t\treturn 0;\n>  \t}\n> +\tif (!strcmp(var, \"pack.usesparse\")) {\n> +\t\tsparse = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n\nThis part is not handled inside of `pack-objects`. Since you are not\nsending '--no-sparse' when the variable 'sparse' is zero, the config\nsetting will automatically be picked up by the pack-objects builtin.\n\nNow, a question of whether you _should_ allow the '--no-sparse' option\nin the 'repack' command, and send it along to the inner command (when\nit is present) is another question.\n\n> @@ -366,6 +374,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \targv_array_push(&cmd.args, \"--all\");\n>  \targv_array_push(&cmd.args, \"--reflog\");\n>  \targv_array_push(&cmd.args, \"--indexed-objects\");\n> +\tif (sparse)\n> +\t\targv_array_push(&cmd.args, \"--sparse\");\n>  \tif (repository_format_partial_clone)\n>  \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n>  \tif (write_bitmaps)\n> \n\nHow about a test with this new option? You can probably just add to\nt5322-pack-objects-sparse.sh.\n\nThanks,\n-Stolee\n"},{"id":"371265","messageId":"b414cb29-05a2-333e-f734-540eb0d1eaa4@gmail.com","threadId":"50716","inReplyTo":"20190312131858.26115-3-nwf20@cl.cam.ac.uk","subject":"Re: [PATCH 2/4] revision walk: optionally use sparse reachability","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-03-12T13:59:46Z","receivedAt":"2019-03-12T13:59:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/12/2019 9:18 AM, Nathaniel Filardo wrote:\n> The only caller that passes a non-zero value to prepare_revision_walk\n> after this patch is builtin/pack-objects.  Without this, sparsity seems\n> to do little good therein, as prepare_revision_walk will densely\n> propagate UNINTERESTING flags from trees to tree contents, before\n> mark_edges_uninteresting has a chance to be faster by being sparse.\n> \n> Signed-off-by: Nathaniel Filardo <nwf20@cl.cam.ac.uk>\n> ---\n>  bisect.c                         |  2 +-\n>  blame.c                          |  2 +-\n>  builtin/checkout.c               |  2 +-\n>  builtin/commit.c                 |  2 +-\n>  builtin/describe.c               |  2 +-\n>  builtin/fast-export.c            |  2 +-\n>  builtin/fmt-merge-msg.c          |  2 +-\n>  builtin/log.c                    | 10 +++++-----\n>  builtin/merge.c                  |  2 +-\n>  builtin/pack-objects.c           |  4 ++--\n>  builtin/rev-list.c               |  2 +-\n>  builtin/shortlog.c               |  2 +-\n>  bundle.c                         |  2 +-\n>  http-push.c                      |  2 +-\n>  merge-recursive.c                |  2 +-\n>  pack-bitmap-write.c              |  2 +-\n>  pack-bitmap.c                    |  4 ++--\n>  reachable.c                      |  4 ++--\n>  ref-filter.c                     |  2 +-\n>  remote.c                         |  2 +-\n>  revision.c                       | 10 ++++++----\n>  revision.h                       |  2 +-\n>  sequencer.c                      |  6 +++---\n>  shallow.c                        |  2 +-\n>  submodule.c                      |  4 ++--\n>  t/helper/test-revision-walking.c |  2 +-\n>  26 files changed, 41 insertions(+), 39 deletions(-)\n\nThis patch is very noisy. Perhaps the pattern established in 4f6d26b1:\n\"list-objects: consume sparse tree walk\" should be reversed and instead\ninclude a 'sparse_tree_walk' setting into 'struct rev_info'.\n\nChanging so many method prototypes is rather invasive and unlikely to\nbenefit many of these callers.\n\nIf the setting is added to 'struct rev_info', then you'll want to remove\nthe parameter from mark_edges_uninteresting().\n\nThanks,\n-Stolee\n"},{"id":"371266","messageId":"ad762ef2fbe9f268068b9a406070591b@cam.ac.uk","threadId":"50716","inReplyTo":"f26c68df-8895-2a2d-0175-5fa3581ab45a@gmail.com","subject":"Re: [PATCH 3/4] repack: add --sparse and pass to pack-objects","fromName":"Dr N.W. Filardo","fromEmail":"nwf20@cam.ac.uk","sentAt":"2019-03-12T14:03:16Z","receivedAt":"2019-03-12T14:20:07Z","isPatch":true,"sender":{"key":"nwf20@cam.ac.uk","avatar":null},"body":"On 2019-03-12 13:47, Derrick Stolee wrote:\n> On 3/12/2019 9:18 AM, Nathaniel Filardo wrote:\n>> The sparse connectivity algorithm saves a whole lot of time when there\n>> are UNINTERESTING trees around.\n> \n> Interesting! Do you have some performance numbers to include with\n> this statement?\n\nNot UNINTERESTING? ;)\n\nNot directly, no, but the performance numbers reported for the next \npatch in\nthe series hinge on using sparse reachability.  It seemed like a good \nidea to\nexpose this knob through repack even if one isn't going to use the\n--assume-pack-keep-transitive flag introduced in the next patch.\n\n>> @@ -48,6 +49,10 @@ static int repack_config(const char *var, const \n>> char *value, void *cb)\n>>  \t\tuse_delta_islands = git_config_bool(var, value);\n>>  \t\treturn 0;\n>>  \t}\n>> +\tif (!strcmp(var, \"pack.usesparse\")) {\n>> +\t\tsparse = git_config_bool(var, value);\n>> +\t\treturn 0;\n>> +\t}\n> \n> This part is not handled inside of `pack-objects`. Since you are not\n> sending '--no-sparse' when the variable 'sparse' is zero, the config\n> setting will automatically be picked up by the pack-objects builtin.\n\nOK, I will drop this hunk.\n\n> Now, a question of whether you _should_ allow the '--no-sparse' option\n> in the 'repack' command, and send it along to the inner command (when\n> it is present) is another question.\n\nI'm inclined to say yes, but am open to suggestions. :)\n\n>> @@ -366,6 +374,8 @@ int cmd_repack(int argc, const char **argv, const \n>> char *prefix)\n>>  \targv_array_push(&cmd.args, \"--all\");\n>>  \targv_array_push(&cmd.args, \"--reflog\");\n>>  \targv_array_push(&cmd.args, \"--indexed-objects\");\n>> +\tif (sparse)\n>> +\t\targv_array_push(&cmd.args, \"--sparse\");\n>>  \tif (repository_format_partial_clone)\n>>  \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n>>  \tif (write_bitmaps)\n>> \n> \n> How about a test with this new option? You can probably just add to\n> t5322-pack-objects-sparse.sh.\n\nCan do.\n\nCheers,\n--nwf;\n\n"}]}