fix: submodule worktree incorrect with symlinked `.git` dir (#2052) #2182

arumugamramaswamy · 2025-09-21T15:11:03Z

fixes #2052

My fix introduced a regression. Looks like we never return relative paths for repo.workdir(). One way I see to fix this is by making the path relative to cwd, but not sure if this is desired. Any thoughts on how we can work around this?

failures:

---- repository::open::submodules::by_their_worktree_checkout_and_git_modules_dir stdout ----
[gix/src/open/repository.rs:308:17] &git_dir = "tests/fixtures/generated-do-not-edit/make_submodules/1482069896-unix/with-submodules/.git/modules/m1"
[gix/src/open/repository.rs:308:17] &wt_path = "tests/fixtures/generated-do-not-edit/make_submodules/1482069896-unix/with-submodules/.git/modules/m1/../../../m1"

thread 'repository::open::submodules::by_their_worktree_checkout_and_git_modules_dir' panicked at gix/tests/gix/repository/open.rs:338:17:
assertion `left == right` failed
  left: "/home/aru/oss/gitoxide/gix/tests/fixtures/generated-do-not-edit/make_submodules/1482069896-unix/with-submodules/m1"
 right: "tests/fixtures/generated-do-not-edit/make_submodules/1482069896-unix/with-submodules/m1"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- repository::worktree::with_core_worktree_config::relative stdout ----
[gix/src/open/repository.rs:308:17] &git_dir = "tests/fixtures/generated-do-not-edit/make_core_worktree_repo/4038243702-unix/absolute-worktree/.git"
[gix/src/open/repository.rs:308:17] &wt_path = "/home/aru/oss/gitoxide/gix/tests/fixtures/generated-do-not-edit/make_core_worktree_repo/4038243702-unix/worktree"
[gix/src/open/repository.rs:308:17] &git_dir = "tests/fixtures/generated-do-not-edit/make_core_worktree_repo/4038243702-unix/relative-worktree/.git"
[gix/src/open/repository.rs:308:17] &wt_path = "tests/fixtures/generated-do-not-edit/make_core_worktree_repo/4038243702-unix/relative-worktree/.git/../../worktree"

thread 'repository::worktree::with_core_worktree_config::relative' panicked at gix/tests/gix/repository/worktree.rs:51:17:
assertion `left == right` failed: relative-worktree|true: work_dir is set to core.worktree config value, relative paths are appended to `git_dir() and made absolute`
  left: "/home/aru/oss/gitoxide/gix/tests/fixtures/generated-do-not-edit/make_core_worktree_repo/4038243702-unix/worktree"
 right: "tests/fixtures/generated-do-not-edit/make_core_worktree_repo/4038243702-unix/worktree"


failures:
    repository::open::submodules::by_their_worktree_checkout_and_git_modules_dir
    repository::worktree::with_core_worktree_config::relative

Byron · 2025-09-22T04:56:23Z

Without having looked at it in detail, it's true that gitoxide attempts to retain the path given by the user, which also means that paths can be relative to the CWD. Or in other words, the paths passed by the user are retained.
I hope that helps.

arumugamramaswamy · 2025-09-22T08:12:49Z

Would it be a breaking change if we use absolute paths for the worktree_dir instead?

Byron · 2025-09-22T17:22:59Z

Absolutely, at least when done unconditionally. But maybe this even should be done when it makes sense? Would the case here qualify using real paths?
Unfortunately, this is one of the cases where asking Git doesn't help as I think it will always use real-paths instead or it's so different in how it deals with paths that it's not really comparable.

arumugamramaswamy · 2025-09-23T16:38:55Z

Looks like I got the tests to pass! However, I'm not sure how I should create the .tar archive.
(The old archive had a path prefixed by /home/aru which was weird)

EliahKagan

Thank you for contributing this!

This is not really a review of the PR as a whole, I just noticed that it looked like I could help out with a couple of things.

Looks like I got the tests to pass! However, I'm not sure how I should create the .tar archive.

I've fixed a bug in the shell script--see below--and I'll push that followed by a regenerated archive. (If it were regenerated first, it would need to be regenerated again after the fix to the script, since the check for whether a committed archive is up to date is based on a hash of the fixture script used to generate it.)

The old archive had a path prefixed by /home/aru which was weird

I think this is worth looking into, assuming it's unintentional. I could be misunderstanding, but I think the intent of the code here is not to cause an absolute path to be written into the repository for the submodule, at least not in the fixture. I have not looked into this (and I don't have time to do so at the moment).

