PATH WALK I: The path-walk API #1818
Open
+1,165
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
commented
Oct 29, 2024
derrickstolee
commented
Oct 29, 2024
derrickstolee
force-pushed
the
api-upstream
branch
2 times, most recently
from
5252076
to
0bb607e
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 10/31/24 2:26 AM, Derrick Stolee via GitGitGadget wrote:
> This is a new series that rerolls the initial "path-walk API" patches of my
> RFC [1] "Path-walk API and applications". This new API (in path-walk.c and
> path-walk.h) presents a new way to walk objects such that trees and blobs
> are walked in batches according to their path.
> > This also replaces the previous version of ds/path-walk that was being
> reviewed in [2]. The consensus was that the series was too long/dense and
> could use some reduction in size. This series takes the first few patches,
> but also makes some updates (which will be described later).
> > [1]
> https://lore.kernel.org/git/[email protected]/
> [RFC] Path-walk API and applications
> > [2]
> https://lore.kernel.org/git/[email protected]/
> [PATCH v2 00/17] pack-objects: add --path-walk option for better deltas
...
> I will include a full range diff relative to the previous versions of these
> patches in [2] in a reply to this cover letter.
Here is the promised range-diff:
1: 98bdc94a773 ! 1: c71f0a0e361 path-walk: introduce an object walk by path
@@ Commit message
In anticipation of a few planned applications, introduce the most basic form
of a path-walk API. It currently assumes that there are no UNINTERESTING
- objects and does not include any complicated filters. It calls a function
+ objects, and does not include any complicated filters. It calls a function
pointer on groups of tree and blob objects as grouped by path. This only
includes objects the first time they are discovered, so an object that
appears at multiple paths will not be included in two batches.
+ These batches are collected in 'struct type_and_oid_list' objects, which
+ store an object type and an oid_array of objects.
+
+ The data structures are documented in 'struct path_walk_context', but in
+ summary the most important are:
+
+ * 'paths_to_lists' is a strmap that connects a path to a
+ type_and_oid_list for that path. To avoid conflicts in path names,
+ we make sure that tree paths end in "/" (except the root path with
+ is an empty string) and blob paths do not end in "/".
+
+ * 'path_stack' is a string list that is added to in an append-only
+ way. This stores the stack of our depth-first search on the heap
+ instead of using recursion.
+
+ * 'path_stack_pushed' is a strmap that stores path names that were
+ already added to 'path_stack', to avoid repeating paths in the
+ stack. Mostly, this saves us from quadratic lookups from doing
+ unsorted checks into the string_list.
+
+ The coupling of 'path_stack' and 'path_stack_pushed' is protected by the
+ push_to_stack() method. Call this instead of inserting into these
+ structures directly.
+
+ The walk_objects_by_path() method initializes these structures and
+ starts walking commits from the given rev_info struct. The commits are
+ used to find the list of root trees which populate the start of our
+ depth-first search.
+
+ The core of our depth-first search is in a while loop that continues
+ while we have not indicated an early exit and our 'path_stack' still has
+ entries in it. The loop body pops a path off of the stack and "visits"
+ the path via the walk_path() method.
+
+ The walk_path() method gets the list of OIDs from the 'path_to_lists'
+ strmap and executes the callback method on that list with the given path
+ and type. If the OIDs correspond to tree objects, then iterate over all
+ trees in the list and run add_children() to add the child objects to
+ their own lists, adding new entries to the stack if necessary.
+
+ In testing, this depth-first search approach was the one that used the
+ least memory while iterating over the object lists. There is still a
+ chance that repositories with too-wide path patterns could cause memory
+ pressure issues. Limiting the stack size could be done in the future by
+ limiting how many objects are being considered in-progress, or by
+ visiting blob paths earlier than trees.
+
There are many future adaptations that could be made, but they are left for
future updates when consumers are ready to take advantage of those features.
@@ Documentation/technical/api-path-walk.txt (new)
+multiple paths possible to reach the same object, then only one of those
+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. In
-+order to walk `UNINTERESTING` objects, the `--boundary` option must be
-+used in the commit walk in order to visit `UNINTERESTING` commits.
-+
+Basics
+------
+
@@ Documentation/technical/api-path-walk.txt (new)
+`revs` struct. The revision walk should only be used to walk commits, and
+the objects will be walked in a separate way based on those starting
+commits.
-++
-+If you want the path-walk API to emit `UNINTERESTING` objects based on the
-+commit walk's boundary, be sure to set `revs.boundary` so the boundary
-+commits are emitted.
+
+Examples
+--------
@@ path-walk.c (new)
+ /**
+ * Store the current list of paths in a stack, to
+ * facilitate depth-first-search without recursion.
++ *
++ * Use path_stack_pushed to indicate whether a path
++ * was previously added to path_stack.
+ */
+ struct string_list path_stack;
++ struct strset path_stack_pushed;
+};
+
++static void push_to_stack(struct path_walk_context *ctx,
++ const char *path)
++{
++ if (strset_contains(&ctx->path_stack_pushed, path))
++ return;
++
++ strset_add(&ctx->path_stack_pushed, path);
++ string_list_append(&ctx->path_stack, path);
++}
++
+static int add_children(struct path_walk_context *ctx,
+ const char *base_path,
+ struct object_id *oid)
@@ path-walk.c (new)
+ if (!o) /* report error?*/
+ continue;
+
-+ /* Skip this object if already seen. */
-+ if (o->flags & SEEN)
-+ continue;
-+ o->flags |= SEEN;
-+
+ strbuf_setlen(&path, base_len);
+ strbuf_add(&path, entry.path, entry.pathlen);
+
@@ path-walk.c (new)
+ CALLOC_ARRAY(list, 1);
+ list->type = type;
+ strmap_put(&ctx->paths_to_lists, path.buf, list);
-+ string_list_append(&ctx->path_stack, path.buf);
+ }
++ push_to_stack(ctx, path.buf);
++
++ /* Skip this object if already seen. */
++ if (o->flags & SEEN)
++ continue;
++ o->flags |= SEEN;
+ oid_array_append(&list->oids, &entry.oid);
+ }
+
@@ path-walk.c (new)
+ .revs = info->revs,
+ .info = info,
+ .path_stack = STRING_LIST_INIT_DUP,
++ .path_stack_pushed = STRSET_INIT,
+ .paths_to_lists = STRMAP_INIT
+ };
+
@@ path-walk.c (new)
+ CALLOC_ARRAY(root_tree_list, 1);
+ root_tree_list->type = OBJ_TREE;
+ strmap_put(&ctx.paths_to_lists, root_path, root_tree_list);
++ push_to_stack(&ctx, root_path);
+
+ if (prepare_revision_walk(info->revs))
+ die(_("failed to setup revision walk"));
+
+ while ((c = get_revision(info->revs))) {
+ struct object_id *oid = get_commit_tree_oid(c);
-+ struct tree *t = lookup_tree(info->revs->repo, oid);
++ struct tree *t;
+ commits_nr++;
+
-+ if (t) {
-+ if (t->object.flags & SEEN)
-+ continue;
-+ t->object.flags |= SEEN;
-+ oid_array_append(&root_tree_list->oids, oid);
-+ } else {
++ oid = get_commit_tree_oid(c);
++ t = lookup_tree(info->revs->repo, oid);
++
++ if (!t) {
+ warning("could not find tree %s", oid_to_hex(oid));
++ continue;
+ }
++
++ if (t->object.flags & SEEN)
++ continue;
++ t->object.flags |= SEEN;
++ oid_array_append(&root_tree_list->oids, oid);
+ }
+
+ trace2_data_intmax("path-walk", ctx.repo, "commits", commits_nr);
+ trace2_region_leave("path-walk", "commit-walk", info->revs->repo);
+
-+ string_list_append(&ctx.path_stack, root_path);
-+
+ trace2_region_enter("path-walk", "path-walk", info->revs->repo);
+ while (!ret && ctx.path_stack.nr) {
+ char *path = ctx.path_stack.items[ctx.path_stack.nr - 1].string;
@@ path-walk.c (new)
+ trace2_region_leave("path-walk", "path-walk", info->revs->repo);
+
+ clear_strmap(&ctx.paths_to_lists);
++ strset_clear(&ctx.path_stack_pushed);
+ string_list_clear(&ctx.path_stack, 0);
+ return ret;
+}
5: 6e89fb219b5 ! 2: 4f9f898fec1 revision: create mark_trees_uninteresting_dense()
@@ Metadata
Author: Derrick Stolee <[email protected]>
## Commit message ##
- revision: create mark_trees_uninteresting_dense()
+ test-lib-functions: add test_cmp_sorted
- The sparse tree walk algorithm was created in d5d2e93577e (revision:
- implement sparse algorithm, 2019-01-16) and involves using the
- mark_trees_uninteresting_sparse() method. This method takes a repository
- and an oidset of tree IDs, some of which have the UNINTERESTING flag and
- some of which do not.
-
- Create a method that has an equivalent set of preconditions but uses a
- "dense" walk (recursively visits all reachable trees, as long as they
- have not previously been marked UNINTERESTING). This is an important
- difference from mark_tree_uninteresting(), which short-circuits if the
- given tree has the UNINTERESTING flag.
-
- A use of this method will be added in a later change, with a condition
- set whether the sparse or dense approach should be used.
+ This test helper will be helpful to reduce repeated logic in
+ t6601-path-walk.sh, but may be helpful elsewhere, too.
Signed-off-by: Derrick Stolee <[email protected]>
- ## revision.c ##
-@@ revision.c: static void add_children_by_path(struct repository *r,
- free_tree_buffer(tree);
+ ## t/test-lib-functions.sh ##
+@@ t/test-lib-functions.sh: test_cmp () {
+ eval "$GIT_TEST_CMP" '"$@"'
}
-+void mark_trees_uninteresting_dense(struct repository *r,
-+ struct oidset *trees)
-+{
-+ struct object_id *oid;
-+ struct oidset_iter iter;
-+
-+ oidset_iter_init(trees, &iter);
-+ while ((oid = oidset_iter_next(&iter))) {
-+ struct tree *tree = lookup_tree(r, oid);
++# test_cmp_sorted runs test_cmp on sorted versions of the two
++# input files. Uses "$1.sorted" and "$2.sorted" as temp files.
+
-+ if (tree && (tree->object.flags & UNINTERESTING))
-+ mark_tree_contents_uninteresting(r, tree);
-+ }
++test_cmp_sorted () {
++ sort <"$1" >"$1.sorted" &&
++ sort <"$2" >"$2.sorted" &&
++ test_cmp "$1.sorted" "$2.sorted" &&
++ rm "$1.sorted" "$2.sorted"
+}
+
- void mark_trees_uninteresting_sparse(struct repository *r,
- struct oidset *trees)
- {
-
- ## revision.h ##
-@@ revision.h: void put_revision_mark(const struct rev_info *revs,
-
- void mark_parents_uninteresting(struct rev_info *revs, struct commit *commit);
- void mark_tree_uninteresting(struct repository *r, struct tree *tree);
-+void mark_trees_uninteresting_dense(struct repository *r, struct oidset *trees);
- void mark_trees_uninteresting_sparse(struct repository *r, struct oidset *trees);
-
- void show_object_with_name(FILE *, struct object *, const char *);
+ # Check that the given config key has the expected value.
+ #
+ # test_cmp_config [-C <dir>] <expected-value>
2: a00ab0c62c9 ! 3: 6f93dff88e7 t6601: add helper for testing path-walk API
@@ Commit message
sets a baseline for the behavior and we can extend it as new options are
introduced.
+ It is important to mention that the behavior of the API will change soon as
+ we start to handle UNINTERESTING objects differently, but these tests will
+ demonstrate the change in behavior.
+
Signed-off-by: Derrick Stolee <[email protected]>
## Documentation/technical/api-path-walk.txt ##
-@@ Documentation/technical/api-path-walk.txt: commits are emitted.
+@@ Documentation/technical/api-path-walk.txt: commits.
Examples
--------
@@ t/t6601-path-walk.sh (new)
+ blobs:6
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
+test_expect_success 'topic only' '
@@ t/t6601-path-walk.sh (new)
+ blobs:5
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
+test_expect_success 'topic, not base' '
@@ t/t6601-path-walk.sh (new)
+ blobs:4
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
+test_expect_success 'topic, not base, boundary' '
@@ t/t6601-path-walk.sh (new)
+ blobs:5
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
+test_done
3: 14375d19392 ! 4: f4bf8be30b5 path-walk: allow consumer to specify object types
@@ Commit message
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.
+ This adds the ability to ask for the commits in a list, as well. We
+ re-use the empty string for this set of objects because these are passed
+ directly to the callback function instead of being part of the
+ 'path_stack'.
+
+ Future changes will add the ability to visit annotated tags.
Signed-off-by: Derrick Stolee <[email protected]>
## Documentation/technical/api-path-walk.txt ##
-@@ Documentation/technical/api-path-walk.txt: If you want the path-walk API to emit `UNINTERESTING` objects based on the
- commit walk's boundary, be sure to set `revs.boundary` so the boundary
- commits are emitted.
+@@ Documentation/technical/api-path-walk.txt: It is also important that you do not specify the `--objects` flag for the
+ the objects will be walked in a separate way based on those starting
+ commits.
+`commits`, `blobs`, `trees`::
+ By default, these members are enabled and signal that the path-walk
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
/* Insert a single list for the root tree into the paths. */
CALLOC_ARRAY(root_tree_list, 1);
root_tree_list->type = OBJ_TREE;
- strmap_put(&ctx.paths_to_lists, root_path, root_tree_list);
--
- if (prepare_revision_walk(info->revs))
+@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
die(_("failed to setup revision walk"));
while ((c = get_revision(info->revs))) {
- struct object_id *oid = get_commit_tree_oid(c);
-- struct tree *t = lookup_tree(info->revs->repo, oid);
+ struct object_id *oid;
-+ struct tree *t;
+ struct tree *t;
commits_nr++;
+ if (info->commits)
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
+ if (!info->trees && !info->blobs)
+ continue;
+
-+ oid = get_commit_tree_oid(c);
-+ t = lookup_tree(info->revs->repo, oid);
-+
- if (t) {
- if (t->object.flags & SEEN)
- continue;
+ oid = get_commit_tree_oid(c);
+ t = lookup_tree(info->revs->repo, 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);
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
+ oid_array_clear(&commit_list->oids);
+ free(commit_list);
+
- string_list_append(&ctx.path_stack, root_path);
-
trace2_region_enter("path-walk", "path-walk", info->revs->repo);
+ while (!ret && ctx.path_stack.nr) {
+ char *path = ctx.path_stack.items[ctx.path_stack.nr - 1].string;
## path-walk.h ##
@@ path-walk.h: struct path_walk_info {
*/
path_fn path_fn;
void *path_fn_data;
++
+ /**
+ * Initialize which object types the path_fn should be called on. This
+ * could also limit the walk to skip blobs if not set.
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
TREE:left/:$(git rev-parse topic:left)
TREE:right/:$(git rev-parse topic:right)
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
- test_cmp expect.sorted out.sorted
+ test_cmp_sorted expect out
'
+test_expect_success 'topic, not base, only blobs' '
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
+ blobs:4
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
+# No, this doesn't make a lot of sense for the path-walk API,
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
+ blobs:0
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
+test_expect_success 'topic, not base, only trees' '
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
+ blobs:0
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
++ test_cmp_sorted expect out
+'
+
test_expect_success 'topic, not base, boundary' '
4: c321f58c62d ! 5: dfd00b2bf0c path-walk: allow visiting tags
@@ Metadata
Author: Derrick Stolee <[email protected]>
## Commit message ##
- path-walk: allow visiting tags
+ path-walk: visit tags and cached objects
- In anticipation of using the path-walk API to analyze tags or include
- them in a pack-file, add the ability to walk the tags that were included
- in the revision walk.
+ The rev_info that is specified for a path-walk traversal may specify
+ visiting tag refs (both lightweight and annotated) and also may specify
+ indexed objects (blobs and trees). Update the path-walk API to walk
+ these objects as well.
- When these tag objects point to blobs or trees, we need to make sure
- those objects are also visited. Treat tagged trees as root trees, but
- put the tagged blobs in their own category.
+ When walking tags, we need to peel the annotated objects until reaching
+ a non-tag object. If we reach a commit, then we can add it to the
+ pending objects to make sure we visit in the commit walk portion. If we
+ reach a tree, then we will assume that it is a root tree. If we reach a
+ blob, then we have no good path name and so add it to a new list of
+ "tagged blobs".
- Be careful about objects that are referred to by multiple references.
+ When the rev_info includes the "--indexed-objects" flag, then the
+ pending set includes blobs and trees found in the cache entries and
+ cache-tree. The cache entries are usually blobs, though they could be
+ trees in the case of a sparse index. The cache-tree stores
+ previously-hashed tree objects but these are cleared out when staging
+ objects below those paths. We add tests that demonstrate this.
+
+ The indexed objects come with a non-NULL 'path' value in the pending
+ item. This allows us to prepopulate the 'path_to_lists' strmap with
+ lists for these paths.
+
+ The tricky thing about this walk is that we will want to combine the
+ indexed objects walk with the commit walk, especially in the future case
+ of walking objects during a command like 'git repack'.
+
+ Whenever possible, we want the objects from the index to be grouped with
+ similar objects in history. We don't want to miss any paths that appear
+ only in the index and not in the commit history.
+
+ Thus, we need to be careful to let the path stack be populated initially
+ with only the root tree path (and possibly tags and tagged blobs) and go
+ through the normal depth-first search. Afterwards, if there are other
+ paths that are remaining in the paths_to_lists strmap, we should then
+ iterate through the stack and visit those objects recursively.
- Co-authored-by: Johannes Schindelin <[email protected]>
- Signed-off-by: Johannes Schindelin <[email protected]>
Signed-off-by: Derrick Stolee <[email protected]>
## Documentation/technical/api-path-walk.txt ##
-@@ Documentation/technical/api-path-walk.txt: If you want the path-walk API to emit `UNINTERESTING` objects based on the
- commit walk's boundary, be sure to set `revs.boundary` so the boundary
- commits are emitted.
+@@ Documentation/technical/api-path-walk.txt: It is also important that you do not specify the `--objects` flag for the
+ the objects will be walked in a separate way based on those starting
+ commits.
-`commits`, `blobs`, `trees`::
+`commits`, `blobs`, `trees`, `tags`::
@@ path-walk.c
#include "trace2.h"
#include "tree.h"
#include "tree-walk.h"
+
++static const char *root_path = "";
++
+ struct type_and_oid_list
+ {
+ enum object_type type;
+@@ path-walk.c: static int walk_path(struct path_walk_context *ctx,
+
+ list = strmap_get(&ctx->paths_to_lists, path);
+
++ if (!list)
++ BUG("provided path '%s' that had no associated list", path);
++
+ /* Evaluate function pointer on this data, if requested. */
+ if ((list->type == OBJ_TREE && ctx->info->trees) ||
+- (list->type == OBJ_BLOB && ctx->info->blobs))
++ (list->type == OBJ_BLOB && ctx->info->blobs)||
++ (list->type == OBJ_TAG && ctx->info->tags))
+ ret = ctx->info->path_fn(path, &list->oids, list->type,
+ ctx->info->path_fn_data);
+
+@@ path-walk.c: static void clear_strmap(struct strmap *map)
+ strmap_init(map);
+ }
+
++static void setup_pending_objects(struct path_walk_info *info,
++ struct path_walk_context *ctx)
++{
++ struct type_and_oid_list *tags = NULL;
++ struct type_and_oid_list *tagged_blobs = NULL;
++ struct type_and_oid_list *root_tree_list = NULL;
++
++ if (info->tags)
++ CALLOC_ARRAY(tags, 1);
++ if (info->blobs)
++ CALLOC_ARRAY(tagged_blobs, 1);
++ if (info->trees)
++ root_tree_list = strmap_get(&ctx->paths_to_lists, root_path);
++
++ /*
++ * Pending objects include:
++ * * Commits at branch tips.
++ * * Annotated tags at tag tips.
++ * * Any kind of object at lightweight tag tips.
++ * * Trees and blobs in the index (with an associated path).
++ */
++ for (size_t i = 0; i < info->revs->pending.nr; i++) {
++ struct object_array_entry *pending = info->revs->pending.objects + i;
++ struct object *obj = pending->item;
++
++ /* Commits will be picked up by revision walk. */
++ if (obj->type == OBJ_COMMIT)
++ continue;
++
++ /* Navigate annotated tag object chains. */
++ while (obj->type == OBJ_TAG) {
++ struct tag *tag = lookup_tag(info->revs->repo, &obj->oid);
++ if (!tag)
++ break;
++ if (tag->object.flags & SEEN)
++ break;
++ tag->object.flags |= SEEN;
++
++ if (tags)
++ oid_array_append(&tags->oids, &obj->oid);
++ obj = tag->tagged;
++ }
++
++ if (obj->type == OBJ_TAG)
++ continue;
++
++ /* We are now at a non-tag object. */
++ if (obj->flags & SEEN)
++ continue;
++ obj->flags |= SEEN;
++
++ switch (obj->type) {
++ case OBJ_TREE:
++ if (!info->trees)
++ continue;
++ if (pending->path) {
++ struct type_and_oid_list *list;
++ char *path = *pending->path ? xstrfmt("%s/", pending->path)
++ : xstrdup("");
++ if (!(list = strmap_get(&ctx->paths_to_lists, path))) {
++ CALLOC_ARRAY(list, 1);
++ list->type = OBJ_TREE;
++ strmap_put(&ctx->paths_to_lists, path, list);
++ }
++ oid_array_append(&list->oids, &obj->oid);
++ free(path);
++ } else {
++ /* assume a root tree, such as a lightweight tag. */
++ oid_array_append(&root_tree_list->oids, &obj->oid);
++ }
++ break;
++
++ case OBJ_BLOB:
++ if (!info->blobs)
++ continue;
++ if (pending->path) {
++ struct type_and_oid_list *list;
++ char *path = pending->path;
++ if (!(list = strmap_get(&ctx->paths_to_lists, path))) {
++ CALLOC_ARRAY(list, 1);
++ list->type = OBJ_BLOB;
++ strmap_put(&ctx->paths_to_lists, path, list);
++ }
++ oid_array_append(&list->oids, &obj->oid);
++ } else {
++ /* assume a root tree, such as a lightweight tag. */
++ oid_array_append(&tagged_blobs->oids, &obj->oid);
++ }
++ break;
++
++ case OBJ_COMMIT:
++ /* Make sure it is in the object walk */
++ if (obj != pending->item)
++ add_pending_object(info->revs, obj, "");
++ break;
++
++ default:
++ BUG("should not see any other type here");
++ }
++ }
++
++ /*
++ * Add tag objects and tagged blobs if they exist.
++ */
++ if (tagged_blobs) {
++ if (tagged_blobs->oids.nr) {
++ const char *tagged_blob_path = "/tagged-blobs";
++ tagged_blobs->type = OBJ_BLOB;
++ push_to_stack(ctx, tagged_blob_path);
++ strmap_put(&ctx->paths_to_lists, tagged_blob_path, tagged_blobs);
++ } else {
++ oid_array_clear(&tagged_blobs->oids);
++ free(tagged_blobs);
++ }
++ }
++ if (tags) {
++ if (tags->oids.nr) {
++ const char *tag_path = "/tags";
++ tags->type = OBJ_TAG;
++ push_to_stack(ctx, tag_path);
++ strmap_put(&ctx->paths_to_lists, tag_path, tags);
++ } else {
++ oid_array_clear(&tags->oids);
++ free(tags);
++ }
++ }
++}
++
+ /**
+ * Given the configuration of 'info', walk the commits based on 'info->revs' and
+ * call 'info->path_fn' on each discovered path.
+@@ path-walk.c: static void clear_strmap(struct strmap *map)
+ */
+ int walk_objects_by_path(struct path_walk_info *info)
+ {
+- const char *root_path = "";
+ int ret = 0;
+ size_t commits_nr = 0, paths_nr = 0;
+ struct commit *c;
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
CALLOC_ARRAY(commit_list, 1);
commit_list->type = OBJ_COMMIT;
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
CALLOC_ARRAY(root_tree_list, 1);
root_tree_list->type = OBJ_TREE;
strmap_put(&ctx.paths_to_lists, root_path, root_tree_list);
-+
+ push_to_stack(&ctx, root_path);
+
+ /*
+ * Set these values before preparing the walk to catch
-+ * lightweight tags pointing to non-commits.
++ * lightweight tags pointing to non-commits and indexed objects.
+ */
+ info->revs->blob_objects = info->blobs;
+ info->revs->tree_objects = info->trees;
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
+ info->revs->blob_objects = info->revs->tree_objects = 0;
+
-+ if (info->tags) {
-+ struct oid_array tagged_blob_list = OID_ARRAY_INIT;
-+ struct oid_array tags = OID_ARRAY_INIT;
-+
-+ trace2_region_enter("path-walk", "tag-walk", info->revs->repo);
-+
-+ /*
-+ * Walk any pending objects at this point, but they should only
-+ * be tags.
-+ */
-+ for (size_t i = 0; i < info->revs->pending.nr; i++) {
-+ struct object_array_entry *pending = info->revs->pending.objects + i;
-+ struct object *obj = pending->item;
-+
-+ if (obj->type == OBJ_COMMIT || obj->flags & SEEN)
-+ continue;
-+
-+ while (obj->type == OBJ_TAG) {
-+ struct tag *tag = lookup_tag(info->revs->repo,
-+ &obj->oid);
-+ if (!(obj->flags & SEEN)) {
-+ obj->flags |= SEEN;
-+ oid_array_append(&tags, &obj->oid);
-+ }
-+ obj = tag->tagged;
-+ }
-+
-+ if ((obj->flags & SEEN))
-+ continue;
-+ obj->flags |= SEEN;
++ trace2_region_enter("path-walk", "pending-walk", info->revs->repo);
++ setup_pending_objects(info, &ctx);
++ trace2_region_leave("path-walk", "pending-walk", info->revs->repo);
+
-+ switch (obj->type) {
-+ case OBJ_TREE:
-+ if (info->trees)
-+ oid_array_append(&root_tree_list->oids, &obj->oid);
-+ break;
-+
-+ case OBJ_BLOB:
-+ if (info->blobs)
-+ oid_array_append(&tagged_blob_list, &obj->oid);
-+ break;
+ while ((c = get_revision(info->revs))) {
+ struct object_id *oid;
+ struct tree *t;
+@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
+
+ free(path);
+ }
+
-+ case OBJ_COMMIT:
-+ /* Make sure it is in the object walk */
-+ add_pending_object(info->revs, obj, "");
-+ break;
++ /* Are there paths remaining? Likely they are from indexed objects. */
++ if (!strmap_empty(&ctx.paths_to_lists)) {
++ struct hashmap_iter iter;
++ struct strmap_entry *entry;
+
-+ default:
-+ BUG("should not see any other type here");
-+ }
++ strmap_for_each_entry(&ctx.paths_to_lists, &iter, entry) {
++ push_to_stack(&ctx, entry->key);
+ }
+
-+ info->path_fn("", &tags, OBJ_TAG, info->path_fn_data);
++ while (!ret && ctx.path_stack.nr) {
++ char *path = ctx.path_stack.items[ctx.path_stack.nr - 1].string;
++ ctx.path_stack.nr--;
++ paths_nr++;
+
-+ if (tagged_blob_list.nr && info->blobs)
-+ info->path_fn("/tagged-blobs", &tagged_blob_list, OBJ_BLOB,
-+ info->path_fn_data);
++ ret = walk_path(&ctx, path);
+
-+ trace2_data_intmax("path-walk", ctx.repo, "tags", tags.nr);
-+ trace2_region_leave("path-walk", "tag-walk", info->revs->repo);
-+ oid_array_clear(&tags);
-+ oid_array_clear(&tagged_blob_list);
++ free(path);
++ }
+ }
+
- while ((c = get_revision(info->revs))) {
- struct object_id *oid;
- struct tree *t;
+ trace2_data_intmax("path-walk", ctx.repo, "paths", paths_nr);
+ trace2_region_leave("path-walk", "path-walk", info->revs->repo);
+
## path-walk.h ##
@@ path-walk.h: struct path_walk_info {
@@ t/t6601-path-walk.sh: test_expect_success 'all' '
+ 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)
-+ TAG::$(git rev-parse refs/tags/second.1)
-+ TAG::$(git rev-parse refs/tags/second.2)
-+ TAG::$(git rev-parse refs/tags/third)
-+ TAG::$(git rev-parse refs/tags/fourth)
-+ TAG::$(git rev-parse refs/tags/tree-tag)
-+ TAG::$(git rev-parse refs/tags/blob-tag)
++ TAG:/tags:$(git rev-parse refs/tags/first)
++ TAG:/tags:$(git rev-parse refs/tags/second.1)
++ TAG:/tags:$(git rev-parse refs/tags/second.2)
++ TAG:/tags:$(git rev-parse refs/tags/third)
++ TAG:/tags:$(git rev-parse refs/tags/fourth)
++ TAG:/tags:$(git rev-parse refs/tags/tree-tag)
++ TAG:/tags:$(git rev-parse refs/tags/blob-tag)
+ tags:7
++ EOF
++
++ test_cmp_sorted expect out
++'
++
++test_expect_success 'indexed objects' '
++ test_when_finished git reset --hard &&
++
++ # stage change into index, adding a blob but
++ # also invalidating the cache-tree for the root
++ # and the "left" directory.
++ echo bogus >left/c &&
++ git add left &&
++
++ test-tool path-walk -- --indexed-objects >out &&
++
++ cat >expect <<-EOF &&
++ commits:0
++ TREE:right/:$(git rev-parse topic:right)
++ trees:1
++ BLOB:a:$(git rev-parse HEAD:a)
++ BLOB:left/b:$(git rev-parse HEAD:left/b)
++ BLOB:left/c:$(git rev-parse :left/c)
++ BLOB:right/c:$(git rev-parse HEAD:right/c)
++ BLOB:right/d:$(git rev-parse HEAD:right/d)
++ blobs:5
++ tags:0
++ EOF
++
++ test_cmp_sorted expect out
++'
++
++test_expect_success 'branches and indexed objects mix well' '
++ test_when_finished git reset --hard &&
++
++ # stage change into index, adding a blob but
++ # also invalidating the cache-tree for the root
++ # and the "right" directory.
++ echo fake >right/d &&
++ git add right &&
++
++ test-tool path-walk -- --indexed-objects --branches >out &&
++
++ cat >expect <<-EOF &&
++ COMMIT::$(git rev-parse topic)
++ COMMIT::$(git rev-parse base)
++ COMMIT::$(git rev-parse base~1)
++ COMMIT::$(git rev-parse base~2)
++ commits:4
++ TREE::$(git rev-parse topic^{tree})
++ TREE::$(git rev-parse base^{tree})
++ TREE::$(git rev-parse base~1^{tree})
++ TREE::$(git rev-parse base~2^{tree})
++ TREE:a/:$(git rev-parse base:a)
++ TREE:left/:$(git rev-parse base:left)
++ TREE:left/:$(git rev-parse base~2:left)
++ TREE:right/:$(git rev-parse topic:right)
++ TREE:right/:$(git rev-parse base~1:right)
++ TREE:right/:$(git rev-parse base~2:right)
++ trees:10
++ BLOB:a:$(git rev-parse base~2:a)
++ BLOB:left/b:$(git rev-parse base:left/b)
++ BLOB:left/b:$(git rev-parse base~2:left/b)
++ BLOB:right/c:$(git rev-parse base~2:right/c)
++ BLOB:right/c:$(git rev-parse topic:right/c)
++ BLOB:right/d:$(git rev-parse base~1:right/d)
++ BLOB:right/d:$(git rev-parse :right/d)
++ blobs:7
++ tags:0
EOF
- sort expect >expect.sorted &&
+ test_cmp_sorted expect out
@@ t/t6601-path-walk.sh: test_expect_success 'topic only' '
BLOB:right/c:$(git rev-parse topic:right/c)
BLOB:right/d:$(git rev-parse base~1:right/d)
@@ t/t6601-path-walk.sh: test_expect_success 'topic only' '
+ tags:0
EOF
- sort expect >expect.sorted &&
+ test_cmp_sorted expect out
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
BLOB:right/c:$(git rev-parse topic:right/c)
BLOB:right/d:$(git rev-parse topic:right/d)
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
+ tags:0
EOF
- sort expect >expect.sorted &&
+ test_cmp_sorted expect out
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only blobs' '
BLOB:right/c:$(git rev-parse topic:right/c)
BLOB:right/d:$(git rev-parse topic:right/d)
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only blobs' '
+ tags:0
EOF
- sort expect >expect.sorted &&
+ test_cmp_sorted expect out
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only commits' '
commits:1
trees:0
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only commits' '
+ tags:0
EOF
- sort expect >expect.sorted &&
+ test_cmp_sorted expect out
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only trees' '
TREE:right/:$(git rev-parse topic:right)
trees:3
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only trees' '
+ tags:0
EOF
- sort expect >expect.sorted &&
+ test_cmp_sorted expect out
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
BLOB:right/c:$(git rev-parse topic:right/c)
BLOB:right/d:$(git rev-parse base~1:right/d)
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
+ tags:0
EOF
- sort expect >expect.sorted &&
-@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
- test_cmp expect.sorted out.sorted
+ test_cmp_sorted expect out
'
+test_expect_success 'trees are reported exactly once' '
6: 238d7d95715 ! 6: 5252076d556 path-walk: add prune_all_uninteresting option
@@ Metadata
Author: Derrick Stolee <[email protected]>
## Commit message ##
- path-walk: add prune_all_uninteresting option
+ path-walk: mark trees and blobs as UNINTERESTING
- This option causes the path-walk API to act like the sparse tree-walk
- algorithm implemented by mark_trees_uninteresting_sparse() in
- list-objects.c.
+ When the input rev_info has UNINTERESTING starting points, we want to be
+ sure that the UNINTERESTING flag is passed appropriately through the
+ objects. To match how this is done in places such as 'git pack-objects', we
+ use the mark_edges_uninteresting() method.
- Starting from the commits marked as UNINTERESTING, their root trees and
- all objects reachable from those trees are UNINTERSTING, at least as we
- walk path-by-path. When we reach a path where all objects associated
- with that path are marked UNINTERESTING, then do no continue walking the
- children of that path.
+ This method has an option for using the "sparse" walk, which is similar in
+ spirit to the path-walk API's walk. To be sure to keep it independent, add a
+ new 'prune_all_uninteresting' option to the path_walk_info struct.
- We need to be careful to pass the UNINTERESTING flag in a deep way on
- the UNINTERESTING objects before we start the path-walk, or else the
- depth-first search for the path-walk API may accidentally report some
- objects as interesting.
+ To check how the UNINTERSTING flag is spread through our objects, extend the
+ 'test-tool path-walk' command to output whether or not an object has that
+ flag. This changes our tests significantly, including the removal of some
+ objects that were previously visited due to the incomplete implementation.
Signed-off-by: Derrick Stolee <[email protected]>
## Documentation/technical/api-path-walk.txt ##
-@@ Documentation/technical/api-path-walk.txt: commits are emitted.
+@@ Documentation/technical/api-path-walk.txt: commits.
While it is possible to walk only commits in this way, consumers would be
better off using the revision walk API instead.
@@ Documentation/technical/api-path-walk.txt: commits are emitted.
## path-walk.c ##
+@@
+ #include "dir.h"
+ #include "hashmap.h"
+ #include "hex.h"
++#include "list-objects.h"
+ #include "object.h"
+ #include "oid-array.h"
+ #include "revision.h"
@@ path-walk.c: struct type_and_oid_list
{
enum object_type type;
@@ path-walk.c: struct type_and_oid_list
#define TYPE_AND_OID_LIST_INIT { \
@@ path-walk.c: static int add_children(struct path_walk_context *ctx,
- strmap_put(&ctx->paths_to_lists, path.buf, list);
- string_list_append(&ctx->path_stack, path.buf);
- }
+ if (o->flags & SEEN)
+ continue;
+ o->flags |= SEEN;
++
+ if (!(o->flags & UNINTERESTING))
+ list->maybe_interesting = 1;
oid_array_append(&list->oids, &entry.oid);
}
@@ path-walk.c: static int walk_path(struct path_walk_context *ctx,
-
- list = strmap_get(&ctx->paths_to_lists, path);
+ if (!list)
+ BUG("provided path '%s' that had no associated list", path);
+ if (ctx->info->prune_all_uninteresting) {
+ /*
@@ path-walk.c: static int walk_path(struct path_walk_context *ctx,
+ &list->oids.oid[i]);
+ if (t && !(t->object.flags & UNINTERESTING))
+ list->maybe_interesting = 1;
-+ } else {
++ } else if (list->type == OBJ_BLOB) {
+ struct blob *b = lookup_blob(ctx->repo,
+ &list->oids.oid[i]);
+ if (b && !(b->object.flags & UNINTERESTING))
+ list->maybe_interesting = 1;
++ } else {
++ /* Tags are always interesting if visited. */
++ list->maybe_interesting = 1;
+ }
+ }
+
@@ path-walk.c: static int walk_path(struct path_walk_context *ctx,
+
/* Evaluate function pointer on this data, if requested. */
if ((list->type == OBJ_TREE && ctx->info->trees) ||
- (list->type == OBJ_BLOB && ctx->info->blobs))
+ (list->type == OBJ_BLOB && ctx->info->blobs)||
@@ path-walk.c: static void clear_strmap(struct strmap *map)
- int walk_objects_by_path(struct path_walk_info *info)
- {
- const char *root_path = "";
-- int ret = 0;
-+ int ret = 0, has_uninteresting = 0;
- size_t commits_nr = 0, paths_nr = 0;
- struct commit *c;
- struct type_and_oid_list *root_tree_list;
-@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
- .path_stack = STRING_LIST_INIT_DUP,
- .paths_to_lists = STRMAP_INIT
- };
-+ struct oidset root_tree_set = OIDSET_INIT;
-
- trace2_region_enter("path-walk", "commit-walk", info->revs->repo);
+ strmap_init(map);
+ }
++static struct repository *edge_repo;
++static struct type_and_oid_list *edge_tree_list;
++
++static void show_edge(struct commit *commit)
++{
++ struct tree *t = repo_get_commit_tree(edge_repo, commit);
++
++ if (!t)
++ return;
++
++ if (commit->object.flags & UNINTERESTING)
++ t->object.flags |= UNINTERESTING;
++
++ if (t->object.flags & SEEN)
++ return;
++ t->object.flags |= SEEN;
++
++ oid_array_append(&edge_tree_list->oids, &t->object.oid);
++}
++
+ static void setup_pending_objects(struct path_walk_info *info,
+ struct path_walk_context *ctx)
+ {
+@@ path-walk.c: static void setup_pending_objects(struct path_walk_info *info,
+ if (tagged_blobs->oids.nr) {
+ const char *tagged_blob_path = "/tagged-blobs";
+ tagged_blobs->type = OBJ_BLOB;
++ tagged_blobs->maybe_interesting = 1;
+ push_to_stack(ctx, tagged_blob_path);
+ strmap_put(&ctx->paths_to_lists, tagged_blob_path, tagged_blobs);
+ } else {
+@@ path-walk.c: static void setup_pending_objects(struct path_walk_info *info,
+ if (tags->oids.nr) {
+ const char *tag_path = "/tags";
+ tags->type = OBJ_TAG;
++ tags->maybe_interesting = 1;
+ push_to_stack(ctx, tag_path);
+ strmap_put(&ctx->paths_to_lists, tag_path, tags);
+ } else {
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
/* Insert a single list for the root tree into the paths. */
CALLOC_ARRAY(root_tree_list, 1);
root_tree_list->type = OBJ_TREE;
+ root_tree_list->maybe_interesting = 1;
strmap_put(&ctx.paths_to_lists, root_path, root_tree_list);
+ push_to_stack(&ctx, root_path);
- /*
@@ path-walk.c: int walk_objects_by_path(struct path_walk_info *info)
- t = lookup_tree(info->revs->repo, oid);
+ if (prepare_revision_walk(info->revs))
+ die(_("failed to setup revision walk"));
- if (t) {
-+ if ((c->object.flags & UNINTERESTING)) {
-+ t->object.flags |= UNINTERESTING;
-+ has_uninteresting = 1;
-+ }
++ /* Walk trees to mark them as UNINTERESTING. */
++ edge_repo = info->revs->repo;
++ edge_tree_list = root_tree_list;
++ mark_edges_uninteresting(info->revs, show_edge,
++ info->prune_all_uninteresting);
++ edge_repo = NULL;
++ edge_tree_list = NULL;
+
- if (t->object.flags & SEEN)
- continue;
- t->object.flags |= SEEN;
-- oid_array_append(&root_tree_list->oids, oid);
-+ if (!oidset_insert(&root_tree_set, oid))
-+ 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)
- oid_array_clear(&commit_list->oids);
- free(commit_list);
+ info->revs->blob_objects = info->revs->tree_objects = 0;
-+ /*
-+ * Before performing a DFS of our paths and emitting them as interesting,
-+ * do a full walk of the trees to distribute the UNINTERESTING bit. Use
-+ * the sparse algorithm if prune_all_uninteresting was set.
-+ */
-+ if (has_uninteresting) {
-+ trace2_region_enter("path-walk", "uninteresting-walk", info->revs->repo);
-+ if (info->prune_all_uninteresting)
-+ mark_trees_uninteresting_sparse(ctx.repo, &root_tree_set);
-+ else
-+ mark_trees_uninteresting_dense(ctx.repo, &root_tree_set);
-+ trace2_region_leave("path-walk", "uninteresting-walk", info->revs->repo);
-+ }
-+ oidset_clear(&root_tree_set);
-+
- string_list_append(&ctx.path_stack, root_path);
-
- trace2_region_enter("path-walk", "path-walk", info->revs->repo);
+ trace2_region_enter("path-walk", "pending-walk", info->revs->repo);
## path-walk.h ##
@@ path-walk.h: struct path_walk_info {
@@ t/helper/test-path-walk.c: int cmd__path_walk(int argc, const char **argv)
## t/t6601-path-walk.sh ##
+@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
+ COMMIT::$(git rev-parse topic)
+ commits:1
+ TREE::$(git rev-parse topic^{tree})
+- TREE:left/:$(git rev-parse topic:left)
++ TREE:left/:$(git rev-parse base~1:left):UNINTERESTING
+ TREE:right/:$(git rev-parse topic:right)
+ trees:3
+- BLOB:a:$(git rev-parse topic:a)
+- BLOB:left/b:$(git rev-parse topic:left/b)
++ BLOB:a:$(git rev-parse base~1:a):UNINTERESTING
++ BLOB:left/b:$(git rev-parse base~1:left/b):UNINTERESTING
+ BLOB:right/c:$(git rev-parse topic:right/c)
+- BLOB:right/d:$(git rev-parse topic:right/d)
++ BLOB:right/d:$(git rev-parse base~1:right/d):UNINTERESTING
+ blobs:4
+ tags:0
+ EOF
+@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base' '
+ test_cmp_sorted expect out
+ '
+
++test_expect_success 'fourth, blob-tag2, not base' '
++ test-tool path-walk -- fourth blob-tag2 --not base >out &&
++
++ cat >expect <<-EOF &&
++ COMMIT::$(git rev-parse topic)
++ commits:1
++ TREE::$(git rev-parse topic^{tree})
++ TREE:left/:$(git rev-parse base~1:left):UNINTERESTING
++ TREE:right/:$(git rev-parse topic:right)
++ trees:3
++ BLOB:a:$(git rev-parse base~1:a):UNINTERESTING
++ BLOB:left/b:$(git rev-parse base~1:left/b):UNINTERESTING
++ BLOB:right/c:$(git rev-parse topic:right/c)
++ BLOB:right/d:$(git rev-parse base~1:right/d):UNINTERESTING
++ BLOB:/tagged-blobs:$(git rev-parse refs/tags/blob-tag2^{})
++ blobs:5
++ TAG:/tags:$(git rev-parse fourth)
++ tags:1
++ EOF
++
++ test_cmp_sorted expect out
++'
++
+ test_expect_success 'topic, not base, only blobs' '
+ test-tool path-walk --no-trees --no-commits \
+ -- topic --not base >out &&
+@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only blobs' '
+ cat >expect <<-EOF &&
+ commits:0
+ trees:0
+- BLOB:a:$(git rev-parse topic:a)
+- BLOB:left/b:$(git rev-parse topic:left/b)
++ BLOB:a:$(git rev-parse base~1:a):UNINTERESTING
++ BLOB:left/b:$(git rev-parse base~1:left/b):UNINTERESTING
+ BLOB:right/c:$(git rev-parse topic:right/c)
+- BLOB:right/d:$(git rev-parse topic:right/d)
++ BLOB:right/d:$(git rev-parse base~1:right/d):UNINTERESTING
+ blobs:4
+ tags:0
+ EOF
+@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, only trees' '
+ cat >expect <<-EOF &&
+ commits:0
+ TREE::$(git rev-parse topic^{tree})
+- TREE:left/:$(git rev-parse topic:left)
++ TREE:left/:$(git rev-parse base~1:left):UNINTERESTING
+ TREE:right/:$(git rev-parse topic:right)
+ trees:3
+ blobs:0
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
cat >expect <<-EOF &&
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
tags:0
EOF
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
- test_cmp expect.sorted out.sorted
+ test_cmp_sorted expect out
'
+-test_expect_success 'trees are reported exactly once' '
+- test_when_finished "rm -rf unique-trees" &&
+- test_create_repo unique-trees &&
+- (
+- cd unique-trees &&
+- mkdir initial &&
+- test_commit initial/file &&
+-
+- git switch -c move-to-top &&
+- git mv initial/file.t ./ &&
+- test_tick &&
+- git commit -m moved &&
+-
+- git update-ref refs/heads/other HEAD
+- ) &&
+-
+- test-tool -C unique-trees path-walk -- --all >out &&
+- tree=$(git -C unique-trees rev-parse HEAD:) &&
+- grep "$tree" out >out-filtered &&
+- test_line_count = 1 out-filtered
+test_expect_success 'topic, not base, boundary with pruning' '
+ test-tool path-walk --prune -- --boundary topic --not base >out &&
+
@@ t/t6601-path-walk.sh: test_expect_success 'topic, not base, boundary' '
+ tags:0
+ EOF
+
-+ sort expect >expect.sorted &&
-+ sort out >out.sorted &&
-+
-+ test_cmp expect.sorted out.sorted
-+'
-+
- test_expect_success 'trees are reported exactly once' '
- test_when_finished "rm -rf unique-trees" &&
- test_create_repo unique-trees &&
++ test_cmp_sorted expect out
+ '
+
+ test_done
|
|
This patch series was integrated into seen via git@9fc6323. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -0,0 +1,45 @@ | |||
| Path-Walk API | |||
There was a problem hiding this comment.
On the Git mailing list, karthik nayak wrote (reply to this):
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> From: Derrick Stolee <[email protected]>
>
> In anticipation of a few planned applications, introduce the most basic form
> of a path-walk API. It currently assumes that there are no UNINTERESTING
> objects, and does not include any complicated filters. It calls a function
> pointer on groups of tree and blob objects as grouped by path. This only
> includes objects the first time they are discovered, so an object that
> appears at multiple paths will not be included in two batches.
>
> These batches are collected in 'struct type_and_oid_list' objects, which
> store an object type and an oid_array of objects.
>
> The data structures are documented in 'struct path_walk_context', but in
> summary the most important are:
>
> * 'paths_to_lists' is a strmap that connects a path to a
> type_and_oid_list for that path. To avoid conflicts in path names,
> we make sure that tree paths end in "/" (except the root path with
> is an empty string) and blob paths do not end in "/".
>
> * 'path_stack' is a string list that is added to in an append-only
> way. This stores the stack of our depth-first search on the heap
> instead of using recursion.
>
> * 'path_stack_pushed' is a strmap that stores path names that were
> already added to 'path_stack', to avoid repeating paths in the
> stack. Mostly, this saves us from quadratic lookups from doing
> unsorted checks into the string_list.
>
> The coupling of 'path_stack' and 'path_stack_pushed' is protected by the
> push_to_stack() method. Call this instead of inserting into these
> structures directly.
>
> The walk_objects_by_path() method initializes these structures and
> starts walking commits from the given rev_info struct. The commits are
> used to find the list of root trees which populate the start of our
> depth-first search.
Isn't this more of breadth-first search? Reading through the code, the
algorithm seems something like:
- For each commit in list of commits (from rev_info)
- Tackle each root tree, add root path to the stack.
- For each path in stack left
- Call the callback provided by client.
- Find all its first level children, add each to the stack.
So wouldn't this go through the tree in level by level basis? Making it
a BFS?
Apart from this, the patch itself looks solid. I ended up writing a
small client to play with this API, and was very pleased how quickly I
could get it running.
[snip]There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/1/24 9:12 AM, karthik nayak wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> >> From: Derrick Stolee <[email protected]>
>>
>> The walk_objects_by_path() method initializes these structures and
>> starts walking commits from the given rev_info struct. The commits are
>> used to find the list of root trees which populate the start of our
>> depth-first search.
> > Isn't this more of breadth-first search? Reading through the code, the
> algorithm seems something like:
> > - For each commit in list of commits (from rev_info)
> - Tackle each root tree, add root path to the stack.
> - For each path in stack left
> - Call the callback provided by client.
> - Find all its first level children, add each to the stack.
> > So wouldn't this go through the tree in level by level basis? Making it
> a BFS?
While we are adding all children to the stack, we only pop off the top
of the stack, making it a DFS. (We do visit the paths in reverse-
lexicographic order, though.)
To make it a BFS, we would need to visit the paths in the order they
are added to the list. Instead, we visit them in Last-In First-Out
order.
I initially had built it as a BFS, but ran into memory issues when
running it on very large repos.
Thanks,
-Stolee
|
User |
| @@ -0,0 +1,45 @@ | |||
| Path-Walk API | |||
There was a problem hiding this comment.
On the Git mailing list, karthik nayak wrote (reply to this):
karthik nayak <[email protected]> writes:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>
>> From: Derrick Stolee <[email protected]>
>>
>> In anticipation of a few planned applications, introduce the most basic form
>> of a path-walk API. It currently assumes that there are no UNINTERESTING
>> objects, and does not include any complicated filters. It calls a function
>> pointer on groups of tree and blob objects as grouped by path. This only
>> includes objects the first time they are discovered, so an object that
>> appears at multiple paths will not be included in two batches.
>>
>> These batches are collected in 'struct type_and_oid_list' objects, which
>> store an object type and an oid_array of objects.
>>
>> The data structures are documented in 'struct path_walk_context', but in
>> summary the most important are:
>>
>> * 'paths_to_lists' is a strmap that connects a path to a
>> type_and_oid_list for that path. To avoid conflicts in path names,
>> we make sure that tree paths end in "/" (except the root path with
>> is an empty string) and blob paths do not end in "/".
>>
>> * 'path_stack' is a string list that is added to in an append-only
>> way. This stores the stack of our depth-first search on the heap
>> instead of using recursion.
>>
>> * 'path_stack_pushed' is a strmap that stores path names that were
>> already added to 'path_stack', to avoid repeating paths in the
>> stack. Mostly, this saves us from quadratic lookups from doing
>> unsorted checks into the string_list.
>>
>> The coupling of 'path_stack' and 'path_stack_pushed' is protected by the
>> push_to_stack() method. Call this instead of inserting into these
>> structures directly.
>>
>> The walk_objects_by_path() method initializes these structures and
>> starts walking commits from the given rev_info struct. The commits are
>> used to find the list of root trees which populate the start of our
>> depth-first search.
>
> Isn't this more of breadth-first search? Reading through the code, the
> algorithm seems something like:
>
> - For each commit in list of commits (from rev_info)
> - Tackle each root tree, add root path to the stack.
> - For each path in stack left
> - Call the callback provided by client.
> - Find all its first level children, add each to the stack.
>
> So wouldn't this go through the tree in level by level basis? Making it
> a BFS?
My bad here, thinking more about it, it is DFS indeed. Although we add
all the children of a level to the stack, we pop each of them from the
stack and end up traversing down that level.
>
> Apart from this, the patch itself looks solid. I ended up writing a
> small client to play with this API, and was very pleased how quickly I
> could get it running.
>
> [snip]
| the objects will be walked in a separate way based on those starting | ||
| commits. | ||
|
|
||
| Examples |
There was a problem hiding this comment.
On the Git mailing list, karthik nayak wrote (reply to this):
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
[snip]
> diff --git a/t/t6601-path-walk.sh b/t/t6601-path-walk.sh
> new file mode 100755
> index 00000000000..1f277b88291
> --- /dev/null
> +++ b/t/t6601-path-walk.sh
> @@ -0,0 +1,118 @@
> +#!/bin/sh
> +
> +test_description='direct path-walk API tests'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup test repository' '
> + git checkout -b base &&
> +
> + mkdir left &&
> + mkdir right &&
> + echo a >a &&
> + echo b >left/b &&
> + echo c >right/c &&
> + git add . &&
> + git commit -m "first" &&
> +
> + echo d >right/d &&
> + git add right &&
> + git commit -m "second" &&
> +
> + echo bb >left/b &&
> + git commit -a -m "third" &&
> +
> + git checkout -b topic HEAD~1 &&
> + echo cc >right/c &&
> + git commit -a -m "topic"
> +'
> +
Nit: Since the root level tree is already special cased out, we only
check one level of path here, would be nice to add another level of tree
to this.
> +test_expect_success 'all' '
> + test-tool path-walk -- --all >out &&
> +
> + cat >expect <<-EOF &&
> + TREE::$(git rev-parse topic^{tree})
> + TREE::$(git rev-parse base^{tree})
> + TREE::$(git rev-parse base~1^{tree})
> + TREE::$(git rev-parse base~2^{tree})
> + TREE:left/:$(git rev-parse base:left)
> + TREE:left/:$(git rev-parse base~2:left)
> + TREE:right/:$(git rev-parse topic:right)
> + TREE:right/:$(git rev-parse base~1:right)
> + TREE:right/:$(git rev-parse base~2:right)
> + trees:9
> + BLOB:a:$(git rev-parse base~2:a)
> + BLOB:left/b:$(git rev-parse base~2:left/b)
> + BLOB:left/b:$(git rev-parse base:left/b)
> + BLOB:right/c:$(git rev-parse base~2:right/c)
> + BLOB:right/c:$(git rev-parse topic:right/c)
> + BLOB:right/d:$(git rev-parse base~1:right/d)
> + blobs:6
> + EOF
> +
> + test_cmp_sorted expect out
> +'
Isn't the order deterministic? Why do we need to sort it?
| + | ||
| It is also important that you do not specify the `--objects` flag for the | ||
| `revs` struct. The revision walk should only be used to walk commits, and | ||
| the objects will be walked in a separate way based on those starting |
There was a problem hiding this comment.
On the Git mailing list, karthik nayak wrote (reply to this):
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> From: Derrick Stolee <[email protected]>
>
> The rev_info that is specified for a path-walk traversal may specify
> visiting tag refs (both lightweight and annotated) and also may specify
> indexed objects (blobs and trees). Update the path-walk API to walk
> these objects as well.
>
> When walking tags, we need to peel the annotated objects until reaching
> a non-tag object. If we reach a commit, then we can add it to the
> pending objects to make sure we visit in the commit walk portion. If we
Nit: s/in/it in/
[snip]
> + case OBJ_BLOB:
> + if (!info->blobs)
> + continue;
> + if (pending->path) {
> + struct type_and_oid_list *list;
> + char *path = pending->path;
> + if (!(list = strmap_get(&ctx->paths_to_lists, path))) {
> + CALLOC_ARRAY(list, 1);
> + list->type = OBJ_BLOB;
> + strmap_put(&ctx->paths_to_lists, path, list);
> + }
> + oid_array_append(&list->oids, &obj->oid);
> + } else {
> + /* assume a root tree, such as a lightweight tag. */
Shouldn't this comment be for tagged blobs?
> + oid_array_append(&tagged_blobs->oids, &obj->oid);
> + }
> + break;
[snip]There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/1/24 10:25 AM, karthik nayak wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> >> From: Derrick Stolee <[email protected]>
>>
>> The rev_info that is specified for a path-walk traversal may specify
>> visiting tag refs (both lightweight and annotated) and also may specify
>> indexed objects (blobs and trees). Update the path-walk API to walk
>> these objects as well.
>>
>> When walking tags, we need to peel the annotated objects until reaching
>> a non-tag object. If we reach a commit, then we can add it to the
>> pending objects to make sure we visit in the commit walk portion. If we
> > Nit: s/in/it in/
thanks!
>> + case OBJ_BLOB:
>> + if (!info->blobs)
>> + continue;
>> + if (pending->path) {
>> + struct type_and_oid_list *list;
>> + char *path = pending->path;
>> + if (!(list = strmap_get(&ctx->paths_to_lists, path))) {
>> + CALLOC_ARRAY(list, 1);
>> + list->type = OBJ_BLOB;
>> + strmap_put(&ctx->paths_to_lists, path, list);
>> + }
>> + oid_array_append(&list->oids, &obj->oid);
>> + } else {
>> + /* assume a root tree, such as a lightweight tag. */
> > Shouldn't this comment be for tagged blobs?
Yes. This is a copy-paste error.
Thanks for the careful reading.
-Stolee
|
This patch series was integrated into seen via git@de42bc5. |
|
This patch series was integrated into seen via git@81ffd2b. |
|
On the Git mailing list, Taylor Blau wrote (reply to this): Hi Stolee,
On Thu, Oct 31, 2024 at 06:26:57AM +0000, Derrick Stolee via GitGitGadget wrote:
>
> Introduction and relation to prior series
> =========================================
>
> This is a new series that rerolls the initial "path-walk API" patches of my
> RFC [1] "Path-walk API and applications". This new API (in path-walk.c and
> path-walk.h) presents a new way to walk objects such that trees and blobs
> are walked in batches according to their path.
>
> This also replaces the previous version of ds/path-walk that was being
> reviewed in [2]. The consensus was that the series was too long/dense and
> could use some reduction in size. This series takes the first few patches,
> but also makes some updates (which will be described later).
>
> [1]
> https://lore.kernel.org/git/[email protected]/
> [RFC] Path-walk API and applications
>
> [2]
> https://lore.kernel.org/git/[email protected]/
> [PATCH v2 00/17] pack-objects: add --path-walk option for better deltas
I apologize for not having a better place to start discussing a topic
which pertains to more than just this immediate patch series, but I
figure here is as good a place as any to do so.
From our earlier discussion, it seems to stand that the path-walk API
is fundamentally incompatible with reachability bitmaps and
delta-islands, making the series a non-starter in environments that
rely significantly one or both of those features. My understanding as a
result is that the path-walk API and feature are more targeted towards
improving client-side repacks and push performance, where neither of the
aforementioned two features are used quite as commonly.
I was discussing this a bit off-list with Peff (who I hope will join the
thread and share his own thoughts), but I wonder if it was a mistake to
discard your '--full-name-hash' idea (or something similar, which I'll
discuss in a bit below) from earlier.
(Repeating a few things that I am sure are obvious to you out loud so
that I can get a grasp on them for my own understanding):
It seems that the problems you've identified which result in poor repack
performance occur when you have files at the same path, but they get
poorly sorted in the delta selection window due to other paths having
the same final 16 characters, so Git doesn't see that much better delta
opportunities exist.
Your series takes into account the full name when hashing, which seems
to produce a clear win in many cases. I'm sure that there are some cases
where it presents a modest regression in pack sizes, but I think that's
fine and probably par for the course when making any changes like this,
as there is probably no easy silver bullet here that uniformly improves
all cases.
I suspect that you could go even further and intern the full path at
which each object occurs, and sort lexically by that. Just stringing
together all of the paths in linux.git only takes 3.099 MiB on my clone.
(Of course, that's unbounded in the number of objects and length of
their pathnames, but you could at least bound the latter by taking only
the last, say, 128 characters, which would be more than good enough for
the kernel, whose longest path is only 102 characters).
Some of the repositories that you've tested on I don't have easy access
to, so I wonder if either doing (a) that, or (b) using some fancier
context-sensitive hash (like SimHash or MinHash) would be beneficial.
I realize that this is taking us back to an idea you've already
presented to the list, but I think (to me, at least) the benefit and
simplicity of that approach has only become clear to me in hindsight
when seeing some alternatives. I would like to apologize for the time
you spent reworking this series back and forth to have the response be
"maybe we should have just done the first thing you suggested". Like I
said, I think to me it was really only clear in hindsight.
In any event, the major benefit to doing --full-name-hash would be that
*all* environments could benefit from the size reduction, not just those
that don't rely on certain other features.
Perhaps just --full-name-hash isn't quite as good by itself as the
--path-walk implementation that this series starts us off implementing.
So in that sense, maybe we want both, which I understand was the
original approach. I see a couple of options here:
- We take both, because doing --path-walk on top represents a
significant enough improvement that we are collectively OK with
taking on more code to improve a more narrow (but common) use-case.
- Or we decide that either the benefit isn't significant enough to
warrant an additional and relatively complex implementation, or in
other words that --full-name-hash by itself is good enough.
Again, I apologize for not having a clearer picture of this all to start
with, and I want to tell you specifically and sincerely that I
appreciate your patience as I wrap my head around all of this. I think
the benefit of --full-name-hash is much clearer and appealing to me now
having had both more time and seeing the series approached in a couple
of different ways. Let me know what you think.
Thanks,
Taylor |
|
This patch series was integrated into seen via git@5212635. |
| the objects will be walked in a separate way based on those starting | ||
| commits. | ||
|
|
||
| Examples |
There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
I haven't looked thoroughly at the rest of the patches yet, but had a
comment about this test. Rearranging:
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> +test_expect_success 'all' '
> + test-tool path-walk -- --all >out &&
> +
> + cat >expect <<-EOF &&
> + TREE::$(git rev-parse topic^{tree})
> + TREE::$(git rev-parse base^{tree})
> + TREE::$(git rev-parse base~1^{tree})
> + TREE::$(git rev-parse base~2^{tree})
> + TREE:left/:$(git rev-parse base:left)
> + TREE:left/:$(git rev-parse base~2:left)
> + TREE:right/:$(git rev-parse topic:right)
> + TREE:right/:$(git rev-parse base~1:right)
> + TREE:right/:$(git rev-parse base~2:right)
> + trees:9
[snip rest of "expect"]
The way you're testing this, wouldn't the tests pass even if the OIDs
aren't emitted in path order? (E.g. if topic:right and base~1:right
were somehow grouped into two different groups, even though they have
the same path.)
I would have expected the test output to be something like:
TREE:right/ $(rp :right topic base~1 base~2)
where rp is a function that takes in a suffix and one or more prefixes -
I haven't figured out its contents yet, but
echo $(git rev-parse HEAD^^ HEAD^ HEAD | sort)
gives us a space-separated list, so it doesn't seem too difficult to
define such a function.
> +static int emit_block(const char *path, struct oid_array *oids,
> + enum object_type type, void *data)
> +{
> + struct path_walk_test_data *tdata = data;
> + const char *typestr;
> +
> + switch (type) {
> + case OBJ_TREE:
> + typestr = "TREE";
> + tdata->tree_nr += oids->nr;
> + break;
> +
> + case OBJ_BLOB:
> + typestr = "BLOB";
> + tdata->blob_nr += oids->nr;
> + break;
> +
> + default:
> + BUG("we do not understand this type");
> + }
> +
> + for (size_t i = 0; i < oids->nr; i++)
> + printf("%s:%s:%s\n", typestr, path, oid_to_hex(&oids->oid[i]));
Then here, you would print typestr and path before the "for" loop. In
the "for" loop you would add oid_to_hex() results to a sorted string
list, have another "for" loop that prints each element preceded by a
space, then print a "\n" after both "for" loops.There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/1/24 6:23 PM, Jonathan Tan wrote:
> I haven't looked thoroughly at the rest of the patches yet, but had a
> comment about this test. Rearranging:
> > "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>> +test_expect_success 'all' '
>> + test-tool path-walk -- --all >out &&
>> +
>> + cat >expect <<-EOF &&
>> + TREE::$(git rev-parse topic^{tree})
>> + TREE::$(git rev-parse base^{tree})
>> + TREE::$(git rev-parse base~1^{tree})
>> + TREE::$(git rev-parse base~2^{tree})
>> + TREE:left/:$(git rev-parse base:left)
>> + TREE:left/:$(git rev-parse base~2:left)
>> + TREE:right/:$(git rev-parse topic:right)
>> + TREE:right/:$(git rev-parse base~1:right)
>> + TREE:right/:$(git rev-parse base~2:right)
>> + trees:9
> > [snip rest of "expect"]
> > The way you're testing this, wouldn't the tests pass even if the OIDs
> aren't emitted in path order? (E.g. if topic:right and base~1:right
> were somehow grouped into two different groups, even though they have
> the same path.)
You are correct that if the path-walk API emitted multiple batches
with the same path name, then we would not detect that via the current
testing strategy.
The main reason to use the sort is to avoid adding a restriction on
the order in which objects appear within the batch.
Your recommendation to group a batch into a single line does not
strike me as a suitable approach, because long lines become hard to
read and difficult to parse diffs. (Also, the order within the batch
becomes baked in as a requirement.)
The biggest question I'd like to ask is this: do you see a risk of
a path being repeated? There are cases where it will happen, such as
indexed objects that are not reachable anywhere else.
The way I would consider modifying these tests to reflect the batching
would be to associate each batch with a number, causing the order of
the paths to become hard-coded in the test. Something like
0:COMMIT::$(git rev-parse ...)
0:COMMIT::$(git rev-parse ...)
1:TREE::$(git rev-parse ...)
1:TREE::$(git rev-parse ...)
2:TREE:right/:$(git rev-parse ...)
3:BLOB:right/a:$(...)
4:TREE:left/:$(git rev-parse ...)
5:BLOB:left/b:$(...)
This would imply some amount of order that maybe should become a
requirement of the API.
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
Derrick Stolee <[email protected]> writes:
> You are correct that if the path-walk API emitted multiple batches
> with the same path name, then we would not detect that via the current
> testing strategy.
>
> The main reason to use the sort is to avoid adding a restriction on
> the order in which objects appear within the batch.
>
> Your recommendation to group a batch into a single line does not
> strike me as a suitable approach, because long lines become hard to
> read and difficult to parse diffs. (Also, the order within the batch
> becomes baked in as a requirement.)
The hashes in a line can be abbreviated if line length is a concern.
Also, note that I am suggesting sorting the OIDs within a line (that is,
a batch), and also sorting the lines (batches) as a whole.
> The biggest question I'd like to ask is this: do you see a risk of
> a path being repeated? There are cases where it will happen, such as
> indexed objects that are not reachable anywhere else.
I was thinking that the whole point of this feature is that we group
objects by path, so it seems desirable to test that paths are not
repeated. (Or repeated as little as possible, if it is not possible
to avoid repetition e.g. in the case you describe.)
> The way I would consider modifying these tests to reflect the batching
> would be to associate each batch with a number, causing the order of
> the paths to become hard-coded in the test. Something like
>
> 0:COMMIT::$(git rev-parse ...)
> 0:COMMIT::$(git rev-parse ...)
> 1:TREE::$(git rev-parse ...)
> 1:TREE::$(git rev-parse ...)
> 2:TREE:right/:$(git rev-parse ...)
> 3:BLOB:right/a:$(...)
> 4:TREE:left/:$(git rev-parse ...)
> 5:BLOB:left/b:$(...)
>
> This would imply some amount of order that maybe should become a
> requirement of the API.
>
> Thanks,
> -Stolee
If we're willing to declare an order in which we will return paths to
the user, that would work too. (I'm not sure that we need to declare an
order, though.)There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/4/24 6:39 PM, Jonathan Tan wrote:
> Derrick Stolee <[email protected]> writes:
> >> The biggest question I'd like to ask is this: do you see a risk of
>> a path being repeated? There are cases where it will happen, such as
>> indexed objects that are not reachable anywhere else.
> > I was thinking that the whole point of this feature is that we group
> objects by path, so it seems desirable to test that paths are not
> repeated. (Or repeated as little as possible, if it is not possible
> to avoid repetition e.g. in the case you describe.)
In addition to determining the order of the batches, it can be
helpful to demonstrate that we don't call the path_fn with an
empty batch! I discovered this while making the appropriate
changes today and putting the fixes in the right places.
Thanks,
-Stolee
|
This patch series was integrated into seen via git@d62781c. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 11/1/24 3:23 PM, Taylor Blau wrote:
> Hi Stolee,
> > On Thu, Oct 31, 2024 at 06:26:57AM +0000, Derrick Stolee via GitGitGadget wrote:
>>
>> Introduction and relation to prior series
>> =========================================
>>
>> This is a new series that rerolls the initial "path-walk API" patches of my
>> RFC [1] "Path-walk API and applications". This new API (in path-walk.c and
>> path-walk.h) presents a new way to walk objects such that trees and blobs
>> are walked in batches according to their path.
>>
>> This also replaces the previous version of ds/path-walk that was being
>> reviewed in [2]. The consensus was that the series was too long/dense and
>> could use some reduction in size. This series takes the first few patches,
>> but also makes some updates (which will be described later).
>>
>> [1]
>> https://lore.kernel.org/git/[email protected]/
>> [RFC] Path-walk API and applications
>>
>> [2]
>> https://lore.kernel.org/git/[email protected]/
>> [PATCH v2 00/17] pack-objects: add --path-walk option for better deltas
> > I apologize for not having a better place to start discussing a topic
> which pertains to more than just this immediate patch series, but I
> figure here is as good a place as any to do so.
> > From our earlier discussion, it seems to stand that the path-walk API
> is fundamentally incompatible with reachability bitmaps and
> delta-islands, making the series a non-starter in environments that
> rely significantly one or both of those features. My understanding as a
> result is that the path-walk API and feature are more targeted towards
> improving client-side repacks and push performance, where neither of the
> aforementioned two features are used quite as commonly.
This is correct. I would go even farther to say that this approach was
designed first and foremost for Git clients and specifically their
performance while computing a thin packfile during "git push". The same
logic to help the push case happens to also help the "git repack" case
significantly.
> I was discussing this a bit off-list with Peff (who I hope will join the
> thread and share his own thoughts), but I wonder if it was a mistake to
> discard your '--full-name-hash' idea (or something similar, which I'll
> discuss in a bit below) from earlier.
I'd be happy to resurrect that series, adding in the learnings from
working on the path-walk feature. It helps that the current series adds
the path-walk API and has no conflicting changes in the pack-objects or
repack builtins. (I can handle those conflicts as things merge down.)
> (Repeating a few things that I am sure are obvious to you out loud so
> that I can get a grasp on them for my own understanding):
> > It seems that the problems you've identified which result in poor repack
> performance occur when you have files at the same path, but they get
> poorly sorted in the delta selection window due to other paths having
> the same final 16 characters, so Git doesn't see that much better delta
> opportunities exist.
> > Your series takes into account the full name when hashing, which seems
> to produce a clear win in many cases. I'm sure that there are some cases
> where it presents a modest regression in pack sizes, but I think that's
> fine and probably par for the course when making any changes like this,
> as there is probably no easy silver bullet here that uniformly improves
> all cases.
> > I suspect that you could go even further and intern the full path at
> which each object occurs, and sort lexically by that. Just stringing
> together all of the paths in linux.git only takes 3.099 MiB on my clone.
> (Of course, that's unbounded in the number of objects and length of
> their pathnames, but you could at least bound the latter by taking only
> the last, say, 128 characters, which would be more than good enough for
> the kernel, whose longest path is only 102 characters).
When the optimization idea is to focus on the full path and not care
about the "locality" of the path name by its later bits, storing the
full name in a list and storing an index into that list would have a
very similar effect.
I'd be interested to explore the idea of storing the full path name.
Based on my exploration with the 'test-tool name-hash' test helper in
that series, I'm not sure that we will make significant gains by doing
so. Worth trying.
> Some of the repositories that you've tested on I don't have easy access
> to, so I wonder if either doing (a) that, or (b) using some fancier
> context-sensitive hash (like SimHash or MinHash) would be beneficial.
I don't know too much about SimHash or MinHash, but based on what I
could gather from some initial reading I'm not sure that they would be
effective without increasing the hash length. We'd also get a different
kind of locality, such as the appearance of a common word would be more
likely to affect the locality than the end of the path.
The size of the hash could probably be mitigated by storing it in the
list of all full paths and accessing them from the index stored on each
to-pack object.
> I realize that this is taking us back to an idea you've already
> presented to the list, but I think (to me, at least) the benefit and
> simplicity of that approach has only become clear to me in hindsight
> when seeing some alternatives. I would like to apologize for the time
> you spent reworking this series back and forth to have the response be
> "maybe we should have just done the first thing you suggested". Like I
> said, I think to me it was really only clear in hindsight.
I always assumed that we'd come back to it eventually. There is also the
extra bit about making the change to the name-hash compatible with the
way name-hashes are stored in the reachability bitmaps. That will need
some work before it is ready for prime time.
> In any event, the major benefit to doing --full-name-hash would be that
> *all* environments could benefit from the size reduction, not just those
> that don't rely on certain other features.
I disagree that all environments will prefer the --full-name-hash. I'm
currently repeating the performance tests right now, and I've added one.
The issues are:
1. The --full-name-hash approach sometimes leads to a larger pack when
using "git push" on the client, especially when the name-hash is
already effective for compressing across paths.
2. A depth 1 shallow clone cannot use previous versions of a path, so
those situations will want to use the normal name hash. This can be
accomplished simply by disabling the --full-name-hash option when
the --shallow option is present; a more detailed version could be
used to check for a large depth before disabling it. This case also
disables bitmaps, so that isn't something to worry about.
> Perhaps just --full-name-hash isn't quite as good by itself as the
> --path-walk implementation that this series starts us off implementing.
> So in that sense, maybe we want both, which I understand was the
> original approach. I see a couple of options here:
> > - We take both, because doing --path-walk on top represents a
> significant enough improvement that we are collectively OK with
> taking on more code to improve a more narrow (but common) use-case.
Doing both doesn't help at all, since the --path-walk approach already
batches by the full path name. The --path-walk approach has a significant
benefit by doing a second pass by the standard name-hash to pick up on the
cross-path deltas. This is why the --path-walk approach with the standard
name hash as consistently provided the most-compact pack-files in all
tests.
Aside: there were some initial tests that showed the --path-walk option
led to slightly larger packfiles, but I've since discovered that those
cases were due to an incorrect walking of indexed paths. This is fixed
by the code in patch 5 of the current series and my WIP patches in [3]
have the performance numbers with this change.
[3] https://github.com/gitgitgadget/git/pull/1819
PATH WALK II: Add --path-walk option to 'git pack-objects'
> - Or we decide that either the benefit isn't significant enough to
> warrant an additional and relatively complex implementation, or in
> other words that --full-name-hash by itself is good enough.
I hope that I've sufficiently communicated that --full-name-hash is not
good enough by itself.
The point I was trying to make by submitting it first was that I believed
it was likely the easiest way for Git servers to gain 90% of the benefits
that the --path-walk approach provides while making it relatively easy to
integrate with other server-side features such as bitmaps and delta islands.
(Maybe the --path-walk approach could also be extended to be compatible
with those features, but it would be a significant investment that rebuilds
those features within the context of the new object walk instead of relying
on the existing implementations. That could easily be a blocker.)
> Again, I apologize for not having a clearer picture of this all to start
> with, and I want to tell you specifically and sincerely that I
> appreciate your patience as I wrap my head around all of this. I think
> the benefit of --full-name-hash is much clearer and appealing to me now
> having had both more time and seeing the series approached in a couple
> of different ways. Let me know what you think.
Thanks for taking the time to engage with the patches. I'm currently
rerunning my performance tests on a rebased copy of the --full-name-hash
patches and will submit a new version when it's ready.
Thanks,
-Stolee
|
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Nov 04, 2024 at 10:48:49AM -0500, Derrick Stolee wrote:
> > I was discussing this a bit off-list with Peff (who I hope will join the
> > thread and share his own thoughts), but I wonder if it was a mistake to
> > discard your '--full-name-hash' idea (or something similar, which I'll
> > discuss in a bit below) from earlier.
>
> I'd be happy to resurrect that series, adding in the learnings from
> working on the path-walk feature. It helps that the current series adds
> the path-walk API and has no conflicting changes in the pack-objects or
> repack builtins. (I can handle those conflicts as things merge down.)
Adding my two cents, the discussion we had came after reading this post:
https://www.jonathancreamer.com/how-we-shrunk-our-git-repo-size-by-94-percent/
I think a few of the low-level details in there are confusing, but it
seemed to me that most of the improvement he mentions is just about
finding better delta candidates. And it seems obvious that our current
pack_name_hash() is pretty rudimentary as context-sensitive hashing
goes and won't do well for long paths with similar endings.
So just swapping that out for something better seems like an easy thing
to do regardless of whether we pursue --path-walk. It doesn't
drastically change how we choose delta pairs so it's not much code and
it shouldn't conflict with other features. And the risk of making
anything worse should be pretty low.
I wouldn't at all be surprised if --path-walk can do better, but if we
do the easy thing first then I think it gives us a better idea of the
cost/benefit it's providing.
I suspect there's room for both in the long run. You seem to be focused
on push size and cost, whereas I think Taylor and I are more interested
in overall repo size and cost of serving bitmapped fetches.
> When the optimization idea is to focus on the full path and not care
> about the "locality" of the path name by its later bits, storing the
> full name in a list and storing an index into that list would have a
> very similar effect.
>
> I'd be interested to explore the idea of storing the full path name.
> Based on my exploration with the 'test-tool name-hash' test helper in
> that series, I'm not sure that we will make significant gains by doing
> so. Worth trying.
The way I look at it is a possible continuum. We want to use pathnames
as a way to sort delta candidates near each other, since we expect them
to have high locality with respect to delta-able contents. The current
name_hash uses a very small bit of that path information and throws away
most of it. The other extreme end is holding the whole path. We may want
to end up in the middle for two reasons:
1. Dealing with whole paths might be costly (though I'm not yet
convinced of that; outside of pathological cases, the number of
paths in a repo tends to pale in comparison to the number of
objects, and the per-object costs dominate during repacking).
2. It's possible that over-emphasizing the path might be a slightly
worse heuristic (and I think this is a potential danger of
--path-walk, too). We still want to find candidate pairs that were
copied or renamed, for example, or that substantially share content
found in different parts of the tree.
So it would be interesting to be able to see the performance of various
points on that line, from full path down to partial paths down to longer
hashes down to the current hash. The true extreme end of course is no
path info at all, but I think we know that sucks; that's why we
implemented the bitmap name-hash extension in the first place.
> I don't know too much about SimHash or MinHash, but based on what I
> could gather from some initial reading I'm not sure that they would be
> effective without increasing the hash length. We'd also get a different
> kind of locality, such as the appearance of a common word would be more
> likely to affect the locality than the end of the path.
Good point. This is all heuristics, of course, but I suspect that the
order of the path is important, and that foo/bar.c and bar/foo.c are
unlikely to be good matches.
> > I realize that this is taking us back to an idea you've already
> > presented to the list, but I think (to me, at least) the benefit and
> > simplicity of that approach has only become clear to me in hindsight
> > when seeing some alternatives. I would like to apologize for the time
> > you spent reworking this series back and forth to have the response be
> > "maybe we should have just done the first thing you suggested". Like I
> > said, I think to me it was really only clear in hindsight.
>
> I always assumed that we'd come back to it eventually. There is also the
> extra bit about making the change to the name-hash compatible with the
> way name-hashes are stored in the reachability bitmaps. That will need
> some work before it is ready for prime time.
Having worked on that feature of bitmaps, I'm not too worried about it.
I think we'd just need to:
- introduce a new bitmap ext with a flag (HASH_CACHE_V2 or something,
either with the new hash, or with a "version" byte at the start for
extensibility).
- when bitmaps are not in use, we're free to use whichever hash we
want internally. If the new hash is consistently better, we'd
probably just enable it by default.
- when packing using on-disk bitmaps, use internally whichever format
the on-disk file provided. Technically the format could even provide
both (in which case we'd prefer the new hash), but I don't see much
point.
- when writing bitmaps, use whichever hash the command-line options
asked for. There's an off chance somebody might want to generate a
.bitmap file whose hashes will be understood by an older version of
git, in which case they'd use --no-full-name-hash or whatever while
repacking.
If we're considering full paths, then that is potentially a bit more
involved, just because we'd want the format to avoid repeating duplicate
paths for each object (plus they're no longer fixed-size). So probably
an extension with packed NUL-terminated path strings, plus a set of
fixed-length offsets into that block, one per object.
> I disagree that all environments will prefer the --full-name-hash. I'm
> currently repeating the performance tests right now, and I've added one.
> The issues are:
>
> 1. The --full-name-hash approach sometimes leads to a larger pack when
> using "git push" on the client, especially when the name-hash is
> already effective for compressing across paths.
That's interesting. I wonder which cases get worse, and if a larger
window size might help. I.e., presumably we are pushing the candidates
further away in the sorted delta list.
> 2. A depth 1 shallow clone cannot use previous versions of a path, so
> those situations will want to use the normal name hash. This can be
> accomplished simply by disabling the --full-name-hash option when
> the --shallow option is present; a more detailed version could be
> used to check for a large depth before disabling it. This case also
> disables bitmaps, so that isn't something to worry about.
I'm not sure why a larger hash would be worse in a shallow clone. As you
note, with only one version of each path the name-similarity heuristic
is not likely to buy you much. But I'd have thought that would be true
for the existing name hash as well as a longer one. Maybe this is the
"over-emphasizing" case.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <[email protected]> writes:
> On Mon, Nov 04, 2024 at 10:48:49AM -0500, Derrick Stolee wrote:
>> I disagree that all environments will prefer the --full-name-hash. I'm
>> currently repeating the performance tests right now, and I've added one.
>> The issues are:
>>
>> 1. The --full-name-hash approach sometimes leads to a larger pack when
>> using "git push" on the client, especially when the name-hash is
>> already effective for compressing across paths.
>
> That's interesting. I wonder which cases get worse, and if a larger
> window size might help. I.e., presumably we are pushing the candidates
> further away in the sorted delta list.
>
>> 2. A depth 1 shallow clone cannot use previous versions of a path, so
>> those situations will want to use the normal name hash. This can be
>> accomplished simply by disabling the --full-name-hash option when
>> the --shallow option is present; a more detailed version could be
>> used to check for a large depth before disabling it. This case also
>> disables bitmaps, so that isn't something to worry about.
>
> I'm not sure why a larger hash would be worse in a shallow clone. As you
> note, with only one version of each path the name-similarity heuristic
> is not likely to buy you much. But I'd have thought that would be true
> for the existing name hash as well as a longer one. Maybe this is the
> "over-emphasizing" case.
I too am curious to hear Derrick explain the above points and what
was learned from the performance tests. The original hash was
designed to place files that are renamed across directories closer
to each other in the list sorted by the name hash, so a/Makefile and
b/Makefile would likely be treated as delta-base candidates while
foo/bar.c and bar/foo.c are treated as unrelated things. A push
of a handful of commits that rename paths would likely place the
rename source of older commits and rename destination of newer
commits into the same delta chain, even with a smaller delta window.
In such a history, uniformly-distributed-without-regard-to-renames
hash is likely to make them into two distinct delta chains, leading
to less optimal delta-base selection.
A whole-repository packing, or a large push or fetch, of the same
history with renamed files are affected a lot less by such negative
effects of full-name hash. When generating a pack with more commits
than the "--window", use of the original hash would mean blobs from
paths that share similar names (e.g., "Makefile"s everywhere in the
directory hierarchy) are placed close to each other, but full-name
hash will likely group the blobs from exactly the same path and
nothing else together, and the resulting delta-chain for identical
(and not similar) paths would be sufficiently long. A long delta
chain has to be broken into multiple chains _anyway_ due to finite
"--depth" setting, so placing blobs from each path into its own
(initial) delta chain, completely ignoring renamed paths, would
likely to give us long enough (initial) delta chain to be split at
the depth limit.
It would lead to a good delta-base selection with smaller window
size quite efficiently with full-name hash.
I think a full-name hash forces a single-commit pack of a wide tree
to give up on deltified blobs, but with the original hash, at least
similar and common files (e.g. Makefile and COPYING) would sit close
together in the delta queue and can be deltified with each other,
which may be where the inefficiency comes from when full-name hash
is used.
|
|
This patch series was integrated into seen via git@36139cc. |
|
This patch series was integrated into seen via git@7a27821. |
|
This patch series was integrated into seen via git@ed0d8f0. |
|
This patch series was integrated into seen via git@7139a38. |
| the objects will be walked in a separate way based on those starting | ||
| commits. | ||
|
|
||
| Examples |
There was a problem hiding this comment.
On the Git mailing list, Patrick Steinhardt wrote (reply to this):
On Thu, Oct 31, 2024 at 06:27:00AM +0000, Derrick Stolee via GitGitGadget wrote:
[snip]
> +int cmd__path_walk(int argc, const char **argv)
> +{
> + 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;
> +
> + argc = parse_options(argc, argv, NULL,
> + options, path_walk_usage,
> + PARSE_OPT_KEEP_UNKNOWN_OPT | PARSE_OPT_KEEP_ARGV0);
> +
> + if (argc > 1)
> + setup_revisions(argc, argv, &revs, NULL);
> + else
> + usage(path_walk_usage[0]);
> +
> + info.revs = &revs;
> + info.path_fn = emit_block;
> + info.path_fn_data = &data;
> +
> + res = walk_objects_by_path(&info);
> +
> + printf("trees:%" PRIuMAX "\n"
> + "blobs:%" PRIuMAX "\n",
> + data.tree_nr, data.blob_nr);
> +
> + return res;
> +}
This function is leaking memory. I'd propose to add below patch on top
to plug them, which makes t6601 pass with the leak sanitizer enabled.
Patrick
diff --git a/t/helper/test-path-walk.c b/t/helper/test-path-walk.c
index 06b103d876..fa3bfe46b5 100644
--- a/t/helper/test-path-walk.c
+++ b/t/helper/test-path-walk.c
@@ -85,7 +85,6 @@ int cmd__path_walk(int argc, const char **argv)
OPT_END(),
};
- initialize_repository(the_repository);
setup_git_directory();
revs.repo = the_repository;
@@ -110,5 +109,6 @@ int cmd__path_walk(int argc, const char **argv)
"tags:%" PRIuMAX "\n",
data.commit_nr, data.tree_nr, data.blob_nr, data.tag_nr);
+ release_revisions(&revs);
return res;
}There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 11/6/24 9:04 AM, Patrick Steinhardt wrote:
> On Thu, Oct 31, 2024 at 06:27:00AM +0000, Derrick Stolee via GitGitGadget wrote:
> [snip]
> This function is leaking memory. I'd propose to add below patch on top
> to plug them, which makes t6601 pass with the leak sanitizer enabled.
Thanks! Applied for the next version.
-Stolee
|
This patch series was integrated into seen via git@6048c6b. |
In anticipation of a few planned applications, introduce the most basic form
of a path-walk API. It currently assumes that there are no UNINTERESTING
objects, and does not include any complicated filters. It calls a function
pointer on groups of tree and blob objects as grouped by path. This only
includes objects the first time they are discovered, so an object that
appears at multiple paths will not be included in two batches.
These batches are collected in 'struct type_and_oid_list' objects, which
store an object type and an oid_array of objects.
The data structures are documented in 'struct path_walk_context', but in
summary the most important are:
* 'paths_to_lists' is a strmap that connects a path to a
type_and_oid_list for that path. To avoid conflicts in path names,
we make sure that tree paths end in "/" (except the root path with
is an empty string) and blob paths do not end in "/".
* 'path_stack' is a string list that is added to in an append-only
way. This stores the stack of our depth-first search on the heap
instead of using recursion.
* 'path_stack_pushed' is a strmap that stores path names that were
already added to 'path_stack', to avoid repeating paths in the
stack. Mostly, this saves us from quadratic lookups from doing
unsorted checks into the string_list.
The coupling of 'path_stack' and 'path_stack_pushed' is protected by the
push_to_stack() method. Call this instead of inserting into these
structures directly.
The walk_objects_by_path() method initializes these structures and
starts walking commits from the given rev_info struct. The commits are
used to find the list of root trees which populate the start of our
depth-first search.
The core of our depth-first search is in a while loop that continues
while we have not indicated an early exit and our 'path_stack' still has
entries in it. The loop body pops a path off of the stack and "visits"
the path via the walk_path() method.
The walk_path() method gets the list of OIDs from the 'path_to_lists'
strmap and executes the callback method on that list with the given path
and type. If the OIDs correspond to tree objects, then iterate over all
trees in the list and run add_children() to add the child objects to
their own lists, adding new entries to the stack if necessary.
In testing, this depth-first search approach was the one that used the
least memory while iterating over the object lists. There is still a
chance that repositories with too-wide path patterns could cause memory
pressure issues. Limiting the stack size could be done in the future by
limiting how many objects are being considered in-progress, or by
visiting blob paths earlier than trees.
There are many future adaptations that could be made, but they are left for
future updates when consumers are ready to take advantage of those features.
Signed-off-by: Derrick Stolee <[email protected]>
This test helper will be helpful to reduce repeated logic in t6601-path-walk.sh, but may be helpful elsewhere, too. Signed-off-by: Derrick Stolee <[email protected]>
Add some tests based on the current behavior, doing interesting checks for different sets of branches, ranges, and the --boundary option. This sets a baseline for the behavior and we can extend it as new options are introduced. Store and output a 'batch_nr' value so we can demonstrate that the paths are grouped together in a batch and not following some other ordering. This allows us to test the depth-first behavior of the path-walk API. However, we purposefully do not test the order of the objects in the batch, so the output is compared to the expected output through a sort. It is important to mention that the behavior of the API will change soon as we start to handle UNINTERESTING objects differently, but these tests will demonstrate the change in behavior. Signed-off-by: Derrick Stolee <[email protected]>
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. We re-use the empty string for this set of objects because these are passed directly to the callback function instead of being part of the 'path_stack'. Future changes will add the ability to visit annotated tags. Signed-off-by: Derrick Stolee <[email protected]>
The rev_info that is specified for a path-walk traversal may specify visiting tag refs (both lightweight and annotated) and also may specify indexed objects (blobs and trees). Update the path-walk API to walk these objects as well. When walking tags, we need to peel the annotated objects until reaching a non-tag object. If we reach a commit, then we can add it to the pending objects to make sure we visit in the commit walk portion. If we reach a tree, then we will assume that it is a root tree. If we reach a blob, then we have no good path name and so add it to a new list of "tagged blobs". When the rev_info includes the "--indexed-objects" flag, then the pending set includes blobs and trees found in the cache entries and cache-tree. The cache entries are usually blobs, though they could be trees in the case of a sparse index. The cache-tree stores previously-hashed tree objects but these are cleared out when staging objects below those paths. We add tests that demonstrate this. The indexed objects come with a non-NULL 'path' value in the pending item. This allows us to prepopulate the 'path_to_lists' strmap with lists for these paths. The tricky thing about this walk is that we will want to combine the indexed objects walk with the commit walk, especially in the future case of walking objects during a command like 'git repack'. Whenever possible, we want the objects from the index to be grouped with similar objects in history. We don't want to miss any paths that appear only in the index and not in the commit history. Thus, we need to be careful to let the path stack be populated initially with only the root tree path (and possibly tags and tagged blobs) and go through the normal depth-first search. Afterwards, if there are other paths that are remaining in the paths_to_lists strmap, we should then iterate through the stack and visit those objects recursively. Signed-off-by: Derrick Stolee <[email protected]>
When the input rev_info has UNINTERESTING starting points, we want to be sure that the UNINTERESTING flag is passed appropriately through the objects. To match how this is done in places such as 'git pack-objects', we use the mark_edges_uninteresting() method. This method has an option for using the "sparse" walk, which is similar in spirit to the path-walk API's walk. To be sure to keep it independent, add a new 'prune_all_uninteresting' option to the path_walk_info struct. To check how the UNINTERSTING flag is spread through our objects, extend the 'test-tool path-walk' command to output whether or not an object has that flag. This changes our tests significantly, including the removal of some objects that were previously visited due to the incomplete implementation. Signed-off-by: Derrick Stolee <[email protected]>
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 11/4/24 7:11 PM, Junio C Hamano wrote:
> Jeff King <[email protected]> writes:
> >> On Mon, Nov 04, 2024 at 10:48:49AM -0500, Derrick Stolee wrote:
>>> I disagree that all environments will prefer the --full-name-hash. I'm
>>> currently repeating the performance tests right now, and I've added one.
>>> The issues are:
>>>
>>> 1. The --full-name-hash approach sometimes leads to a larger pack when
>>> using "git push" on the client, especially when the name-hash is
>>> already effective for compressing across paths.
>>
>> That's interesting. I wonder which cases get worse, and if a larger
>> window size might help. I.e., presumably we are pushing the candidates
>> further away in the sorted delta list.
I think the cases that make things get worse with --full-name-hash are:
1. The presence of renames, partitioning objects that used to fit into
the same bucket (in the case of directory renames).
2. Some standard kinds of files may appear several times across the
tree but do not change very often and are similar across path.
3. Common patterns across similar file types, such as similar includes
in .c and .h files or other kinds of boilerplate in different
languages.
A larger window size will always expand the range of possible deltas, at
a cost to the time taken to compute the deltas. My first experiment in
these repositories was to increase the window size to 250 (from the default
10). This caused a very slow repack, but the repositories shrunk.
For example, the big Javascript monorepo that repacked to ~100 GB with
default settings would repack to ~30 GB with --window=250. This was an
indicator that delta compression would work if we can find the right pairs
to use for deltas.
The point of the two strategies (--full-name-hash and --path-walk) is
about putting objects close to each other in better ways than the name
hash sort.
>>> 2. A depth 1 shallow clone cannot use previous versions of a path, so
>>> those situations will want to use the normal name hash. This can be
>>> accomplished simply by disabling the --full-name-hash option when
>>> the --shallow option is present; a more detailed version could be
>>> used to check for a large depth before disabling it. This case also
>>> disables bitmaps, so that isn't something to worry about.
>>
>> I'm not sure why a larger hash would be worse in a shallow clone. As you
>> note, with only one version of each path the name-similarity heuristic
>> is not likely to buy you much. But I'd have thought that would be true
>> for the existing name hash as well as a longer one. Maybe this is the
>> "over-emphasizing" case.
I'm confused by your wording of "larger hash" because the hash size
is the exact same: 32 bits. It's just that the --full-name-hash option
has fewer collisions by abandoning the hope of locality.
In a depth 1 shallow clone, there are no repeated paths, so any hash
collisions are true collisions instead of good candidates for deltas.
The full name hash is essentially random, so the delta compression
algorithm basically says:
1. Sort by type.
2. Within each type, sort the objects randomly.
With that sort, the delta compression scan is less effective than the
standard name hash.
> I too am curious to hear Derrick explain the above points and what
> was learned from the performance tests. The original hash was
> designed to place files that are renamed across directories closer
> to each other in the list sorted by the name hash, so a/Makefile and
> b/Makefile would likely be treated as delta-base candidates while
> foo/bar.c and bar/foo.c are treated as unrelated things. A push
> of a handful of commits that rename paths would likely place the
> rename source of older commits and rename destination of newer
> commits into the same delta chain, even with a smaller delta window.
> In such a history, uniformly-distributed-without-regard-to-renames
> hash is likely to make them into two distinct delta chains, leading
> to less optimal delta-base selection.
Yes. This is the downside of the --full-name-hash compared to the
standard name hash. When repacking an entire repository, the effect
of these renames is typically not important in the long run as it's
basically a single break in the delta chain. The downside comes in
when doing a small fetch or push where the rename has more impact.
The --path-walk approach does not suffer from this problem because
it has a second pass that sorts by the name hash and looks for
better deltas than the ones that already exist. Thus, it gets the
best of both worlds.
The performance impact of the two passes of the --path-walk
approach is interesting, as you'd typically expect this to always
be slower. However:
1. The delta compression within each batch only compares the
objects within that batch. We do not compare these objects to
unrelated objects, which can be expensive and wasteful. This
also means that small batches may even be smaller than the
delta window, reducing the number of comparisons.
2. In the second pass, the delta calculation can short-circuit if
the computed delta would be larger than the current-best delta.
Thus, the good deltas from the first pass make the second pass
faster.
> A whole-repository packing, or a large push or fetch, of the same
> history with renamed files are affected a lot less by such negative
> effects of full-name hash. When generating a pack with more commits
> than the "--window", use of the original hash would mean blobs from
> paths that share similar names (e.g., "Makefile"s everywhere in the
> directory hierarchy) are placed close to each other, but full-name
> hash will likely group the blobs from exactly the same path and
> nothing else together, and the resulting delta-chain for identical
> (and not similar) paths would be sufficiently long. A long delta
> chain has to be broken into multiple chains _anyway_ due to finite
> "--depth" setting, so placing blobs from each path into its own
> (initial) delta chain, completely ignoring renamed paths, would
> likely to give us long enough (initial) delta chain to be split at
> the depth limit.
> > It would lead to a good delta-base selection with smaller window
> size quite efficiently with full-name hash.>
> I think a full-name hash forces a single-commit pack of a wide tree
> to give up on deltified blobs, but with the original hash, at least
> similar and common files (e.g. Makefile and COPYING) would sit close
> together in the delta queue and can be deltified with each other,
> which may be where the inefficiency comes from when full-name hash
> is used.
Yes, this is a good summary of why this works for the data
efficiency in long histories. Your earlier observations are why
the full-name hash has demonstrated issues with smaller time scales.
These numbers are carefully detailed in the performance tests in
the refreshed series [1]. The series also has a way to disable the
full-name hash when serving a shallow clone for this reason.
[1] https://lore.kernel.org/git/[email protected]/
Thanks,
-Stolee |
derrickstolee
force-pushed
the
api-upstream
branch
from
0bb607e
to
a4aaa3b
Compare
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <[email protected]> writes:
>> Jeff King <[email protected]> writes:
>>
>>> That's interesting. I wonder which cases get worse, and if a larger
>>> window size might help. I.e., presumably we are pushing the candidates
>>> further away in the sorted delta list.
>
> I think the cases that make things get worse with --full-name-hash are:
>
> 1. The presence of renames, partitioning objects that used to fit into
> the same bucket (in the case of directory renames).
>
> 2. Some standard kinds of files may appear several times across the
> tree but do not change very often and are similar across path.
>
> 3. Common patterns across similar file types, such as similar includes
> in .c and .h files or other kinds of boilerplate in different
> languages.
> ...
> In a depth 1 shallow clone, there are no repeated paths, so any hash
> collisions are true collisions instead of good candidates for deltas.
Or #2 and #3 above, where large boilerplates are shared across
similarly named files.
> Yes. This is the downside of the --full-name-hash compared to the
> standard name hash. When repacking an entire repository, the effect
> of these renames is typically not important in the long run as it's
> basically a single break in the delta chain. The downside comes in
> when doing a small fetch or push where the rename has more impact.
Yes. Due to --depth limit, we need to break delta chains somewhere
anyway, and a rename boundary is just as good place as any other in
a sufficiently long chain.
> The --path-walk approach does not suffer from this problem because
> it has a second pass that sorts by the name hash and looks for
> better deltas than the ones that already exist. Thus, it gets the
> best of both worlds.
Yes, at the cost of being more complex :-)
> The performance impact of the two passes of the --path-walk
> approach is interesting, as you'd typically expect this to always
> be slower. However:
>
> 1. The delta compression within each batch only compares the
> objects within that batch. We do not compare these objects to
> unrelated objects, which can be expensive and wasteful. This
> also means that small batches may even be smaller than the
> delta window, reducing the number of comparisons.
Interesting.
> 2. In the second pass, the delta calculation can short-circuit if
> the computed delta would be larger than the current-best delta.
> Thus, the good deltas from the first pass make the second pass
> faster.
Yes, the early give-up codepath helps when you already found
something semi-decently good delta base.
Thanks for a good summary. |
|
This patch series was integrated into seen via git@7b2f6a0. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 11/10/24 9:56 PM, Junio C Hamano wrote:
> Derrick Stolee <[email protected]> writes:
>> The --path-walk approach does not suffer from this problem because
>> it has a second pass that sorts by the name hash and looks for
>> better deltas than the ones that already exist. Thus, it gets the
>> best of both worlds.
> > Yes, at the cost of being more complex :-)
True. I hope that the results sufficiently justify the complexity.
Further, the later uses of the path-walk API (git backfill and git
survey) build upon this up-front investment.
Thanks,
-Stolee
|
|
On the Git mailing list, Jeff King wrote (reply to this): On Mon, Nov 11, 2024 at 11:56:01AM +0900, Junio C Hamano wrote:
> > Yes. This is the downside of the --full-name-hash compared to the
> > standard name hash. When repacking an entire repository, the effect
> > of these renames is typically not important in the long run as it's
> > basically a single break in the delta chain. The downside comes in
> > when doing a small fetch or push where the rename has more impact.
>
> Yes. Due to --depth limit, we need to break delta chains somewhere
> anyway, and a rename boundary is just as good place as any other in
> a sufficiently long chain.
We don't necessarily have to break the chains due to depth limits,
because they are not always linear. They can end up as bushy trees,
like:
A <- B <- C
\
<- D <- E
\
<- F
We might get that way because it mirrors the shape of history (e.g., if
D and E happened on a side branch from B and C). But we can also get
there from a linear history by choosing a delta that balances size
versus depth. In the above example, the smallest sequence might be:
A <- B <- C <- D <- E <- F
but if we have a limit of 3 and A-B-C already exists, then we might
reject the C-D delta and choose A-D instead (or I guess if it really is
linear, probably B-D is more likely). That may be a larger delta by
itself, but it is still better than storing a full copy of the object.
And we find it by having those candidates close together in the sorted
list, reject C-D based on depth, and then moving on to the next
candidate.
I'm not sure in practice how often we find these kinds of deltas. If you
look at, say, all the deltas for "Makefile" in git.git like this:
git rev-list --objects --all --reflog --full-history -- Makefile |
perl -lne 'print $1 if /(.*) Makefile/' |
git cat-file --batch-check='%(objectsize:disk) %(objectname) %(deltabase)'
my repo has 33 full copies (you can see non-deltas by grepping for
"0{40}$" in the output) out of 4473 total. So it's not like we never
break chains. But we can use graphviz to visualize it by piping the
above through:
perl -alne '
BEGIN { print "digraph {" }
print "node_$F[1] [label=$F[0]]";
print "node_$F[1] -> node_$F[2]" if $F[2] !~ /^0+$/;
END { print "}" }
'
and then piping it through "dot" to make an svg, or using an interactive
viewer like "xdot" (the labels are the on-disk size of each object). I
see a lot of wide parts of the graph in the output.
Of course this may all depend on packing patterns, too. I did my
investigations after running "git repack -adf" to generate what should
be a pretty reasonable pack. You might see something different from
incremental repacking over time.
I'm not sure what any of this means for --path-walk, of course. ;)
Ultimately we care about resulting size and time to compute, so if it
can do better on those metrics then it doesn't matter what the graph
looks like. But maybe those tools can help us understand where things
might go better (or worse) with various approaches.
-Peff |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, Nov 08, 2024 at 10:17:24AM -0500, Derrick Stolee wrote:
> > > I'm not sure why a larger hash would be worse in a shallow clone. As you
> > > note, with only one version of each path the name-similarity heuristic
> > > is not likely to buy you much. But I'd have thought that would be true
> > > for the existing name hash as well as a longer one. Maybe this is the
> > > "over-emphasizing" case.
>
> I'm confused by your wording of "larger hash" because the hash size
> is the exact same: 32 bits. It's just that the --full-name-hash option
> has fewer collisions by abandoning the hope of locality.
>
> In a depth 1 shallow clone, there are no repeated paths, so any hash
> collisions are true collisions instead of good candidates for deltas.
> The full name hash is essentially random, so the delta compression
> algorithm basically says:
>
> 1. Sort by type.
> 2. Within each type, sort the objects randomly.
>
> With that sort, the delta compression scan is less effective than the
> standard name hash.
Ah, OK. I'm sorry, I had not really investigated the full-name-hash and
misunderstood what it was doing. I thought we were using a larger hash
in order to give more locality hints. I.e., to let us distinguish
"foo/long-path.c" from "bar/long-path.c", but still retain some locality
between them.
But it is throwing away all locality outside of the exact name. So yes,
it would never find a rename from "foo/" to "bar/", as those mean the
name-hash is effectively random.
So I guess getting back to what Taylor and I had talked about off-list:
we were wondering if there was a way to provide a better "slider" for
locality as part of the normal delta candidate sorting process. I think
you could store the full pathname and then doing a sort based on the
reverse of the string (so "foo.c" and "bar.c" would compare "c", then
".", etc). And that would let you tell the difference between
"foo/long-path.c" and "bar/long-path.c" (preferring the identical
filenames over the merely similar ones), but still sort them together
versus "some-unrelated-path.c".
And what I wondered (and what I had initially thought full-name-hash was
doing) was whether we could store some fixed-size hash that produces a
similar distribution (modulo collisions) to what that reverse-name sort
would do.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Jeff King <[email protected]> writes:
>> Yes. Due to --depth limit, we need to break delta chains somewhere
>> anyway, and a rename boundary is just as good place as any other in
>> a sufficiently long chain.
>
> We don't necessarily have to break the chains due to depth limits,
> because they are not always linear. They can end up as bushy trees,
True. And being able to pair blobs before and after a rename will
give us more candidates to place in a single bushy tree, so in that
sense, with a short segment of history, it is understandable that
the full-name hash fails to have as many candidates as the original
hash gives us. But with sufficiently large number of blobs at the
same path that are similar (i.e. not a "pushing a short segment of
history", but an initial clone), splitting what could be one delta
family into two delta families at the rename boundary is not too
bad, as long as both halves have enough blobs to deltify against
each other.
> I'm not sure in practice how often we find these kinds of deltas. If you
> look at, say, all the deltas for "Makefile" in git.git like this:
>
> git rev-list --objects --all --reflog --full-history -- Makefile |
> perl -lne 'print $1 if /(.*) Makefile/' |
> git cat-file --batch-check='%(objectsize:disk) %(objectname) %(deltabase)'
>
> my repo has 33 full copies (you can see non-deltas by grepping for
> "0{40}$" in the output) out of 4473 total. So it's not like we never
> break chains. But we can use graphviz to visualize it by piping the
> above through:
>
> perl -alne '
> BEGIN { print "digraph {" }
> print "node_$F[1] [label=$F[0]]";
> print "node_$F[1] -> node_$F[2]" if $F[2] !~ /^0+$/;
> END { print "}" }
> '
>
> and then piping it through "dot" to make an svg, or using an interactive
> viewer like "xdot" (the labels are the on-disk size of each object). I
> see a lot of wide parts of the graph in the output.
>
> Of course this may all depend on packing patterns, too. I did my
> investigations after running "git repack -adf" to generate what should
> be a pretty reasonable pack. You might see something different from
> incremental repacking over time.
That is very true. I forgot that we do things to encourage bushy
delta-base selection. One thing I also am happy to see is the
effect of our "clever" delta-base selection, where the algorithm
does not blindly favor the delta-base that makes the resulting delta
absolutely minimal, but takes the depth of the delta-base into
account (i.e. a base at a much shallower depth is preferred over a
base near the depth limit, even if it results in a slightly larger
delta data).
> I'm not sure what any of this means for --path-walk, of course. ;)
> Ultimately we care about resulting size and time to compute, so if it
> can do better on those metrics then it doesn't matter what the graph
> looks like.
True, too. Another thing that we care about is the time to access
data, and favoring shallow delta chain, even with the help of the
in-core delta-base cache, has merit.
Thanks. |
|
This patch series was integrated into seen via git@92b8a39. |
|
This patch series was integrated into seen via git@48ed211. |
|
This patch series was integrated into seen via git@55c864b. |
|
This patch series was integrated into seen via git@f5a1600. |
|
This patch series was integrated into seen via git@a70389e. |
|
This patch series was integrated into seen via git@b2e52c6. |
|
This patch series was integrated into seen via git@40c428d. |
|
This patch series was integrated into seen via git@7a8b6cd. |
| the objects will be walked in a separate way based on those starting | ||
| commits. | ||
|
|
||
| Examples |
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Sat, Nov 09, 2024 at 07:41:09PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[email protected]>
>
> Add some tests based on the current behavior, doing interesting checks
> for different sets of branches, ranges, and the --boundary option. This
> sets a baseline for the behavior and we can extend it as new options are
> introduced.
>
> Store and output a 'batch_nr' value so we can demonstrate that the paths are
> grouped together in a batch and not following some other ordering. This
> allows us to test the depth-first behavior of the path-walk API. However, we
> purposefully do not test the order of the objects in the batch, so the
> output is compared to the expected output through a sort.
>
> It is important to mention that the behavior of the API will change soon as
> we start to handle UNINTERESTING objects differently, but these tests will
> demonstrate the change in behavior.
>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
Nice. I like the approach of implementing the API in a single commit,
and then demonstrating a trivial "caller" by way of a custom test
helper. I think that the artifact of having a test helper here is useful
on its own, but it also serves as a good example of how to use the API,
and provides something to actually test the implementation with.
I'm going to steal this pattern the next time I need to work on
something that necessitates a complex new API ;-).
> +static int emit_block(const char *path, struct oid_array *oids,
> + enum object_type type, void *data)
> +{
> + struct path_walk_test_data *tdata = data;
> + const char *typestr;
> +
> + switch (type) {
> + case OBJ_TREE:
> + typestr = "TREE";
> + tdata->tree_nr += oids->nr;
> + break;
> +
> + case OBJ_BLOB:
> + typestr = "BLOB";
> + tdata->blob_nr += oids->nr;
> + break;
> + default:
> + BUG("we do not understand this type");
> + }
I think you could write this as:
if (type == OBJ_TREE)
tdata->tree_nr += oids->nr;
else if (type == OBJ_BLOB)
tdata->blob_nr += oids->nr;
else
BUG("we do not understand this type");
typestr = type_name(type);
Which DRYs things up a bit and uses the type_name() helper. That will
give you strings like "tree" and "blob" instead of "TREE" and "BLOB",
but I'm not sure if the casing is important.
Thanks,
Taylor
| + | ||
| It is also important that you do not specify the `--objects` flag for the | ||
| `revs` struct. The revision walk should only be used to walk commits, and | ||
| the objects will be walked in a separate way based on those starting |
There was a problem hiding this comment.
On the Git mailing list, Taylor Blau wrote (reply to this):
On Sat, Nov 09, 2024 at 07:41:10PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/path-walk.c b/path-walk.c
> index 24cf04c1e7d..2ca08402367 100644
> --- a/path-walk.c
> +++ b/path-walk.c
> @@ -98,6 +98,10 @@ static int add_children(struct path_walk_context *ctx,
> if (S_ISGITLINK(entry.mode))
> continue;
>
> + /* If the caller doesn't want blobs, then don't bother. */
> + if (!ctx->info->blobs && type == OBJ_BLOB)
> + continue;
> +
I was going to ask why we're not reusing the existing property of the
rev_info struct to specify what object type(s) we do/don't want, but the
answer is obvious: we don't want that to change the behavior of the
commit-level walk which is used to seed the actual path walk based on
its results.
However, it would be kind of nice to have a single place to specify how
you want to traverse objects, and using the rev_info struct seems like a
good choice there since you can naively ask Git to parse command-line
arguments like --blobs, --trees, --objects, etc. and set the appropriate
bits.
I wonder if it might make sense to do something like:
unsigned int tmp_blobs = revs->blob_objects;
unsigned int tmp_trees = revs->tree_objects;
unsigned int tmp_tags = revs->tag_objects;
if (prepare_revision_walk(revs))
die(_("failed to setup revision walk"));
/* commit-level walk */
revs->blob_objects = tmp_blobs;
revs->tree_objects = tmp_trees;
revs->tag_objects = tmp_tags;
/* path-level walk */
I don't have strong feelings about it, but it feels like this approach
could be cleaner from a caller's perspective. But I can see an argument
to the contrary that it does introduce some awkwardness with the
dual-use of those fields.
Thanks,
Taylor
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Sat, Nov 09, 2024 at 07:41:06PM +0000, Derrick Stolee via GitGitGadget wrote:
> Derrick Stolee (6):
> path-walk: introduce an object walk by path
> test-lib-functions: add test_cmp_sorted
> t6601: add helper for testing path-walk API
> path-walk: allow consumer to specify object types
> path-walk: visit tags and cached objects
> path-walk: mark trees and blobs as UNINTERESTING
My apologies for taking so long to review this. Having rad through the
patches in detail, a couple of thoughts:
- First, I like the structure that you decided on for this series. It
nicely demonstrates a minimal caller for this new API instead of
implementing a bunch of untested code. I think that's a great way to
lay out things up until this point.
- Second, I read through the existing API and only had minor comments.
I read through the implementation in detail and found it to match my
expectation of how each step should function.
So my take-away from spending a few hours with this series is that
everything seems on track so far, and I think this is in a good spot to
build on for more path-walk features.
That all said, I am still not totally sold on the idea that we need a
separate path-based traversal given the significant benefits of the
full-name hash approach that I reviewed earlier today.
To be clear, I am totally willing to believe that there are some
benefits to doing the path-walk approach on top, but I think we should
consider those benefits relative to the large amount of highly
non-trivial code that we're adding in order to power it.
So I'm not strongly opposed or in favor of the approach pursued starting
in this series, I just think that it's worth spending time as a group
(beyond just you and I) considering the benefits and costs associated
with it.
As for the patches themselves, I think that cooking them for a long time
in 'next' makes most sense. We will want to land this patch series if
and only if we decide that the traversal powered by this API is the
right approach. IOW, I don't think it makes sense to have the path-walk
stuff in the tree if it has no callers outside of the test helper
provided by this series.
OK. I think that's a good stopping point for me on the list today, and I
look forward to your responses :-).
Thanks,
Taylor |
|
This patch series was integrated into seen via git@f9d124a. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction and relation to prior series
This is a new series that rerolls the initial "path-walk API" patches of my RFC [1] "Path-walk API and applications". This new API (in path-walk.c and path-walk.h) presents a new way to walk objects such that trees and blobs are walked in batches according to their path.
This also replaces the previous version of ds/path-walk that was being reviewed in [2]. The consensus was that the series was too long/dense and could use some reduction in size. This series takes the first few patches, but also makes some updates (which will be described later).
[1] https://lore.kernel.org/git/[email protected]/
[RFC] Path-walk API and applications
[2] https://lore.kernel.org/git/[email protected]/
[PATCH v2 00/17] pack-objects: add --path-walk option for better deltas
This series only introduces the path-walk API, but does so to the full complexity required to later add the integration with
git pack-objectsto improve packing compression in both time and space for repositories with many name hash collisions. The compression also at least improves for other repositories, but may not always have an improvement in time.Some of the changes that are present in this series that differ from the previous version are motivated directly by discoveries made by testing the feature in Git for Windows and microsoft/git forks that shipped these features for fast delivery of these improvements to users who needed them. That testing across many environments informed some things that needed to be changed, and in this series those changes are checked by tests in the t6601-path-walk.sh test script and the
test-tool path-walktest helper. Thus, the code being introduced in this series is covered by tests even though it is not integrated into thegitexecutable.Discussion of follow-up applications
By splitting this series out into its own, I was able to reorganize the patches such that each application can be build independently off of this series. These are available as pending PRs in gitgitgadget/git:
Better delta compression with 'git pack-objects' [3]: This application allows an option in 'git pack-objects' to change how objects are walked in order to group objects with the same path for early delta compression before using the name hash sort to look for cross-path deltas. This helps significantly in repositories with many name-hash collisions. This reduces the size of 'git push' pacifies via a config option and reduces the total repo size in 'git repack'.
The 'git backfill' command [4]: This command downloads missing blobs in a bloodless partial clone. In order to save space and network bandwidth, it assumes that objects at a common path are likely to delta well with each other, so it downloads missing blobs in batches via the path-walk API. This presents a way to use blobless clones as a pseudo-resumable clone, since the initial clone of commits and trees is a smaller initial download and the batch size allows downloading blobs incrementally. When pairing this command with the sparse-checkout feature, the path-walk API is adjusted to focus on the paths within the sparse-checkout. This allows the user to only download the files they are likely to need when inspecting history within their scope without downloading the entire repository history.
The 'git survey' command [5]. This application begins the work to mimic the behavior of git-sizer, but to use internal data structures for better performance and careful understanding of how objects are stored. Using the path-walk API, paths with many versions can be considered in a batch and sorted into a list to report the paths that contribute most to the size of the repository. A version of this command was used to help confirm the issues with the name hash collisions. It was also used to diagnose why some repacks using the --path-walk option were taking more space than without for some repositories. (More on this later.)
Question for reviewers: I am prepped to send these three applications to the mailing list, but I'll refrain for now to avoid causing too much noise for folks. Would you like to see them on-list while this series is under review? Or would you prefer to explore the PRs ([3] [4] and [5])?
[3] #1819
PATH WALK II: Add --path-walk option to 'git pack-objects'
[4] #1820
PATH WALK III: Add 'git backfill' command
[5] #1821
PATH WALK IV: Add 'git survey' command
Structure of the Patch Series
This patch series attempts to create the simplest version of the API in patch 1, then build functionality incrementally. During the process, each change will introduce an update to:
The core of the API relies on using a 'struct rev_info' to define an initial set of objects and some form of a commit walk to define what range of objects to visit. Initially, only a subset of 'struct rev_info' options work as expected. For example:
Changes in v1 (since previous version)
There are a few hard-won learnings from previous versions of this series due to testing this in the wild with many different repositories.
Initially, the 'git pack-objects --path-walk' feature was not tested with the '--shallow' option because it was expected that this option was for servers creating a pack containing shallow commits. However, this option is also used when pushing from a shallow clone, and this was a critical feature that we needed to reduce the size of commits pushed from automated environments that were bootstrapped by shallow clones. The crux of the change is in Patch 5 and how UNINTERESTING objects are handled. We no longer need to push the UNINTERESTING flag around the objects ourselves and can use existing logic in list-objects.c to do so. This allows using the --objects-edge-aggressive option when necessary to reduce the object count when pushing from a shallow clone. (The pack-objects series expands on tests to cover this integration point.)
When looking into cases where 'git repack -adf' outperformed 'git repack -adf --path-walk', I discovered that the issue did not reproduce in a bare repository. This is due to 'git repack' iterating over all indexed objects before walking commits. I had inadvertently put all indexed objects in their own category, leading to no good deltas with previous versions of those files; I had also not used the 'path' option from the pending list, so these objects had invalid name hash values. You will see in patch 4 that the pending list is handled quite differently and the '--indexed-objects' option is tested directly within t6601.
I added a new 'test_cmp_sorted' helper because I wanted to simplify some repeated sections of t6601.
Patch 1 has significantly more context than it did before.
Annotated tags are given a name of "/tags" to differentiate them slightly from root trees and commits.
Changes in v2
Updated the test helper to output the batch number, allowing us to confirm that OIDs are grouped appropriately. This also signaled a few cases where the callback function was being called on an empty set.
This change has resulted in significant changes to the test data, including reordered lines and prepended batch numbers.
Thanks to Patrick for providing a recommended change to remove memory leaks from the test helper.
Thanks, -Stolee
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: karthik nayak [email protected]