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

[Jeff King <peff@peff.net>]

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

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

> 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