Re: [PATCH] Make fetch-pack play nicer with servers which do not speak multi_ack

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 25, 2005, 09:51 UTC
Message-ID: <Pine.LNX.4.63.0510251104470.24174@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7vy84igfrl.fsf@assigned-by-dhcp.cox.net>

Hi,

On Tue, 25 Oct 2005, Junio C Hamano wrote:

Show 11 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > +		if (insert)
> > +			insert_by_date(commit, &rev_list);
> > +		else
> >  			commit_list_insert(commit, &rev_list);
> 
> Hmph.  I do not think insert_by_date and commit_list_insert on
> the same commit_list would play well together, since the former
> assumes that the list is already sorted by age (i.e. younger ones
> near the head, older ones close to the tail).

I´m sorry. The 2nd time, it should be &(rev_list_end->next), not &rev_list. (See my reply to myself). In effect, rev_list would be filled first by insert_by_date() with the refs we have and the known-to-be-common refs from the other side. From then on, only commit_list_insert would effectively *append* the ancestors. So it does play nicely. If there´s not yet another bug.

Show 10 quoted lines

> But you are right.  If you made 20 commits on top of my "master"
> branch head, we should send those 20 (and the commit you based
> on your changes, which the other end has), way before sending
> the ancient v0.99 tag.  Probably, we should never be sending
> v0.99 tag as "have" if we are going to send your "master" branch
> head, since the commit that tag refers to is reachable by your
> "master" branch head but there are a lot more commit between
> them, some of which will give us better "common" selected, and
> that v0.99 tag is what the other end said they have so is known
> to be ACKed if sent.

You´re right. Complete common refs are sent even if they are ancestors of other complete common refs. I´ll think about that.

Show 19 quoted lines

> Although we are doing a traversal on the fetch side to figure
> out the commit ancestry chain, I think we are not fully
> utilizing the information we receive from the other end.
> Typically we pull specific heads only, so get_remote_heads()
> culls the information about remote refs and you would not have a
> chance to know that v0.99 tag you have in your .git/refs/tags is
> something the remote also has, unless you asked for that
> particular tag.  I wonder if we can take advantage of that
> information without discarding it.  For example,
> get_remote_heads() itself knows the object name of v0.99.8 tag
> and the object name of the commit the tag refers to, and you
> have those objects locally already.  If you have many local
> branches, and v0.99.8 commit is reachable from the heads of all
> of your local branches, then you should be able to tell that you
> do not have to (and you do not want to) send "have" for v0.99.7
> and older tags even though they are also in your .git/refs/,
> since you know they are all reachable from v0.99.8 commit and
> likely to be worse common than v0.99.8.  The best common is
> somewhere between v0.99.8 and the tips of your local branches.

IOW v0.99.7 is an ancestor of v0.99.8, which is complete. Therefore, the former should not be sent. Remember that we do not have to send the optimal edges:

If v0.99.7 were the optimal common rev for some_branch, but we send v0.99.8, the server side will do a "git-rev-list some_branch ^v0.99.8", which has the same effect as if passing "^v0.99.7" instead.

So let´s change the definition of COMMON_REF to not include COMMON. Then, do not mark the COMMON_REFs in find_common, but before that. And mark their ancestors (as far as they were already parsed in everything_local()) as COMMON.

Oh, and pass nr_match=0 to get_remote_heads() and do the culling after using the information.

> This is a minor nit but I think you no longer need to have the
> rev_list_end in this version.  It is only used to maintain
> itself and not for any other purpose as far as I can see.

My mistake: the revs should be appended, not unshifted into rev_list.

If they got unshifted, that would mean that we first send all the ancestors of the youngest refs until we get a common rev, then all the ancestors of the 2nd youngest, etc.

When the revs are appended instead, first all the refs get sent (sorted by date), then the parents of them (sorted by the date of the refs), then their grand parents, etc.

While writing this, I cannot remember why I decided not to insert_by_date() all the time.

Ciao, Dscho