regex_matches baseline check fails due to rev-parse changes in git 2.47.0

[EliahKagan]

Update: See #1622 (comment) on the cause of this bug.

Current behavior 😯

When git is git 2.47.0 and tests are run with GIX_TEST_IGNORE_ARCHIVES=1 to force fixture scripts--and in particular the make_rev_spec_parse_repos.sh script--to run, the find_youngest_matching_commit::regex_matches rev-spec test fails:

     Summary [  72.831s] 2560 tests run: 2559 passed (1 leaky), 1 failed, 3 skipped
        FAIL [   0.024s] gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches
error: test run failed

More specifically:

        FAIL [   0.024s] gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches

--- STDOUT:              gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches ---

running 1 test
test revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches ... FAILED

failures:

failures:
    revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 297 filtered out; finished in 0.01s


--- STDERR:              gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches ---
thread 'revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches' panicked at gix/tests/gix/revision/spec/from_bytes/util.rs:181:13:
assertion `left == right` failed: :/mes.age: left (ours) should match right (git): Ok(Spec { inner: Include(Sha1(ef80b4b77b167f326351c93284dc0eb00dd54ff4)), path: None, first_ref: None, second_ref: None, repo: Repository { kind: WorkTree { is_linked: false }, git_dir: "tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/.git", work_dir: Some("tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph") } })
  left: Some(Include(Sha1(ef80b4b77b167f326351c93284dc0eb00dd54ff4)))
 right: Some(Include(Sha1(9f9eac6bd1cd4b4cc6a494f044b28c985a22972b)))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

As detailed below, the assertion that fails is not actually the assertion present directly in the test case function, but is instead an assertion present in a helper that checks against a baseline generated by git when running a fixture script.

I have observed the failures in Ubuntu 24.04 LTS with git 2.47.0 installed from the git-core PPA, in Arch Linux with git 2.47.0-1 (which is a downstream build of git 2.47.0 that is essentially the same), and in Arch Linux with manual builds of the upstream git source code at the 2.47.0 tag. The latter is the most valuable, because I also built the 2.46.2 tag to verify that the failure does not occur. Full details of most runs are in this gist. As shown there, I verified that there are no other failures, that there are no failures when GIX_TEST_IGNORE_ARCHIVES=1 is not used, and that the failures are not triggered or otherwise affected by the changes in #1620.

The cause

The failure is triggered by a change in behavior from git 2.46.2 to git 2.47.0, where the computed baseline values placed in gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/baseline.git differ. The behavior of gitoxide agrees with the behavior of git 2.46.2 but disagrees with the behavior of git 2.47.0. Running the test suite in two worktrees, both at the tip of gitoxide's main branch, and diffing the generated baseline.git file in each of the generated fixture directories, reveals:

ek in 🌐 catenary in ~/repos
❯ git --no-pager diff --no-index gitoxide-with-git-{2.46.2,2.47.0}/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/baseline.git
diff --git a/gitoxide-with-git-2.46.2/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/baseline.git b/gitoxide-with-git-2.47.0/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/baseline.git
index 50eaff2..cf023f9 100644
--- a/gitoxide-with-git-2.46.2/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/baseline.git
+++ b/gitoxide-with-git-2.47.0/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/baseline.git
@@ -1,11 +1,11 @@
 :/message
-ef80b4b77b167f326351c93284dc0eb00dd54ff4
+9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
 :/!-message
-55e825ebe8fd2ff78cad3826afb696b96b576a7e
+0270e757e023fedde198947489215e34bdaf1502
 :/mes.age
-ef80b4b77b167f326351c93284dc0eb00dd54ff4
+9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
 :/!-mes.age
-55e825ebe8fd2ff78cad3826afb696b96b576a7e
+0270e757e023fedde198947489215e34bdaf1502
 :/not there
 1
 @^{/!-B}

This is from:

gitoxide/gix/tests/fixtures/make_rev_spec_parse_repos.sh

Lines 351 to 354 in 6487269

	baseline ":/message" # finds 'message recent' instead of 'initial message'
	baseline ":/!-message" # above, negated
	baseline ":/mes.age" # regexes work too
	baseline ":/!-mes.age" # negated above

Where baseline is defined as:

gitoxide/gix/tests/fixtures/make_rev_spec_parse_repos.sh

