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, 16:50 UTC
Message-ID: <xmqqk11z4ksl.fsf@gitster.c.googlers.com>
In-Reply-To: <20200428033438.GA2369457@coredump.intra.peff.net>

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.

Show 9 quoted lines

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