Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-28 Thread Junio C Hamano
Thomas Rast writes: > I'm also mildly surprised that it ended up being correct, albeit with > some extra work from you :-) I actually am not all that surprised. It just shows that the original code was layered in more or less the right way. At the the bottom layer we would want a way to paint

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-28 Thread Thomas Rast
Junio C Hamano writes: > Thomas Rast writes: > >> diff --git i/commit.c w/commit.c >> index 65a8485..70427ab 100644 >> --- i/commit.c >> +++ w/commit.c >> @@ -837,10 +837,13 @@ int in_merge_bases(struct commit *commit, struct >> commit **reference, int num) >> struct commit_list *bases, *b

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-27 Thread Junio C Hamano
Thomas Rast writes: > diff --git i/commit.c w/commit.c > index 65a8485..70427ab 100644 > --- i/commit.c > +++ w/commit.c > @@ -837,10 +837,13 @@ int in_merge_bases(struct commit *commit, struct commit > **reference, int num) > struct commit_list *bases, *b; > int ret = 0; > > -

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Junio C Hamano
Jeff King writes: > I thought you were just interested in speeding up is_descendent_of. You > should be able to do that without a generation number. Just start from A > and B as above, do the two-color painting, and do not add the parents of > any two-color commits (because you know they are ance

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Junio C Hamano
Thomas Rast writes: > Junio C Hamano writes: > ... >> Start from A and B. Follow from B to find 'x' and paint it in blue, >> follow from A to find 'y' and paint it in amber. Follow from 'x' to >> '1', paint it in blue. Follow from 'y' to '1', paint it in amber >> but notice that it already is

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Junio C Hamano
Thomas Rast writes: > Well, yeah, you snipped this part from my original post :-) > > } Even if this turns out to be flawed, we should also identify uses of > } in_merge_bases() where the real question was is_descendant_of() [I > } somewhat suspect that's all of them], and then replace is_descend

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Jeff King
On Fri, Aug 24, 2012 at 11:43:40AM +0200, Thomas Rast wrote: > > Start from A and B. Follow from B to find 'x' and paint it in blue, > > follow from A to find 'y' and paint it in amber. Follow from 'x' to > > '1', paint it in blue. Follow from 'y' to '1', paint it in amber > > but notice that i

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Nguyen Thai Ngoc Duy
On Thu, Aug 23, 2012 at 9:20 PM, Thomas Rast wrote: > At the very least it should be possible to change in_merge_bases() to > not do any of the post-filtering; perhaps like the patch below. It > passes the test suite. The whole "merge bases of A and a list of Bs" > thing is blowing my overheated

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Thomas Rast
Junio C Hamano writes: > Junio C Hamano writes: > >> Thomas Rast writes: >> >>> At the very least it should be possible to change in_merge_bases() to >>> not do any of the post-filtering; perhaps like the patch below. >> >> I do not recall the details but the post-filtering was added after >> t

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-24 Thread Thomas Rast
Junio C Hamano writes: > As a corollary, the "is pu@{0} a fast-forward of pu@{1}?" check does > not need merge base computation at all. The only thing it needs to > do is to prove pu@{1} is reachable from pu@{0}, and that can be done > the same way in which '1' can be proved unreachable from '2'

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-23 Thread Junio C Hamano
Junio C Hamano writes: > The objective of this second traversal is very different from the > first one, though. We do not need _all_ the merge bases between '1' > and '2'; we do not even need merge bases. > > The only thing we need to prove that '1' is a merge base (i.e. not > an ancestor of any

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-23 Thread Junio C Hamano
Junio C Hamano writes: > Thomas Rast writes: > >> At the very least it should be possible to change in_merge_bases() to >> not do any of the post-filtering; perhaps like the patch below. > > I do not recall the details but the post-filtering was added after > the initial naive version without it

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-23 Thread Junio C Hamano
Thomas Rast writes: > At the very least it should be possible to change in_merge_bases() to > not do any of the post-filtering; perhaps like the patch below. I do not recall the details but the post-filtering was added after the initial naive version without it that was posted to the list did no

Re: in_merge_bases() is too expensive for recent "pu" update

2012-08-23 Thread Thomas Rast
Nguyen Thai Ngoc Duy writes: > I just did a "git fetch". It took 19 seconds (measured with > gettimeofday) to finish in_merge_bases() in update_local_ref() in > fetch.c, just to print this line > > + a4f2db3...b95a282 pu -> origin/pu (forced update) > > It's partly my fault because I'm