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

Kevin Bracey [off-list ref] writes:

Although I realised after
sending my mail you could also use

   git log --ancestry-path --left-right E...F --not $(git merge-base
--all E F)

which looks like we're having to repeat ourselves because it's not
paying attention...

You are half wrong; "--left-right" is about "do we show the </>/=
marker in the output?", so it is true that it does not make sense
without "...", but the reverse is not true: A...B does not and
should not imply --left-right.

On 13/05/2013 01:22, Junio C Hamano wrote:

Kevin Bracey [off-list ref] writes:

quoted

    git log --ancestry-path --left-right E...F --not $(git merge-base
--all E F)

which looks like we're having to repeat ourselves because it's not
paying attention...

You are half wrong; "--left-right" is about "do we show the </>/=
marker in the output?", so it is true that it does not make sense
without "...", but the reverse is not true: A...B does not and
should not imply --left-right.

The repetition I meant is that by the definition of ancestry-path, the 
above would seem to be equivalent to

   git log --ancestry-path --left-right E F --not $(git merge-base --all E F) $(git merge-base --all E F)

Anyway, revised separated-out version of the patch follows.

Kevin

Kevin Bracey venit, vidit, dixit 13.05.2013 16:26:

On 13/05/2013 01:22, Junio C Hamano wrote:

quoted

Kevin Bracey [off-list ref] writes:

quoted

    git log --ancestry-path --left-right E...F --not $(git merge-base
--all E F)

which looks like we're having to repeat ourselves because it's not
paying attention...

You are half wrong; "--left-right" is about "do we show the </>/=
marker in the output?", so it is true that it does not make sense
without "...", but the reverse is not true: A...B does not and
should not imply --left-right.

The repetition I meant is that by the definition of ancestry-path, the 
above would seem to be equivalent to

   git log --ancestry-path --left-right E F --not $(git merge-base --all E F) $(git merge-base --all E F)

Anyway, revised separated-out version of the patch follows.

Kevin

It is certainly true that "git log --cherry" needs much less information
than what the merge base machinery provides. I've been experimenting
with that in order to get the speedup which is necessary for replacing
the "git cherry" code with calls into the revision walker using "--cherry".

But I can't wrap my head around the feature proposed here, sorry.

Michael

This patch is a revised form of the one in my "history traversal refinements"
series. Pulled out to allow it to proceed faster, given that John Keeping has
found a use for the command.

I suggest this is placed onto pu ahead of the existing series, dropping the
equivalent final commit there. And then hopefully this can proceed to next
faster.

(Dropping that commit will drop the only --ancestry-path A...B test in t6111,
meaning no immediate dependencies. But the next version of that series will be
sent with t6111 testing and expecting a pass due to this fix being in.)

Kevin Bracey (2):
  t6019: demonstrate --ancestry-path A...B breakage
  revision.c: treat A...B merge bases as if manually specified

 revision.c                        | 17 +++++++++++++++++
 revision.h                        |  1 +
 t/t6019-rev-list-ancestry-path.sh | 21 ++++++++++++++++++++-
 3 files changed, 38 insertions(+), 1 deletion(-)

-- 
1.8.3.rc0.28.g4b02ef5

The documentation assures users that "A...B" is defined as "A B --not
$(git merge-base --all A B)". This wasn't in fact quite true, because
the calculated merge bases were not sent to add_rev_cmdline().

The main effect of this was that although

  git rev-list --ancestry-path A B --not $(git merge-base --all A B)

worked, the simpler form

  git rev-list --ancestry-path A...B

failed with a "no bottom commits" error.

Other potential users of bottom commits could also be affected by this
problem, if they examine revs->cmdline_info; I came across the issue in
my proposed history traversal refinements series.

So ensure that the calculated merge bases are sent to add_rev_cmdline(),
flagged with new 'whence' enum value REV_CMD_MERGE_BASE.

Signed-off-by: Kevin Bracey <redacted>
---
 revision.c                        | 17 +++++++++++++++++
 revision.h                        |  1 +
 t/t6019-rev-list-ancestry-path.sh |  2 +-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index a67b615..7f7a8ab 100644
--- a/revision.c
+++ b/revision.c

@@ -915,6 +915,19 @@ static void add_rev_cmdline(struct rev_info *revs,
 	info->nr++;
 }
 
+static void add_rev_cmdline_list(struct rev_info *revs,
+				 struct commit_list *commit_list,
+				 int whence,
+				 unsigned flags)
+{
+	while (commit_list) {
+		struct object *object = &commit_list->item->object;
+		add_rev_cmdline(revs, object, sha1_to_hex(object->sha1),
+				whence, flags);
+		commit_list = commit_list->next;
+	}
+}
+
 struct all_refs_cb {
 	int all_flags;
 	int warned_bad_reflog;

@@ -1092,6 +1105,7 @@ static void prepare_show_merge(struct rev_info *revs)
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");
 	bases = get_merge_bases(head, other, 1);
+	add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING);
 	add_pending_commit_list(revs, bases, UNINTERESTING);
 	free_commit_list(bases);
 	head->object.flags |= SYMMETRIC_LEFT;

