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

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
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 09:50:02 -0700	[thread overview]
Message-ID: <xmqqk11z4ksl.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200428033438.GA2369457@coredump.intra.peff.net> (Jeff King's message of "Mon, 27 Apr 2020 23:34:38 -0400")

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.

> Or going in the opposite direction: if we think that covering the rest
> of the test suite with a diversity of umasks isn't worthwhile, we could
> just set "umask" in test-lib.sh. That would solve this problem and any
> future ones.

Seen from the point of view of giving us a stable testing
environment, it certainly feels like an easy and simple thing to do.
I do not offhand see any downsides in that approach.

On the other hand, we use and rely on test-specified umask only in a
few tests (t0001, t1301 and t1304).  Perhaps we should discourage
tests outside these to rely too heavily on exact perm bits?

For example, I wonder if we should have used

	test -r commit-graph && ! test -w commit-graph 

to ensure the file is read-only to the user who is testing, instead
of relying on parsing "ls -l" output, which IIRC has bitten us with
xattr and forced us to revise test_modebits helper in 5610e3b0 (Fix
testcase failure when extended attributes are in use, 2008-10-19).
That would make the test require SANITY instead, though.

> I also wondered if it would be simpler to just limit the scope of the
> test, like so:
> ...
> +	# check only user mode bits, as we do not want to rely on
> +	# test environment umask
> +	grep ^-r-- actual
>  '
> ...
> but maybe there's some value in checking the whole thing.

Yeah, I guess we are wondering about the same thing.

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

Automatically figuring out POSIXPERM the above approach shoots for
is a much larger change, so I am not in a haste to implement it and
it may be OK to only do "umask 022" after we set POSIXPERM for
everybody but MINGW at least as the initial cut, but that would mean
we would run for quite a long time with the testing user's umask
during the setup process, which is unsatisfying.