If there is such a path in the committed .tar archive that will be present in the immediately forthcoming commits, it would probably start with /home/ek (since my username is ek).

Please note that, other than the specific part of the shell script that I've modified as described below, I have not really reviewed this PR at all--I'm only submitting this as a review because it allows me to comment on a specific line of code.

gix/tests/fixtures/make_submodules.sh

EliahKagan · 2025-09-23T19:09:56Z

This test failure occurs on CI on macOS:

        FAIL [   0.638s] gix::gix status::index_worktree::iter::submodule_in_symlinked_dir
  stdout ───

    running 1 test
    test status::index_worktree::iter::submodule_in_symlinked_dir ... FAILED

    failures:

    failures:
        status::index_worktree::iter::submodule_in_symlinked_dir

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 335 filtered out; finished in 0.61s
    
  stderr ───
    Error: IndexAsWorktreeWithRenames(DirWalk(WorktreeRootIsFile { root: "/System/Volumes/Data/home/ek/repos/gitoxide/gix/tests/fixtures/generated-do-not-edit/make_submodules/2131503439-unix/symlinked-git-dir" }))

But I don't think the problem is macOS-specific. I suspect it will happen whenever we don't set GIX_TEST_IGNORE_ARCHIVES.

This arises from the problem you noticed a hint of in #2182 (comment), regarding the inclusion of a hard-coded path to your home directory when you tried generating the archives. In addition, this path is somehow being appended--maybe as though it were a relative path?--to /System/Volumes/Data.

Feel free to rewind (i.e. drop) e1d9ae3, of course, if it turns out to be useful to do so. (By itself removing that won't fix whatever is causing this, though.)

If the behavior of hard-coding an absolute path that is specific to the current machine in the fixture is really intended, then this could be solved by adding the fixture archive to the appropriate .gitignore file so that gix-testtools will not use or expect a generated archive and will instead regenerate the fixture on every machine where the test suite is run. Since this can itself hide some kinds of problems, and since make_submodules already existed with code that does not require this measure, it might be best to split the new code out into its own fixture script, if feasible.

Of course, all of that is still only applicable if the behavior of the fixture script (and associated tests) is as intended, such that the generated fixture really should hard-code an absolute path that is only correct on the system where it is run.

cc @Byron

Byron

Thanks a lot for tackling this! The code itself looks mostly good to me, so I think the real issue here comes from the way the tests work.

I hope my comments in that regard make sense, but if not, I could also take it form here if you like. Just let me know.
Thanks again!

gix/src/open/repository.rs

gix/tests/fixtures/make_submodules.sh

arumugamramaswamy · 2025-09-24T14:08:06Z

Thanks @Byron, @EliahKagan! The PR is ready for another of review:)

Edit: sorry, let me get CI to pass 😅

Co-authored-by: Eliah Kagan <[email protected]>

Byron

Thanks a lot for taking another look!

I now spent some time with this and realise that some changes are needed.
The most important one is a test which at least makes clear what's under test, and something as complex as a status isn't that.

Thanks again for bearing with me here - if you are out of time I could also take it from here.

Byron · 2025-09-25T03:29:49Z

gix/src/open/repository.rs

+                // the reason we use realpath instead of gix_path::normalize here is because there
+                // could be any intermediate symlinks (for example due to a symlinked .git
+                // directory)
+                worktree_dir = gix_path::realpath(&wt_path).ok();


Using realpath should be conditional, based on if the path exists or not.
Because what's under test here really is that normalize can mess up paths that have symlinks in them, yielding an invalid path.

So I think there should even be a warning in gix_path::normalize() to tell people to check if the path is still valid.
In a follow-up, one should check all call-sites as they will have the same issue.

Byron · 2025-09-25T03:30:54Z

gix/tests/gix/status.rs

        }

