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

Re: [PATCH v2 00/29] Create a reference backend for packed refs

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 23, 2017, 21:47 UTC
Message-ID
<xmqqefuacr6y.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20170623200024.amgced62hue2iffj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Fri, Jun 23, 2017 at 03:01:59PM -0400, Jeff King wrote:
>
>> On Fri, Jun 23, 2017 at 09:01:18AM +0200, Michael Haggerty wrote:
>> 
>> > * Change patch 17 "packed_ref_store: support iteration" to always
>> >   iterate over the packed refs using `DO_FOR_EACH_INCLUDE_BROKEN`.
>> >   This switches off the check in the packed-ref iterator of whether a
>> >   reference is broken. This is now checked only in
>> >   `files_ref_iterator_advance()`, after the packed and loose
>> >   references have been merged together. It also saves some work.
>> 
>> I'm curious why you prefer this solution to just removing the code
>> entirely. Wouldn't it be an error to call the packed ref iterator
>> without INCLUDE_BROKEN? The "entries may not be valid" thing is a
>> property of the packed-refs concept itself, not a particular caller's
>> view of it.
>
> Speculating on my own question. I guess it would prepare us for a day
> when a possible ref store is to use a packed-refs _without_ loose refs.
> IOW, the property is defined on packed-refs today, but a possible future
> direction would be to use it by itself. But maybe I'm just making things
> up.

OK. In other words, it's not a packed-refs's characteristics that cruft are allowed. It's that a ref storage that is implemented as an overlay of one storage (which happens to be the loose one) on top of another (which happens to be the packed refs file) allows the latter one to have cruft if (and only if) that broken one is covered by the former one.

Show 7 quoted lines
> At any rate, I've read through the whole series and dropped a few
> comments in there. Overall it looks good. If I had one complaint, it's
> that the individual commits all look obviously correct but it is hard to
> judge whether the bigger steps they are taking are the right thing. I
> mostly have faith in you, as I know that your end goal is a good one,
> and that you're very familiar with this code.  But just something I
> noticed while reviewing.
Thanks.
Previous: Jeff KingNext: Jeff King
Message 49 of 52 in “Create a reference backend for packed refs”
  1. 00/29 Create a reference backend for packed refsMichael Haggerty, Jun 23, 2017
  2. 01/29 t1408: add a test of stale packed refs covered by loose refsMichael Haggerty, Jun 23, 2017
  3. Jeff KingJun 23, 2017
  4. Junio C HamanoJun 23, 2017
  5. 02/29 add_packed_ref(): teach function to overwrite existing refsMichael Haggerty, Jun 23, 2017
  6. 07/29 validate_packed_ref_cache(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  7. 05/29 packed_ref_store: move `packed_refs_lock` member hereMichael Haggerty, Jun 23, 2017
  8. 03/29 packed_ref_store: new structMichael Haggerty, Jun 23, 2017
  9. 09/29 get_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  10. 04/29 packed_ref_store: move `packed_refs_path` hereMichael Haggerty, Jun 23, 2017
  11. 13/29 rollback_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  12. 12/29 commit_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  13. 14/29 get_packed_ref(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  14. 06/29 clear_packed_ref_cache(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  15. 17/29 packed_ref_store: support iterationMichael Haggerty, Jun 23, 2017
  16. Jeff KingJun 23, 2017
  17. 16/29 packed_peel_ref(): new function, extracted from `files_peel_ref()`Michael Haggerty, Jun 23, 2017
  18. 19/29 packed-backend: new module for handling packed referencesMichael Haggerty, Jun 23, 2017
  19. Jeff KingJun 23, 2017
  20. Stefan BellerJun 23, 2017
  21. Junio C HamanoJun 23, 2017
  22. 20/29 packed_ref_store: make class into a subclass of `ref_store`Michael Haggerty, Jun 23, 2017
  23. Jeff KingJun 23, 2017
  24. 21/29 commit_packed_refs(): report errors rather than dyingMichael Haggerty, Jun 23, 2017
  25. 22/29 commit_packed_refs(): use a staging file separate from the lockfileMichael Haggerty, Jun 23, 2017
  26. Jeff KingJun 23, 2017
  27. Michael HaggertyJun 24, 2017
  28. Jeff KingJun 24, 2017
  29. 24/29 packed_refs_lock(): report errors via a `struct strbuf *err`Michael Haggerty, Jun 23, 2017
  30. 26/29 clear_packed_ref_cache(): don't protest if the lock is heldMichael Haggerty, Jun 23, 2017
  31. Jeff KingJun 23, 2017
  32. 25/29 packed_refs_unlock(), packed_refs_is_locked(): new functionsMichael Haggerty, Jun 23, 2017
  33. 23/29 packed_refs_lock(): function renamed from lock_packed_refs()Michael Haggerty, Jun 23, 2017
  34. 28/29 repack_without_refs(): don't lock or unlock the packed refsMichael Haggerty, Jun 23, 2017
  35. Jeff KingJun 23, 2017
  36. Michael HaggertyJul 1, 2017
  37. 27/29 commit_packed_refs(): remove call to `packed_refs_unlock()`Michael Haggerty, Jun 23, 2017
  38. Jeff KingJun 23, 2017
  39. 29/29 read_packed_refs(): die if `packed-refs` contains bogus dataMichael Haggerty, Jun 23, 2017
  40. Jeff KingJun 23, 2017
  41. Michael HaggertyJul 1, 2017
  42. 11/29 lock_packed_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  43. 10/29 add_packed_ref(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  44. 15/29 repack_without_refs(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  45. 18/29 packed_read_raw_ref(): new function, replacing `resolve_packed_ref()`Michael Haggerty, Jun 23, 2017
  46. 08/29 get_packed_ref_cache(): take a `packed_ref_store *` parameterMichael Haggerty, Jun 23, 2017
  47. Jeff KingJun 23, 2017
  48. Jeff KingJun 23, 2017
  49. Junio C HamanoJun 23, 2017
  50. Jeff KingJun 24, 2017
  51. Michael HaggertyJun 24, 2017
  52. Junio C HamanoJun 23, 2017

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.