On Tue, Jul 30, 2024 at 09:03:35AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> >> + if (!is_absolute_path(check) && strcmp(check, ".")) {
> >> + warning(_("safe.directory '%s' not absolute"),
> >> + check);
> >> + goto next;
> >> + }
> >
> > This is_absolute_path() check is redundant, isn't it? If we are checking
> > for a literal ".", then we know the path must be non-absolute.
>
> What I meant was "If it is not absolute, that is an error, but if
> the thing is a dot, that is allowed as an exception".
>
> Is the lack of "!" confusing, I wonder? We could rewrite it to
> make it more explicit:
Oh, right, I totally misread it. You'd think after 25+ years of writing
C that I would be able to get the strcmp() return value right in my
head... :)
So yeah, it is doing the right thing.
> if (is_absolute_path(check) || !strcmp(check, ".")) {
> ; /* OK */
> } else {
> warning(_("not absolute %s"), check);
> goto next;
> }
Hmm. Yeah, that probably would have softened my confusion, but it's also
kind of hard to read. I think what you wrote originally is just fine,
and I just mis-read it.
> My earlier draft for v3 had the check for dot a lot earlier in the
> function, i.e.
>
> - } else if (!strcmp(value, "*")) {
> + } else if (!strcmp(value, "*") || !strcmp(value, ".")) {
> data->is_safe = 1;
>
> and this part said "If not absolute, that is an error" without
> anything about dot.
>
> But then I changed my mind and made it unsafe to do this:
>
> cd .git/refs && git -c safe.directory=. foo
>
> as safe.directory=. means "A repository at the current directory of
> the process is allowed" and the repository in this case is not at "."
> but at "..", meaning "." is a lot stricter than "*".
I could see arguments in either direction, and I don't have a strong
opinion between them.
-Peff
