GIT_CEILING_DIRECTORIES with relative search directory

[jpgrayson]

Duplicates

• I have searched the existing issues

Current behavior 😯

Calling git_repository::ThreadSafeRepository::discover_with_environment_overrides(".") with:

• GIT_CEILING_DIRECTORIES = /absolute/path/to/repo/..
• and the current directory /absolute/path/to/repo

results in Error::NoMatchingCeilingDir.

Under the hood, the crux of the problem seems to be in find_ceiling_height(). In this scenario, we land in this match arm:

match (search_dir.is_absolute(), ceiling_dir.is_absolute()) {
    ...,
    (false, true) => {
        let stripped = ceiling_dir.as_ref().strip_prefix(cwd?).ok()?.to_owned();
        ceiling_dir = stripped.into();
    }
    ...,
};

Where:

• search_dir is "." and thus not absolute
• ceiling_dir is "/absolute/path/to" (because the "/.." was normalized away in parse_ceiling_dirs())
• cwd is Some("/absolute/path/to/repo")

It is unclear to me the purpose of ceiling_dir.as_ref().strip_prefix(cwd?) in the above. It fails in this scenario because cwd is not a prefix of ceiling_dir, but my question is why isn't search_dir given any consideration? Don't we want to know how many path components of search_dir remain after stripping ceiling_dir?

Here is a patch that works for me. I can post a PR if this seems like the right approach:

