Re: [PATCH v3 2/3] safe.directory: normalize the configured path

Jeff King [off-list ref] writes:

On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:

quoted

@@ -1236,14 +1236,43 @@ static int safe_directory_cb(const char *key, const char *value,
 
 		if (!git_config_pathname(&allowed, key, value)) {
 			const char *check = allowed ? allowed : value;
-			if (ends_with(check, "/*")) {
-				size_t len = strlen(check);
-				if (!fspathncmp(check, data->path, len - 1))

BTW, one oddity I noticed in the existing code:

Under what circumstances will "allowed" be NULL in that ternary? I think
if git_config_pathname() returns non-zero, then we called
interpolate_path(). It can return NULL, but in that case
git_config_pathname() will die(). We might change that later, but then
I'd expect it to return non-zero. So I suspect the whole "check"
variable could just be dropped in favor of using "allowed".

Obviously not new in your patch, but maybe worth fixing while in the
area? I think it comes from an evil merge in b8bdb2f283 (Merge branch
'jc/safe-directory-leading-path', 2024-06-12).

I think it deserves to be a separate change, probably a preliminary
clean-up, as it predates that by a few years, and goes back to the
initial introduction of the safe.directory feature.  The merge you
found had this bit:

diff --cc setup.c
index e47946d0e7,4c5de0960b..e112545f71

--- a/setup.c
+++ b/setup.c

@@@ -1230,13 -1176,21 +1230,20 @@@ static int safe_directory_cb(const cha
  	} else if (!strcmp(value, "*")) {
  		data->is_safe = 1;
  	} else {
- 		char *interpolated = NULL;
- 
- 		if (!git_config_pathname(&interpolated, key, value) &&
- 		    !fspathcmp(data->path, interpolated ? interpolated : value))
- 			data->is_safe = 1;
- 
- 		free(interpolated);
 ...


Notice that in the original, the code is prepared for the case where
interpolated is NULL when fspathcmp() needs to use it or value,
which is when git_config_pathname() returned 0/success.

It came from 8959555c (setup_git_directory(): add an owner check for
the top-level directory, 2022-03-02) that introduced the
safe.directory feature:

+		if (!git_config_pathname(&interpolated, key, value) &&
+		    !fspathcmp(data->path, interpolated ? interpolated : value))
+			data->is_safe = 1;

where it shared the same assumption.

On Tue, Jul 30, 2024 at 09:22:09AM -0700, Junio C Hamano wrote:

quoted

Obviously not new in your patch, but maybe worth fixing while in the
area? I think it comes from an evil merge in b8bdb2f283 (Merge branch
'jc/safe-directory-leading-path', 2024-06-12).

I think it deserves to be a separate change, probably a preliminary
clean-up, as it predates that by a few years, and goes back to the
initial introduction of the safe.directory feature.  The merge you
found had this bit:

Ah, yeah, I was busy looking at the assignments and didn't notice the
ternary in the function call. So yeah, it comes from that earlier
commit, but I think it was equally useless there.

And yes, fixing it definitely should be a separate commit.

-Peff