Re: [RFC/PATCH 0/2] merge-base: add --merge-child option

From: John Keeping <john@keeping.me.uk>
Date: May 12, 2013, 16:58 UTC
Message-ID: <20130512165829.GM2299@serenity.lan>
In-Reply-To: <20130512162823.GK2299@serenity.lan>

On Sun, May 12, 2013 at 05:28:24PM +0100, John Keeping wrote:

Show 26 quoted lines

> However, this doesn't seem to make a difference to the time taken when I
> add in --cherry-mark (which is why I was partially correct in the
> parallel thread - it doesn't have the effect on cherry-mark that I want
> it to):
> 
>     $ time git rev-list --ancestry-path --left-right --count --cherry-mark \
>             origin/master...git-gui/master 
>     2056    5       0
> 
>     real    0m32.266s
>     user    0m31.522s
>     sys     0m0.749s
> 
>     $ time git rev-list  --left-right --count --cherry-mark \
>             origin/master...git-gui/master
>     31959   5       0
> 
>     real    0m32.140s
>     user    0m31.337s
>     sys     0m0.807s
> 
> This seems to be caused by the code in revision.c::limit_list() which
> does the cherry detection then limits to left/right and only then
> applies the ancestry path.  I haven't looked further than that, but is
> there any reason not to apply the ancestry path restriction before
> looking for patch-identical commits?

With the patch below, the --ancestry-path version drops to under 2 seconds.

I'm not sure if this is a good idea though. It helps me say "I know nothing that isn't on the ancestry path can be patch-identical, so don't bother checking if it is" but it regresses users who want the full cherry-pick check while only limiting the output.

Perhaps we need --cherry-no-uninteresting to apply the first 3 hunks of the patch at runtime :-S

-- >8 --

diff --git a/revision.c b/revision.c
index de3b058..d721d83 100644
--- a/revision.c
+++ b/revision.c
@@ -837,7 +837,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 	for (p = list; p; p = p->next) {
 		struct commit *commit = p->item;
 		unsigned flags = commit->object.flags;
-		if (flags & BOUNDARY)
+		if (flags & (BOUNDARY | UNINTERESTING))
 			;
 		else if (flags & SYMMETRIC_LEFT)
 			left_count++;
@@ -858,7 +858,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		struct commit *commit = p->item;
 		unsigned flags = commit->object.flags;
 
-		if (flags & BOUNDARY)
+		if (flags & (BOUNDARY | UNINTERESTING))
 			continue;
 		/*
 		 * If we have fewer left, left_first is set and we omit
@@ -879,7 +879,7 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 		struct patch_id *id;
 		unsigned flags = commit->object.flags;
 
-		if (flags & BOUNDARY)
+		if (flags & (BOUNDARY | UNINTERESTING))
 			continue;
 		/*
 		 * If we have fewer left, left_first is set and we omit
@@ -1103,17 +1103,18 @@ static int limit_list(struct rev_info *revs)
 		show(revs, newlist);
 		show_early_output = NULL;
 	}
-	if (revs->cherry_pick || revs->cherry_mark)
-		cherry_pick_list(newlist, revs);
-
-	if (revs->left_only || revs->right_only)
-		limit_left_right(newlist, revs);
 
 	if (bottom) {
 		limit_to_ancestry(bottom, newlist);
 		free_commit_list(bottom);
 	}
 
+	if (revs->cherry_pick || revs->cherry_mark)
+		cherry_pick_list(newlist, revs);
+
+	if (revs->left_only || revs->right_only)
+		limit_left_right(newlist, revs);
+
 	/*
 	 * Check if any commits have become TREESAME by some of their parents
 	 * becoming UNINTERESTING.