Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository

From: Jeff King <peff@peff.net>
Date: Apr 27, 2020, 20:58 UTC
Message-ID: <20200427205808.GA1732530@coredump.intra.peff.net>
In-Reply-To: <20200427172111.GA58509@syl.local>

On Mon, Apr 27, 2020 at 11:21:11AM -0600, Taylor Blau wrote:

Show 13 quoted lines

> 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