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

Message

Taylor Blau April 20, 2020, 10:50 p.m. UTC

Hi,

A colleague of mine pointed out they had occasionally observed that some
layers of a commit-graph chain appear to be have both read and write
permission bits for the user, while others are marked as read-only.

After some investigation, I realized that this only happens to the first
layer in a commit-graph-chain, and occurs when we move a non-split graph
into a chain as the new base. Since write_commit_graph() just renames in
this case, we carry over the 0644 permissions that we create non-split
graphs with, even though split graphs are created to be read-only.

Initially, I resolved this by 'chmod(graph, 0444)'-ing the graph after
renaming it into place, but it ends up being much cleaner if we
introduce an additional parameter in 'hold_lock_file_for_update' for a
mode.

The first two patches set this up, and the third patch uses it in
commit-graph.c, and corrects some test fallout. Eventually, we may want
to take another look at all of this and always create lockfiles with
permission 0444, but that change is much larger than this one and could
instead be done over time.

Thanks,
Taylor

Taylor Blau (3):
  tempfile.c: introduce 'create_tempfile_mode'
  lockfile.c: introduce 'hold_lock_file_for_update_mode'
  commit-graph.c: write non-split graphs as read-only

 commit-graph.c          |  3 ++-
 lockfile.c              | 18 ++++++++++--------
 lockfile.h              | 19 +++++++++++++++++--
 t/t5318-commit-graph.sh | 11 ++++++++++-
 t/t6600-test-reach.sh   |  2 ++
 tempfile.c              |  6 +++---
 tempfile.h              |  7 ++++++-
 7 files changed, 50 insertions(+), 16 deletions(-)

--
2.26.1.108.gadb95c98e4

Comments

Junio C Hamano April 20, 2020, 11:23 p.m. UTC | #1

Taylor Blau <[e-mail]> writes:

> The first two patches set this up, and the third patch uses it in
> commit-graph.c, and corrects some test fallout. Eventually, we may want
> to take another look at all of this and always create lockfiles with
> permission 0444, but that change is much larger than this one and could
> instead be done over time.

So,... is the problem that this did not follow the common pattern of
creating (while honoring umask) and then call adjust_shared_perm(),
which I thought was the common pattern for files in $GIT_DIR/?

Taylor Blau April 20, 2020, 11:39 p.m. UTC | #2

On Mon, Apr 20, 2020 at 04:23:13PM -0700, Junio C Hamano wrote:
> Taylor Blau <[e-mail]> writes:
>
> > The first two patches set this up, and the third patch uses it in
> > commit-graph.c, and corrects some test fallout. Eventually, we may want
> > to take another look at all of this and always create lockfiles with
> > permission 0444, but that change is much larger than this one and could
> > instead be done over time.
>
> So,... is the problem that this did not follow the common pattern of
> creating (while honoring umask) and then call adjust_shared_perm(),
> which I thought was the common pattern for files in $GIT_DIR/?

We do call adjust_shared_perm() from (based on what's currently in
master) 'write_commit_graph_file' -> 'hold_lockfile_for_update' ->
'lock_file_timeout' -> 'lock_file' -> 'create_tempfile'.

But do we want commit-graphs to be respecting 'core.sharedRepository'
here? Either we:

  * do want to respect core.sharedRepository, in which case the
    current behavior of always setting split-graph permissions to '0444'
    is wrong, or

  * 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?


Thanks,
Taylor

Junio C Hamano April 21, 2020, 1:17 a.m. UTC | #3

Taylor Blau <[e-mail]> 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.

>   * 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().

Jeff King April 21, 2020, 7:01 a.m. UTC | #4

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

> Taylor Blau <[e-mail]> 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

Junio C Hamano April 21, 2020, 6:59 p.m. UTC | #5

Jeff King <[e-mail]> writes:

>> 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.

Yup, that is what I meant.  I am aware that these 0444/0666 are
limited with umask at open(O_CREAT) time, and then we later call
adjust_shared_perm().  I thought Taylor meant to always make it
readable by everybody, which was the only point I was objecting to.

> 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.

Yes.