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

From: Jeff King <peff@peff.net>
To: Taylor Blau <me@ttaylorr.com>
Cc: Junio C Hamano <gitster@pobox.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: Mon, 27 Apr 2020 23:34:38 -0400	[thread overview]
Message-ID: <20200428033438.GA2369457@coredump.intra.peff.net> (raw)
In-Reply-To: <20200427235935.GA14984@syl.local>

On Mon, Apr 27, 2020 at 05:59:35PM -0600, Taylor Blau wrote:

> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > index fb0aae61c3..901eb3ecfb 100755
> > --- a/t/t5318-commit-graph.sh
> > +++ b/t/t5318-commit-graph.sh
> > @@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '
> >  	test_oid_init
> >  '
> >
> > +test_expect_success POSIXPERM 'tweak umask for modebit tests' '
> > +	umask 022
> > +'
> > +
> >  test_expect_success 'verify graph with no graph file' '
> >  	cd "$TRASH_DIRECTORY/full" &&
> >  	git commit-graph verify
> 
> Looks good to me; this is definitely necessary. For what it's worth, it
> passes for me, but my system may not have as restrictive a umask as
> others.

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.

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.

I also wondered if it would be simpler to just limit the scope of the
test, like so:

diff --git b/t/t5318-commit-graph.sh a/t/t5318-commit-graph.sh
index fb0aae61c3..5f1c746ad1 100755
--- b/t/t5318-commit-graph.sh
+++ a/t/t5318-commit-graph.sh
@@ -98,9 +98,10 @@ test_expect_success 'write graph' '
 
 test_expect_success POSIXPERM 'write graph has correct permissions' '
 	test_path_is_file $objdir/info/commit-graph &&
-	echo "-r--r--r--" >expect &&
 	test_modebits $objdir/info/commit-graph >actual &&
-	test_cmp expect actual
+	# check only user mode bits, as we do not want to rely on
+	# test environment umask
+	grep ^-r-- actual
 '
 
 graph_git_behavior 'graph exists' full commits/3 commits/1

but maybe there's some value in checking the whole thing.

-Peff
