Add path walk API and its use in 'git pack-objects'

[derrickstolee]

This is a follow up to #5157 as well as motivated by the RFC in gitgitgadget#1786.

We have ways of walking all objects, but it is focused on visiting a single commit and then expanding the new trees and blobs reachable from that commit that have not been visited yet. This means that objects arrive without any locality based on their path.

Add a new "path walk API" that focuses on walking objects in batches according to their type and path. This will walk all annotated tags, all commits, all root trees, and then start a depth-first search among all paths in the repo to collect trees and blobs in batches.

The most important application for this is being fast-tracked to Git for Windows: git pack-objects --path-walk. This application of the path walk API discovers the objects to pack via this batched walk, and automatically groups objects that appear at a common path so they can be checked for delta comparisons.

This use completely avoids any name-hash collisions (even the collisions that sometimes occur with the new --full-name-hash option) and can be much faster to compute since the first pass of delta calculations does not waste time on objects that are unlikely to be diffable.

Some statistics are available in the commit messages.

[dscho]

Wow, what an incredible amount of work!

I would like to work on a couple of rough edges before integrating this, no major work needed, therefore we should be able to get this integrated today, in time for tomorrow's -rc0.

[derrickstolee]

Range-diff for latest push:

 1:  8b6ba9390c !  1:  bd4446ef3b path-walk: introduce an object walk by path
    @@ Documentation/technical/api-path-walk.txt (new)
     +paths is used to visit the object.
     +
     +When walking a range of commits with some `UNINTERESTING` objects, the
    -+objects with the `UNINTERESTING` flag are included in these batches.
    ++objects with the `UNINTERESTING` flag are included in these batches. In
    ++order to walk `UNINTERESTING` objects, the `--boundary` option must be
    ++used in the commit walk in order to visit `UNINTERESTING` commits.
     +
     +Basics
     +------
    @@ path-walk.c (new)
     +
     +/*
     + * For each path in paths_to_explore, walk the trees another level
    -+ * and add any found blobs to the batch (but only if they don't
    -+ * exist and haven't been added yet).
    ++ * and add any found blobs to the batch (but only if they exist and
    ++ * haven't been added yet).
     + */
     +static int walk_path(struct path_walk_context *ctx,
     +		     const char *path)
 2:  591438e6dc !  2:  4d1d9ae8df t6601: add helper for testing path-walk API
    @@ t/helper/test-path-walk.c (new)
     +#include "oid-array.h"
     +
     +struct path_walk_test_data {
    -+	uint32_t tree_nr;
    -+	uint32_t blob_nr;
    ++	size_t tree_nr;
    ++	size_t blob_nr;
     +};
     +
     +static int emit_block(const char *path, struct oid_array *oids,
    @@ t/helper/test-path-walk.c (new)
     +
     +	res = walk_objects_by_path(&info);
     +
    -+	printf("trees:%d\nblobs:%d\n",
    ++	printf("trees:%" PRIuMAX "\n"
    ++	       "blobs:%" PRIuMAX "\n",
     +	       data.tree_nr, data.blob_nr);
     +
     +	return res;
 3:  f2c5f20045 !  3:  9aad76f169 path-walk: allow consumer to specify object types
    @@ Metadata
      ## Commit message ##
         path-walk: allow consumer to specify object types
     
    -    This adds the ability to ask for the commits as a single list. This will
    -    also reduce the calls in 'git backfill' to be a BUG() statement if called
    -    with anything other than blobs.
    +    We add the ability to filter the object types in the path-walk API so
    +    the callback function is called fewer times.
    +
    +    This adds the ability to ask for the commits in a list, as well. Future
    +    changes will add the ability to visit annotated tags.
     
         Signed-off-by: Derrick Stolee <stolee@gmail.com>
     
    @@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
      		if (t)
      			oid_array_append(&root_tree_list->oids, oid);
      		else
    - 			warning("could not find tree %s", oid_to_hex(oid));
    -+
    - 	}
    - 
    +@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
      	trace2_data_intmax("path-walk", ctx.repo, "commits", commits_nr);
      	trace2_region_leave("path-walk", "commit-walk", info->revs->repo);
      
    @@ t/helper/test-path-walk.c
      #include "oid-array.h"
      
      struct path_walk_test_data {
    -+	uint32_t commit_nr;
    - 	uint32_t tree_nr;
    - 	uint32_t blob_nr;
    ++	size_t commit_nr;
    + 	size_t tree_nr;
    + 	size_t blob_nr;
      };
     @@ t/helper/test-path-walk.c: static int emit_block(const char *path, struct oid_array *oids,
      	const char *typestr;
    @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
      
      	res = walk_objects_by_path(&info);
      
    --	printf("trees:%d\nblobs:%d\n",
    +-	printf("trees:%" PRIuMAX "\n"
    ++	printf("commits:%" PRIuMAX "\n"
    ++	       "trees:%" PRIuMAX "\n"
    + 	       "blobs:%" PRIuMAX "\n",
     -	       data.tree_nr, data.blob_nr);
    -+	printf("commits:%d\ntrees:%d\nblobs:%d\n",
     +	       data.commit_nr, data.tree_nr, data.blob_nr);
      
      	return res;
 4:  7521576f8c !  4:  0851c92c55 path-walk: allow visiting tags
    @@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
     +		info->path_fn("", &tags, OBJ_TAG, info->path_fn_data);
     +
     +		if (tagged_blob_list.nr && info->blobs)
    -+			info->path_fn("tagged-blobs", &tagged_blob_list, OBJ_BLOB,
    ++			info->path_fn("/tagged-blobs", &tagged_blob_list, OBJ_BLOB,
     +				      info->path_fn_data);
     +
     +		trace2_data_intmax("path-walk", ctx.repo, "tags", tags.nr);
    @@ path-walk.h: struct path_walk_info {
     
      ## t/helper/test-path-walk.c ##
     @@ t/helper/test-path-walk.c: struct path_walk_test_data {
    - 	uint32_t commit_nr;
    - 	uint32_t tree_nr;
    - 	uint32_t blob_nr;
    -+	uint32_t tag_nr;
    + 	size_t commit_nr;
    + 	size_t tree_nr;
    + 	size_t blob_nr;
    ++	size_t tag_nr;
      };
      
      static int emit_block(const char *path, struct oid_array *oids,
    @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
      	}
     @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
      
    - 	res = walk_objects_by_path(&info);
    - 
    --	printf("commits:%d\ntrees:%d\nblobs:%d\n",
    + 	printf("commits:%" PRIuMAX "\n"
    + 	       "trees:%" PRIuMAX "\n"
    +-	       "blobs:%" PRIuMAX "\n",
     -	       data.commit_nr, data.tree_nr, data.blob_nr);
    -+	printf("commits:%d\ntrees:%d\nblobs:%d\ntags:%d\n",
    ++	       "blobs:%" PRIuMAX "\n"
    ++	       "tags:%" PRIuMAX "\n",
     +	       data.commit_nr, data.tree_nr, data.blob_nr, data.tag_nr);
      
      	return res;
    @@ t/t6601-path-walk.sh: test_expect_success 'all' '
      	BLOB:right/c:$(git rev-parse topic:right/c)
      	BLOB:right/d:$(git rev-parse base~1:right/d)
     -	blobs:6
    -+	BLOB:tagged-blobs:$(git rev-parse refs/tags/blob-tag^{})
    -+	BLOB:tagged-blobs:$(git rev-parse refs/tags/blob-tag2^{})
    ++	BLOB:/tagged-blobs:$(git rev-parse refs/tags/blob-tag^{})
    ++	BLOB:/tagged-blobs:$(git rev-parse refs/tags/blob-tag2^{})
     +	BLOB:child/file:$(git rev-parse refs/tags/tree-tag^{}:child/file)
     +	blobs:10
     +	TAG::$(git rev-parse refs/tags/first)
 5:  d1af7aa423 =  5:  9ecc60d738 revision: create mark_trees_uninteresting_dense()
 6:  1571d7b177 !  6:  223b050ede path-walk: add prune_all_uninteresting option
    @@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
     +		} else {
      			warning("could not find tree %s", oid_to_hex(oid));
     +		}
    - 
    ++
     +		if (t && (c->object.flags & UNINTERESTING)) {
     +			t->object.flags |= UNINTERESTING;
     +			has_uninteresting = 1;
    @@ t/helper/test-path-walk.c: static int emit_block(const char *path, struct oid_ar
     +	for (size_t i = 0; i < oids->nr; i++) {
     +		struct object *o = lookup_unknown_object(the_repository,
     +							 &oids->oid[i]);
    -+		printf("%s:%s:%s", typestr, path, oid_to_hex(&oids->oid[i]));
    -+
    -+		if (o->flags & UNINTERESTING)
    -+			printf(":UNINTERESTING");
    -+		printf("\n");
    ++		printf("%s:%s:%s%s\n", typestr, path, oid_to_hex(&oids->oid[i]),
    ++		       o->flags & UNINTERESTING ? ":UNINTERESTING" : "");
     +	}
      
      	return 0;
 7:  e4d3374ec6 =  7:  787124f2ed pack-objects: extract should_attempt_deltas()
 8:  d10ebfd55a !  8:  775278f7b2 pack-objects: add --path-walk option
    @@ Commit message
     
         Signed-off-by: Derrick Stolee <stolee@gmail.com>
     
    + ## Documentation/git-pack-objects.txt ##
    +@@ Documentation/git-pack-objects.txt: SYNOPSIS
    + 	[--cruft] [--cruft-expiration=<time>]
    + 	[--stdout [--filter=<filter-spec>] | <base-name>]
    + 	[--shallow] [--keep-true-parents] [--[no-]sparse]
    +-	[--full-name-hash] < <object-list>
    ++	[--full-name-hash] [--path-walk] < <object-list>
    + 
    + 
    + DESCRIPTION
    +@@ Documentation/git-pack-objects.txt: raise an error.
    + 	Restrict delta matches based on "islands". See DELTA ISLANDS
    + 	below.
    + 
    ++--path-walk::
    ++	By default, `git pack-objects` walks objects in an order that
    ++	presents trees and blobs in an order unrelated to the path they
    ++	appear relative to a commit's root tree. The `--path-walk` option
    ++	enables a different walking algorithm that organizes trees and
    ++	blobs by path. This has the potential to improve delta compression
    ++	especially in the presence of filenames that cause collisions in
    ++	Git's default name-hash algorithm. Due to changing how the objects
    ++	are walked, this option is not compatible with `--delta-islands`,
    ++	`--shallow`, or `--filter`.
    + 
    + DELTA ISLANDS
    + -------------
    +
      ## Documentation/technical/api-path-walk.txt ##
     @@ Documentation/technical/api-path-walk.txt: Examples
      --------
    @@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)
     +static inline int is_oid_interesting(struct repository *repo,
     +				     struct object_id *oid)
     +{
    -+	struct object *o = lookup_unknown_object(repo, oid);
    ++	struct object *o = lookup_object(repo, oid);
     +	return o && !(o->flags & UNINTERESTING);
     +}
     +
 9:  095a10dbb0 =  9:  62a8358c73 pack-objects: introduce GIT_TEST_PACK_PATH_WALK
10:  b854dfcc52 ! 10:  de63133baa repack: add --path-walk option
    @@ Documentation/git-repack.txt: SYNOPSIS
      
      DESCRIPTION
      -----------
    +@@ Documentation/git-repack.txt: linkgit:git-multi-pack-index[1]).
    + 	Write a multi-pack index (see linkgit:git-multi-pack-index[1])
    + 	containing the non-redundant packs.
    + 
    ++--path-walk::
    ++	This option passes the `--path-walk` option to the underlying
    ++	`git pack-options` process (see linkgit:git-pack-objects[1]).
    ++	By default, `git pack-objects` walks objects in an order that
    ++	presents trees and blobs in an order unrelated to the path they
    ++	appear relative to a commit's root tree. The `--path-walk` option
    ++	enables a different walking algorithm that organizes trees and
    ++	blobs by path. This has the potential to improve delta compression
    ++	especially in the presence of filenames that cause collisions in
    ++	Git's default name-hash algorithm. Due to changing how the objects
    ++	are walked, this option is not compatible with `--delta-islands`
    ++	or `--filter`.
    ++
    + CONFIGURATION
    + -------------
    + 
     
      ## builtin/repack.c ##
     @@ builtin/repack.c: static char *packdir, *packtmp_name, *packtmp;
11:  3d42b0adc6 ! 11:  2be08bb37b pack-objects: enable --path-walk via config
    @@ Commit message
     
         Signed-off-by: Derrick Stolee <stolee@gmail.com>
     
    + ## Documentation/config/feature.txt ##
    +@@ Documentation/config/feature.txt: walking fewer objects.
    + +
    + * `pack.allowPackReuse=multi` may improve the time it takes to create a pack by
    + reusing objects from multiple packs instead of just one.
    +++
    ++* `pack.usePathWalk` may speed up packfile creation and make the packfiles be
    ++significantly smaller in the presence of certain filename collisions with Git's
    ++default name-hash.
    + 
    + feature.manyFiles::
    + 	Enable config options that optimize for repos with many files in the
    +
      ## Documentation/config/pack.txt ##
     @@ Documentation/config/pack.txt: pack.useSparse::
      	commits contain certain types of direct renames. Default is
12:  af3c37b26b = 12:  e0a1e66426 scalar: enable path-walk during push via config
13:  8084dd2024 = 13:  0238b0e4d0 pack-objects: refactor path-walk delta phase
14:  e91c3b9394 = 14:  946bd8f35c pack-objects: thread the path-based compression

[derrickstolee]

My previous push forgot to add the parse-opts to test-tool path-walk, so I've done so in this push.

Range-diff since last push

 1:  bd4446ef3b =  1:  bd4446ef3b path-walk: introduce an object walk by path
 2:  4d1d9ae8df !  2:  30c651a8c5 t6601: add helper for testing path-walk API
    @@ t/helper/test-path-walk.c (new)
     +#include "pretty.h"
     +#include "revision.h"
     +#include "setup.h"
    ++#include "parse-options.h"
     +#include "path-walk.h"
     +#include "oid-array.h"
     +
    ++static const char * const path_walk_usage[] = {
    ++	N_("test-tool path-walk <options> -- <revision-options>"),
    ++	NULL
    ++};
    ++
     +struct path_walk_test_data {
     +	size_t tree_nr;
     +	size_t blob_nr;
    @@ t/helper/test-path-walk.c (new)
     +
     +int cmd__path_walk(int argc, const char **argv)
     +{
    -+	int argi, res;
    ++	int res;
     +	struct rev_info revs = REV_INFO_INIT;
     +	struct path_walk_info info = PATH_WALK_INFO_INIT;
     +	struct path_walk_test_data data = { 0 };
    ++	struct option options[] = {
    ++		OPT_END(),
    ++	};
     +
     +	initialize_repository(the_repository);
     +	setup_git_directory();
     +	revs.repo = the_repository;
     +
    -+	for (argi = 0; argi < argc; argi++) {
    -+		if (!strcmp(argv[argi], "--"))
    -+			break;
    -+	}
    ++	argc = parse_options(argc, argv, NULL,
    ++			     options, path_walk_usage,
    ++			     PARSE_OPT_KEEP_UNKNOWN_OPT | PARSE_OPT_KEEP_ARGV0);
     +
    -+	if (argi < argc)
    -+		setup_revisions(argc - argi, argv + argi, &revs, NULL);
    ++	if (argc > 1)
    ++		setup_revisions(argc, argv, &revs, NULL);
     +	else
    -+		die("usage: test-tool path-walk <options> -- <rev opts>");
    ++		usage(path_walk_usage[0]);
     +
     +	info.revs = &revs;
     +	info.path_fn = emit_block;
 3:  9aad76f169 !  3:  a0d51fb05d path-walk: allow consumer to specify object types
    @@ path-walk.h: struct path_walk_info {
       * Given the configuration of 'info', walk the commits based on 'info->revs' and
     
      ## t/helper/test-path-walk.c ##
    -@@
    - #include "oid-array.h"
    +@@ t/helper/test-path-walk.c: static const char * const path_walk_usage[] = {
    + };
      
      struct path_walk_test_data {
     +	size_t commit_nr;
    @@ t/helper/test-path-walk.c: static int emit_block(const char *path, struct oid_ar
      		typestr = "TREE";
      		tdata->tree_nr += oids->nr;
     @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
    - 	revs.repo = the_repository;
    + 	struct path_walk_info info = PATH_WALK_INFO_INIT;
    + 	struct path_walk_test_data data = { 0 };
    + 	struct option options[] = {
    ++		OPT_BOOL(0, "blobs", &info.blobs,
    ++			 N_("toggle inclusion of blob objects")),
    ++		OPT_BOOL(0, "commits", &info.commits,
    ++			 N_("toggle inclusion of commit objects")),
    ++		OPT_BOOL(0, "trees", &info.trees,
    ++			 N_("toggle inclusion of tree objects")),
    + 		OPT_END(),
    + 	};
      
    - 	for (argi = 0; argi < argc; argi++) {
    -+		if (!strcmp(argv[argi], "--no-blobs"))
    -+			info.blobs = 0;
    -+		if (!strcmp(argv[argi], "--no-trees"))
    -+			info.trees = 0;
    -+		if (!strcmp(argv[argi], "--no-commits"))
    -+			info.commits = 0;
    - 		if (!strcmp(argv[argi], "--"))
    - 			break;
    - 	}
     @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
      
      	res = walk_objects_by_path(&info);
 4:  0851c92c55 !  4:  fd1addc05c path-walk: allow visiting tags
    @@ t/helper/test-path-walk.c: static int emit_block(const char *path, struct oid_ar
      		BUG("we do not understand this type");
      	}
     @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
    - 			info.trees = 0;
    - 		if (!strcmp(argv[argi], "--no-commits"))
    - 			info.commits = 0;
    -+		if (!strcmp(argv[argi], "--no-tags"))
    -+			info.tags = 0;
    - 		if (!strcmp(argv[argi], "--"))
    - 			break;
    - 	}
    + 			 N_("toggle inclusion of blob objects")),
    + 		OPT_BOOL(0, "commits", &info.commits,
    + 			 N_("toggle inclusion of commit objects")),
    ++		OPT_BOOL(0, "tags", &info.tags,
    ++			 N_("toggle inclusion of tag objects")),
    + 		OPT_BOOL(0, "trees", &info.trees,
    + 			 N_("toggle inclusion of tree objects")),
    + 		OPT_END(),
     @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
      
      	printf("commits:%" PRIuMAX "\n"
 5:  9ecc60d738 =  5:  5de5815c92 revision: create mark_trees_uninteresting_dense()
 6:  223b050ede !  6:  d835e3b218 path-walk: add prune_all_uninteresting option
    @@ t/helper/test-path-walk.c: static int emit_block(const char *path, struct oid_ar
      	return 0;
      }
     @@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
    - 			info.commits = 0;
    - 		if (!strcmp(argv[argi], "--no-tags"))
    - 			info.tags = 0;
    -+		if (!strcmp(argv[argi], "--prune"))
    -+			info.prune_all_uninteresting = 1;
    - 		if (!strcmp(argv[argi], "--"))
    - 			break;
    - 	}
    + 			 N_("toggle inclusion of tag objects")),
    + 		OPT_BOOL(0, "trees", &info.trees,
    + 			 N_("toggle inclusion of tree objects")),
    ++		OPT_BOOL(0, "prune", &info.prune_all_uninteresting,
    ++			 N_("toggle pruning of uninteresting paths")),
    + 		OPT_END(),
    + 	};
    + 
     
      ## t/t6601-path-walk.sh ##
     @@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
 7:  787124f2ed =  7:  48761b8008 pack-objects: extract should_attempt_deltas()
 8:  775278f7b2 =  8:  fd41482cbc pack-objects: add --path-walk option
 9:  62a8358c73 =  9:  c82afd67e3 pack-objects: introduce GIT_TEST_PACK_PATH_WALK
10:  de63133baa = 10:  bfa052fcf1 repack: add --path-walk option
11:  2be08bb37b = 11:  6d22afb7c7 pack-objects: enable --path-walk via config
12:  e0a1e66426 = 12:  54de70e074 scalar: enable path-walk during push via config
13:  0238b0e4d0 = 13:  213f9bd986 pack-objects: refactor path-walk delta phase
14:  946bd8f35c = 14:  4b68c8e6ad pack-objects: thread the path-based compression

[derrickstolee]

32-bit Linux complained about size_t and PRIuMAX so I changed them to uintmax_t in the test helper.

[dscho]

32-bit Linux complained about size_t and PRIuMAX so I changed them to uintmax_t in the test helper.

Good point!

[dscho]

Thank you for indulging me, all of my concerns are addressed!

[melroy89]

Can/will this pr be created upstream as well into git?

[dscho]

Can/will this pr be created upstream as well into git?

@melroy89 Already here: gitgitgadget#1813