@@ -1179,6 +1193,9 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi
 
 			if (symmetric) {
 				exclude = get_merge_bases(a, b, 1);
+				add_rev_cmdline_list(revs, exclude,
+						     REV_CMD_MERGE_BASE,
+						     flags_exclude);
 				add_pending_commit_list(revs, exclude,
 							flags_exclude);
 				free_commit_list(exclude);

diff --git a/revision.h b/revision.h
index 01bd2b7..878a555 100644
--- a/revision.h
+++ b/revision.h

@@ -35,6 +35,7 @@ struct rev_cmdline_info {
 			REV_CMD_PARENTS_ONLY,
 			REV_CMD_LEFT,
 			REV_CMD_RIGHT,
+			REV_CMD_MERGE_BASE,
 			REV_CMD_REV
 		} whence;
 		unsigned flags;

diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh
index 5287f6a..dd5b0e5 100755
--- a/t/t6019-rev-list-ancestry-path.sh
+++ b/t/t6019-rev-list-ancestry-path.sh

@@ -81,7 +81,7 @@ test_expect_success 'rev-list F...I' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'rev-list --ancestry-path F...I' '
+test_expect_success 'rev-list --ancestry-path F...I' '
 	for c in F H I; do echo $c; done >expect &&
 	git rev-list --ancestry-path --format=%s F...I |
 	sed -e "/^commit /d" |

-- 
1.8.3.rc0.28.g4b02ef5

Signed-off-by: Kevin Bracey <redacted>
---
 t/t6019-rev-list-ancestry-path.sh | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/t6019-rev-list-ancestry-path.sh b/t/t6019-rev-list-ancestry-path.sh
index 39b4cb0..5287f6a 100755
--- a/t/t6019-rev-list-ancestry-path.sh
+++ b/t/t6019-rev-list-ancestry-path.sh

@@ -13,6 +13,9 @@ test_description='--ancestry-path'
 #
 #  D..M -- M.t                 == M
 #  --ancestry-path D..M -- M.t == M
+#
+#  F...I                 == F G H I
+#  --ancestry-path F...I == F H I
 
 . ./test-lib.sh
 

@@ -63,13 +66,29 @@ test_expect_success 'rev-list D..M -- M.t' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rev-list --ancestry-patch D..M -- M.t' '
+test_expect_success 'rev-list --ancestry-path D..M -- M.t' '
 	echo M >expect &&
 	git rev-list --ancestry-path --format=%s D..M -- M.t |
 	sed -e "/^commit /d" >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success 'rev-list F...I' '
+	for c in F G H I; do echo $c; done >expect &&
+	git rev-list --format=%s F...I |
+	sed -e "/^commit /d" |
+	sort >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'rev-list --ancestry-path F...I' '
+	for c in F H I; do echo $c; done >expect &&
+	git rev-list --ancestry-path --format=%s F...I |
+	sed -e "/^commit /d" |
+	sort >actual &&
+	test_cmp expect actual
+'
+
 #   b---bc
 #  / \ /
 # a   X

-- 
1.8.3.rc0.28.g4b02ef5

On Mon, May 13, 2013 at 04:45:43PM +0200, Michael J Gruber wrote:

Kevin Bracey venit, vidit, dixit 13.05.2013 16:26:

quoted

On 13/05/2013 01:22, Junio C Hamano wrote:

quoted

Kevin Bracey [off-list ref] writes:

quoted

    git log --ancestry-path --left-right E...F --not $(git merge-base
--all E F)

which looks like we're having to repeat ourselves because it's not
paying attention...

You are half wrong; "--left-right" is about "do we show the </>/=
marker in the output?", so it is true that it does not make sense
without "...", but the reverse is not true: A...B does not and
should not imply --left-right.

The repetition I meant is that by the definition of ancestry-path, the 
above would seem to be equivalent to

   git log --ancestry-path --left-right E F --not $(git merge-base --all E F) $(git merge-base --all E F)

Anyway, revised separated-out version of the patch follows.

Kevin

It is certainly true that "git log --cherry" needs much less information
than what the merge base machinery provides. I've been experimenting
with that in order to get the speedup which is necessary for replacing
the "git cherry" code with calls into the revision walker using "--cherry".

I think the revision machinery is the same speed as the "git cherry"
code, it's just that "git cherry" ignores merges and the cherry code in
revision.c doesn't.

Since the patch ID of a merge is just being calculated to its first
parent, I don't think it's meaningful to consider merges for "log
--cherry" but I can't quite convince myself that there's no corner case
where it is.

The following patch makes the revision cherry machinery ignore merges
unconditionally.  With it applied, there's not noticeable difference in
speed between "git cherry" and "git log --cherry".

-- >8 --

diff --git a/revision.c b/revision.c
index a67b615..19d0683 100644
--- a/revision.c
+++ b/revision.c

@@ -640,6 +640,11 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 
 		if (flags & BOUNDARY)
 			continue;
+
+		/* Patch ID is meaningless for merges. */
+		if (commit->parents && commit->parents->next)
+			continue;
+
 		/*
 		 * If we have fewer left, left_first is set and we omit
 		 * commits on the right branch in this loop.  If we have

@@ -661,6 +666,11 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
 
 		if (flags & BOUNDARY)
 			continue;
+
+		/* Patch ID is meaningless for merges. */
+		if (commit->parents && commit->parents->next)
+			continue;
+
 		/*
 		 * If we have fewer left, left_first is set and we omit
 		 * commits on the left branch in this loop.

-- 
1.8.3.rc3.372.g721bad8

John Keeping wrote:

I guess merges should be skipped in the left-vs-right tally earlier,
too?

		if (flags & BOUNDARY)
			;
		else if (commit->parents && commit->parents->next)
			;
		else if (flags & SYMMETRIC_LEFT)
			left_count++;
		else
			right_count++;

With that tweak (or without it --- a sloppy count is fine), this
patch makes sense to me.  I guess some tests would be useful to
demonstrate that --cherry doesn't notice duplicate first-parent
diffs in merges.

Thanks,
Jonathan