diff --git a/git-discover/src/upwards/util.rs b/git-discover/src/upwards/util.rs
index de84dfe35..416e8ac1f 100644
--- a/git-discover/src/upwards/util.rs
+++ b/git-discover/src/upwards/util.rs
@@ -38,18 +38,24 @@ pub(crate) fn shorten_path_with_cwd(cursor: PathBuf, cwd: Option<&Path>) -> Path
 /// Find the number of components parenting the `search_dir` before the first directory in `ceiling_dirs`.
 /// `search_dir` needs to be absolutized, and we absolutize every ceiling as well.
 pub(crate) fn find_ceiling_height(search_dir: &Path, ceiling_dirs: &[PathBuf], cwd: Option<&Path>) -> Option<usize> {
+    let search_realpath;
+    let search_dir = if search_dir.is_absolute() {
+        search_dir
+    } else {
+        if let Some(cwd) = cwd {
+            search_realpath = git_path::realpath_opts(search_dir, cwd, git_path::realpath::MAX_SYMLINKS).ok()?;
+        } else {
+            search_realpath = git_path::realpath(search_dir).ok()?;
+        }
+        search_realpath.as_path()
+    };
     ceiling_dirs
         .iter()
         .filter_map(|ceiling_dir| {
             let mut ceiling_dir = git_path::absolutize(ceiling_dir, cwd);
-            match (search_dir.is_absolute(), ceiling_dir.is_absolute()) {
-                (true, false) => ceiling_dir = cwd?.join(ceiling_dir.as_ref()).into(),
-                (false, true) => {
-                    let stripped = ceiling_dir.as_ref().strip_prefix(cwd?).ok()?.to_owned();
-                    ceiling_dir = stripped.into();
-                }
-                (false, false) | (true, true) => {}
-            };
+            if !ceiling_dir.is_absolute() {
+                ceiling_dir = cwd?.join(ceiling_dir.as_ref()).into();
+            }
             search_dir
                 .strip_prefix(ceiling_dir.as_ref())
                 .ok()

But wait, there's more.

With the above repair in place, there is another possible issue: Repository::git_dir() and Repository::work_dir() return relative paths when the search directory is relative (e.g. ".").

In git_discover::upwards_opts() (a.k.a. discover_opts()), the directory is "absolutized" with:

        let dir = git_path::absolutize(directory.as_ref(), cwd.as_deref());

However, despite its name, absolutize() does not convert all paths to be absolute. Specifically, "." will remain simply "." even when Some(cwd) is provided to absolutize()

The reason I identify this as possibly being a problem is because git2::Repository::path() and workdir() always provide absolute paths. So perhaps a problem insofar as gitoxide may want to avoid surprising users coming from git2-rs.

That and absolutize() might either be renamed ("normalize()"?) or actually ensure that absolute paths are returned when possible. If absolutize() ensured that an absolute path was returned, it would also obviate the above problem with GIT_CEILING_DIRECTORY in addition to ensuring that Repository::git_dir() and work_dir() always return absolute paths.

A patch for that might look something like this:

diff --git a/git-path/src/convert.rs b/git-path/src/convert.rs
index 2ddbcbbd6..86a6c7e4f 100644
--- a/git-path/src/convert.rs
+++ b/git-path/src/convert.rs
@@ -221,16 +221,25 @@ pub fn to_windows_separators<'a>(path: impl Into<Cow<'a, BStr>>) -> Cow<'a, BStr
 /// Resolve relative components virtually without accessing the file system, e.g. turn `a/./b/c/.././..` into `a`,
 /// without keeping intermediate `..` and `/a/../b/..` becomes `/`.
 /// Note that we might access the `current_dir` if we run out of path components to pop off. If unset, we continue
-/// which might lead to an invalid/uninteded path.
+/// which might lead to an invalid/unintended path.
 pub fn absolutize<'a>(path: impl Into<Cow<'a, Path>>, mut current_dir: Option<impl Into<PathBuf>>) -> Cow<'a, Path> {
-    use std::path::Component::ParentDir;
+    use std::path::Component::{CurDir, ParentDir};
 
     let path = path.into();
-    if !path.components().any(|c| matches!(c, ParentDir)) {
+    if !path.components().any(|c| matches!(c, CurDir | ParentDir)) {
         return path;
     }
+
     let mut components = path.components();
-    let mut path = PathBuf::from_iter(components.next());
+    let mut path = match components.next() {
+        component @ Some(CurDir) => if let Some(cwd) = current_dir.take() {
+            cwd.into()
+        } else {
+            PathBuf::from_iter(component)
+        },
+        component => PathBuf::from_iter(component),
+    };
+
     for component in components {
         if let ParentDir = component {
             if path.as_os_str() == "." || (!path.pop() && path.as_os_str().is_empty()) {
@@ -244,7 +253,12 @@ pub fn absolutize<'a>(path: impl Into<Cow<'a, Path>>, mut current_dir: Option<im
         }
     }
     if path.as_os_str().is_empty() {
-        PathBuf::from(".").into()
+        if let Some(cwd) = current_dir.take() {
+            let cwd = cwd.into();
+            cwd.into()
+        } else {
+            PathBuf::from(".").into()
+        }
     } else {
         path.into()
     }
diff --git a/git-path/tests/convert/absolutize.rs b/git-path/tests/convert/absolutize.rs
index 83d37f517..4e54cd5d4 100644
--- a/git-path/tests/convert/absolutize.rs
+++ b/git-path/tests/convert/absolutize.rs
@@ -14,6 +14,20 @@ fn no_change_if_there_are_no_trailing_relative_components() {
     }
 }
 
+#[test]
+fn paths_relative_to_cwd() -> crate::Result {
+    let cwd = std::env::current_dir()?;
+    assert_eq!(absolutize(p("."), Some(&cwd)), cwd);
+    assert_eq!(absolutize(p("./a"), Some(&cwd)), cwd.join("a"));
+    assert_eq!(absolutize(p("./a/.."), Some(&cwd)), cwd);
+    assert_eq!(absolutize(p("./a/./.."), Some(&cwd)), cwd);
+    assert_eq!(absolutize(p("./a/./../.."), Some(&cwd)), cwd.parent().unwrap());
+    assert_eq!(absolutize(p("./."), Some(&cwd)), cwd);
+    assert_eq!(absolutize(p("././../"), Some(&cwd)), cwd.parent().unwrap());
+    assert_eq!(absolutize(p("a/../"), Some(&cwd)), cwd);
+    Ok(())
+}
+

Expected behavior 🤔

I expect that git_repository::ThreadSafeRepository::discover_with_environment_overrides(".") would succeed when the GIT_CEILING_DIRECTORIES environment variable is set.

Or that the various repository discovery functions would all explicitly prohibit relative search paths.

Steps to reproduce 🕹

No response

[Byron]

First of all, thanks so much for this wonderfully analysed issue! I am loving these patches, too, and have a feeling that Stacked Git helps with that :).

It fails in this scenario because cwd is not a prefix of ceiling_dir, but my question is why isn't search_dir given any consideration? Don't we want to know how many path components of search_dir remain after stripping ceiling_dir?

This ceiling dir logic is definitely something I struggled with and am sure there is plenty wrong with it and it's just lacking tests to show that. With your use-case, I believe a test should be built to reproduce it, allowing a fix.
When writing the existing tests I constantly struggled with not being able to use chdir at affects all other tests, but now I know that one can get isolation by (simply) adding another integration test file, one with just a single tests which can then do with the process what it wants.

However, despite its name, absolutize() does not convert all paths to be absolute. Specifically, "." will remain simply "." even when Some(cwd) is provided to absolutize()

This sounds like a bug and I am surprised there is no test-case for that 😳.

The reason I identify this as possibly being a problem is because git2::Repository::path() and workdir() always provide absolute paths. So perhaps a problem insofar as gitoxide may want to avoid surprising users coming from git2-rs.

This is where gitoxide differs in approach as it aims to be more similar to git. This means that if ./ is provided by the user, gitoxide will work with that, which also results in much shorter paths being played back to the user. There are cases when gitoxide will provide more useful paths than git as well, and once you try gix exclude query (to query .gitignore) you will see that git yields paths that are relative to the root of the repository even though the CWD is in some subdirectory, which is confusion, whereas gitoxide naturally produces paths that match the user's CWD.

However, as a library, gitoxide must not change values that affect the entire process which is owned by application code, which is why it can't chdir like git does either. Hence the current approach which hopefully will soon be without surprises once the remaining issues are squelched.

Maybe some documentation can help to make that behaviour clearer to git2 users, and explain that these would pass canonicalized paths to get the results they might expect.

Here is a patch that works for me. I can post a PR if this seems like the right approach:

I definitely welcome a PR with the patches as you see fit. If they don't break any existing behaviour I consider them correct, and I will give those patches a closer look and work with you once the PR has landed.

On another note, I will be studying how stacked-git works as I am always looking to understand all workflows that git enables, so I am particularly excited to see you contribute to gitoxide and will do my best to make this a pleasant experience. The same goes for support if you have any questions or need anything when integrating gitoxide into stgit.

[jpgrayson]

When writing the existing tests I constantly struggled with not being able to use chdir at affects all other tests, but now I know that one can get isolation by (simply) adding another integration test file, one with just a single tests which can then do with the process what it wants.

I was also seeing the difficulty in testing this. I could use some hints regarding how to add a new integration test. Would this fit in with the stuff in the top-level tests/ directory? We would need some rust code to exercise this issue--would it also make sense to have an example in, e.g., git-discover/examples that discovers a repo in this particular way?

Alternatively, I've been looking at how feasible it would be to allow the user to provide the current directory to gitoxide. The immediate goal would be to allow unit tests to be written for this scenario, but there may be other benefits such as reducing the number of system calls. My initial audit shows the following uses of std::env::current_dir() in gitoxide (excluding any tests):

• git_discover::upwards_opts()
• git_repository::Repository::prefix()
• git_repository::Path::from_dot_git_dir()
• git_path::realpath()
• git_odb::alternate::resolve()

There is already a pattern of functions that take a cwd argument. This could be extended to include git_repository::open::Options and git_discover::upwards::Options, which I think would be sufficient to write meaningful unit tests. The risk would be that if a unit test (or a regular user) lies about the actual current directory, then any filesystem accesses using relative paths would fail. This may make this idea a non-starter. What do you think about limiting gitoxide's use of stg::env::current_dir()?

This is where gitoxide differs in approach as it aims to be more similar to git. This means that if ./ is provided by the user, gitoxide will work with that, which also results in much shorter paths being played back to the user. There are cases when gitoxide will provide more useful paths than git as well, and once you try gix exclude query (to query .gitignore) you will see that git yields paths that are relative to the root of the repository even though the CWD is in some subdirectory, which is confusion, whereas gitoxide naturally produces paths that match the user's CWD.

Fair enough. There are trade-offs to normalizing everything to absolute paths versus maintaining relative paths. And agreed that git maintains relative paths. I'll make sure that any changes I work on in this space take care to preserve relative paths.

I definitely welcome a PR with the patches as you see fit. If they don't break any existing behaviour I consider them correct, and I will give those patches a closer look and work with you once the PR has landed.

I've posted #609 as a first step. I got hung up for a while because make tests was failing in my environment, first due to not having git-lfs setup, and second due to having tag.gpgSign configured in my global config.

Now I'm trying to figure out why cd git-worktree; cargo test --features=internal-testing-to-avoid-being-run-by-cargo-test-all fails in my environment.

I feel pretty confident in the patch I posted above for solving this problem, but I need to get the baseline tests working and then either add integration or unit tests to cover this case. Your guidance on the testing approach is welcome.

On another note, I will be studying how stacked-git works as I am always looking to understand all workflows that git enables, so I am particularly excited to see you contribute to gitoxide and will do my best to make this a pleasant experience. The same goes for support if you have any questions or need anything when integrating gitoxide into stgit.

Glad to have you take notice of StGit. I just released StGit 2.0, which was a complete rewrite in Run (from Python). gitoxide was not sufficiently capable when I started this rewrite over a year ago, so I used libgit2. I tried to only use libgit2 and avoid executing subordinate git processes, but libgit2 behaves differently than git on patch application, merge, and a number of other subtle cases. So StGit uses libgit2 primarily as the interface to the repository's object database (object, branch, and reference lookup) and configuration while still relying on git for most operations that mutate the repository.

At this point, it seems like gitoxide may be sufficiently capable to replace the use of libgit2 in StGit. So I'm starting to experiment with integrating gitoxide into StGit. I like that gitoxide is fast. I like that gitoxide is config-aware. And I like that it is written in Rust.

[Byron]

I was also seeing the difficulty in testing this. I could use some hints regarding how to add a new integration test. Would this fit in with the stuff in the top-level tests/ directory? We would need some rust code to exercise this issue--would it also make sense to have an example in, e.g., git-discover/examples that discovers a repo in this particular way?

Indeed, I think it's enough to drop a new file into ./tests and put on #[test] inside, which can take charge of the whole process. There would nothing else needed to test this I believe and am happy to add an example of this to the PR when it lands.

Alternatively, I've been looking at how feasible it would be to allow the user to provide the current directory to gitoxide.
What do you think about limiting gitoxide's use of stg::env::current_dir()?

I think you have discovered another inconsistency and I took the liberty to fix it immediately to the point where std::env::current_dir() is only called from git-repository which is considered porcelain. This should help with testing as well and as a matter of fact I added a few more tests to absolutize() in the process, allowing it to inform about failures as well.

I've posted #609 as a first step. I got hung up for a while because make tests was failing in my environment, first due to not having git-lfs setup, and second due to having tag.gpgSign configured in my global config.

git-lfs should not be able to do that nor is it required to run tests. If it somehow is an issue, it's a bug and I'd love to know more specifics to help with a fix. I could verify that in a fresh clone without any other setup make tests would run successfully. For context, git-lfs is used to allow CI download cached fixtures to avoid storing binary files in git and to avoid having CI execute potentially slow scripts, but it's strictly optional to have these and scripts will be executed if no archive is available.

Now I'm trying to figure out why cd git-worktree; cargo test --features=internal-testing-to-avoid-being-run-by-cargo-test-all fails in my environment.

Strange, I hope another patch for a fix is possible here as well. I imagine that not all scripts are perfectly portable though, maybe it's that as well.

I feel pretty confident in the patch I posted above for solving this problem, but I need to get the baseline tests working and then either add integration or unit tests to cover this case. Your guidance on the testing approach is welcome.

I think it would be best to do that in a PR, it would allow me to experiment as well.

At this point, it seems like gitoxide may be sufficiently capable to replace the use of libgit2 in StGit. So I'm starting to experiment with integrating gitoxide into StGit. I like that gitoxide is fast. I like that gitoxide is config-aware. And I like that it is written in Rust.

From what you mentioned, I believe it is even though I didn't analyse it myself yet. If you find anything lacking we should crate a tracking ticket here and I will keep you posted on the progress towards delivering the required features.

PS: I have been using stg for #611 and it was super useful to go back and forth between the commits and refresh them as I discovered more shortcomings. It was my first time and quite transformative, thinking that stg will be part of my workflow from now on. git rebase I rarely manage to use correctly and stg works out of the box for me, and is fun as well! Next time I will try not to stg commit --all before pushing and just do that once everything works as it should, because this time I ended up uncommitting and comitting a lot as I went back to make edits. And yes, I am curious what else I will find knowing that I barely scratched the surface.

[jpgrayson]

I've submitted #616 to address this issue.

Indeed, I think it's enough to drop a new file into ./tests and put on #[test] inside, which can take charge of the whole process. There would nothing else needed to test this I believe and am happy to add an example of this to the PR when it lands.

I ended up adding a test in git-discover's unit tests instead of in the top-level tests. As I note in the commit message, this approach is fragile. I would like to take you up on your offer to help get the tests straightened out.

I think you have discovered another inconsistency and I took the liberty to fix it immediately to the point where std::env::current_dir() is only called from git-repository which is considered porcelain.

Great! Glad to have sparked this idea.

This should help with testing as well and as a matter of fact I added a few more tests to absolutize() in the process, allowing it to inform about failures as well.

As I've given absolutize() more consideration, I'm less confident that its approach to normalize the path without consulting the filesystem is valid. Of particular interest is this statement from the std::Path::components() documentation:

Note that no other normalization takes place; in particular, a/c and a/b/../c are distinct, to account for the possibility that b is a symbolic link (so its parent isn’t a).

So Path::components() specifically doesn't do what absolutize() does because the presence of symlinked directories could break a static approach to resolving .. components. It seems like perhaps the main goal of absolutize() is to avoid the expense of using Path::canonicalize(), but there may be a correctness trade-off at play. The next step for this would be to craft a test cases with symlinked directories that might trip-up absolutize().

git-lfs should not be able to do that nor is it required to run tests

The first failure signature I ran into involved failure to decompress tar.xz files. I eventually figured out this was because the files were the ascii text stubs used by git-lfs. After setting up git-lfs, that particular failure signature morphed and I was able to debug the next issue, which was with tag.gpgsign (#609). And then the GIT_EDITOR issue (#615). At this point, with git-lfs uninstalled from the repo, I am able to run make tests successfully. BUT I end up with oodles of modified tar.xz files in my working tree unless I use GITOXIDE_TEST_IGNORE_ARCHIVES=1, but that seems contrary to the instructions found in DEVELOPMENT.md for running on Linux.

From what you mentioned, I believe it is even though I didn't analyse it myself yet. If you find anything lacking we should crate a tracking ticket here and I will keep you posted on the progress towards delivering the required features.

That sounds great. Thanks for taking an interest in supporting StGit.

I have been using stg [...]

And that's especially great. Thank you for taking the time to explore StGit and give it a try. It's really gratifying for me that you've found it to be useful. I am happy to help with any StGit related trouble you might run into--just let me know.

[Byron]

I ended up adding a test in git-discover's unit tests instead of in the top-level tests. As I note in the commit message, this approach is fragile. I would like to take you up on your offer to help get the tests straightened out.

It's done and I was happily playing with different configurations, and if you have any other ideas for testing, it should be straightforward now to add them accordingly.

I've given absolutize() more consideration, I'm less confident that its approach to normalize the path without consulting the filesystem is valid.

That's a fair concern and I am taking it seriously. To me the only question is if the resulting path is valid if the source path was valid, and my answer to that is positive. absolutize() exists for that reason alone as it wants to keep paths similar to what the user provided while allowing certain path algorithms to work. These algorithms assume that both input paths are of the same 'kind' so they are actually comparable. I absolute agree that the perfectly safe way to deal with this would be to properly normalize the paths so comparisons are guaranteed to be 'apples to apples', and maybe that's a future direction for git repository discovery as well. The problem to solve then is how to re-shape the path so that it is similar to what the user provided.
Also for context, path handling didn't start like this, but used canonicalization (or similar) initially. When trying gix exclude query though the produced paths were annoyingly long and very inconvenient. The current state produces exactly the right paths and I hope it stays that way :).

The first failure signature I ran into involved failure to decompress tar.xz files.

When I previously tested I didn't consider that git would probably call the git-lfs filter when checking out. Trying again, it won't even clone anymore, a situation I am not particularly happy with (even though it's probably the safest thing to do). It seemed to be outright impossible to get past that without altering .gitattributes.

After manipulating the checkout into shape to run cargo test -r git-discover, it worked without issues related to the archives not being valid files. Despite the issues with git-lfs, at list this works like it should as invalid archives or those that our outdated will simply be ignored and replaced with some that work.

At this point, with git-lfs uninstalled from the repo[…]

That's interesting, it shouldn't be needed to install git-lfs in the repo via git lfs install for a checkout, maybe it's needed for commits to work though by merrit of it registering some hooks that I thought shouldn't be needed except for pre-push maybe to upload git-lfs objects.

BUT I end up with oodles of modified tar.xz files in my working tree unless I use GITOXIDE_TEST_IGNORE_ARCHIVES=1, but that seems contrary to the instructions found in DEVELOPMENT.md for running on Linux.

After all that filter-madness I am not surprised that this happens. Even assuming filters worked fine these archives would probably look different on your platform than on mine so an apparent change can be expected. However, it's not nice for contributors to have to be aware of that. I am a bit unhappy that this issue evaded me for so long, and that I don't really have a solution for it either.

Maybe git-testtools could detect if the archive that it just failed to load is an unexpanded git-lfs file and avoid overwriting those with archives, because if git-lfs would have been present the archives would be there as well so it wouldn't want to overwrite them in the first place. And since that's implemented now I hope the situation will be improved next time that happens.

And that's especially great. Thank you for taking the time to explore StGit and give it a try.

In the past hours I have graduated to being a fan as well despite still barely touching the surface. The key improvement stg grants me is a better organization of commits which now tend to represent an area of work in each branch. Going back and forth between patches is extra effort, and overall there is more 'meta' to development now, but it's a price worth paying for more ogranized commits and history that overall is more discoverable and useful. Whenever I am interacting with the git source repository I am in awe of their awesome commit messages, and bet their workflow has something to do with it. Now I feel to get one step closer to that.

I am happy to help with any StGit related trouble you might run into--just let me know.

I'd love to start a conversation and ask questions, is the forum the right place for that?

---

With all this said and done, I believe it's time to close this issue as the fix is available in main. Please let me know once anything else comes up without hesitation.

[jpgrayson]

I am happy to help with any StGit related trouble you might run into--just let me know.
I'd love to start a conversation and ask questions, is the forum the right place for that?

I'd prefer using discussions for public conversations and filing issues for any unclear/unexpected behaviors.

With all this said and done, I believe it's time to close this issue as the fix is available in main. Please let me know once anything else comes up without hesitation.

Thanks for working with me on this issue. Glad to be able to help with gitoxide. And kudos for taking on such an ambitious project!

[Byron]

I'd prefer using discussions for public conversations and filing issues for any unclear/unexpected behaviors.

Thanks for letting me know, I posted a new discussion with my most pressing question.

Thanks for working with me on this issue. Glad to be able to help with gitoxide. And kudos for taking on such an ambitious project!

You are very welcome! And after having strolled through the stg docs and discussions a little it feels like stg is monumental as well, I mean, look at this gorgeous 2.0 changelog ❤️. The whole project instills the feeling of immense quality and makes me feel save while letting it meddle with my worktree and git repository in ways I can't even imagine! Great project, and I hope one day it can be fully powered by gitoxide to get rid of the last remaining latencies.