gitoxide icon indicating copy to clipboard operation
gitoxide copied to clipboard

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

Open arumugamramaswamy opened this issue 3 months ago • 6 comments

fixes https://github.com/GitoxideLabs/gitoxide/issues/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

arumugamramaswamy avatar Sep 21 '25 15:09 arumugamramaswamy

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.

Byron avatar Sep 22 '25 04:09 Byron

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

arumugamramaswamy avatar Sep 22 '25 08:09 arumugamramaswamy

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.

Byron avatar Sep 22 '25 17:09 Byron

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)

arumugamramaswamy avatar Sep 23 '25 16:09 arumugamramaswamy

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 https://github.com/GitoxideLabs/gitoxide/pull/2182#issuecomment-3324790414, 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) e1d9ae3c47917607634941a12ba4eff3d1b0ef6d, 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

EliahKagan avatar Sep 23 '25 19:09 EliahKagan

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

Edit: sorry, let me get CI to pass 😅

arumugamramaswamy avatar Sep 24 '25 14:09 arumugamramaswamy