| Message ID | f83437f130812d172167d335ba2d13b1545a9f58.1588004647.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: write non-split graphs as read-only | expand |
On Mon, Apr 27, 2020 at 10:28:05AM -0600, Taylor Blau wrote: > Non-layered commit-graphs use 'adjust_shared_perm' to make the > commit-graph file readable (or not) to a combination of the user, group, > and others. > > Call 'adjust_shared_perm' for split-graph layers to make sure that these > also respect 'core.sharedRepository'. The 'commit-graph-chain' file > already respects this configuration since it uses > 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually > in 'create_tempfile_mode'). It occurs to me that we might want to apply the same treatment to 'commit-graph-chain's, too. Junio: I'm not sure if you want to apply the below in this series on top, or if you'd prefer me send it as a separate series. Either way, here's a patch to do just that: -- >8 -- Subject: [PATCH] commit-graph.c: make 'commit-graph-chain's read-only In a previous commit, we made incremental graph layers read-only by using 'git_mkstemp_mode' with permissions '0444'. There is no reason that 'commit-graph-chain's should be modifiable by the user, since they are generated at a temporary location and then atomically renamed into place. To ensure that these files are read-only, too, use 'hold_lock_file_for_update_mode' with the same read-only permission bits, and let the umask and 'adjust_shared_perm' take care of the rest. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- commit-graph.c | 3 ++- t/t5324-split-commit-graph.sh | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/commit-graph.c b/commit-graph.c index d05a55901d..b2dfd7701f 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1378,7 +1378,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) if (ctx->split) { char *lock_name = get_chain_filename(ctx->odb); - hold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR); + hold_lock_file_for_update_mode(&lk, lock_name, + LOCK_DIE_ON_ERROR, 0444); fd = git_mkstemp_mode(ctx->graph_name, 0444); if (fd < 0) { diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 61136c737f..a8b12c8110 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -362,6 +362,8 @@ do test_line_count = 1 graph-files && echo "$modebits" >expect && test_modebits $graphdir/graph-*.graph >actual && + test_cmp expect actual && + test_modebits $graphdir/commit-graph-chain >actual && test_cmp expect actual ' done <<\EOF -- 2.26.0.113.ge9739cdccc
On Mon, Apr 27, 2020 at 11:21:11AM -0600, Taylor Blau wrote: > On Mon, Apr 27, 2020 at 10:28:05AM -0600, Taylor Blau wrote: > > Non-layered commit-graphs use 'adjust_shared_perm' to make the > > commit-graph file readable (or not) to a combination of the user, group, > > and others. > > > > Call 'adjust_shared_perm' for split-graph layers to make sure that these > > also respect 'core.sharedRepository'. The 'commit-graph-chain' file > > already respects this configuration since it uses > > 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually > > in 'create_tempfile_mode'). > > It occurs to me that we might want to apply the same treatment to > 'commit-graph-chain's, too. Yeah, perhaps. I think we've been fairly inconsistent about modes here. Really, just about _everything_ in .git is meant to be immutable, because we generally use rename() to update files atomically. And there's no reason anybody should ever be opening them for writing. I think we started with dropping the write-bit on loose and packed object files because those are extra-precious (even if everything else were corrupted, your history, etc, is all still there). And certainly you can't update them without invalidating their checksum trailers anyway. So I think there's really three levels that could make sense: 1. Many files in .git should lose their write bits, because Git will never update them except through rename. This makes things safer against accidental changes, but more annoying when you do want to edit by hand. Probably .git/config should likely be exempted, as we'd encourage people to hand-edit even if it's not atomic. But having to chmod before hand-editing a packed-refs file while debugging is not a huge burden. 2. Any file written via hashfd() should be marked immutable. It cannot be edited without rewriting the whole contents and updating the trailer anyway. That would imply that commit-graph and chain files should be immutable, but the commit-graph-chain file should not. 3. Everything except actual object files should retain their write bit. It's a minor annoyance when touching the repo by hand (e.g., "rm" is sometimes pickier even about deletion), and it's not like there's a rash of people accidentally overwriting their refs/ files (they're just as likely to screw themselves by deleting them). I admit I don't overly care much between the three. And your patches look fine to me, as they're just making the commit-graph code consistent with itself (and that inconsistency is wrong under any of the mental models above ;) ). The exception is the final patch, which is an actual bug-fix for people using core.sharedRepository. I suspect the feature is used infrequently enough that nobody noticed. -Peff
diff --git a/commit-graph.c b/commit-graph.c index 5b5047a7dd..d05a55901d 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -1386,6 +1386,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx) return -1; } + if (adjust_shared_perm(ctx->graph_name)) { + error(_("unable to adjust shared permissions for '%s'"), + ctx->graph_name); + return -1; + } + f = hashfd(fd, ctx->graph_name); } else { hold_lock_file_for_update_mode(&lk, ctx->graph_name, diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 53b2e6b455..61136c737f 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -351,4 +351,22 @@ test_expect_success 'split across alternate where alternate is not split' ' test_cmp commit-graph .git/objects/info/commit-graph ' +while read mode modebits +do + test_expect_success POSIXPERM "split commit-graph respects core.sharedrepository $mode" ' + rm -rf $graphdir $infodir/commit-graph && + git reset --hard commits/1 && + test_config core.sharedrepository "$mode" && + git commit-graph write --split --reachable && + ls $graphdir/graph-*.graph >graph-files && + test_line_count = 1 graph-files && + echo "$modebits" >expect && + test_modebits $graphdir/graph-*.graph >actual && + test_cmp expect actual + ' +done <<\EOF +0666 -r--r--r-- +0600 -r-------- +EOF + test_done
Non-layered commit-graphs use 'adjust_shared_perm' to make the commit-graph file readable (or not) to a combination of the user, group, and others. Call 'adjust_shared_perm' for split-graph layers to make sure that these also respect 'core.sharedRepository'. The 'commit-graph-chain' file already respects this configuration since it uses 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually in 'create_tempfile_mode'). Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- commit-graph.c | 6 ++++++ t/t5324-split-commit-graph.sh | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+)