Lines 5 to 11 in 6487269

	function baseline() {
	local spec=${1:?first argument is the spec to test}
	{
	echo "$spec"
	git rev-parse -q --verify "$spec" 2>/dev/null || echo $?
	}>> baseline.git
	}

The change is due to different behavior of git rev-parse from git 2.46.2 to git 2.47.0, causing the expectations represented and commented there about which matching item it selected no longer to hold. The change does not seem to relate to any differences in how the different versions of git actually create the fixture repositories. This is shown by running it on both fixture repositories, after running tests in the gitoxide-with-git-2.46.2 and gitoxide-with-git-2.47.0 worktrees in the manner implied by those worktrees' names.

ek in 🌐 catenary in complex_graph on  main [?]
❯ pwd
/home/ek/repos/gitoxide-with-git-2.46.2/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph

ek in 🌐 catenary in complex_graph on  main [?]
❯ PATH="$HOME/git-2.46.2/bin:$PATH" git rev-parse ':/mes.age'
ef80b4b77b167f326351c93284dc0eb00dd54ff4

ek in 🌐 catenary in complex_graph on  main [?]
❯ PATH="$HOME/git-2.47.0/bin:$PATH" git rev-parse ':/mes.age'
9f9eac6bd1cd4b4cc6a494f044b28c985a22972b

ek in 🌐 catenary in complex_graph on  main [?]
❯ pwd
/home/ek/repos/gitoxide-with-git-2.47.0/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph

ek in 🌐 catenary in complex_graph on  main [?]
❯ PATH="$HOME/git-2.46.2/bin:$PATH" git rev-parse ':/mes.age'
ef80b4b77b167f326351c93284dc0eb00dd54ff4

ek in 🌐 catenary in complex_graph on  main [?]
❯ PATH="$HOME/git-2.47.0/bin:$PATH" git rev-parse ':/mes.age'
9f9eac6bd1cd4b4cc6a494f044b28c985a22972b

Thus, whichever version of git is used to produce the fixture repositories, the effect of git rev-parse varies solely by which version of git is used to run git rev-parse itself.

To show that the test failure is really happening in the baseline comparison and not elsewhere, consider the test failure output with a backtrace:

        FAIL [   0.587s] gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches

--- STDOUT:              gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches ---

running 1 test
test revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches ... FAILED

failures:

failures:
    revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 244 filtered out; finished in 0.58s


--- STDERR:              gix::gix revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches ---
failed to extract 'tests/fixtures/generated-archives/make_rev_spec_parse_repos.tar': Ignoring archive at 'tests/fixtures/generated-archives/make_rev_spec_parse_repos.tar' as GIX_TEST_IGNORE_ARCHIVES is set.
thread 'revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches' panicked at gix/tests/gix/revision/spec/from_bytes/util.rs:181:13:
assertion `left == right` failed: :/mes.age: left (ours) should match right (git): Ok(Spec { inner: Include(Sha1(ef80b4b77b167f326351c93284dc0eb00dd54ff4)), path: None, first_ref: None, second_ref: None, repo: Repository { kind: WorkTree { is_linked: false }, git_dir: "tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph/.git", work_dir: Some("tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph") } })
  left: Some(Include(Sha1(ef80b4b77b167f326351c93284dc0eb00dd54ff4)))
 right: Some(Include(Sha1(9f9eac6bd1cd4b4cc6a494f044b28c985a22972b)))
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14
   2: core::panicking::assert_failed_inner
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:405:23
   3: core::panicking::assert_failed
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:365:5
   4: gix::revision::spec::from_bytes::util::compare_with_baseline
             at ./tests/gix/revision/spec/from_bytes/util.rs:181:13
   5: gix::revision::spec::from_bytes::util::parse_spec_opts
             at ./tests/gix/revision/spec/from_bytes/util.rs:153:5
   6: gix::revision::spec::from_bytes::util::parse_spec
             at ./tests/gix/revision/spec/from_bytes/util.rs:196:5
   7: gix::revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches
             at ./tests/gix/revision/spec/from_bytes/regex.rs:99:13
   8: gix::revision::spec::from_bytes::regex::find_youngest_matching_commit::regex_matches::{{closure}}
             at ./tests/gix/revision/spec/from_bytes/regex.rs:95:23
   9: core::ops::function::FnOnce::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5
  10: core::ops::function::FnOnce::call_once
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

The regex_matches test has multiple assertions, but the failure happens when evaluating an expression to be used in the first assertion:

