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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 28, 2020, 21:05 UTC
Message-ID: <xmqqd07r1fu6.fsf@gitster.c.googlers.com>
In-Reply-To: <20200428205913.GC4000@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

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

I checked "git grep umask t/" hits, and I thought everybody was using it inside POSIXPERM.

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

That is sloppy, but it might be deliberate that it does not check the result?

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

Yup.

Show 8 quoted lines

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

I think the most pragmatic would be to just squash in what is already there ;-)