Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Jeff King
On Wed, Aug 29, 2012 at 05:05:40PM -0400, Jeff King wrote: You would want this on top: [...] but t6024 still fails (it clearly is finding a different merge base than the test expects). I'll trace through it, but it will have to be later tonight. The problem in t6024 is caused by the fact

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Jeff King
On Thu, Aug 30, 2012 at 08:54:21AM -0400, Jeff King wrote: On Wed, Aug 29, 2012 at 05:05:40PM -0400, Jeff King wrote: You would want this on top: [...] but t6024 still fails (it clearly is finding a different merge base than the test expects). I'll trace through it, but it will have

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Jeff King
On Thu, Aug 30, 2012 at 09:03:27AM -0400, Jeff King wrote: So I was able to have my queue behave just like commit_list by fixing the stability issue. But I still have no clue what is going on in t6024. It does this for each commit it makes: [...] GIT_AUTHOR_DATE=2006-12-12 23:00:00 git

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Junio C Hamano
Jeff King p...@peff.net writes: Anyway, since this isn't yielding any performance benefit, I'm not going to go down that route. But stability of the queue is something that we need to consider if we ever do replace commit_list with a different data structure. Here's the patch to make the

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Jeff King
On Thu, Aug 30, 2012 at 09:23:25AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: Anyway, since this isn't yielding any performance benefit, I'm not going to go down that route. But stability of the queue is something that we need to consider if we ever do replace

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Jeff King
On Thu, Aug 30, 2012 at 09:33:48AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: The script originally comes from here: http://thread.gmane.org/gmane.comp.version-control.git/33566/focus=33852 and the discussion implies that the AUTHOR_DATEs were added to avoid a

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Junio C Hamano
Jeff King p...@peff.net writes: Compared to dropping an O(n^2) operation to O(lg n), that's probably not even going to be noticeable. Yeah, that is something we would want to use when we eventually update the main revision traverser, not this codepath localized to the merge-base. Thanks. I

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-30 Thread Junio C Hamano
Jeff King p...@peff.net writes: So the issue is that when you do a recursive merge with multiple bases, the order in which you visit the recursive bases is going to impact the exact conflicts you see. Yeah, that explains it. So the test is not broken or racy, which is good. It is just

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-29 Thread Junio C Hamano
Jeff King p...@peff.net writes: The merge-base functions internally keep a commit lists and insert by date, which causes a linear search of the commit list for each insertion. Let's use a priority queue instead. Unfortunately, from my experiments, this didn't actually cause any speedup.

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-29 Thread Jeff King
On Wed, Aug 29, 2012 at 09:36:43AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: The merge-base functions internally keep a commit lists and insert by date, which causes a linear search of the commit list for each insertion. Let's use a priority queue instead.

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-29 Thread Jeff King
On Wed, Aug 29, 2012 at 04:53:32PM -0400, Jeff King wrote: Thanks. This seems to break t6010-merge-base.sh for me, though... Interesting. It works fine here, even under --valgrind. Did you apply the patches directly on top of 1251cc7? Hmm, this does seem to break t6024 for me, though.

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-29 Thread Jeff King
On Wed, Aug 29, 2012 at 04:55:25PM -0400, Jeff King wrote: On Wed, Aug 29, 2012 at 04:53:32PM -0400, Jeff King wrote: Thanks. This seems to break t6010-merge-base.sh for me, though... Interesting. It works fine here, even under --valgrind. Did you apply the patches directly on top

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-29 Thread Jeff King
On Wed, Aug 29, 2012 at 05:00:32PM -0400, Jeff King wrote: Hmm, this does seem to break t6024 for me, though. Probably because: /* Clean up the result to remove stale ones */ - free_commit_list(list); - list = result; result = NULL; - while (list) { - struct

Re: [PATCH 2/2] commit: use a priority queue in merge base functions

2012-08-29 Thread Junio C Hamano
Jeff King p...@peff.net writes: +while (result.nr) +commit_list_append(queue_pop(result), tail); +queue_clear(result); +queue_clear(list); +return ret; I forgot to to port the STALE flag handling here. Figures.. Thanks. -- To unsubscribe from this list: send