Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only

From: Jeff King <peff@peff.net>
Date: Apr 21, 2020, 07:01 UTC
Message-ID: <20200421070135.GA2906244@coredump.intra.peff.net>
In-Reply-To: <xmqqlfmpprhq.fsf@gitster.c.googlers.com>

On Mon, Apr 20, 2020 at 06:17:05PM -0700, Junio C Hamano wrote:

Show 7 quoted lines

> Taylor Blau <me@ttaylorr.com> writes:
> 
> >   * do want to respect core.sharedRepository, in which case the
> >     current behavior of always setting split-graph permissions to '0444'
> >     is wrong, or
> 
> Yes, I would say "always 0444" is wrong.

I'm not sure. That's what we do for loose objects, packs, etc. The mode we feed to git_mkstemp_mode(), etc, is not the true mode we expect to end up with. We know that it will be modified by the umask, and then perhaps by adjust_shared_perm().

If you are arguing that there are only two interesting modes: 0444 and 0666, and those could be represented by a read-only/read-write enum, I'd agree with that. But the rest of the code already spells those as 0444 and 0666, and I don't see that as a big problem (in fact, there's a bit of an advantage because the same constants work at both high and low levels of the call stack, so you don't have to decide where to put the translation).

Show 14 quoted lines

> >   * we do not want to respect core.sharedRepository, in which case these
> >     patches are doing what we want by setting all commit-graph files to
> >     have read-only permissions.
> >
> > My hunch is that we do not want to abide by core.sharedRepository here
> > for the same reason that, say, loose objects are read-only. What do you
> > think?
> 
> I thought that adjusting perm for sharedRepository is orthogonal to
> the read-only vs read-write.  If a file ought to be writable by the
> owner, we would make it group writable in a group shared repository.
> If a file is readable by the owner, we make sure it is readable by
> group in such a repository (and we do not want to flip write bit
> on).  That happens by calling path.c::calc_shared_perm().

Right. I think adjust_shared_perm() should already be doing what we want, and we should continue to call it. But it should not be responsible for this "read-only versus read-write" decision. That happens much earlier, and it adjusts as appropriate.

-Peff