Do not normalize safe.directory paths #6668

csware · 2023-12-11T09:20:08Z

Vanilla Git does not do it either (cf. https://github.com/gitster/git/blob/master/setup.c#L1150) and calling git_fs_path_prettify_dir can cause performance issues (cf. issue #6649).

Still needed: expansion of "~/"... (but not this PR)

ethomson · 2023-12-13T12:15:34Z

src/libgit2/repository.c

-
-		if (git_fs_path_prettify_dir(&data->tmp, test_path, NULL) == 0 &&
-		    strcmp(data->tmp.ptr, data->repo_path) == 0)
+		git_str_sets(&tmp, data->repo_path);


We were passing the git_str here to avoid re-allocing a git_str on every call. We can just re-use the one we have. No need to undo that, I don't think.

Also, you're looking at data->repo_path, not test_path, which we've mutated to handle WSL paths. We should keep that.

ethomson · 2023-12-13T12:15:57Z

src/libgit2/repository.c

@@ -582,10 +582,14 @@ static int validate_ownership_cb(const git_config_entry *entry, void *payload)
 		         strncmp(test_path, "//wsl.localhost/", strlen("//wsl.localhost/")) != 0)
 			test_path++;
 #endif
-
-		if (git_fs_path_prettify_dir(&data->tmp, test_path, NULL) == 0 &&


I think that instead of prettify, we need only call path_to_dir.

I don't think that using path_to_dir is the right way. This would accept safe.directory entries ending with a slash which vanilla Git does not accept.

Is that actually a problem, though? We also accept //wsl.localhost/... prefixed paths which Git for Windows doesn't accept.

From my point of view, I would keep it as consistent as possible.

Let's do one fix at the time. It's ok for me if we take your suggested patch.

From my point of view, I would keep it as consistent as possible.

OK. Let's do that. This feels like a (minor) bug in git that it doesn't allow trailing slashes, but I've often said that libgit2's goal is to be "bug for bug compatible".

@ethomson
I updated the branch and hope that I did not forget anything.

ethomson · 2023-12-13T12:19:57Z

Agree that we don't need to hit the filesystem on every excluded path. I think that the simplest fix is:

diff --git a/src/libgit2/repository.c b/src/libgit2/repository.c
index 05ece6efc..9fa616fb0 100644
--- a/src/libgit2/repository.c
+++ b/src/libgit2/repository.c
@@ -583,8 +583,11 @@ static int validate_ownership_cb(const git_config_entry *entry, void *payload)
                        test_path++;
 #endif

-               if (git_fs_path_prettify_dir(&data->tmp, test_path, NULL) == 0 &&
-                   strcmp(data->tmp.ptr, data->repo_path) == 0)
+               if (git_str_sets(&data->tmp, test_path) < 0 ||
+                   git_fs_path_to_dir(&data->tmp) < 0)
+                       return -1;
+
+               if (strcmp(data->tmp.ptr, data->repo_path) == 0)
                        *data->is_safe = true;
        }

This keeps the git_str that is used between invocations of the callback to avoid re-allocs, but switches from prettify_path to path_to_dir, which should avoid touching the filesystem and merely append a / since that's our (sort of dubious) pattern for directories.

This does not add the trailing / check on inputs. I don't know how valuable that is - I think that specifying a folder with a trailing slash is perfectly legitimate and that git should allow it. But if you're bullish on exact compatibility here, I think that's a reasonable opinion as well.

ethomson · 2023-12-13T12:23:10Z

Thanks @csware for digging in here to understand the perf problem - it's slow for me to develop / test on Windows - and apologies for causing it in the first place.

csware · 2023-12-13T14:56:26Z

@ethomson I'd like to talk to you about libgit2, some open PRs and TortoiseGit. Would be nice if we could meet next year.

Vanilla Git does not do it either (cf. https://github.com/gitster/git/blob/master/setup.c#L1150) and calling git_fs_path_prettify_dir can cause performance issues (cf. issue libgit2#6649). Signed-off-by: Sven Strickroth <email@cs-ware.de>

Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware · 2023-12-18T14:23:12Z

@ethomson
Is there an open point here?

ethomson · 2024-01-14T01:08:16Z

Manually merged with a tweak - thanks again!

csware · 2024-01-14T13:51:28Z

This is also relevant for 1.7, right?!

csware · 2024-01-14T22:13:10Z

@ethomson
Is there a reason why you ,merged both commits and then reverted the second one which improves the error message? Particularly, the hint regarding %(prefix)/"is missing now.

I don't quite understand your tweak either. Now more copies are needed, right? Particularly, I don't understand the comment regarding the backslash.

csware · 2024-02-14T21:56:14Z

src/libgit2/repository.c

@@ -582,9 +581,7 @@ static int validate_ownership_cb(const git_config_entry *entry, void *payload)
 		         strncmp(test_path, "//wsl.localhost/", strlen("//wsl.localhost/")) != 0)


Remove this else if part if %(prefix)/ is not required.

csware force-pushed the safe.directory branch from 8c5ed29 to 9a2df4d Compare December 11, 2023 11:45

ethomson reviewed Dec 13, 2023

View reviewed changes

csware force-pushed the safe.directory branch from 9a2df4d to 9586445 Compare December 13, 2023 14:55

csware force-pushed the safe.directory branch from 9586445 to 4e5a980 Compare December 13, 2023 18:53

csware added 2 commits December 13, 2023 20:16

Do not normalize safe.directory paths

779263b

Vanilla Git does not do it either (cf. https://github.com/gitster/git/blob/master/setup.c#L1150) and calling git_fs_path_prettify_dir can cause performance issues (cf. issue libgit2#6649). Signed-off-by: Sven Strickroth <email@cs-ware.de>

Improve error message

73f034c

Signed-off-by: Sven Strickroth <email@cs-ware.de>

csware force-pushed the safe.directory branch from 4e5a980 to 73f034c Compare December 13, 2023 19:16

ethomson merged commit 73f034c into libgit2:main Jan 14, 2024

csware deleted the safe.directory branch January 14, 2024 22:15

csware mentioned this pull request Feb 14, 2024

Repository ownership validation does not skip %(prefix)/ on Windows #6737

Closed

csware commented Feb 14, 2024

View reviewed changes

ethomson added bug git compatibility labels Feb 18, 2024

branchvincent mentioned this pull request Mar 21, 2024

libgit2 1.8.1 libgit2@1.7 1.7.2 (new formula) Homebrew/homebrew-core#166767

Merged

		@@ -582,9 +581,7 @@ static int validate_ownership_cb(const git_config_entry entry, void payload)
		strncmp(test_path, "//wsl.localhost/", strlen("//wsl.localhost/")) != 0)

Do not normalize safe.directory paths #6668

Do not normalize safe.directory paths #6668

Uh oh!

Conversation

csware commented Dec 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Dec 13, 2023

Uh oh!

ethomson commented Dec 13, 2023

Uh oh!

csware commented Dec 13, 2023

Uh oh!

csware commented Dec 18, 2023

Uh oh!

ethomson commented Jan 14, 2024

Uh oh!

csware commented Jan 14, 2024

Uh oh!

csware commented Jan 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csware commented Dec 11, 2023 •

edited

Loading

csware commented Jan 14, 2024 •

edited

Loading