+        #[test]
+        fn submodule_in_symlinked_dir() -> crate::Result {


This test should be removed in favor of specific tests which make clear what's under test.
This test 'only' tests if the status succeeds, otherwise it fails with something like.

Error: IndexAsWorktreeWithRenames(TrackedFileModifications(SubmoduleStatus { rela_path: "m1", source: SubmoduleStatus(NextStatusItem(IndexWorktree(IndexAsWorktreeWithRenames(DirWalk(WorktreeRootIsFile { root: "tests/fixtures/generated-do-not-edit/make_submodule_with_symlinked_git_dir/3844911589-unix/m1" }))))) }))

Byron · 2025-09-25T03:34:43Z

gix/src/open/repository.rs

+                    if let Some(rel_path) = worktree_dir.as_deref().and_then(|p| p.strip_prefix(current_dir).ok()) {
+                        worktree_dir = Some(rel_path.to_path_buf());
+                    }
+                }


If we have this, there must be a test for this. Maybe it's OK to leave it out for now, since this symlink handling is already quite a special case.

arumugamramaswamy force-pushed the main branch from 0533ad7 to 52f54f2 Compare September 23, 2025 16:33

EliahKagan reviewed Sep 23, 2025

View reviewed changes

gix/tests/fixtures/make_submodules.sh Outdated Show resolved Hide resolved

Byron reviewed Sep 24, 2025

View reviewed changes

gix/src/open/repository.rs Outdated Show resolved Hide resolved

gix/tests/fixtures/make_submodules.sh Outdated Show resolved Hide resolved

arumugamramaswamy requested review from EliahKagan and Byron September 24, 2025 14:08

fix: worktree when .git is symlink (GitoxideLabs#2052)

3ee9b63

Co-authored-by: Eliah Kagan <[email protected]>

Byron force-pushed the main branch from 1a6c81f to 3ee9b63 Compare September 25, 2025 03:25

Byron requested changes Sep 25, 2025

View reviewed changes

This was referenced Sep 29, 2025

fix: make realpath conditional (#2052 follow up to #2182) arumugamramaswamy/gitoxide#1

Closed

fix: make realpath conditional (#2052 follow up to #2182) #2199

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: submodule worktree incorrect with symlinked `.git` dir (#2052) #2182

fix: submodule worktree incorrect with symlinked `.git` dir (#2052) #2182

arumugamramaswamy commented Sep 21, 2025 •

edited

Loading

Uh oh!

Byron commented Sep 22, 2025

Uh oh!

arumugamramaswamy commented Sep 22, 2025

Uh oh!

Byron commented Sep 22, 2025

Uh oh!

arumugamramaswamy commented Sep 23, 2025

Uh oh!

EliahKagan left a comment

Uh oh!

Uh oh!

EliahKagan commented Sep 23, 2025

Uh oh!

Byron left a comment

Uh oh!

Uh oh!

Uh oh!

arumugamramaswamy commented Sep 24, 2025 •

edited

Loading

Uh oh!

Byron left a comment •

edited

Loading

Uh oh!

Byron Sep 25, 2025

Uh oh!

Byron Sep 25, 2025

Uh oh!

Byron Sep 25, 2025

Uh oh!

Uh oh!

Uh oh!

fix: submodule worktree incorrect with symlinked .git dir (#2052) #2182

Are you sure you want to change the base?

fix: submodule worktree incorrect with symlinked .git dir (#2052) #2182

Conversation

arumugamramaswamy commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Sep 22, 2025

Uh oh!

arumugamramaswamy commented Sep 22, 2025

Uh oh!

Byron commented Sep 22, 2025

Uh oh!

arumugamramaswamy commented Sep 23, 2025

Uh oh!

EliahKagan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EliahKagan commented Sep 23, 2025

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arumugamramaswamy commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Byron Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Byron Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Byron Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fix: submodule worktree incorrect with symlinked `.git` dir (#2052) #2182

fix: submodule worktree incorrect with symlinked `.git` dir (#2052) #2182

arumugamramaswamy commented Sep 21, 2025 •

edited

Loading

arumugamramaswamy commented Sep 24, 2025 •

edited

Loading

Byron left a comment •

edited

Loading