Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let nonstandard worktree fixtures work even if Git < 2.37.2 #1334

Merged
merged 1 commit into from
Apr 3, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Apr 3, 2024

This reorders the operations in the nonstandard worktree fixtures to work even when Git is at a lower version than 2.37.2. This makes root_enters_directory_with_dot_git_in_reconfigured_worktree_tracked pass on more systems, including common development systems.

When fixture scripts are run on a system whose Git version is lower than 2.37.2, including currently maintained downstream versions with backported security (but not most other) fixes on some popular OSes (e.g. Ubuntu 22.04.4 LTS with 2.34.1-1ubuntu1.10), the gix-dir nonstandard worktree fixtures had silently failed to stage one of two intended files. They appeared to succeed, but created a repo with only the seemingly-outside file, and not the inside file.

This is due to the bug git/git@27128996b8 fixed in Git. The nonstandard worktree fixtures had needed Git to support the first condition noted there, where core.worktree is the parent of the default worktree, in order to stage both files.

The change made here overcomes the problem by staging before rather than after moving .git and setting core.worktree. The git add command used to stage is adjusted to remain equivalent in its effect. (Committing can be, and is, still done last.)

CI hadn't suffered/shown the problem, since while GHA ubuntu-latest runners are currently Ubuntu 22.04, they have many newer software versions installed on them, currently including Git 2.43.2.

The original test failure

Prior to this change, the failure on affected systems (those with insufficiently recent git) is in the assertion about the value of entries:

--- STDERR:              gix-dir::dir walk::root_enters_directory_with_dot_git_in_reconfigured_worktree_tracked ---
thread 'walk::root_enters_directory_with_dot_git_in_reconfigured_worktree_tracked' panicked at gix-dir/tests/walk/mod.rs:2917:5:
assertion failed: `(left == right)`: everything is tracked, so it won't try to detect git repositories anyway

Diff < left / right > :
 [
     (
         Entry {
             rela_path: "dir-with-dot-git/inside",
<            status: Untracked,
>            status: Tracked,
             property: None,
             disk_kind: Some(
                 File,
             ),
<            index_kind: None,
>            index_kind: Some(
>                File,
>            ),
             pathspec_match: Some(
                 Always,
             ),
         },
         None,
     ),
 ]


note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Details of how I ran the tests are included below.

Rationale

Since the fix is straightforward, I think it is worth applying here in gitoxide's test fixtures, even though the actual bug involved in triggering the failure is not in any code carried by gitoxide but rather in Git.

However, if that is not judged to be worthwhile, then I recommend the issue be noted somewhere, preferably in a comment in or near the relevant fixtures or the one test that is currently failing on affected systems. In that case, this PR could be modified to add such a comment instead of making the change to the fixture script. Or it could be closed and I could open a new one.

Some test fixtures in gitoxide have associated generated archives that may need to be updated under some circumstances. I believe that is inapplicable here, because no generated archive associated with the gix-dir test fixture is tracked. (In addition, the fixture changes here do not change the way the repository created by the fixture is set up, so even if generated archives were tracked for it, I don't think there would be any need to regenerate them.)

Verification

I performed a number of tests, building various Git versions and testing them with the fixture shell script fragment to verify that the problem is caused by the bug cited. Having narrowed down where the problem was in Git, I also verified that building Git from the bugfix's parent commit triggered the problem in an actual gitoxide test run, while building Git from the bugfix commit averted it; and I ran that full test with a few released versions of Git as well. I took these precautions:

  • Each of the handful of tests on gitoxide was done on a fresh clone, to ensure I wasn't leaving any fixture output behind (which I might had I manually removed it), since neither cargo clean nor git clean -dXf are sufficient to remove it (I believe because git sees these are nested repositories it shouldn't touch).
  • Each clone of gitoxide was followed by git lfs install and git lfs pull in the cloned directory, and these commands were always done with the system git, even when it was a different version that I then placed earlier in the path to be used in the tests.
  • I always ran all non-doctest tests, using cargo nextest run --all --no-fail-fast from the root of the gitoxide working tree. (The exception to this is shown above, where for demonstration I did another clone and reran just the affected test, to show its failure clearly.)

No test ever failed except the one being investigated.

Some notes from that, and a script I used in otherwise manual bisection, can be seen in this gist. But I think the only actually important information is the result of manually trying out the fixtures on system and current versions of Git, with the version of the fixture before the change versus after. So I've reproduced that as follows. These are on an Ubuntu 22.04 system (x86_64).

Git 2.44.0 built from source, with the old (current) version of the fixture

