Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

Matthijs Kooijman [off-list ref] writes:

[administrivia: you seem to have mail-followup-to that points at you
and the list; is that really needed???]

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.

Do you mean "smaller" (not "bigger")?

Hmph.

In your discussion (including the comment), you talk about "shallow
root" (I think that is the same as what we call "shallow boundary"),
but in this added block, there is nothing that checks CLIENT_SHALLOW
or SHALLOW flags to special case that.

Is it a good idea to unconditionally do this for all "have"
revisions?

Also there is another loop that iterates over "have" revisions just
above the precontext.  I wonder if this added code belongs in that
loop.

Hi Junio,

[administrivia: you seem to have mail-followup-to that points at you
and the list; is that really needed???]

I'm not subscribed to the list, so yes :-)

quoted

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.

Do you mean "smaller" (not "bigger")?

Yes, I meant smaller (reworded this first sentence a few times and then messed
up :-)

quoted

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);
+	}

Hmph.

In your discussion (including the comment), you talk about "shallow
root" (I think that is the same as what we call "shallow boundary"),

I think so, yes. I mean to refer to the commits referenced in
.git/shallow, that have their parents "hidden".

but in this added block, there is nothing that checks CLIENT_SHALLOW
or SHALLOW flags to special case that.

Is it a good idea to unconditionally do this for all "have"
revisions?

That's what I meant in my mail with "applying the fix unconditionally" -
there is probably some check needed (I discussed a few options in the
mail as well).

Note that this entire do_rev_list function is only called when there are
shallow revisions involved, so there is also a basic "only when shallow"
check in place.

Also there is another loop that iterates over "have" revisions just
above the precontext.  I wonder if this added code belongs in that
loop.

I think we could add it there, yes. On the other hand, if we only want
to execute this code when there are shallow boundaries in the list of
revisions to send (as I suggested in my previous mail), then we can't
move this code up.

Gr.

Matthijs

Hi Junio,

I haven't got a reply to my mail yet. Could you have a look, so I can
update and resubmit my patch?

On Fri, Jul 12, 2013 at 09:11:57AM +0200, Matthijs Kooijman wrote:

quoted

[administrivia: you seem to have mail-followup-to that points at you
and the list; is that really needed???]
In your discussion (including the comment), you talk about "shallow
root" (I think that is the same as what we call "shallow boundary"),

I think so, yes. I mean to refer to the commits referenced in
.git/shallow, that have their parents "hidden".

Could you confirm that I got the terms right here (or is the shallow
boundary the first hidden commit?)

quoted

but in this added block, there is nothing that checks CLIENT_SHALLOW
or SHALLOW flags to special case that.

Is it a good idea to unconditionally do this for all "have"
revisions?

That's what I meant in my mail with "applying the fix unconditionally" -
there is probably some check needed (I discussed a few options in the
mail as well).

Note that this entire do_rev_list function is only called when there are
shallow revisions involved, so there is also a basic "only when shallow"
check in place.

My proposal was to only apply the fix for all have revisions when the
previous history traversal came across some shallow boundary commits. If
this happens, then that shallow boundary commit will be a "new" one and
it will have prevented the history traversal from finding the full list
of relevant "have" commits. In this case, we should just use all "have"
commits instead.

Now, looking at the code, I see a few options for detecting this case:

 1 Modify mark_edges_uninteresting to return a boolean (or have an
   output argument) if any of the commits in the list of commits to find
   (not the edges) is a shallow boundary.
 2 Modify mark_edges_uninteresting to have a "show_shallow" argument
   that gets called for every shallow boundary. The show_shallow
   function passed would then simply keep a boolean if it is passed at
   least once.
 3 Add another loop over the commits _after_ the call to
   mark_edges_uninteresting, that simply looks for any shallow boundary
   commit.

The last option seems sensible to me, since it prevents modifying the
somewhat generic mark_edges_uninteresting function for this specific
usecase. On the other hand, it does mean that the list of commits is
looped twice, not sure what that means for performance.

Before I go and implement one of these, which option seems best to you?

Gr.

Matthijs

Matthijs Kooijman [off-list ref] writes:

quoted

quoted

In your discussion (including the comment), you talk about "shallow
root" (I think that is the same as what we call "shallow boundary"),

I think so, yes. I mean to refer to the commits referenced in
.git/shallow, that have their parents "hidden".

Could you confirm that I got the terms right here (or is the shallow
boundary the first hidden commit?)

As long as you are consistent it is fine. I _think_ boundary refers
to what is recorded in the .git/shallow file, so they are commits
that are missing from our repository, and their immediate children
are available.

My proposal was to only apply the fix for all have revisions when the
previous history traversal came across some shallow boundary commits. If
this happens, then that shallow boundary commit will be a "new" one and
it will have prevented the history traversal from finding the full list
of relevant "have" commits. In this case, we should just use all "have"
commits instead.

Now, looking at the code, I see a few options for detecting this case:

 1 Modify mark_edges_uninteresting to return a boolean (or have an
   output argument) if any of the commits in the list of commits to find
   (not the edges) is a shallow boundary.
 2 Modify mark_edges_uninteresting to have a "show_shallow" argument
   that gets called for every shallow boundary. The show_shallow
   function passed would then simply keep a boolean if it is passed at
   least once.
 3 Add another loop over the commits _after_ the call to
   mark_edges_uninteresting, that simply looks for any shallow boundary
   commit.

The last option seems sensible to me, since it prevents modifying the
somewhat generic mark_edges_uninteresting function for this specific
usecase. On the other hand, it does mean that the list of commits is
looped twice, not sure what that means for performance.

Before I go and implement one of these, which option seems best to you?

My gut feeling without looking at any patch is that the simplest
(i.e. 3.) would be the best among these three.

But I suspect, with any of these approaches, you would need to be
very careful futzing with the edge ones.  It may have an interesting
interactions with --thin transfer.

On Thu, Aug 8, 2013 at 8:01 AM, Junio C Hamano [off-list ref] wrote:

Matthijs Kooijman [off-list ref] writes:

quoted

quoted

quoted

In your discussion (including the comment), you talk about "shallow
root" (I think that is the same as what we call "shallow boundary"),

I think so, yes. I mean to refer to the commits referenced in
.git/shallow, that have their parents "hidden".

Could you confirm that I got the terms right here (or is the shallow
boundary the first hidden commit?)

As long as you are consistent it is fine. I _think_ boundary refers
to what is recorded in the .git/shallow file, so they are commits
that are missing from our repository, and their immediate children
are available.

Haven't found time to read the rest yet, but this I can answer.
.git/shallow records graft points. If a commit is in .git/shallow and
it exists in the repository, the commit is considered to have no
parents regardless of what's recorded in repository. So .git/shallow
refers to the new roots, not the missing bits.
-- 
Duy