Re: [PATCH 1/4] resolve_ref: close race condition for packed refs

[Michael Haggerty <mhagger@alum.mit.edu>]

On 05/07/2013 04:38 AM, Jeff King wrote:
> [...]
> This patch solves it by factoring out the fallback code from
> the lstat() case and calling it from both places. We do not
> need to do the same thing for the symlink/readlink code
> path, even though it might receive ENOENT, too, because
> symlinks cannot be packed (so if it disappears after the
> lstat, it is truly being deleted).

To be really pedantic, we should handle all kinds of changes that
another process might make simultaneously:

* symlink -> deleted

  Was already OK, as you explained above.

* regular file -> deleted

  Handled by your patch

* symlink -> regular file

  Could come from

      update-ref --no-deref

  if the old version of the reference were a symlink-based symbolic
  reference.  Oops, wait, that's broken anyway:

      $ ln -sf master .git/refs/heads/foo
      $ git for-each-ref
      4204906e2ee75f9b7224860c40df712d112c004b commit	refs/heads/foo
      4204906e2ee75f9b7224860c40df712d112c004b commit	refs/heads/master
      $ git update-ref --no-deref refs/heads/foo master
      $ dir .git/refs/heads/foo
      lrwxrwxrwx 1 mhagger mhagger 6 May 13 01:07 .git/refs/heads/foo ->
master

  But supposing that were fixed and it occurred between the call to
  lstat() and the call to readlink(), then readlink() would fail with
  errno == EINVAL and we should respond by repeating the code starting
  with the lstat().

* regular file -> symlink

  Could come from overwriting a reference with a symlink-based symbolic
  reference.  But this could only happen if the parallel process were
  an old version of Git

I think it's been quite a while since Git has used symlinks for symbolic
references, so I doubt that it is worth fixing the second two cases.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/