ek@instance-20240317-140932:~/tmp/2.44.0-oldscript$ git init nonstandard-worktree
(cd nonstandard-worktree
  mkdir dir-with-dot-git
  mv .git dir-with-dot-git

  git -C dir-with-dot-git config core.worktree "$PWD"
  touch dir-with-dot-git/inside
  touch seemingly-outside
  git -C dir-with-dot-git add inside ../seemingly-outside
  git -C dir-with-dot-git commit -m "init"
)
Initialized empty Git repository in /home/ek/tmp/2.44.0-oldscript/nonstandard-worktree/.git/
[main (root-commit) 3b5a103] init
 2 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 dir-with-dot-git/inside
 create mode 100644 seemingly-outside
ek@instance-20240317-140932:~/tmp/2.44.0-oldscript$ git -C nonstandard-worktree/dir-with-dot-git config --list --local
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.worktree=/home/ek/tmp/2.44.0-oldscript/nonstandard-worktree
ek@instance-20240317-140932:~/tmp/2.44.0-oldscript$ git -C nonstandard-worktree/dir-with-dot-git show
commit 3b5a103764b1d377838471a99e41d861474edb7b (HEAD -> main)
Author: Eliah Kagan <[email protected]>
Date:   Wed Apr 3 01:42:30 2024 +0000

    init

diff --git a/dir-with-dot-git/inside b/dir-with-dot-git/inside
new file mode 100644
index 0000000..e69de29
diff --git a/seemingly-outside b/seemingly-outside
new file mode 100644
index 0000000..e69de29

Git 2.44.0 built from source, with the new (proposed) version of the fixture

ek@instance-20240317-140932:~/tmp/2.44.0-newscript$ git init nonstandard-worktree
(cd nonstandard-worktree
  mkdir dir-with-dot-git
  touch dir-with-dot-git/inside
  touch seemingly-outside
  git add dir-with-dot-git/inside seemingly-outside
  mv .git dir-with-dot-git
  git -C dir-with-dot-git config core.worktree "$PWD"
  git -C dir-with-dot-git commit -m "init"
)
Initialized empty Git repository in /home/ek/tmp/2.44.0-newscript/nonstandard-worktree/.git/
[main (root-commit) 2bbcb74] init
 2 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 dir-with-dot-git/inside
 create mode 100644 seemingly-outside
ek@instance-20240317-140932:~/tmp/2.44.0-newscript$ git -C nonstandard-worktree/dir-with-dot-git config --list --local
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.worktree=/home/ek/tmp/2.44.0-newscript/nonstandard-worktree
ek@instance-20240317-140932:~/tmp/2.44.0-newscript$ git -C nonstandard-worktree/dir-with-dot-git show
commit 2bbcb7470d0512735d1a5491f9508d552be10eef (HEAD -> main)
Author: Eliah Kagan <[email protected]>
Date:   Wed Apr 3 01:44:25 2024 +0000

    init

diff --git a/dir-with-dot-git/inside b/dir-with-dot-git/inside
new file mode 100644
index 0000000..e69de29
diff --git a/seemingly-outside b/seemingly-outside
new file mode 100644
index 0000000..e69de29

Git 2.34.1 provided by the system, with the old (current) version of the fixture

In this case, and none of the others, the fixture fails to stage one of the two files, leading to the spurious test failure this PR seeks to rectify.

ek@instance-20240317-140932:~/tmp/2.34.1-oldscript$ git init nonstandard-worktree
(cd nonstandard-worktree
  mkdir dir-with-dot-git
  mv .git dir-with-dot-git

  git -C dir-with-dot-git config core.worktree "$PWD"
  touch dir-with-dot-git/inside
  touch seemingly-outside
  git -C dir-with-dot-git add inside ../seemingly-outside
  git -C dir-with-dot-git commit -m "init"
)
Initialized empty Git repository in /home/ek/tmp/2.34.1-oldscript/nonstandard-worktree/.git/
[main (root-commit) 8ec306b] init
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 seemingly-outside
ek@instance-20240317-140932:~/tmp/2.34.1-oldscript$ git -C nonstandard-worktree/dir-with-dot-git config --list --local
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.worktree=/home/ek/tmp/2.34.1-oldscript/nonstandard-worktree
ek@instance-20240317-140932:~/tmp/2.34.1-oldscript$ git -C nonstandard-worktree/dir-with-dot-git show
commit 8ec306bc3d0eef8d256368afb29f743ad8b58535 (HEAD -> main)
Author: Eliah Kagan <[email protected]>
Date:   Wed Apr 3 01:47:58 2024 +0000

    init

diff --git a/seemingly-outside b/seemingly-outside
new file mode 100644
index 0000000..e69de29

Git 2.34.1 provided by the system, with the new (proposed) version of the fixture

