Re: [PATCH v3 2/5] setup: extract path_allowlist_apply()

From: Christian Couder <christian.couder@gmail.com>
Date: Sep 28, 2026, 13:40 UTC
Message-ID: <CAP8UFD0da+K3FLVAgmds8CUr3aFrLjsmG7qO3mYX4foNLMYrMg@mail.gmail.com>
In-Reply-To: <xmqq33vjy6qz.fsf@gitster.g>

On Tue, Sep 8, 2026 at 7:48 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 7 quoted lines

>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > For clarity, let's change the `int is_safe` to `bool safe` in
> > `struct safe_directory_data`.
>
> I am not sure if this clarifies, though.

The change is now explained in the following way:

    +    As the new path_allowlist_apply() function reports its result through
    +    a `bool *matches` argument, let's also change the `int is_safe` member
    +    of `struct safe_directory_data` to a `bool`, so that its address can
    +    be passed as that argument.

and only the type of the variable is changed in v4. The "is_safe" original name is kept.

Show 43 quoted lines

> > diff --git a/setup.c b/setup.c
> > index dfe05d9a03..366a7dc5c0 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -1338,67 +1338,105 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,
> >       }
> >  }
> >
> > +void path_allowlist_apply(const char *allowed, const char *target_path,
> > +                       bool *matches,
> > +                       bool (*allow_path)(const char *path, void *cbdata),
> > +                       void *allow_path_cbdata)
> > +{
> > +     char *normalized = NULL;
> > +
> > +     if (!allowed || !*allowed) {
> > +             *matches = false;
> > +             return;
> > +     }
> > +
> > +     if (!strcmp(allowed, "*")) {
> > +             *matches = true;
> > +             return;
> > +     }
> > +
> > +     if (!allow_path(allowed, allow_path_cbdata))
> > +             return;
> > +
> > +     /*
> > +      * A .gitconfig in $HOME may be shared across different
> > +      * machines and the config variable entries may or may not
> > +      * exist as paths on all of these machines.  In other words,
> > +      * it is not a warning worthy event when there is no such path
> > +      * on this machine---the entry may be useful elsewhere.
> > +      */
>
> This is inherited from the preimage and not something you would want
> to fix in this patch, but I do not think ignoring missing path like
> this is healthy.  You do not know if the path given is missing by
> design (i.e., the set of paths is union of paths that could exist)
> or if it is missing due to an error (i.e., a filesystem that should
> have been mounted is not mounted).  In the latter case, ignoring it
> may make the system behave in a way that the user did not intend to.

In dc0edbb01c (safe.directory: normalize the configured path, 2024-07-30) you say:

     - A configured safe.directory may be coming from .gitignore in the
       home directory that may be shared across machines.  The path
       meant to match with an entry may not necessarily exist on all of
       such machines, so not being able to convert them to real path on
       this machine is *not* a condition that is worthy of warning.
       Hence, we ignore a path that cannot be converted to a real path.

So I don't know what is the right thing to do. Maybe it's safer to warn by default but have a config option to not warn? Or maybe we should remember the unresolvable entries, and mention them only when the overall check fails?

Anyway I can add a NEEDSWORK here for now in a separate patch in this series or maybe in a followup series. In v4 nothing was changed regarding this.