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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 30, 2024, 16:22 UTC
Message-ID: <xmqqv80metou.fsf@gitster.g>
In-Reply-To: <20240730074307.GB562212@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 22 quoted lines

> On Mon, Jul 29, 2024 at 06:10:03PM -0700, Junio C Hamano wrote:
>
>> @@ -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.