git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 8/8] reflog_expire(): lock symbolic refs themselves, not their referent

From
Stefan Beller <sbeller@google.com>
Date
Feb 13, 2015, 18:21 UTC
Message-ID
<CAGZ79kZpCjcGeifbLztpNUSq7-3Yy2_GEVPGEQsxrgoZfLFU+g@mail.gmail.com>
In-Reply-To
<xmqqoaoxoffe.fsf@gitster.dls.corp.google.com>
On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I also realized that Git's current policy is probably not tenable if one
>> process is re-seating a symref at the same time as another process is
>> expiring its reflog. The "git reflog expire HEAD" might grab
>> "refs/heads/master.lock" then start rewriting "logs/HEAD". Meanwhile,
>> "git checkout foo" would grab "HEAD.lock" (not being blocked by the
>> "expire" process), then rewrite it to "ref: refs/heads/foo", then grab
>> "refs/heads/foo.lock" before updating "logs/HEAD". So both processes
>> could be writing "logs/HEAD" at the same time.
>> ...
>> Switching to holding only "HEAD.lock" while updating "logs/HEAD" is the
>> right solution,...
>
> We convinced ourselves that not locking the symref is wrong, but
> have we actually convinced us that not locking the underlying ref,
> as long as we have a lock on the symref, is safe?
>
> To protect you, the holder of a lock on refs/remotes/origin/HEAD
> that happens to point at refs/remotes/origin/next, from somebody who
> is updating the underlying refs/remotes/origin/next directly without
> going through the symbolic ref (e.g. receive-pack), wouldn't the
> other party need to find any and all symbolic refs that point at the
> underlying ref and take locks on them?

As we're just modifying the ref log of HEAD in this case, we don't bother with where the HEAD points to. The other party may change refs/remotes/origin/next without us noticing, but we don't care here as all we do is rewriting the ref log for HEAD.

If the other party were to modify HEAD (point it to some other branch, or forward the branch pointed to), they'd be expected to lock HEAD and would fail as we have the lock?

Show 5 quoted lines
>
> As dereferencing a symbolic ref in the forward direction is much
> cheaper than in the reverse, and because you need to dereference it
> anyway, I wonder if we want the endgame to be "hold locks on both",
> not "just hold a lock on the symlink".
That would be the safest option indeed.
Previous: Junio C HamanoNext: Junio C Hamano
Message 37 of 46 in “Fix some problems with reflog expiration”
  1. 0/8 Fix some problems with reflog expirationMichael Haggerty, Feb 9, 2015
  2. 1/8 write_ref_sha1(): remove check for lock == NULLMichael Haggerty, Feb 9, 2015
  3. Stefan BellerFeb 10, 2015
  4. Jeff KingFeb 11, 2015
  5. 2/8 write_ref_sha1(): Move write elision test to callersMichael Haggerty, Feb 9, 2015
  6. Junio C HamanoFeb 12, 2015
  7. 3/8 lock_ref_sha1_basic(): do not set force_write for missing referencesMichael Haggerty, Feb 9, 2015
  8. Stefan BellerFeb 10, 2015
  9. Jeff KingFeb 11, 2015
  10. Stefan BellerFeb 11, 2015
  11. Michael HaggertyFeb 12, 2015
  12. 4/8 reflog: fix documentationMichael Haggerty, Feb 9, 2015
  13. Stefan BellerFeb 10, 2015
  14. 5/8 reflog: rearrange the manpageMichael Haggerty, Feb 9, 2015
  15. Stefan BellerFeb 10, 2015
  16. Michael HaggertyFeb 12, 2015
  17. Junio C HamanoFeb 12, 2015
  18. 6/8 reflog_expire(): ignore --updateref for symbolic referencesMichael Haggerty, Feb 9, 2015
  19. Stefan BellerFeb 11, 2015
  20. Michael HaggertyFeb 12, 2015
  21. Stefan BellerFeb 12, 2015
  22. Junio C HamanoFeb 12, 2015
  23. Jeff KingFeb 12, 2015
  24. Michael HaggertyFeb 13, 2015
  25. 7/8 reflog_expire(): never update a reference to null_sha1Michael Haggerty, Feb 9, 2015
  26. Eric SunshineFeb 9, 2015
  27. Michael HaggertyFeb 12, 2015
  28. 8/8 reflog_expire(): lock symbolic refs themselves, not their referentMichael Haggerty, Feb 9, 2015
  29. Stefan BellerFeb 11, 2015
  30. Junio C HamanoFeb 11, 2015
  31. Stefan BellerFeb 11, 2015
  32. Michael HaggertyFeb 12, 2015
  33. Stefan BellerFeb 12, 2015
  34. Michael HaggertyFeb 13, 2015
  35. Stefan BellerFeb 13, 2015
  36. Junio C HamanoFeb 13, 2015
  37. Stefan BellerFeb 13, 2015
  38. Junio C HamanoFeb 13, 2015
  39. Stefan BellerFeb 13, 2015
  40. Junio C HamanoFeb 13, 2015
  41. Michael HaggertyFeb 13, 2015
  42. Junio C HamanoFeb 13, 2015
  43. Michael HaggertyFeb 14, 2015
  44. Stefan BellerFeb 9, 2015
  45. refs.c: get rid of force_write flagStefan Beller, Feb 10, 2015
  46. Michael HaggertyFeb 12, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.