Jeff King <peff@xxxxxxxx> writes:
> 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.
