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
Junio C Hamano <gitster@pobox.com>
Date
Feb 13, 2015, 19:12 UTC
Message-ID
<xmqqa90hocc5.fsf@gitster.dls.corp.google.com>
In-Reply-To
<CAGZ79kYRi3KYcvNQbkhP0uLFgpJzD+h=P+smOLQy2Msv0C_1kw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
Show 32 quoted lines
> On Fri, Feb 13, 2015 at 10:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Fri, Feb 13, 2015 at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>>> 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?
>>
>> I was not talking about HEAD in what you are responding to, though.
>> Don't we maintain a reflog on refs/remotes/origin/HEAD?  Is it OK to
>> allow its entries to become inconsistent with the underlying ref?
>
> Yes we do have a HEAD ref log, and that ref log would differ from
> the underlying ref log. I don't know if that is desired, but I
> would tend to not like it.

HEAD (or refs/remotes/origin/HEAD) reflog and reflog for refs/heads/master (or refs/remotes/origin/next) would have to be different as long as we allow symbolic refs to be repointed to different refs. If HEAD refers to 'next' today, and at the tip of next sits commit X, the reflog for both of them would record the fact that they were pointing at X. If you repoint HEAD to point at 'master' (e.g. "git checkout master") whose tip is at commit Y, then reflog for HEAD would record the fact that now HEAD points at Y, and reflogs for 'master' or 'next' would not change merely because you switched where HEAD points at.

And there is anything to like or not to like about it.

As we are trying to see a way to move forward to do the right thing around reflog, I was wondering if locking only the symbolic ref is a sensible endgame. "The right thing" being:

   When a symbolic ref S points at underlying ref R, and if R's tip
   changes from X to Y, we would want to know from the reflog of S
   that S used to point at X and then changed to point at Y.

Replace S and R with either (HEAD, refs/heads/master) or (refs/remotes/origin/HEAD, refs/remotes/origin/next) in the above and we want both to be true.

How best to achieve that, and what is the set of right ref(s) to take lock(s) on? I am not very much interested in how incorrect today's code might behave. That is not the central point when discussing what is the best way forward.

> So the other party updating the underlying ref would also need to lock
> HEAD then,

Yes, that is what I meant. Your "also" can be read in two different ways ("other party, too" or "HEAD, too"), though, and I think we want both ;-). That is why I hinted that it was iffy to state that we only have to take the lock only on S and not on R, but only as a workaround to keep older implementation out we take both---once they get extinct we can get away with by taking a lock only on S.

When pushing to update 'master' and 'next' into a repository whose 'HEAD' points at 'master', we would want to take locks on 'next' (no question), but is it sensible to take the lock on 'HEAD' and deliberately leave 'master' unlocked? Or is it more sensible to take all locks on the underlying refs involved (i.e. 'next' and 'master') and in addition any symbolic refs that are pointing at these refs involved (i.e. 'HEAD')?

Previous: Stefan BellerNext: Michael Haggerty
Message 40 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.