[v2,0/4] commit-graph: write non-split graphs as read-only

Message

Taylor Blau April 27, 2020, 4:27 p.m. UTC

Hi,

Here's an updated version of my series to resolve a difference between
the permissions that split- and non-split commit-graphs are written
with.

Not much has changed since last time, since the main discussion revolved
around differences between what mode the user passes and how that
interacts with 'adjust_shared_perm' as well as why split-graphs are
always read-only for everybody.

Patches 1-3 remain mostly the same, with only some additional
documentation in the patches and headers about the interaction between
the 'mode' parameter and the umask and 'core.sharedRepository'. The
fourth patch is new, and makes sure that split-graphs are also
respecting 'core.sharedRepository' (and never have permissions above
0444).

The end-state of this is that both split- and non-split graphs have at
most permission 0444, and both respect core.sharedRepository.

Thanks in advance for another look.


Taylor Blau (4):
  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: ensure graph layers respect core.sharedRepository

 commit-graph.c                |  9 ++++++++-
 lockfile.c                    | 18 ++++++++++--------
 lockfile.h                    | 32 ++++++++++++++++++++++++++++----
 t/t5318-commit-graph.sh       | 11 ++++++++++-
 t/t5324-split-commit-graph.sh | 18 ++++++++++++++++++
 t/t6600-test-reach.sh         |  2 ++
 tempfile.c                    |  6 +++---
 tempfile.h                    | 10 +++++++++-
 8 files changed, 88 insertions(+), 18 deletions(-)

Range-diff against v1:
1:  aa86e8df40 ! 1:  03c975b0bd tempfile.c: introduce 'create_tempfile_mode'
    @@ Commit message
         creates a temporary file with global read-write permissions, introduce a
         variant here that allows specifying the mode.

    +    Note that the mode given to 'create_tempfile_mode' is not guaranteed to
    +    be written to disk, since it is subject to both the umask and
    +    'core.sharedRepository'.
    +
         Arguably, all temporary files should have permission 0444, since they
         are likely to be renamed into place and then not written to again. This
         is a much larger change than we may want to take on in this otherwise
    @@ tempfile.c: static void deactivate_tempfile(struct tempfile *tempfile)

      ## tempfile.h ##
     @@ tempfile.h: struct tempfile {
    +  * Attempt to create a temporary file at the specified `path`. Return
       * a tempfile (whose "fd" member can be used for writing to it), or
       * NULL on error. It is an error if a file already exists at that path.
    ++ * Note that `mode` will be further modified by the umask, and possibly
    ++ * `core.sharedRepository`, so it is not guaranteed to have the given
    ++ * mode.
       */
     -struct tempfile *create_tempfile(const char *path);
     +struct tempfile *create_tempfile_mode(const char *path, int mode);
2:  dad37d4233 ! 2:  c1c84552bc lockfile.c: introduce 'hold_lock_file_for_update_mode'
    @@ Commit message
         functions, and leave the existing functions alone by inlining their
         definitions in terms of the new mode variants.

    -    Note that even though the commit-graph machinery only calls
    +    Note that, like in the previous commit, 'hold_lock_file_for_update_mode'
    +    is not guarenteed to set the given mode, since it may be modified by
    +    both the umask and 'core.sharedRepository'.
    +
    +    Note also that even though the commit-graph machinery only calls
         'hold_lock_file_for_update', that this is defined in terms of
         'hold_lock_file_for_update_timeout', and so both need an additional mode
         parameter here.
    @@ lockfile.c: NORETURN void unable_to_lock_die(const char *path, int err)
      			unable_to_lock_die(path, errno);

      ## lockfile.h ##
    +@@
    +  * functions. In particular, the state diagram and the cleanup
    +  * machinery are all implemented in the tempfile module.
    +  *
    ++ * Permission bits
    ++ * ---------------
    ++ *
    ++ * If you call either `hold_lock_file_for_update_mode` or
    ++ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested
    ++ * mode for the underlying temporary file. Note that the file isn't
    ++ * guaranteed to have this exact mode, since it may be limited by either
    ++ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`
    ++ * for more.
    +  *
    +  * Error handling
    +  * --------------
     @@ lockfile.h: struct lock_file {
    -  * timeout_ms is -1, retry indefinitely. The flags argument and error
    -  * handling are described above.
    +  * file descriptor for writing to it, or -1 on error. If the file is
    +  * currently locked, retry with quadratic backoff for at least
    +  * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if
    +- * timeout_ms is -1, retry indefinitely. The flags argument and error
    +- * handling are described above.
    ++ * timeout_ms is -1, retry indefinitely. The flags argument, error
    ++ * handling, and mode are described above.
       */
     -int hold_lock_file_for_update_timeout(
     +int hold_lock_file_for_update_timeout_mode(
3:  622fd92cee ! 3:  86cf29ce9c commit-graph.c: write non-split graphs as read-only
    @@ Commit message
         commit-graph.c: write non-split graphs as read-only

         In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
    -    allow the caller to specify the permission bits used when acquiring a
    -    temporary file.
    +    allow the caller to specify the permission bits (prior to further
    +    adjustment by the umask and shared repository permissions) used when
    +    acquiring a temporary file.

         Use this in the commit-graph machinery for writing a non-split graph to
         acquire an opened temporary file with permissions read-only permissions
         to match the split behavior. (In the split case, Git uses
    -    'git_mkstemp_mode' for each of the commit-graph layers with permission
    +    git_mkstemp_mode' for each of the commit-graph layers with permission
         bits '0444').

         One can notice this discrepancy when moving a non-split graph to be part
-:  ---------- > 4:  f83437f130 commit-graph.c: ensure graph layers respect core.sharedRepository
--
2.26.0.113.ge9739cdccc