Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

From: Jeff King <peff@peff.net>
Date: Apr 28, 2020, 20:59 UTC
Message-ID: <20200428205913.GC4000@coredump.intra.peff.net>
In-Reply-To: <xmqqk11z4ksl.fsf@gitster.c.googlers.com>

On Tue, Apr 28, 2020 at 09:50:02AM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > If we're just doing this for a single test, perhaps it would be better
> > to set the umask in that test (perhaps even in a subshell to avoid
> > touching other tests). I guess that's a little awkward here because the
> > write and the mode-check happen in separate snippets.
> 
> Yes, and we cannot afford to place the writing side under POSIXPERM
> prerequisite.

Do we need POSIXPERM just to call umask?

We call it unconditionally in t1304, for example. I could certainly believe it doesn't do anything useful or predictable on other systems, but it would not surprise me if it is a silent noop. It might return non-zero, though (the call in t1304 is not inside a test snippet).

Show 12 quoted lines

> Among various approaches on plate, my preference is to use "umask
> 022" around the place where we prepare the $TRASH_DIRECTORY and do
> so only when POSIXPERM is there in the test-lib.sh.  I do not know
> if we should do so before or after creating the $TRASH_DIRECTORY;
> my gut feeling is that in the ideal world, we should be able to
> 
>  - create trash directory
> 
>  - use the directory to automatically figure out POSIXPERM
> 
>  - if POSIXPERM is set, use umask 022 and chmod og=rx the trash
>    directory

I don't think we do any actual filesystem tests for POSIXPERM. It's purely based on "uname -s", and we could check it much earlier. So unless actually probing the filesystem is worth doing, we could just punt on that part easily.

That said, I think this does get complicated when interacting with t1304, for example, which explicitly creates an 077 umask for the trash directory.

This is looking like a much deeper rabbit hole than it's worth going down. I think the pragmatic thing is to just stick a "umask 022" near the new test (or possibly "test_might_fail umask 022" inside the commit-graph writing test).

-Peff