Matthijs KooijmanHi folks,
while playing with shallow fetches, I've found that in some circumstances running git fetch with --depth can return too many objects (in particular, _all_ the objects for the requested revisions are returned, even when some of those objects are already known to the client).
This happens when a client issues a fetch with a depth bigger or equal to the number of commits the server is ahead of the client. In this case, the revisions to be sent over will be completely detached from any revisions the client already has (history-wise), causing the server to effectively ignore all objects the client has (as advertised using its have lines) and just send over _all_ objects (needed for the revisions it is sending over).
I've traced this down to the way do_rev_list in upload-pack.c works. If I've poured over the code enough to understand it, this is what happens: - The new shallow roots are made into graft points without parents. - The "want" commits are added to the pending list (revs->pending) - The "have" commits are marked uninteresting and added to the pending list - prepare_revision_walk is called, which adds everything from the pending list into the commmit list (revs->commits) - limit_list is called, which traverses the history of each interesting commit in the commit list (i.e., all want revisions), up to excluding the first uninteresting commit (i.e. a have revision). The result of this is the new commit list.
This means the commit list now contains all commits that the client wants, up to (excluding) any commits he already has or up to (including) any (new) shallow roots. - mark_edges_uninteresting is called, which marks the tree of every parent of each edge in the commit list as uninteresting (in practice, this marks the tree of each uninteresting parent, since those are by definition the only kinds of revisions that can be beyond the edge). - All trees and blobs that are referenced by trees in the commit list but are not marked as uninteresting, are passed to git-pack-objects to put into the pack.
Normally, the list of commits to send over is connected to the client's existing commits (which are marked as uninteresting). This means that only the trees of those uninteresting ("have") commits that are actually (direct) predecessors of the commits to send over are marked as uninteresting. This is probably useful, since it prevents having to go over all trees the client has (for other branches, for example) and instead limits to the trees that are the most likely to contain duplicate (or similar, for delta-ing) objects.
However, in the "detached shallow fetch" case, this assumption is no longer valid. There will be no uninteresting commits as parents for the commit list, since all edge commits will be shallow roots (hence have no parents). Ideally, one would find out which of the "detached" "have" revisions are the closest to the new shallow roots, but with the current code these shallow roots have their parents cut off long before this code even runs, so this is probably not feasible.
Instead, what we can do in this case, is simply mark the trees of all "have" commits as uninteresting. This prevents all objects that are contained in the "have" commits themselves from being sent to the client, which can be a big win for bigger repositories. Marking them all is is probably more work than strictly needed, but is easy to implement.
I have created a mockup patch which does this, and also adds a test case demonstrating the problem. Right now, the above fix is applied always, even in cases where it isn't needed.
Looking at the code, I think it would be good to let mark_edges_uninteresting look for shallow roots in the commit list (or perhaps just add another loop over the commit list inside do_rev_list) and only apply the fix if any shallow roots are in the commit list (meaning at least a part of the history to send over is detached from the clients current history). I haven't implemented this yet, wanting to get some feedback first.
Also, I'm not quite sure how this fits in with the concept of "thin packs". There might be some opportunities missing here as well, though git-pack-objects is called without --thin when shallow roots are involved. I think this is related to the "-" prefixed commit sha's that are sent to git-pack-objects, but I couldn't found any documentation on what the - prefix is supposed to mean.
(On a somewhat related note, show_commit in upload-pack.c checks the BOUNDARY flag, but AFAICS the revs->boundary flag is never set, so BOUNDARY cannot ever be set in this case either?)
How does this patch look?
Gr.
Matthijs
--- t/t5500-fetch-pack.sh | 11 +++++++++++ upload-pack.c | 8 ++++++++ 2 files changed, 19 insertions(+)
Show changes to 2 files +19 −0
t/t5500-fetch-pack.sh, upload-pack.c
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index fd2598e..a022d65 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -393,6 +393,17 @@ test_expect_success 'fetch in shallow repo unreachable shallow objects' ' git fsck --no-dangling ) ' +test_expect_success 'fetch creating new shallow root' ' + ( + git clone "file://$(pwd)/." shallow10 && + git commit --allow-empty -m empty && + cd shallow10 && + git fetch --depth=1 --progress 2> actual && + # This should fetch only the empty commit, no tree or + # blob objects + grep "remote: Total 1" actual + ) +' test_expect_success 'setup tests for the --stdin parameter' ' for head in C D E F diff --git a/upload-pack.c b/upload-pack.c index 59f43d1..5885f33 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -122,6 +122,14 @@ static int do_rev_list(int in, int out, void *user_data) if (prepare_revision_walk(&revs)) die("revision walk setup failed"); mark_edges_uninteresting(revs.commits, &revs, show_edge); + /* In case we create a new shallow root, make sure that all + * we don't send over objects that the client already has just + * because their "have" revisions are no longer reachable from + * the shallow root. */ + for (i = 0; i < have_obj.nr; i++) { + struct commit *commit = (struct commit *)have_obj.objects[i].item; + mark_tree_uninteresting(commit->tree); + } if (use_thin_pack) for (i = 0; i < extra_edge_obj.nr; i++) fprintf(pack_pipe, "-%s\n", sha1_to_hex(
-- 1.8.3.2.736.ge92bb95.dirty
