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

From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, peff@peff.net, 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 16:54:09 -0700	[thread overview]
Message-ID: <xmqqr1w85vtq.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <86cf29ce9c1e6dc1fc881458c18850c2893b092a.1588004647.git.me@ttaylorr.com> (Taylor Blau's message of "Mon, 27 Apr 2020 10:28:02 -0600")

Taylor Blau <me@ttaylorr.com> writes:

> In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
> allow the caller to specify the permission bits (prior to further
> adjustment by the umask and shared repository permissions) used when
> acquiring a temporary file.
>
> Use this in the commit-graph machinery for writing a non-split graph to
> acquire an opened temporary file with permissions read-only permissions
> to match the split behavior. (In the split case, Git uses
> git_mkstemp_mode' for each of the commit-graph layers with permission
> bits '0444').
>
> One can notice this discrepancy when moving a non-split graph to be part
> of a new chain. This causes a commit-graph chain where all layers have
> read-only permission bits, except for the base layer, which is writable
> for the current user.
>
> Resolve this discrepancy by using the new
> 'hold_lock_file_for_update_mode' and passing the desired permission
> bits.
>
> Doing so causes some test fallout in t5318 and t6600. In t5318, this
> occurs in tests that corrupt a commit-graph file by writing into it. For
> these, 'chmod u+w'-ing the file beforehand resolves the issue. The
> additional spot in 'corrupt_graph_verify' is necessary because of the
> extra 'git commit-graph write' beforehand (which *does* rewrite the
> commit-graph file). In t6600, this is caused by copying a read-only
> commit-graph file into place and then trying to replace it. For these,
> make these files writable.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  commit-graph.c          |  3 ++-
>  t/t5318-commit-graph.sh | 11 ++++++++++-
>  t/t6600-test-reach.sh   |  2 ++
>  3 files changed, 14 insertions(+), 2 deletions(-)

This step, queued as 3a5c7d70 (commit-graph.c: write non-split
graphs as read-only, 2020-04-27), starts failing 5318#9 at least for
me.  Do we need to loosen umask while running this test to something
not more strict than 022 or something silly like that?

Here is what I'll use as a workaround for today's pushout.

commit f062d1c028bcc839f961e8904c38c476b9deeec3
Author: Junio C Hamano <gitster@pobox.com>
Date:   Mon Apr 27 16:50:30 2020 -0700

    SQUASH??? force known umask if you are going to check the resulting mode bits

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
