On Mon, Apr 20, 2020 at 06:17:05PM -0700, Junio C Hamano wrote: > Taylor Blau <me@xxxxxxxxxxxx> 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). > > * 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
