Allow --ancestry-path to take an argument

From: Elijah Newren <[email protected]>

We have long allowed users to run e.g.

    git log --ancestry-path next..seen

which shows all commits which satisfy all three of these criteria:
* are an ancestor of seen
* are not an ancestor next
* have next as an ancestor

This commit allows another variant:

    git log --ancestry-path=$TOPIC next..seen

which shows all commits which satisfy all of these criteria:
* are an ancestor of seen
* are not an ancestor of next
* have $TOPIC in their ancestry-path
that last bullet can be defined as commits meeting any of these criteria:
* are an ancestor of $TOPIC
* have $TOPIC as an ancestor
* are $TOPIC

revision.c +1 -1

index 0c6e26cd9c8..660f1dd1b9b 100644
@@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
 			   struct commit_list **list, struct prio_queue *queue)
 {
 	struct commit_list *parent = commit->parents;
-	unsigned left_flag;
+	unsigned left_flag, ancestry_flag;
 
 	if (commit->object.flags & ADDED)
 		return 0;

revision.c +5 -4

+static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)
 {
 	struct commit_list *p;
 	struct commit_list *rlist = NULL;

@@ -1323,7 +1333,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li
 	for (p = list; p; p = p->next)
 		commit_list_insert(p->item, &rlist);
 
-	for (p = bottom; p; p = p->next)
+	for (p = bottoms; p; p = p->next)
 		p->item->object.flags |= TMP_MARK;
 
 	/*

@@ -1356,38 +1366,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li
 	 */
 
 	/*
-	 * The ones that are not marked with TMP_MARK are uninteresting
+	 * The ones that are not marked with either TMP_MARK or
+	 * ANCESTRY_PATH are uninteresting
 	 */
 	for (p = list; p; p = p->next) {
 		struct commit *c = p->item;
-		if (c->object.flags & TMP_MARK)
+		if (c->object.flags & (TMP_MARK | ANCESTRY_PATH))
 			continue;
 		c->object.flags |= UNINTERESTING;
 	}
 
-	/* We are done with the TMP_MARK */

Quoted 8 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

We have long allowed users to run e.g.

    git log --ancestry-path next..seen

which shows all commits which satisfy all three of these criteria:
* are an ancestor of seen
* are not an ancestor next
* have next as an ancestor

Is it a very good example, though? Nothing builds on next, and next is not an ancestor of seen, so the command without --ancestry-path does give us individual commits that are not in 'next' yet, plus all the merge commits in master..seen but with --ancestry-path the answer is most likely an empty set.

If you replace 'next' with 'master', it does start to make sense, but that is a bit too straight first-parent merge chain that may not be all that interesting.

Quoted 11 lines

This commit allows another variant:

    git log --ancestry-path=$TOPIC next..seen

which shows all commits which satisfy all of these criteria:
* are an ancestor of seen
* are not an ancestor of next
* have $TOPIC in their ancestry-path
that last bullet can be defined as commits meeting any of these criteria:
* are an ancestor of $TOPIC
* have $TOPIC as an ancestor
* are $TOPIC

So, I have en/ancestry-path-in-a-range topic merged somewhere between 'master' and 'seen'. Here is what I see:

    $ seen/git log --oneline --ancestry-path=en/ancestry-path-in-a-range master..seen
    21aef6c754 Merge branch 'ab/submodule-helper-leakfix' into seen
    2a57fcc25e Merge branch 'ab/submodule-helper-prep' into seen
    72ff5f5d3a ###
    edb5cf4c31 Merge branch 'cw/remote-object-info' into seen
    cc8f65a665 Merge branch 'ag/merge-strategies-in-c' into seen
    c1bacacabf Merge branch 'es/mark-gc-cruft-as-experimental' into seen
    fdf2d207d2 Merge branch 'js/bisect-in-c' into seen
    2a1bbfc016 Merge branch 'po/glossary-around-traversal' into seen
    7ecf004b9e Merge branch 'vd/scalar-enables-fsmonitor' into jch
    9dba189986 Merge branch 'en/ancestry-path-in-a-range' into jch
    4461e34d7d revision: allow --ancestry-path to take an argument
    0605b4aad9 rev-list-options.txt: fix simple typo

which is very much expected. Two commits are what we want to highlight, and its merge into the first-parent-chain that leads to 'seen', and all its descendants on 'seen' are shown.

Due to the way "ancestry-path" is defined, replacing the value to "--ancestry-path" from 4461e34d7d to 0605b4aad9 would not change the output, which is also expected. If this were a three-commit topic, giving the middle commit would find both the first one (i.e. the ancestor) and the third one (i.e. the descendant) in the topic, while excluding the much-less-interesting base commit and its ancestors the topic builds on.

I am not exactly sure when this feature is useful, though. It is handy to be able to enumerate descendants of a given commit, so perhaps the user knows about 0605b4aad9 and is trying to find other commits on the same topic, or something? But then the merges nearer the tip of 'seen' than 9dba189986 are not very useful for that purpose. It somehow feels like a solution in search of a problem.

Quoted 14 lines

revision.c +1 -1

index 0c6e26cd9c8..660f1dd1b9b 100644
@@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
                         struct commit_list **list, struct prio_queue *queue)
 {
      struct commit_list *parent = commit->parents;
-     unsigned left_flag;
+     unsigned left_flag, ancestry_flag;
 
      if (commit->object.flags & ADDED)
              return 0;

revision.c static int process_parents(struct rev_info *revs, struct commit *commit, · line 1164

Quoted 4 lines

revision.c +1 -0

@@ -1161,6 +1161,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
              return 0;
 
      left_flag = (commit->object.flags & SYMMETRIC_LEFT);
+     ancestry_flag = (commit->object.flags & ANCESTRY_PATH);

Wouldn't we want

        if (revs->ancestry_path)
                ancestry_flag = (commit->object.flags & ANCESTRY_PATH);

instead, so that the propagation of contaminated flag bits ...

revision.c static int process_parents(struct rev_info *revs, struct commit *commit, · line 1187

Quoted 9 lines

revision.c +2 -0

      for (parent = commit->parents; parent; parent = parent->next) {
              struct commit *p = parent->item;

@@ -1181,6 +1182,8 @@ static int process_parents(struct rev_info *revs, struct commit *commit,
                      if (!*slot)
                              *slot = *revision_sources_at(revs->sources, commit);
              }
+             if (revs->ancestry_path)
+                     p->object.flags |= ancestry_flag;
              p->object.flags |= left_flag;

... can become a simple

                p->object.flags |= ancestry_flag;

here? Or even just use a single variable to compute the set of flags to pass down i.e.

        pass_flags = commit->object.flags & (SYMMETRIC_LEFT | ANCESTRY_PATH);

before the loop, and then pass these two bits down at once, i.e.

(quoted diff) +1 -1

-               p->object.flags |= left_flag;
+               p->object.flags |= pass_flags;

taking advantage of the fact that ANCESTRY_PATH and SYMMETRIC_LEFT bits can be set to any object only when these features are in use?

Or did I misread the patch and sometimes ANCESTRY_PATH bit is set on objects even when revs->ancestry_path is not in use?

Quoted 30 lines

revision.c +5 -4

+static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)
 {
      struct commit_list *p;
      struct commit_list *rlist = NULL;

@@ -1323,7 +1333,7 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li
      for (p = list; p; p = p->next)
              commit_list_insert(p->item, &rlist);
 
-     for (p = bottom; p; p = p->next)
+     for (p = bottoms; p; p = p->next)
              p->item->object.flags |= TMP_MARK;
 
      /*

@@ -1356,38 +1366,39 @@ static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li
       */
 
      /*
-      * The ones that are not marked with TMP_MARK are uninteresting
+      * The ones that are not marked with either TMP_MARK or
+      * ANCESTRY_PATH are uninteresting
       */
      for (p = list; p; p = p->next) {
              struct commit *c = p->item;
-             if (c->object.flags & TMP_MARK)
+             if (c->object.flags & (TMP_MARK | ANCESTRY_PATH))
                      continue;
              c->object.flags |= UNINTERESTING;
      }
 
-     /* We are done with the TMP_MARK */

revision.c static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *li · line 1385

Quoted 10 lines

revision.c +4 -3

+     /* We are done with TMP_MARK and ANCESTRY_PATH */
      for (p = list; p; p = p->next)
-             p->item->object.flags &= ~TMP_MARK;
-     for (p = bottom; p; p = p->next)
-             p->item->object.flags &= ~TMP_MARK;
+             p->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);
+     for (p = bottoms; p; p = p->next)
+             p->item->object.flags &= ~(TMP_MARK | ANCESTRY_PATH);
      free_commit_list(rlist);
 }

We have called process_parents() to paint ancestor commits that can be reached from the commit(s) of interest. This helper function is called after that is done, and propagates the ancestry_path bit in the reverse direction, i.e. from parent to child.

Once we are done with this processing, we no longer need ANCESTRY_PATH bit because the surviving ones without UNINTERESTING bit set are the commits on the ancestry_path. OK.

From: Elijah Newren <[email protected]>

t/t6019-rev-list-ancestry-path.sh +0 -0

index af57a04b7ff..d3657737fa6 100755
@@ -8,8 +8,13 @@ test_description='--ancestry-path'
 #   /                     \
 #  A-------K---------------L--M

Review

Quoted 7 lines · Elijah Newren via GitGitGadget

(quoted diff) +7 -0

+test_expect_success 'rev-list --ancestry-path=F D..M' '
+	test_write_lines E F J L M >expect &&
+	git rev-list --ancestry-path=F --format=%s D..M |
+	sed -e "/^commit /d" |
+	sort >actual &&
+	test_cmp expect actual
+'

These tests follow the patterns from other tests in this file, but it also has bad patterns. Specifically, the 'git rev-list' command is fed directly into a pipe. I include a patch below that applies directly on this one to rewrite these tests. If you want, you could rebase to have that test refactor happen before you add your new --ancestry-path=<X> option tests.

Quoted 15 lines · Derrick Stolee

Review

Quoted 7 lines · Elijah Newren via GitGitGadget

(quoted diff) +7 -0

+test_expect_success 'rev-list --ancestry-path=F D..M' '
+	test_write_lines E F J L M >expect &&
+	git rev-list --ancestry-path=F --format=%s D..M |
+	sed -e "/^commit /d" |
+	sort >actual &&
+	test_cmp expect actual
+'

These tests follow the patterns from other tests in this file, but it also has bad patterns. Specifically, the 'git rev-list' command is fed directly into a pipe. I include a patch below that applies directly on this one to rewrite these tests. If you want, you could rebase to have that test refactor happen before you add your new --ancestry-path=<X> option tests.

Thanks, I was going to comment on the same, but your solution is much better (I was just going to suggest using intermediate files).

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 60

Quoted 10 lines

[...]

t/t6019-rev-list-ancestry-path.sh +3 -6

-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_ancestry () {
+	args=$1
+	expected=$2

Maybe add &&-chaining here (we do it in some cases, but I'm not sure when such assignments would ever fail).

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 65

Quoted 5 lines

t/t6019-rev-list-ancestry-path.sh +5 -0

+	test_expect_success "rev-list $args" "
+		test_write_lines $expected >expect &&
+		git rev-list --format=%s $args >raw &&
+
+		if test -n \"$expected\"

Aren't you making things harder for yourself here than required by using ""-quoting for the body of the test.

We eval it into existence, so you can use ''-quotes, and then you don't need to escape e.g. the "" quotes here for expected, no?

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 67

Quoted 2 lines

t/t6019-rev-list-ancestry-path.sh +2 -0

+		then
+			sed -e \"/^commit /d\" raw | sort >actual &&

nit for debuggability (and not correctness), maybe using intermediate files here would be nicer? And then perhaps call them "actual" and "actual.sorted".

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 68

Quoted 2 lines

t/t6019-rev-list-ancestry-path.sh +1 -0

+			test_cmp expect actual || return 1

No need for a "return 1" here when we're not in a loop. It's redundant, and makes the -x output on failure confusing ("why didn't I fail on the test_cmp, but on this stray return?...").

...

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 70

Quoted 2 lines

t/t6019-rev-list-ancestry-path.sh +2 -0

+		else
+			test_must_be_empty raw

...which would also allow you to extract much of this if/else at the cost of not using test_must_be_empty, i.e. just make the "expected" empty unless "$expected" is provided. Just a thought/nit, we could also leave this as-is :)

Also does the "compare rev" part of this want test_cmp_rev instead?

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 60

Quoted 3 lines · Derrick Stolee

t/t6019-rev-list-ancestry-path.sh +3 -0

+test_ancestry () {
+     args=$1
+     expected=$2

Maybe add &&-chaining here (we do it in some cases, but I'm not sure when such assignments would ever fail).

Quoted 9 lines · Derrick Stolee

Review

Quoted 7 lines · Elijah Newren via GitGitGadget

(quoted diff) +7 -0

+test_expect_success 'rev-list --ancestry-path=F D..M' '
+    test_write_lines E F J L M >expect &&
+    git rev-list --ancestry-path=F --format=%s D..M |
+    sed -e "/^commit /d" |
+    sort >actual &&
+    test_cmp expect actual
+'

Quoted 6 lines

These tests follow the patterns from other tests in this file, but it also has bad patterns. Specifically, the 'git rev-list' command is fed directly into a pipe. I include a patch below that applies directly on this one to rewrite these tests. If you want, you could rebase to have that test refactor happen before you add your new --ancestry-path=<X> option tests.

Thanks, I was going to comment on the same, but your solution is much better (I was just going to suggest using intermediate files).

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 60

Quoted 10 lines

[...]

t/t6019-rev-list-ancestry-path.sh +3 -6

-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_ancestry () {
+     args=$1
+     expected=$2

Maybe add &&-chaining here (we do it in some cases, but I'm not sure when such assignments would ever fail).

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 65

Quoted 5 lines

t/t6019-rev-list-ancestry-path.sh +5 -0

+     test_expect_success "rev-list $args" "
+             test_write_lines $expected >expect &&
+             git rev-list --format=%s $args >raw &&
+
+             if test -n \"$expected\"

Aren't you making things harder for yourself here than required by using ""-quoting for the body of the test.

We eval it into existence, so you can use ''-quotes, and then you don't need to escape e.g. the "" quotes here for expected, no?

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 67

Quoted 2 lines

t/t6019-rev-list-ancestry-path.sh +2 -0

+             then
+                     sed -e \"/^commit /d\" raw | sort >actual &&

nit for debuggability (and not correctness), maybe using intermediate files here would be nicer? And then perhaps call them "actual" and "actual.sorted".

Quoted 1 line

+ test_cmp expect actual || return 1

No need for a "return 1" here when we're not in a loop. It's redundant, and makes the -x output on failure confusing ("why didn't I fail on the test_cmp, but on this stray return?...").

...

t/t6019-rev-list-ancestry-path.sh test_expect_success setup ' · line 70

Quoted 2 lines

t/t6019-rev-list-ancestry-path.sh +2 -0

+             else
+                     test_must_be_empty raw

...which would also allow you to extract much of this if/else at the cost of not using test_must_be_empty, i.e. just make the "expected" empty unless "$expected" is provided. Just a thought/nit, we could also leave this as-is :)

Also does the "compare rev" part of this want test_cmp_rev instead?

Quoted 2 lines · Ævar Arnfjörð Bjarmason

[...]
Also does the "compare rev" part of this want test_cmp_rev instead?

Um, I don't see any "compare rev" part of this, or any revision comparing. What are you referring to?

From: Derrick Stolee <[email protected]>
Subject: [PATCH] t6019: modernize tests with helper

The tests in t6019 are repetive, so create a helper that greatly simplifies the test script.

In addition, update the common pattern that places 'git rev-list' on the left side of a pipe, which can hide some exit codes. Send the output to a 'raw' file that is then consumed by other tools so the Git exit code is verified as zero.

Signed-off-by Derrick Stolee <[email protected]>

Quoted 1 line · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Code looks good!

Quoted 7 lines

t/t6019-rev-list-ancestry-path.sh +0 -0

index af57a04b7ff..d3657737fa6 100755
@@ -8,8 +8,13 @@ test_description='--ancestry-path'
 #   /                     \
 #  A-------K---------------L--M

t/t6019-rev-list-ancestry-path.sh test_description='--ancestry-path' · line 17

Quoted 10 lines

t/t6019-rev-list-ancestry-path.sh +7 -2

 #
-#  D..M                 == E F G H I J K L M
-#  --ancestry-path D..M == E F H I J L M
+#  D..M                                     == E F G H I J K L M
+#  --ancestry-path                     D..M == E F   H I J   L M
+#  --ancestry-path=F                   D..M == E F       J   L M
+#  --ancestry-path=G                   D..M ==     G H I J   L M
+#  --ancestry-path=H                   D..M == E   G H I J   L M
+#  --ancestry-path=K                   D..M ==             K L M
+#  --ancestry-path=K --ancestry-path=F D..M == E F       J K L M

These are good examples, because they help clarify what I had initially been confused about: you include things in _both_ directions. In particular, "--ancestry-path=K --ancestry-path=f D..M" you are kind of taking the union of the following queries:

--ancestry-path D..K
--ancestry-path K..M
--ancestry-path D..F
--ancestry-path F..M

I did check just in case, but specifying multiple ranges such as

        --ancestry-path D..K K..M

does not do what is expected.

t/t6019-rev-list-ancestry-path.sh test_description='--ancestry-path'

Quoted 8 lines

t/t6019-rev-list-ancestry-path.sh +7 -0

+test_expect_success 'rev-list --ancestry-path=F D..M' '
+     test_write_lines E F J L M >expect &&
+     git rev-list --ancestry-path=F --format=%s D..M |
+     sed -e "/^commit /d" |
+     sort >actual &&
+     test_cmp expect actual
+'

These tests follow the patterns from other tests in this file, but it also has bad patterns. Specifically, the 'git rev-list' command is fed directly into a pipe. I include a patch below that applies directly on this one to rewrite these tests. If you want, you could rebase to have that test refactor happen before you add your new --ancestry-path=<X> option tests.

t/t6019-rev-list-ancestry-path.sh

Quoted 9 lines

t/t6019-rev-list-ancestry-path.sh +8 -0

+test_expect_success 'rev-list --ancestry-path=G D..M' '
+     test_write_lines G H I J L M >expect &&
+     git rev-list --ancestry-path=G --format=%s D..M |
+     sed -e "/^commit /d" |
+     sort >actual &&
+     test_cmp expect actual
+'
+test_expect_success 'rev-list --ancestry-path=H D..M' '

nit: needs extra whitespace between tests. The above test pair needs one, too. This becomes moot with the patch I provide.

Thanks,
-Stolee

--- >8 ---

From 9ac4e81dba0d7801513a09f5fe307d01357123dd Mon Sep 17 00:00:00 2001
From: Derrick Stolee <[email protected]>
Date: Thu, 18 Aug 2022 11:25:04 -0400
Subject: [PATCH] t6019: modernize tests with helper

The tests in t6019 are repetive, so create a helper that greatly simplifies the test script.

In addition, update the common pattern that places 'git rev-list' on the left side of a pipe, which can hide some exit codes. Send the output to a 'raw' file that is then consumed by other tools so the Git exit code is verified as zero.

Signed-off-by Derrick Stolee <[email protected]>

---
 t/t6019-rev-list-ancestry-path.sh | 131 +++++++-----------------------
 1 file changed, 31 insertions(+), 100 deletions(-)

t/t6019-rev-list-ancestry-path.sh +31 -101

index d3657737fa60..18941a80ce67 100755
@@ -55,111 +55,42 @@ test_expect_success setup '
        test_commit M
 '
 
-test_expect_success 'rev-list D..M' '
-       test_write_lines E F G H I J K L M >expect &&
-       git rev-list --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'rev-list --ancestry-path D..M' '
-       test_write_lines E F H I J L M >expect &&
-       git rev-list --ancestry-path --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'rev-list --ancestry-path=F D..M' '
-       test_write_lines E F J L M >expect &&
-       git rev-list --ancestry-path=F --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-test_expect_success 'rev-list --ancestry-path=G D..M' '
-       test_write_lines G H I J L M >expect &&
-       git rev-list --ancestry-path=G --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-test_expect_success 'rev-list --ancestry-path=H D..M' '
-       test_write_lines E G H I J L M >expect &&
-       git rev-list --ancestry-path=H --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'rev-list --ancestry-path=K D..M' '
-       test_write_lines K L M >expect &&
-       git rev-list --ancestry-path=K --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'rev-list --ancestry-path=F --ancestry-path=K D..M' '
-       test_write_lines E F J K L M >expect &&
-       git rev-list --ancestry-path=F --ancestry-path=K --format=%s D..M |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
-
-test_expect_success 'rev-list D..M -- M.t' '
-       echo M >expect &&
-       git rev-list --format=%s D..M -- M.t |
-       sed -e "/^commit /d" >actual &&
-       test_cmp expect actual
-'
-
-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_ancestry () {
+       args=$1
+       expected=$2
+       test_expect_success "rev-list $args" "
+               test_write_lines $expected >expect &&
+               git rev-list --format=%s $args >raw &&
+
+               if test -n \"$expected\"
+               then
+                       sed -e \"/^commit /d\" raw | sort >actual &&
+                       test_cmp expect actual || return 1
+               else
+                       test_must_be_empty raw
+               fi
+       "
+}
 
-test_expect_success 'rev-list F...I' '
-       test_write_lines F G H I >expect &&
-       git rev-list --format=%s F...I |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
+test_ancestry "D..M" "E F G H I J K L M"
 
-test_expect_success 'rev-list --ancestry-path F...I' '
-       test_write_lines F H I >expect &&
-       git rev-list --ancestry-path --format=%s F...I |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
+test_ancestry "--ancestry-path D..M" "E F H I J L M"
+test_ancestry "--ancestry-path D..M" "E F H I J L M"
+test_ancestry "--ancestry-path=F D..M" "E F J L M"
+test_ancestry "--ancestry-path=G D..M" "G H I J L M"
+test_ancestry "--ancestry-path=H D..M" "E G H I J L M"
+test_ancestry "--ancestry-path=K D..M" "K L M"
+test_ancestry "--ancestry-path=K --ancestry-path=F D..M" "E F J K L M"
 
-# G.t is dropped in an "-s ours" merge
-test_expect_success 'rev-list G..M -- G.t' '
-       git rev-list --format=%s G..M -- G.t |
-       sed -e "/^commit /d" >actual &&
-       test_must_be_empty actual
-'
+test_ancestry "D..M -- M.t" "M"
+test_ancestry "--ancestry-path D..M -- M.t" "M"
 
-test_expect_success 'rev-list --ancestry-path G..M -- G.t' '
-       echo L >expect &&
-       git rev-list --ancestry-path --format=%s G..M -- G.t |
-       sed -e "/^commit /d" >actual &&
-       test_cmp expect actual
-'
+test_ancestry "F...I" "F G H I"
+test_ancestry "--ancestry-path F...I" "F H I"
 
-test_expect_success 'rev-list --ancestry-path --simplify-merges G^..M -- G.t' '
-       test_write_lines G L >expect &&
-       git rev-list --ancestry-path --simplify-merges --format=%s G^..M -- G.t |
-       sed -e "/^commit /d" |
-       sort >actual &&
-       test_cmp expect actual
-'
+test_ancestry "G..M -- G.t" ""
+test_ancestry "--ancestry-path G..M -- G.t" "L"
+test_ancestry "--ancestry-path --simplify-merges G^..M -- G.t" "G L"
 
 #   b---bc
 #  / \ /
--

2.37.1.vfs.0.0.rebase

From: Elijah Newren <[email protected]>

We have long allowed users to run e.g.

    git log --ancestry-path master..seen

which shows all commits which satisfy all three of these criteria:
* are an ancestor of seen
* are not an ancestor master

Documentation/rev-list-options.txt +1 -2

index 2f85726745a..001e49cee55 100644
@@ -389,12 +389,15 @@ Default mode::
 	merges from the resulting history, as there are no selected
 	commits contributing to this merge.
 
---ancestry-path::
+--ancestry-path[=<commit>]::
 	When given a range of commits to display (e.g. 'commit1..commit2'
-	or 'commit2 {caret}commit1'), only display commits that exist

Documentation/rev-list-options.txt +4 -4

@@ -1304,13 +1304,20 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop
 }
 
 /*
- * "rev-list --ancestry-path A..B" computes commits that are ancestors
+ * "rev-list --ancestry-path=C A..B" computes commits that are ancestors
  * of B but not ancestors of A but further limits the result to those
- * that are descendants of A.  This takes the list of bottom commits and
- * the result of "A..B" without --ancestry-path, and limits the latter
- * further to the ones that can reach one of the commits in "bottom".
+ * that have C in their ancestry path (i.e. are either ancestors of C,
+ * descendants of C, or are C).  If multiple --ancestry-path=$COMMITTISH
+ * arguments are supplied, we limit the result to those that have at

Documentation/rev-list-options.txt +1 -1

@@ -2213,7 +2220,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			       const struct setup_revision_opt* opt)
 {
 	const char *arg = argv[0];
-	const char *optarg;
+	const char *optarg = NULL;
 	int argcount;
 	const unsigned hexsz = the_hash_algo->hexsz;

Thanks - overall this looks good. I only have some minor textual comments and one small code comment.

Quoted 2 lines · Elijah Newren via GitGitGadget

From: Elijah Newren <[email protected]>

Quoted 5 lines

We have long allowed users to run e.g.

    git log --ancestry-path master..seen

which shows all commits which satisfy all three of these criteria:
* are an ancestor of seen
* are not an ancestor master

are not an ancestor *of* master

Quoted 12 lines

Documentation/rev-list-options.txt +1 -2

index 2f85726745a..001e49cee55 100644
@@ -389,12 +389,15 @@ Default mode::
      merges from the resulting history, as there are no selected
      commits contributing to this merge.
 
---ancestry-path::
+--ancestry-path[=<commit>]::
      When given a range of commits to display (e.g. 'commit1..commit2'
-     or 'commit2 {caret}commit1'), only display commits that exist

Documentation/rev-list-options.txt Default mode:: · line 400

Quoted 10 lines

Documentation/rev-list-options.txt +7 -3

-     directly on the ancestry chain between the 'commit1' and
-     'commit2', i.e. commits that are both descendants of 'commit1',
-     and ancestors of 'commit2'.
+     or 'commit2 {caret}commit1'), only display commits in that
+     range where <commit> is part of the ancestry chain.  By "part of
+     the ancestry chain", we mean including <commit> itself and
+     commits that are either ancestors or descendants of <commit>.
+     If no commit is specified, use 'commit1' (the excluded part of
+     the range) as <commit>.  Can be passed multiple times to look for
+     commits in the ancestry range of multiple commits.

"Ancestry chain" seems to be used multiple times in the Git codebase, apparently with slightly different definitions, so probably best to clear up at least this part by not reusing the term. It's also probably not worth introducing a new term "ancestry range". Maybe rewrite to say:

When given a range of commits to display (e.g. 'commit1..commit2' or 'commit2 {caret}commit1'), only display commits in that range that are ancestors of <commit>, descendants of <commit>, or <commit> itself. If no commit is specified, use 'commit1' (the excluded part of the range) as <commit>. Can be passed multiple times; if so, a commit is included if it is any of the commits given or if it is an ancestor or descendant of one of them.

Quoted 4 lines

Documentation/rev-list-options.txt +0 -0

@@ -568,11 +571,10 @@ Note the major differences in `N`, `P`, and `Q` over `--full-history`:
 
 There is another simplification mode available:
 

Documentation/rev-list-options.txt Note the major differences in `N`, `P`, and `Q` over `--full-history`: · line 577

Quoted 9 lines

Documentation/rev-list-options.txt +4 -5

---ancestry-path::
-     Limit the displayed commits to those directly on the ancestry
-     chain between the ``from'' and ``to'' commits in the given commit
-     range. I.e. only display commits that are ancestor of the ``to''
-     commit and descendants of the ``from'' commit.
+--ancestry-path[=<commit>]::
+     Limit the displayed commits to those containing <commit> in their
+     ancestry path.  I.e. only display <commit> and commits which have
+     <commit> as either a direct ancestor or descendant.

Can we refer back to the documentation of --ancestry-path instead?

Quoted 13 lines

Documentation/rev-list-options.txt +4 -4

@@ -1304,13 +1304,20 @@ static int still_interesting(struct commit_list *src, timestamp_t date, int slop
 }
 
 /*
- * "rev-list --ancestry-path A..B" computes commits that are ancestors
+ * "rev-list --ancestry-path=C A..B" computes commits that are ancestors
  * of B but not ancestors of A but further limits the result to those
- * that are descendants of A.  This takes the list of bottom commits and
- * the result of "A..B" without --ancestry-path, and limits the latter
- * further to the ones that can reach one of the commits in "bottom".
+ * that have C in their ancestry path (i.e. are either ancestors of C,
+ * descendants of C, or are C).  If multiple --ancestry-path=$COMMITTISH
+ * arguments are supplied, we limit the result to those that have at

Documentation/rev-list-options.txt static int still_interesting(struct commit_list *src, timestamp_t date, int slop · line 1320

Quoted 10 lines

Documentation/rev-list-options.txt +8 -1

+ * least one of those COMMITTISH in their ancestry path. If
+ * --ancestry-path is specified with no commit, we use all bottom
+ * commits for C.
+ *
+ * Before this function is called, ancestors of C will have already been
+ * marked with ANCESTRY_PATH previously, so we just need to also mark
+ * the descendants here, then collect both sets of commits.
  */
-static void limit_to_ancestry(struct commit_list *bottom, struct commit_list *list)
+static void limit_to_ancestry(struct commit_list *bottoms, struct commit_list *list)

I thought the original description of this function ("This takes the list...") to be clear and it would be nice to retain it. So, e.g.:

"rev-list --ancestry-path=C_0 [--ancestry-path=C_1 ...] A..B" computes commits that are ancestors of B but not ancestors of A but further limits the result to those that have any of C in their ancestry path (i.e. are either ancestors of any of C, descendants of any of C, or are any of C). If --ancestry-path is specified with no commit, we use all bottom commits for C.

Before this function is called, ancestors of C will have already been marked with ANCESTRY_PATH previously.

This takes the list of bottom commits and the result of "A..B" without --ancestry-path, and limits the latter further to the ones that have any of C in their ancestry path. Since the ancestors of C have already been marked (a prerequisite of this function), we just need to mark the descendants, then exclude any commit that does not have any of these marks.

Optional: Besides that, from what I can tell, sometimes the C commits themselves are marked with ANCESTRY_PATH (when they are explicitly specified) and sometimes they are not (when they are not explicitly specified). It's not a bug here, but it might be worth handling that in the ancestry_path_need_bottoms codepath (instead of explicitly setting TMP_MARK on the bottoms in limit_to_ancestry() - that way, I think we can also use ANCESTRY_PATH instead of TMP_MARK throughout the ancestry path codepaths, but I haven't tested it), at least to prevent possible future bugs.

Quoted 8 lines

Documentation/rev-list-options.txt +1 -1

@@ -2213,7 +2220,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
                             const struct setup_revision_opt* opt)
 {
      const char *arg = argv[0];
-     const char *optarg;
+     const char *optarg = NULL;
      int argcount;
      const unsigned hexsz = the_hash_algo->hexsz;

[snip]

Documentation/rev-list-options.txt static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg · line 2291

Quoted 7 lines

Documentation/rev-list-options.txt +2 -1

@@ -2280,10 +2287,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
              revs->first_parent_only = 1;
      } else if (!strcmp(arg, "--exclude-first-parent-only")) {
              revs->exclude_first_parent_only = 1;
-     } else if (!strcmp(arg, "--ancestry-path")) {
+     } else if (!strcmp(arg, "--ancestry-path") ||
+                skip_prefix(arg, "--ancestry-path=", &optarg)) {

This and the above hunk might cause bugs if --ancestry-path was first specified with a commit and then specified without. Probably best to break this up into separate "else if" branches, even though there is a bit of code duplication (and also remove the "= NULL" addition in the above hunk).

Documentation/rev-list-options.txt struct rev_info { · line 168

Quoted 5 lines

Documentation/rev-list-options.txt +1 -0

@@ -164,6 +165,7 @@ struct rev_info {
                      cherry_mark:1,
                      bisect:1,
                      ancestry_path:1,
+                     ancestry_path_need_bottoms:1,

Might be better named as ancestry_path_implicit_bottoms? And probably worth documenting, e.g.

True if --ancestry-path was specified without an argument. The bottom revisions are implicitly the arguments in this case.

Quoted 9 lines

Optional: Besides that, from what I can tell, sometimes the C commits themselves are marked with ANCESTRY_PATH (when they are explicitly specified) and sometimes they are not (when they are not explicitly specified). It's not a bug here, but it might be worth handling that in the ancestry_path_need_bottoms codepath (instead of explicitly setting TMP_MARK on the bottoms in limit_to_ancestry() - that way, I think we can also use ANCESTRY_PATH instead of TMP_MARK throughout the ancestry path codepaths, but I haven't tested it), at least to prevent possible future bugs.

That sounds like you're trying to duplicate the bug in my first attempt at this patch. If you try to coalesce ANCESTRY_PATH and TMP_MARK, then you not only get all descendants of C, you also get all descendants of any ancestor of C, which defeats the whole point of my changes.

It's true that I don't mark implicit C commits with ANCESTRY_PATH, but those are always bottom commits that are the excluded end of a range anyway. While those could be marked without causing problems, it would always be a waste of effort.

Changes since v1:

* Tweaked the commit message, and incorporated Junio's suggestion to update left_flag and ancestry_flag together.

revision.h struct rev_info { · line 173

Quoted 7 lines · Elijah Newren via GitGitGadget

ancestry_path:1,

revision.h +6 -0

+
+                     /* True if --ancestry-path was specified without an
+                      * argument. The bottom revisions are implicitly
+                      * the arguments in this case.
+                      */
+                     ancestry_path_implicit_bottoms:1,

The above comment explains why this is called "implicit" very well. The earlier round used "need", but the word missed the essense (i.e. ancestry_path argument is implicit and not given, so we need to compute bottoms and use them instead). The new name is certainly better.

Changes since v2:

* Incorporated Stolee's suggested patch as a preliminary cleanup of t6019. Slightly modified his patch by:
* fixing the "repetitive" typo pointed out by Eric
* removed the unnecessary "return 1" pointed out by Ævar
* switched "rev-list" to "log" since we are using --format anyway, in order to remove the need to call "sed" afterward

Quoted 8 lines · Elijah Newren via GitGitGadget

Changes since v2:

* Incorporated Stolee's suggested patch as a preliminary cleanup of t6019. Slightly modified his patch by:
* fixing the "repetitive" typo pointed out by Eric
* removed the unnecessary "return 1" pointed out by Ævar
* switched "rev-list" to "log" since we are using --format anyway, in order to remove the need to call "sed" afterward

Thanks! I appreciate you fixing all of my mistakes and incorporating all of the existing feedback. It also looks nicer as your patch 2 than as a third on top.

This version looks good to me.

Quoted 10 lines · Derrick Stolee

Quoted 8 lines · Elijah Newren via GitGitGadget

Changes since v2:

* Incorporated Stolee's suggested patch as a preliminary cleanup of t6019. Slightly modified his patch by:
* fixing the "repetitive" typo pointed out by Eric
* removed the unnecessary "return 1" pointed out by Ævar
* switched "rev-list" to "log" since we are using --format anyway, in order to remove the need to call "sed" afterward

Quoted 5 lines

Thanks! I appreciate you fixing all of my mistakes and incorporating all of the existing feedback. It also looks nicer as your patch 2 than as a third on top.

This version looks good to me.

Yup, it looks good to me, too. Thanks, both.

Let's mark it for 'next' and merge it down soonish.