gitoxide/gix/tests/gix/revision/spec/from_bytes/regex.rs

Lines 93 to 101 in 6487269

	#[test]
	#[cfg(feature = "revparse-regex")]
	fn regex_matches() {
	let repo = repo("complex_graph").unwrap();
	assert_eq!(
	parse_spec(":/mes.age", &repo).unwrap(),
	Spec::from_id(hex_to_id("ef80b4b77b167f326351c93284dc0eb00dd54ff4").attach(&repo))
	);

That assertion is not really what is failing. Rather, failure occurs in an assertion in the implementation of parse_spec in util.rs. parse_spec calls parse_spec_opts, which calls compare_with_baseline. Because parse_spec_opts passes BaselineExpectation::Same as the expectation parameter of compare_with_baseline, this assertion runs:

gitoxide/gix/tests/gix/revision/spec/from_bytes/util.rs

Lines 179 to 185 in 6487269

	match expectation {
	BaselineExpectation::Same => {
	assert_eq!(
	actual, expected,
	"{spec}: left (ours) should match right (git): {res:?}"
	);
	}

That assertion is what is actually failing.

Expected behavior 🤔

All tests should pass even when run with GIX_TEST_IGNORE_ARCHIVES=1, but I am not sure whether the old or new rev-parse behavior should be preferred. If the new behavior is preferred then I think the behavior of gitoxide will also have to change, and in this case, something should probably be done so that GIX_TEST_IGNORE_ARCHIVES=1 can be used even with older versions of git.

I don't know if the new git behavior is intentional, and if not then this might be considered a bug in git rather than in gitoxide or in gitoxide's test suite. See below regarding the specific git behavior involved here.

To the best of my knowledge, this is the only case where baseline assertions checked in the gitoxide test suite fail outside of Windows. On Windows, we do have such failures, comprising at least some of the failures reported in #1358; see also the comment discussion in #1345, especially at and below #1345 (comment). But as far as I know there is no relationship, conceptual or otherwise, between the failure here and any of the failures in #1358, other than both being related to baseline assertions. Furthermore, the failures there are probably lower priority, because we do not currently ever commit archives generated by running the test suite on Windows with GIX_TEST_IGNORE_ARCHIVES=1, and because CI does not run tests that way.

In contrast, this issue will eventually cause the test job on CI to fail, once the GitHub-hosted ubuntu-latest runner has git 2.47.0. Going by the details there, some macOS images already have git 2.47.0, but no others, yet.

Please note that this is not related to #1623, which fixes an unrelated failure in the CI test job that is already occurring.

Git behavior

The underlying cause here is a change in git behavior. In the hope that it will help identify the specific change, this section examines the specific differences corresponding to the baseline.git change shown above:

 :/message
-ef80b4b77b167f326351c93284dc0eb00dd54ff4
+9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
 :/!-message
-55e825ebe8fd2ff78cad3826afb696b96b576a7e
+0270e757e023fedde198947489215e34bdaf1502
 :/mes.age
-ef80b4b77b167f326351c93284dc0eb00dd54ff4
+9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
 :/!-mes.age
-55e825ebe8fd2ff78cad3826afb696b96b576a7e
+0270e757e023fedde198947489215e34bdaf1502

To investigate, I ran:

ek in 🌐 catenary in complex_graph on  main [?]
❯ pwd
/home/ek/repos/gitoxide-with-git-2.46.2/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph

ek in 🌐 catenary in complex_graph on  main [?]
❯ (PATH="$HOME/git-2.46.2/bin:$PATH"; git version; for x in ':/message' ':/!-message' ':/mes.age' ':/!-mes.age'; do (echo; set -x; git show "$(git rev-parse "$x")"); done) &>~/2.46.2

ek in 🌐 catenary in complex_graph on  main [?]
❯ (PATH="$HOME/git-2.47.0/bin:$PATH"; git version; for x in ':/message' ':/!-message' ':/mes.age' ':/!-mes.age'; do (echo; set -x; git show "$(git rev-parse "$x")"); done) &>~/2.47.0-cross

ek in 🌐 catenary in complex_graph on  main [?]
❯ pwd
/home/ek/repos/gitoxide-with-git-2.47.0/gix/tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/3848495901-unix/complex_graph

ek in 🌐 catenary in complex_graph on  main [?]
❯ (PATH="$HOME/git-2.47.0/bin:$PATH"; git version; for x in ':/message' ':/!-message' ':/mes.age' ':/!-mes.age'; do (ech
o; set -x; git show "$(git rev-parse "$x")"); done) &>~/2.47.0

ek in 🌐 catenary in complex_graph on  main [?]
❯ (PATH="$HOME/git-2.46.2/bin:$PATH"; git version; for x in ':/message' ':/!-message' ':/mes.age' ':/!-mes.age'; do (echo; set -x; git show "$(git rev-parse "$x")"); done) &>~/2.46.2-cross

As expected, the logs 2.46.2 and 2.46.2-cross have the same contents, as do the logs 2.47.0 and 2.47.0-cross. The reason I expect this is that, as noted above, I think the only behavioral difference is in the git rev-parse commands and that the generated fixture repositories are (aside from their baseline.git files shown rev-parse results) equivalent in all relevant respects.

Here is the content of the 2.46.2 log:

git version 2.46.2

++ git rev-parse :/message
+ git show ef80b4b77b167f326351c93284dc0eb00dd54ff4
commit ef80b4b77b167f326351c93284dc0eb00dd54ff4
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:21:13 2005 -0700

    C

     message recent

diff --git a/file b/file
index edc79ae..419b800 100644
--- a/file
+++ b/file
@@ -1,3 +1,4 @@
 i
 j
 f
+c

++ git rev-parse ':/!-message'
+ git show 55e825ebe8fd2ff78cad3826afb696b96b576a7e
commit 55e825ebe8fd2ff78cad3826afb696b96b576a7e
Merge: 5b3f9e2 ef80b4b
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:22:13 2005 -0700

    A

diff --cc file
index a4e7183,419b800..fe27474
--- a/file
+++ b/file
@@@ -1,8 -1,4 +1,10 @@@
 +g
 +h
  i
  j
 +d
 +e
  f
 +b
+ c
++a

++ git rev-parse :/mes.age
+ git show ef80b4b77b167f326351c93284dc0eb00dd54ff4
commit ef80b4b77b167f326351c93284dc0eb00dd54ff4
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:21:13 2005 -0700

    C

     message recent

diff --git a/file b/file
index edc79ae..419b800 100644
--- a/file
+++ b/file
@@ -1,3 +1,4 @@
 i
 j
 f
+c

++ git rev-parse ':/!-mes.age'
+ git show 55e825ebe8fd2ff78cad3826afb696b96b576a7e
commit 55e825ebe8fd2ff78cad3826afb696b96b576a7e
Merge: 5b3f9e2 ef80b4b
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:22:13 2005 -0700

    A

diff --cc file
index a4e7183,419b800..fe27474
--- a/file
+++ b/file
@@@ -1,8 -1,4 +1,10 @@@
 +g
 +h
  i
  j
 +d
 +e
  f
 +b
+ c
++a

In contrast, here is the content of the 2.47.0 log:

git version 2.47.0

++ git rev-parse :/message
+ git show 9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
commit 9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:13:13 2005 -0700

    G

     initial message

diff --git a/file b/file
new file mode 100644
index 0000000..01058d8
--- /dev/null
+++ b/file
@@ -0,0 +1 @@
+g

++ git rev-parse ':/!-message'
+ git show 0270e757e023fedde198947489215e34bdaf1502
commit 0270e757e023fedde198947489215e34bdaf1502
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:14:13 2005 -0700

    H

diff --git a/file b/file
new file mode 100644
index 0000000..6e9f0da
--- /dev/null
+++ b/file
@@ -0,0 +1 @@
+h

++ git rev-parse :/mes.age
+ git show 9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
commit 9f9eac6bd1cd4b4cc6a494f044b28c985a22972b
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:13:13 2005 -0700

    G

     initial message

diff --git a/file b/file
new file mode 100644
index 0000000..01058d8
--- /dev/null
+++ b/file
@@ -0,0 +1 @@
+g

++ git rev-parse ':/!-mes.age'
+ git show 0270e757e023fedde198947489215e34bdaf1502
commit 0270e757e023fedde198947489215e34bdaf1502
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:14:13 2005 -0700

    H

diff --git a/file b/file
new file mode 100644
index 0000000..6e9f0da
--- /dev/null
+++ b/file
@@ -0,0 +1 @@
+h

Steps to reproduce 🕹

Most of the details are covered above, and the exact commands run for all tests can be seen in the gist.

How I built Git

I built, installed, and tested git 2.46.2 and git 2.47.0 on Arch Linux by running:

gh repo clone git/git
cd git
git switch -d v2.46.2
make prefix=~/git-2.46.2 -j10
make prefix=~/git-2.46.2 install
make prefix=~/git-2.46.2 test
gix clean -xde
git switch -d v2.47.0
make prefix=~/git-2.47.0 -j10
make prefix=~/git-2.47.0 install
make prefix=~/git-2.47.0 test

I could have done test and install in the other order (and test presumably didn't use the prefix value in any way). No unexpected failures occurred (i.e., as expected, some tests were reported as broken, but no tests had a status reported as failing).

In hindsight, I should have done gh repo clone git/git -- --recurse-submodules to get the sha1collisiondetection submodule. But I believe that makes no difference here: the tests do not seem to relate to the distinction between SHA1 and Hardened SHA1. Furthermore the failures only occur with 2.47.0, not with 2.46.2. Finally, I observe the failure with 2.47.0 when installed from the Arch Linux repositories on this system, as well as when installed from the git-core PPA on a separate Ubuntu 24.04.1 LTS system; I believe both of those builds do have hardened SHA-1.

How I tested gitoxide - full runs

Aside from some ad-hoc experiments running the full test suite on other systems to ensure that what I was observing was not due to the system being Arch Linux or to details of how I things up there, I ran the full test suite and varied three things, testing twelve combinations:

• current tip of main (6487269) vs. before #1620 was merged (37c1e4c, called old-main in the gist)
• git 2.46.2 vs. git 2.47.0 vs. downstream Arch Linux git 2.47.0-1 (2.47.0 and 2.47.0-1 behaved the same)
• undefined GIX_TEST_IGNORE_ARCHIVES vs. defining GIX_TEST_IGNORE_ARCHIVES=1

In the gist, logs described as individual runs are actually runs both without, and then with, GIX_TEST_IGNORE_ARCHIVES=1.

I always ran gix clean -xde between runs (including between not defining GIX_TEST_IGNORE_ARCHIVES and defining it as 1).

I ran the test suite with cargo nextest run --all --no-fail-fast, varying the git command used by prefixing PATH="$HOME/git-2.46.2/bin:$PATH", PATH="$HOME/git-2.47.0/bin:$PATH", or (for the downstream git 2.47.0-1) no such prefix. For example:

PATH="$HOME/git-2.47.0/bin:$PATH" GIX_TEST_IGNORE_ARCHIVES=1 cargo nextest run --all --no-fail-fast

How I tested gitoxide - the specific failing test

To verify that the effect was not due to the interaction of multiple tests, I ran the specific failing test as well, including after running gix clean -xde. This is further detailed in the gist and it is also how I produced the output showing the backtrace. Specifically, the backtrace shown above was produced with:

cd gix/tests/gix/revision/spec/from_bytes
GIX_TEST_IGNORE_ARCHIVES=1 RUST_BACKTRACE=1 cargo nextest run regex_matches

(That used the downstream 2.47.0-1--which behaves the same as 2.47.0, at least with respect to what this issue investigates--and thus omits a PATH=... prefix.)

[Byron]

Thanks a lot for the detailed report!

It is my hope that once all the Windows tests are working reliably, the final step to mostly deterministic CI runs is to proactively test for changes on CI with GIX_TEST_IGNORE_ARCHIVES=1 set.

Looking at the graph itself, it's notable that Git v2.46.2 find [c], indeed the first matching commit…

…but v2.47.0 finds [g], the first matching commit if one would follow first first parents first.

This probably has to do with a change in the underlying traversal order, and I vaguely remember that traversing the first parents first can be done as performance optimisation. I think I have seen that in the commit-graph traversal employed by merge-base searches.
Merge-bases are probably unrelated here, but some performance optimization might not be.

On the other hand, I also wouldn't be surprised if this is an unintended side-effect of another change to the way Git performs traversals.

Regarding the failing tests, it's obviously very dependent on the traversal order of the commit-graph even though that isn't what's under test. So I'd think that could be fixed by adjusting the commit messages to be unique enough to only allow one possible match.

[EliahKagan]

So I'd think that could be fixed by adjusting the commit messages to be unique enough to only allow one possible match.

Isn't part of the purpose of these particular tests to check these kind of situations where traversal order matters? That is my impression because it seems like the comment here is documenting intent:

gitoxide/gix/tests/fixtures/make_rev_spec_parse_repos.sh

Line 351 in 6487269

baseline ":/message" # finds 'message recent' instead of 'initial message'

Or is the idea that we should no longer attempt to test that?

[Byron]

That's indeed great evidence that this was intended all along. Thinking about it more, it does make sense as rev-parse needs to do a traversal so gitoxide has to try and match it.

It's a bit strange to see this was changed so fundamentally in v2.47, especially since the release notes don't seem to mention any of it.

Then I don't know what to do, as I do find the previous behaviour more intuitive. Maybe we just keep this open and wait, maybe it disappears as sudden as it appeared.

[EliahKagan]

It is my hope that once #1358 are working reliably, the final step to mostly deterministic CI runs is to proactively test for changes on CI with GIX_TEST_IGNORE_ARCHIVES=1 set.

Do you also want macOS runs on CI with GIX_TEST_IGNORE_ARCHIVES=1? If so, then that's an opportunity to prepare for when this issue breaks the test job once ubuntu-latest has git 2.47.0, since I believe macOS runners already (since recently) have that version of git. This relates to...

Then I don't know what to do, as I do find the previous behaviour more intuitive. Maybe we just keep this open and wait, maybe it disappears as sudden as it appeared.

I agree that we should not remove or weaken the tests and that waiting might lead to a further update to Git that reverses the behavior.

It may be useful to prepare for CI breakage by making it so that the test suite automatically skips the affected test case when running on CI with git >=2.47.0. How best to go about that--and also when--may depend on the above macOS question.

(I encountered this issue when working on something else in gitoxide that is conceptually unrelated, and I'm in no hurry to do anything potentially unnecessary on this issue, so if the best way forward right now is just to wait, that is fine by me.)

[Byron]

Do you also want macOS runs on CI with GIX_TEST_IGNORE_ARCHIVES=1? If so, then that's an opportunity to prepare for when this issue breaks the test job once ubuntu-latest has git 2.47.0, since I believe macOS runners already (since recently) have that version of git.

Doing something depending on the version of Git could be interesting to gitoxide users as well. Unless you are interested in adding a version-dependent skip of the test, I think it's OK to wait until it breaks. It's well-known to two people now so I think the issue can easily be 'fixed' by ignoring the test on Linux as quick-fix, and to selectively skip it based on the parsed version in a follow-up.

It's a bit sad that once it's skipped, there is no way to easily learn if it still works - so maybe the skip should be implemented so that it will also test for the opposite case - "it's skipped, but the test actually succeeds, fix it".

[EliahKagan]

Doing something depending on the version of Git could be interesting to gitoxide users as well.

If you mean having gitoxide's own search behavior vary based on what version of Git is installed, I recommend against doing that by default, because:

1. It is hard to avoid relying on behavior one observes consistently during development, especially if one observes it both in interactive local use and on CI, and especially when one observes related software--in this case, Git--also to have that behavior. I also think developers of some Git-related software may be more likely to use more recent versions of Git than their users use (even if their users are also software developers).

   So if gitoxide behaves like the git command it finds on the system where it runs, for functionality where gitoxide will behave differently elsewhere, this would make it very easy for developers of applications that use gitoxide to write bugs based on implicit inaccurate assumptions that deployment or end-user environments match development and CI environments.

2. Except in the narrow range of situations where both gitoxide is in a direct or indirect subprocess of git and git has itself prepended its git-core directory to PATH, there need not be any single value that should be understood as "the version of Git." Git may be absent or installed in multiple places at different versions. The git that gitoxide finds may even be a build that is bundled with a (third) application or otherwise serves a specific purpose.

   I think this is less of a problem when taking configuration from git--because the configuration comes from a file, gix config reveals its origin, and it can be overridden in a lower scope--than in inferring behavior from the version.

3. This does not seem to be an area where gitoxide needs to behave the same as Git for reasons of compatibility. This is in contrast to something like GIT_CONFIG_PARAMETERS, where supporting it would be almost entirely for the purpose of interoperability with a locally available git, and where it would be necessary either to support or otherwise account for how its values use two different syntaxes depending on the git version.

These points relate indirectly (and in the case of the GIT_CONFIG_PARAMETERS example, directly) to some of my thoughts discussed in #1585 (review). Accordingly, I acknowledge that when I open the issues mentioned in #1585 (comment) (which I have not yet done), putting the broader ideas in more coherent form may end up revealing that I am mistaken about some of these things.

For the question of whether the gitoxide behavior related to this issue should match that of git 2.46.2 or git 2.47.0, I think that, unless this change goes away in a git 2.47.* patch release, we should eventually figure out what has caused the change, and whether it is intentional:

• If it is unintentional and a bug, then it should be changed back in Git. In that case, I can report it to the Git mailing list, and gitoxide should not change to match the new behavior.
• If it is unintentional but not a bug, and should not be changed back in Git, then gitoxide should be changed to match the new behavior, unless there are disadvantages to gitoxide doing that.
• If it is intentional, then gitoxide should be changed to match the new behavior, unless there are serious disadvantages to doing so or it is infeasible to do so.
• If it turns out that gitoxide ought to behave according to the older rule some of the time and according to the newer rule other times, then a gitoxide.* configuration variable could be added for it. This variable could recognize a value that causes gitoxide's behavior to be based on what Git version, if any, is found. But for the reasons above, I think that would still not be the best default value of this configuration variable.

I can eventually look further into how this change has arisen in Git. If I still cannot figure it out, then I could inquire on the Git mailing list. This specific research is not my immediate main priority for gitoxide-related research, but it is something I am willing to do eventually, if the old behavior is not restored in the mean time by a patch release.

Unless you are interested in adding a version-dependent skip of the test, I think it's OK to wait until it breaks.

Yes, I was thinking of doing something like that soon. I think the subexpression that fails in this way--and the others that would fail if it didn't--can be extracted and modified so that they do the check without a panic. If not, then the baseline check implementation--which is also part of the test suite--could be modified to allow this.

Then if the baseline checks triggered by the regex_matches test fail, check if both we are on CI and git reports its version as 2.47.0 or higher, and avoid failing the test in this case, or at least skip the assertions that would fail. If necessary, checking for CI could be omitted and this skipping logic could also be run locally.

However:

• I would probably wait to do this until it is failing on CI (as you have suggested).

  The exception is if we are going to want to start running tests with GIX_TEST_IGNORE_ARCHIVES=1 on macOS on CI. The reason that would motivate me to make the change sooner is that, unlike on Windows, on macOS we do not have a long-standing situation where tests fail on macOS when run that way, and GitHub-hosted macOS runners seem already to have git 2.47.0 installed. So this would easily let me start with a situation where all but the affected test pass on CI.

• There may be a better way to do it, that better keeps track of which tests are being skipped under what conditions and that provides utility beyond this specific test.

  If GIX_TEST_IGNORE_ARCHIVES or one or more other environment variables were to accept a syntax allowing specific fixtures to be overridden as having their archives ignored or not ignored, then CI steps could specify GIX_TEST_IGNORE_ARCHIVES=1 or something like it while also explicitly preventing specific broken fixtures from running.

  I plan to consider this idea further and open a PR to implement or an issue to discuss it if it still seems useful. But maybe you have thoughts on it now.

[Byron]

If you mean having gitoxide's own search behavior vary based on what version of Git is installed, I recommend against doing that by default, because:

No, that's (fortunately) not what I meant. The idea was to use this to selectively alter the expected outcome of the failing test, while providing this feature to gitoxide users. Some may rely on calling out to git for pushing/fetching, and maybe there are differences depending on the git version as well.

I can eventually look further into how this change has arisen in Git. If I still cannot figure it out, then I could inquire on the Git mailing list. This specific research is not my immediate main priority for gitoxide-related research, but it is something I am willing to do eventually, if the old behavior is not restored in the mean time by a patch release.

Thank you! I agree that there are more important things to do and that this regression can be taken care of later if it doesn't resolve itself.

I would probably wait to do this until it is failing on CI (as you have suggested).

I think so, too.

I plan to consider this idea further and open a PR to implement or an issue to discuss it if it still seems useful. But maybe you have thoughts on it now.

Let's wait, I think when this is implemented, if at all, decent solutions will be straightforward enough, and they can take different shapes independently of the first ideas shared here even.

[EliahKagan]

Sounds good. If I notice that CI is immediately about to fail for this reason, then I'll make note of that. Otherwise, I'll wait until I observe it does actually fail. (I suggest keeping this issue open until either there is a lasting change in gitoxide that fixes it or the changed Git behavior has gone away.)