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

From: Jeff King <peff@peff.net>
Date: Jul 30, 2024, 20:08 UTC
Message-ID: <20240730200838.GA567852@coredump.intra.peff.net>
In-Reply-To: <xmqq4j86g948.fsf@gitster.g>

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

Show 16 quoted lines

> Jeff King <peff@peff.net> 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.

Show 6 quoted lines

> 	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.

Show 17 quoted lines

> 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