Re: [PATCH] packed_ref_store: handle a packed-refs file that is a symlink

On Thu, Jul 27, 2017 at 10:19:48AM -0700, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

Change `commit_packed_refs()` to use `get_locked_file_path()` to find
the path of the file that it should overwrite. Since that path was
properly resolved when the lockfile was created, this restores the
pre-42dfa7ecef behavior.

Because when we take a lock hold_lock_file() eventually calls into
lock_file() which by default takes the lock on the target of the
symbolic link (which is the sensible default, which is triggered in
this codepath), so this change to use the name of that file is all
that is needed.

Makes sense.  Makes me wonder why we need a separate .new file
(instead of writing into the .lock instead), but that is a different
issue.

It comes from 42dfa7ece (commit_packed_refs(): use a staging file
separate from the lockfile, 2017-06-23). That commit explains that we
want to be able to put the new contents into service before we release
the lock. But it doesn't say why that's useful.

I recall from past discussions that this will help close some races,
and e5cc7d7d2 (repack_without_refs(): don't lock or unlock the packed
refs, 2017-07-01) alludes to this. I think the races in question have to
do with holding the packed-refs lock while pruning the just-packed
files, but I'm having trouble digging up specifics in the archive.

-Peff