ek@instance-20240317-140932:~/tmp/2.34.1-newscript$ git init nonstandard-worktree
(cd nonstandard-worktree
  mkdir dir-with-dot-git
  touch dir-with-dot-git/inside
  touch seemingly-outside
  git add dir-with-dot-git/inside seemingly-outside
  mv .git dir-with-dot-git
  git -C dir-with-dot-git config core.worktree "$PWD"
  git -C dir-with-dot-git commit -m "init"
)
Initialized empty Git repository in /home/ek/tmp/2.34.1-newscript/nonstandard-worktree/.git/
[main (root-commit) ba4e92a] init
 2 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 dir-with-dot-git/inside
 create mode 100644 seemingly-outside
ek@instance-20240317-140932:~/tmp/2.34.1-newscript$ git -C nonstandard-worktree/dir-with-dot-git config --list --local
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.worktree=/home/ek/tmp/2.34.1-newscript/nonstandard-worktree
ek@instance-20240317-140932:~/tmp/2.34.1-newscript$ git -C nonstandard-worktree/dir-with-dot-git show
commit ba4e92a9c5a813e07e9c5d007a8cd8bfc32c709f (HEAD -> main)
Author: Eliah Kagan <[email protected]>
Date:   Wed Apr 3 01:51:13 2024 +0000

    init

diff --git a/dir-with-dot-git/inside b/dir-with-dot-git/inside
new file mode 100644
index 0000000..e69de29
diff --git a/seemingly-outside b/seemingly-outside
new file mode 100644
index 0000000..e69de29

This reorders the operations in the nonstandard worktree fixtures
to work even when Git is at a lower version than 2.37.2. This makes
root_enters_directory_with_dot_git_in_reconfigured_worktree_tracked
pass on more systems, including common development systems.

When fixture scripts are run on a system whose Git version is lower
than 2.37.2, including currently maintained downstream versions
with backported security (but not most other) fixes on some popular
OSes (e.g. Ubuntu 22.04.4 LTS with 2.34.1-1ubuntu1.10), the gix-dir
nonstandard worktree fixtures had silently failed to stage one of
two intended files. They appeared to succeed, but created a repo
with only the "seemingly-outside" file, and not the "inside" file.

This is due to the bug git/git@27128996b8
fixed in Git. The nonstandard worktree fixtures had needed Git to
support the first condition noted there, where core.worktree is the
parent of the default worktree, in order to stage both files.

The change made here overcomes the problem by staging before rather
than after moving .git and setting core.worktree. The `git add`
command used to stage is adjusted to remain equivalent in its
effect. (Committing can be, and is, still done last.)

CI hadn't suffered/shown the problem, since while GHA ubuntu-latest
runners are currently Ubuntu 22.04, they have many newer software
versions installed on them, currently including Git 2.43.2.
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fantastic to see you here, welcome!!

However, if that is not judged to be worthwhile, then I recommend the issue be noted somewhere, preferably in a comment in or near the relevant fixtures or the one test that is currently failing on affected systems. In that case, this PR could be modified to add such a comment instead of making the change to the fixture script. Or it could be closed and I could open a new one.

For future reference, if you think the PR is worthwhile, 99.9% of the cases, I predict, it will be merged in one form or another. I hope this serves as encouragement to not include disclaimers like these in future :).

  • Each of the handful of tests on gitoxide was done on a fresh clone, to ensure I wasn't leaving any fixture output behind (which I might had I manually removed it), since neither cargo clean nor git clean -dXf are sufficient to remove it (I believe because git sees these are nested repositories it shouldn't touch).

I suggest to try gix clean - it's able to deal with this repository properly, and will not leave repositories inside of excludes directories behind.

  • Each clone of gitoxide was followed by git lfs install and git lfs pull in the cloned directory, and these commands were always done with the system git, even when it was a different version that I then placed earlier in the path to be used in the tests.

git lfs was removed from the repository as due to its implementation in GitHub and the payment model, it became prohibitive. The issue was that there were too many clones that caused LFS bandwidth to be consumed, and despite just being less than 5MB or so each, I hit the bandwidth limit of a meager 1GB. Buying 50GB would have worked, but they are not carried over, while all I would have needed is maybe 1.5GB per month.

This was why I had to replace all LFS files with copies of the respective resource and start storing them in Git.

And even if git lfs would still be there, the system is able to auto-generate fixtures if the archive can't be extracted (as it's still an LFS manifest, for example).

and these commands were always done with the system git, even when it was a different version that I then placed earlier in the path to be used in the tests.

This attention to detail is no less than impressive! gitoxide is incredibly happy to have you :).

@Byron Byron merged commit 37732fb into GitoxideLabs:main Apr 3, 2024
18 checks passed
@EliahKagan EliahKagan deleted the nonstandard-worktree branch April 3, 2024 15:09
@EliahKagan
Copy link
Member Author

I suggest to try gix clean - it's able to deal with this repository properly, and will not leave repositories inside of excludes directories behind.

Thanks! gix clean -xde removes them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants