Re: [PATCH 2/4] reftable/stack: register new tables as tempfiles

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> It is consistent. The problem is rather that `mks_tempfile_m()` takes a
> mode as input, but still ends up applying the umask to that mode.

Ah, OK.  

> Thus,
> using that function without a subsequent call to chmod(3P) would end up
> mishandling "core.sharedRepository".

It is understandable because this is meant for tempfile; you want to
create it, expect to use it yourself and not by others, before you
are done with it.  There is no place in this sequence where you need
to open access up to those who share access to the repository.

The rest of the message is primarily for my own education.

I got a bit curious how other parts of the system does this.  For
example, writing an loose object calls git_mkstemp_mode() with 0444
and then finalize_object_file() calls adjust_shared_perm() after it
renames the file to its final name at the end.  A tight umask like
0700 may demote the original 0444 to 0400, but adjust_shared_perm()
expands the 0400 appropriately.

The index file is more interesting.  We bypass the whole mkstemp
thing (as we know the final filename and add .lock after it), and
the call chain looks like this:

    repo_refresh_and_write_index() ->
      repo_hold_locked_index() ->
        hold_lock_file_for_update() ->
          lock_file() ->
            create_tempfile_mode() ->
              open()
              activate_tempfile()
              adjust_shared_perm()
      refresh_index()
      write_locked_index() ->
        commit_locked_index() ->
          commit_lock_file_to() ->
            rename_tempfile().

After we create the file with open() and before returning to the
caller, hold_lock_file_for_update() already sets the permission
bits correctly, so the "committing" phase to rename the written
lockfile into its final place becomes merely a rename without any
futzing with permission bits.

So in short, for a file that we intend to create, write, and then
commit to the final name, we use at least two approaches:

 - Let mkstemp_mode() do its thing, and fix the permission bits
   later with adjust_shared_perm().

 - Let hold_lock_file_for_update() take care of the permission bits.

I sense there might be some clean-up opportunities around here.
After all, lockfile is (or at least pretends to be) built on top of
tempfile, and it is for more permanent (as opposed to temporary)
files, but it somehow wasn't a good fit to wrap new tables in this
series?

Thanks.