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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, dstolee@microsoft.com, mhagger@alum.mit.edu
Subject: Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only
Date: Tue, 28 Apr 2020 16:59:13 -0400	[thread overview]
Message-ID: <20200428205913.GC4000@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqk11z4ksl.fsf@gitster.c.googlers.com>

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