[WORK-IN-PROGRESS] Introduce the path walk API into Git for Windows

[dscho]

This is a huge feature, and it would be good to let users play with it. To make it safer to play with, it needs to be guarded behind command-line/config options.

[dscho]

I only got around to looking at the backfill part yet, will continue reviewing later.

[dscho]

We might want to add this, out of an abundance of caution:

else if (list->type != type)
    warning("both blob and tree objects found at '%s'", path.buf);

[dscho]

Continuing on errors may be the right thing to do always. On the off-chance that a user may want to return early on error, we may want to add a flag for that.

[dscho]

I wonder whether this is safe. If the same path appears multiple times on the stack, e.g. if two merged branches separately removed the same file, wouldn't we then remove the paths_to_lists item prematurely? This might be intentional, though, as we stop once we're hitting a blob that we'd already seen. Therefore in a history like this:

... - A - B - C
        \   /
          D

where file.txt was present in A, deleted in B and D, and missing from C, we would probably hit the file in B first, walk its history all the way until the commit that added it, then remove the item from paths_to_lists, then re-add it when encountering it in D, see that we handled it in A already, and re-remove it. That might be what we need here, but I have not been able to wrap my head around this enough yet to be confident.

[derrickstolee]

It is important to realize that a path appears at most once. The walk goes like this:

1. Walk all commits that we are going to walk. Initialize the "stack" to include only the root path ("")
2. Walk all root trees that we are going to walk, initializing the "stack" with the list of all paths that appear as entries of those trees.
3. Continue walking the stack, knowing that we visit each path only once.

[dscho]

Ah, the crucial part I was missing is that we first walk all commits, build up the huge, giant list for "" (i.e. all the top-level trees). Then we iterate over all of those before going any further. Crucially, no single tree walk happens until every last of the commits has been walked.

For some reason, I got the impression that we're doing a depth-first search where we immediately iterate into the full tree of the first commit, until we exhausted all of the blobs reachable from that root tree, and only then walk to the next commit. But that's not what we do ;-)

We basically walk all the paths in a depth-first manner, but not in alphabetical order because we end up plucking off the paths from the top of the stack (i.e. the "alphabetically last" item seen so far, but that only holds true if the root tree has no child trees, otherwise the order gets messy real quickly). So the order is somewhat jumbled, I guess ;-)

Thank you for explaining this so patiently. I think my confusion might be a good motivation to add a little primer about the order to the commit message of the "path-walk: introduce an object walk by path" commit.

[derrickstolee]

I was working on a technical doc for the API but don't have it committed or pushed anywhere. This confirms that it is necessary.

[dscho]

For performance reasons, it might make sense to pass in base_path as a strbuf, so that only one scratch buffer is used for the entirety of the path walk (at least in single-threaded mode). But we can do that later.

[dscho]

Do we have to release the allocated memory somewhere?

[dscho]

Another batch of my review.

[dscho]

This should probably say "tags" instead of "initial"? And likewise the other path_fn("initial", ...) call may want to say "commits" instead?

And maybe even "tags" is misleading, because there could be blobs tracked under the file name tags? It is a valid path name, after all. We could play games by using a path that is invalid, such as .git/refs/tags (but what to do for commits? .git/refs/heads is wrong, they are not necessarily branch tips). Maybe better just use "".

The same goes for tagged-trees and tagged-blobs, too.

Also, do we want to prevent the function from being called with zero tags in case the caller asked for tags to be included but there were none to be found?

[derrickstolee]

This "initial" string comes from the way that this is emitted in the other object walk. I'll try to find pointers to that or reasoning for that to be important.

[dscho]

So that means that you have to look at the object type first in the callback, and only interpret the path parameter as actual path in the OBJ_BLOB case?

[derrickstolee]

Now, we want the path to matter for trees, too. When considering deltas in the pack-objects case, these paths don't matter for commits or tags because everything is sorted by type before compared by name-hash, so the set of deltas that are considered are the same in each case.

[dscho]

I fear that we have to handle tags in revs->pending first, accumulating the list before walking the commits, but then only processing them afterward (to avoid double-accounting for blobs that are both tagged directly and that are reachable via a tree).

I've just added a test helper in 489ce0c and used the left-over of t8100-*.sh -d:

$ ./bin-wrappers/test-tool -C ./t/trash\ directory.t8100-git-survey/ path-walk --no-blobs --tags --no-commits -v four
visiting path 'initial' (count: 0) of type 'tag'

$ git -C ./t/trash\ directory.t8100-git-survey/ rev-parse four four^0
dcd3534d11452b820a49532b41e7494f3ddfa2a8
5e53728740668d567dad45ca14d258b8f75fb7c1

$ ./bin-wrappers/test-tool -C ./t/trash\ directory.t8100-git-survey/ path-walk --no-blobs --tags --commits -v four
visiting path 'initial' (count: 7) of type 'commit'
        5e53728740668d567dad45ca14d258b8f75fb7c1
        6cf975cad5d4c3afe9d9c1beb519506ab26f2968
        5e51a08864ae54b185d9ccb441e3b84272a1d962
        bd080f6764f92d2e5f3c14d9ab1241495b919820
        123d744603d8c13997f7dd86b4f066cc68305762
        446a0a30049ffec3f6a1af16c07e8c0941b11660
        d8452bb98e031b9f6d1e3804a8cecc14e3124c9e
visiting path 'initial' (count: 0) of type 'tag'

As you can see, there is a tag object four but it is not reported because the revision walk removes it from the pending list.

[derrickstolee]

This tag stuff is the weakest part of the path-walk API and demonstrates the need for a git rev-list-like test helper that checks which objects are collected for each path and object type.

[dscho]

Hopefully the test helper I implemented is a good start?

[dscho]

Speaking from experience, we must prefix this with a message that the command is experimental and that its options and its output and operation are subject to frequent change. I.e. something like this.

[dscho]

This PR is superseded by #5171, #5172 and #5174.