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
