fileops: fix creation of directory in filesystem root #5131
Conversation
|
Funny. CI now fails on Linux because we now started correctly creating repos in a "/non/existing/path", where the first path component doesn't exist either. This is obviously the correct thing to do, because |
pks-t
force-pushed
the
pks/fileops-mkdir-in-root
branch
from
8dd032a
to
06dea64
Compare
|
@ethomson: are we executing tests as root? Otherwise we wouldn't be able to create the "/usr/non/" directory... |
We are. |
|
On Mon, Jun 24, 2019 at 09:40:59AM -0700, Edward Thomson wrote:
> @ethomson: are we executing tests as root? Otherwise we wouldn't be able to create the "/usr/non/" directory...
We are.
Saw that already. I'll put up a PR in a few minutes to fix that.
|
|
I'm curious what the status of this is. Is there anything I can do to help? |
pks-t
force-pushed
the
pks/fileops-mkdir-in-root
branch
from
06dea64
to
d1e06fe
Compare
Probably not too much, but in fact you've just caused me to rethink my approach to how to fix the tests for now. So thanks for asking :) This was previously blocked on the fact that we were testing in Docker images as root user. But as my changes in libgit2/docker-build#1 weren't yet merged, I've decided to go an alternative route. Thinking about it now the previous test was arguably wrong anyway, so I think we're better off with the new tests now. The only thing left now is a review to get this merged. It doesn't rely on the Docker PR anymore, even though I still think that we should avoid building and testing as root user. |
pks-t
force-pushed
the
pks/fileops-mkdir-in-root
branch
3 times, most recently
from
029c0f1
to
24e9503
Compare
In commit 45f24e7 (git_repository_init: stop traversing at windows root, 2019-04-12), we have fixed `git_futils_mkdir` to correctly handle the case where we create a directory in Windows-style filesystem roots like "C:\repo". The problem here is an off-by-one: previously, to that commit, we've been checking wether the parent directory's length is equal to the root directory's length incremented by one. When we call the function with "/example", then the parent directory's length ("/") is 1, but the root directory offset is 0 as the path is directly rooted without a drive prefix. This resulted in `1 == 0 + 1`, which was true. With the change, we've stopped incrementing the root directory length, and thus now compare `1 <= 0`, which is false. The previous way of doing it was kind of finicky any non-obvious, which is also why the error was introduced. So instead of just re-adding the increment, let's explicitly add a condition that aborts finding the parent if the current parent path is "/". Making this change causes Azure Pipelines to fail the testcase repo::init::nonexistent_paths on Unix-based systems. This is because we have just fixed creating directories in the filesystem root, which previously didn't work. As Docker-based tests are running as root user, we are thus able to create the non-existing path and will now succeed to create the repository that was expected to actually fail. Let's split this up into three different tests: - A test to verify that we do not create repos in a non-existing parent directoy if the flag `GIT_REPOSITORY_INIT_MKPATH` is not set. - A test to verify that we fail if the root directory does not exist. As there is a common root directory on Unix-based systems that always exist, we can only test for this on Windows-based systems. - A test to verify that we fail if trying to create a repository in an unwriteable parent directory. We can only test this if not running tests as root user, as CAP_DAC_OVERRIDE will cause us to ignore permissions when creating files.
pks-t
force-pushed
the
pks/fileops-mkdir-in-root
branch
from
24e9503
to
5ae22a6
Compare
|
As always, thanks a lot for your review @tiennou! |
In commit 45f24e7 (git_repository_init: stop traversing at
windows root, 2019-04-12), we have fixed
git_futils_mkdirtocorrectly handle the case where we create a directory in
Windows-style filesystem roots like "C:\repo".
The problem here is an off-by-one: previously, to that commit,
we've been checking wether the parent directory's length is equal
to the root directory's length incremented by one. When we call
the function with "/example", then the parent directory's length
("/") is 1, but the root directory offset is 0 as the path is
directly rooted without a drive prefix. This resulted in
1 == 0 + 1, which was true. With the change, we've stopped incrementingthe root directory length, and thus now compare
1 <= 0, whichis false.
The previous way of doing it was kind of finicky any non-obvious,
which is also why the error was introduced. So instead of just
re-adding the increment, let's explicitly add a condition that
aborts finding the parent if the current parent path is "/".
Fixes #5130