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

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/