Re: [PATCH v3 0/2] Correctly handle transient files in shared repositories

Junio C Hamano [off-list ref] writes:

quoted

The only change relative to v2 is that the second commit message
clarifies why apply, fsck and fast-import are left unchanged.

I do not think it clarifies to make "fsck" whose lost and found are
written to GIT_DIR and "apply"'s ".rej" share the same "we dunno"
reasoning, though.

I'd say we should go with this one.  I think the reasoning for
"fsck" should be a lot clearer this way.

-- >8 --
Author: Johannes Schindelin [off-list ref]
Date:   Mon Jan 11 19:35:54 2016 +0100

    Handle more file writes correctly in shared repos
    
    In shared repositories, we have to be careful when writing files whose
    permissions do not allow users other than the owner to write them.
    
    In particular, we force the marks file of fast-export and the FETCH_HEAD
    when fetching to be rewritten from scratch.
    
    This commit does not touch other calls to fopen() that want to
    write files:
    
     - commands that write to working tree files (core.sharedRepository
       does not affect permission bits of working tree files),
       e.g. .rej file created by "apply --reject", result of applying a
       previous conflict resolution by "rerere", "git merge-file".
    
     - git am, when splitting mails (git-am correctly cleans up its directory
       after finishing, so there is no need to share those files between users)
    
     - git fsck, when writing lost&found blobs (they are written in the
       file under its object name, so an existing file with tighter
       permission that you cannot write into is OK, because what you are
       failing to write is the same contents that the file already has
       anyway).
    
     - git submodule clone, when writing the .git file, because the file
       will not be overwritten
    
     - git_terminal_prompt() in compat/terminal.c, because it is not writing to
       a file at all
    
     - git diff --output, because the output file is clearly not intended to be
       shared between the users of the current repository
    
     - git fast-import, when writing a crash report, because the reports' file
       names are unique due to an embedded process ID
    
     - mailinfo() in mailinfo.c, because the output is clearly not intended to
       be shared between the users of the current repository
    
     - check_or_regenerate_marks() in remote-testsvn.c, because this is only
       used for Git's internal testing
    
    Note that this patch does not touch callers of write_file() and
    write_file_gently(), which would benefit from the same scrutiny as
    to usage in shared repositories.  Most notable users are branch,
    daemon, submodule & worktree, and a worrisome call in transport.c
    when updating one ref (which ignores the shared flag).
    
    Signed-off-by: Johannes Schindelin [off-list ref]
    Signed-off-by: Junio C Hamano [off-list ref]

On Mon, Jan 11, 2016 at 01:22:07PM -0800, Junio C Hamano wrote:

     - git fsck, when writing lost&found blobs (they are written in the
       file under its object name, so an existing file with tighter
       permission that you cannot write into is OK, because what you are
       failing to write is the same contents that the file already has
       anyway).

I'm not sure I buy this argument. Yes, you should not be writing
anything else, but that does not change the fact that "fsck" will
unceremoniously abort:

  $ git init
  $ echo foo | git hash-object -w --stdin
  $ git fsck --lost-found
  notice: HEAD points to an unborn branch (master)
  Checking object directories: 100% (256/256), done.
  notice: No default references
  dangling blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99

  $ chmod -w .git/lost-found/other/257cc5642cb1a054f08cc83f2d943e56fd3ebe99 
  $ $ git fsck --lost-found
  notice: HEAD points to an unborn branch (master)
  Checking object directories: 100% (256/256), done.
  notice: No default references
  dangling blob 257cc5642cb1a054f08cc83f2d943e56fd3ebe99
  fatal: Could not open '.git/lost-found/other/257cc5642cb1a054f08cc83f2d943e56fd3ebe99': Permission denied

So I think this would be a reasonable candidate (or alternatively, to
treat EPERM on an existing file as a soft error). I am totally fine not
to address it as part of this series, though.

-Peff