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

Re: [PATCH v2 28/51] refs.c: rename ref_array -> ref_dir

From
Junio C Hamano <gitster@pobox.com>
Date
Dec 13, 2011, 00:45 UTC
Message-ID
<7v7h21xps9.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1323668338-1764-29-git-send-email-mhagger@alum.mit.edu>
mhagger@alum.mit.edu writes:
Show 27 quoted lines
> From: Michael Haggerty <mhagger@alum.mit.edu>
>
> This purely textual change is in preparation for storing references
> hierarchically, when the old ref_array structure will represent one
> "directory" of references.  Rename functions that deal with this
> structure analogously, and also rename the structure's "refs" member
> to "entries".
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c |  166 ++++++++++++++++++++++++++++++++--------------------------------
>  1 files changed, 83 insertions(+), 83 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index fe6d657..b74ef80 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -106,9 +106,9 @@ struct ref_value {
>  	unsigned char peeled[20];
>  };
>  
> -struct ref_array {
> +struct ref_dir {
>  	int nr, alloc;
> -	struct ref_entry **refs;
> +	struct ref_entry **entries;
>  };

The s/refs/entries/ renaming is a sane thing to do; on the other hand, I somehow find the s/ref_array/ref_dir/ renaming is a short-sighted change and undesirable, as you are essentially declaring that "if you use this structure, the contents you store there are expected to be named hierarchically", forbidding users that want to use a simple flat array.

BUT. That was an observation before I continued reading the remainder of the series.

I think the above observation primarily come from my worries around the extra ref stuff, which by nature does not fit well with the hierarchical naming (they do not even have any meaningful names). Sorting or requiring uniqueness among them do not make any sense, let alone cutting their names in hierarchical boundaries.

As an alternative, it _might_ make sense to get rid of "add_extra_ref()" API from refs.c and make it the responsibility for its users to add their extra anchor points where they use for_each_ref() to find out all anchor points in the history from the refs.c subsystem. If we go that route, I fully agree that "s/ref_array/ref_dir/" renaming is the right thing to do, as refs.c subsystem will _only_ handle the hierarchical ref namespace and nothing else after such a change.

A bit of background refresher.

"git clone --reference=../another-repo.git $origin" is a typical user of the add_extra_ref() API. It runs an equivalent of ls-remote against the reference repository, and uses add_extra_ref() to cause its later call to for_each_ref() to return the names of the objects at the tips of refs in that reference repository.

During the object transfer (i.e. the equivalent of "git fetch") from the origin, "clone" and "upload-pack" negotiate what objects need to be sent, and this is done by the receiving end telling what it has to the sending end, and the sending end uses this information to subtract what the receiving end claims to already have from what is going to be sent.

And the enumeration of what the receiving end, especially when there is no "reference", is done using for_each_ref(), as object transfer considers everything reachable from the refs complete. add_extra_ref() is a _hack_ to cause for_each_ref() to include objects that actually do _not_ sit at the tip of any of our refs.

When cloning, we do not have any ref, and after clone is done, we do not want the refs the repository we are borrowing its objects from to be our ref (after all, we may be using a local copy of linux-3.0 repository as our reference but cloning from linux-2.4 history), and that is why this hack was invented (in the old scripted version, we tentatively created real refs in our ref namespace and removed them after clone finished).

In the case of "clone", these extra refs are registered with names taken from the repository we are borrowing from, so they may be unique and without conflict among them if you are borrowing from one repository, but if you borrow from more than one repositories, it is likely that all of them have "refs/heads/master" and there is no reason to expect that the extra refs added with add_extra_ref() API have unique names. Worse, in "receive-pack" that runs on the receiving end when you "git push" your history, "refs" that exist in repositories that the receiving repository is borrowing from (i.e. it has $GIT_DIR/objects/info/alternates) are advertised with ".have" in their names (not using anything refs/* is to avoid the pusher from mistakenly think there is such a ref that is eligible for "matching push" logic), and these ".have" entries obviously are not unique. The code also uses the same add_extra_ref() hack to cause for_each_ref() to report them.

The removal of this hack, taking receive-pack as an example, would be to stop using add_extra_ref() but instead to keep a local list of object names that the code currently uses add_extra_ref() to keep track of, and then modify write_head_info() to feed show_ref_cb() with these object names in addition to the current for_each_ref() callback.

It may make the resulting code cleaner if we go that route, and if we were to do so, I think the right place to do so in the series would be either at the very beginning of the series (as part of the preliminary clean-up), or immediately before "do_for_each_ref: correctly terminate while processing extra_refs" (making that commit unnecessary, as after such a fix, refs.c API won't have to worry about the extra_refs hack anymore).

What do you think?
Previous: mhagger@alum.mit.eduNext: Michael Haggerty
Message 39 of 79 in “ref-api-C and ref-api-D re-roll”
  1. 00/51 ref-api-C and ref-api-D re-rollmhagger@alum.mit.edu, Dec 12, 2011
  2. 01/51 struct ref_entry: document name membermhagger@alum.mit.edu, Dec 12, 2011
  3. 02/51 refs: rename "refname" variablesmhagger@alum.mit.edu, Dec 12, 2011
  4. Junio C HamanoDec 13, 2011
  5. 03/51 refs: rename parameters result -> sha1mhagger@alum.mit.edu, Dec 12, 2011
  6. 04/51 clear_ref_array(): rename from free_ref_array()mhagger@alum.mit.edu, Dec 12, 2011
  7. 05/51 is_refname_available(): remove the "quiet" argumentmhagger@alum.mit.edu, Dec 12, 2011
  8. 06/51 parse_ref_line(): add docstringmhagger@alum.mit.edu, Dec 12, 2011
  9. 07/51 add_ref(): add docstringmhagger@alum.mit.edu, Dec 12, 2011
  10. 08/51 is_dup_ref(): extract function from sort_ref_array()mhagger@alum.mit.edu, Dec 12, 2011
  11. Jeff KingDec 12, 2011
  12. Michael HaggertyDec 12, 2011
  13. Junio C HamanoDec 12, 2011
  14. Junio C HamanoDec 12, 2011
  15. Michael HaggertyDec 13, 2011
  16. Michael HaggertyDec 13, 2011
  17. 09/51 refs: change signatures of get_packed_refs() and get_loose_refs()mhagger@alum.mit.edu, Dec 12, 2011
  18. 10/51 get_ref_dir(): change signaturemhagger@alum.mit.edu, Dec 12, 2011
  19. 11/51 resolve_gitlink_ref(): improve docstringmhagger@alum.mit.edu, Dec 12, 2011
  20. 12/51 Pass a (ref_cache *) to the resolve_gitlink_*() helper functionsmhagger@alum.mit.edu, Dec 12, 2011
  21. 13/51 resolve_gitlink_ref_recursive(): change to work with struct ref_cachemhagger@alum.mit.edu, Dec 12, 2011
  22. 14/51 repack_without_ref(): remove temporarymhagger@alum.mit.edu, Dec 12, 2011
  23. 15/51 create_ref_entry(): extract function from add_ref()mhagger@alum.mit.edu, Dec 12, 2011
  24. 16/51 add_ref(): take a (struct ref_entry *) parametermhagger@alum.mit.edu, Dec 12, 2011
  25. 17/51 do_for_each_ref(): correctly terminate while processesing extra_refsmhagger@alum.mit.edu, Dec 12, 2011
  26. Junio C HamanoDec 12, 2011
  27. 18/51 do_for_each_ref_in_array(): new functionmhagger@alum.mit.edu, Dec 12, 2011
  28. 19/51 do_for_each_ref_in_arrays(): new functionmhagger@alum.mit.edu, Dec 12, 2011
  29. 20/51 repack_without_ref(): reimplement using do_for_each_ref_in_array()mhagger@alum.mit.edu, Dec 12, 2011
  30. Junio C HamanoDec 12, 2011
  31. 21/51 names_conflict(): new function, extracted from is_refname_available()mhagger@alum.mit.edu, Dec 12, 2011
  32. 22/51 names_conflict(): simplify implementationmhagger@alum.mit.edu, Dec 12, 2011
  33. 23/51 is_refname_available(): reimplement using do_for_each_ref_in_array()mhagger@alum.mit.edu, Dec 12, 2011
  34. 24/51 refs.c: reorder definitions more logicallymhagger@alum.mit.edu, Dec 12, 2011
  35. 25/51 free_ref_entry(): new functionmhagger@alum.mit.edu, Dec 12, 2011
  36. 26/51 check_refname_component(): return 0 for zero-length componentsmhagger@alum.mit.edu, Dec 12, 2011
  37. 27/51 struct ref_entry: nest the value part in a unionmhagger@alum.mit.edu, Dec 12, 2011
  38. 28/51 refs.c: rename ref_array -> ref_dirmhagger@alum.mit.edu, Dec 12, 2011
  39. Junio C HamanoDec 13, 2011
  40. Michael HaggertyDec 13, 2011
  41. Junio C HamanoDec 13, 2011
  42. Michael HaggertyDec 13, 2011
  43. Junio C HamanoDec 13, 2011
  44. Michael HaggertyDec 13, 2011
  45. Junio C HamanoDec 13, 2011
  46. Junio C HamanoDec 14, 2011
  47. Jeff KingDec 14, 2011
  48. Michael HaggertyDec 15, 2011
  49. Jeff KingDec 15, 2011
  50. Michael HaggertyJan 17, 2012
  51. Michael HaggertyFeb 10, 2012
  52. Jeff KingFeb 10, 2012
  53. Junio C HamanoFeb 10, 2012
  54. Michael HaggertyFeb 11, 2012
  55. 29/51 refs: store references hierarchicallymhagger@alum.mit.edu, Dec 12, 2011
  56. 30/51 sort_ref_dir(): do not sort if already sortedmhagger@alum.mit.edu, Dec 12, 2011
  57. Junio C HamanoDec 12, 2011
  58. 31/51 refs: sort ref_dirs lazilymhagger@alum.mit.edu, Dec 12, 2011
  59. 32/51 do_for_each_ref(): only iterate over the subtree that was requestedmhagger@alum.mit.edu, Dec 12, 2011
  60. 33/51 get_ref_dir(): keep track of the current ref_dirmhagger@alum.mit.edu, Dec 12, 2011
  61. 34/51 refs: wrap top-level ref_dirs in ref_entriesmhagger@alum.mit.edu, Dec 12, 2011
  62. 35/51 get_packed_refs(): return (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  63. 36/51 get_loose_refs(): return (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  64. 37/51 is_refname_available(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  65. 38/51 find_ref(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  66. 39/51 read_packed_refs(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  67. 40/51 add_ref(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  68. 41/51 find_containing_direntry(): use (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  69. 42/51 search_ref_dir(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  70. 43/51 add_entry(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  71. 44/51 do_for_each_ref_in_dir*(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  72. 45/51 sort_ref_dir(): take (ref_entry *) instead of (ref_dir *)mhagger@alum.mit.edu, Dec 12, 2011
  73. 46/51 struct ref_dir: store a reference to the enclosing ref_cachemhagger@alum.mit.edu, Dec 12, 2011
  74. 47/51 read_loose_refs(): take a (ref_entry *) as argumentmhagger@alum.mit.edu, Dec 12, 2011
  75. 48/51 refs: read loose references lazilymhagger@alum.mit.edu, Dec 12, 2011
  76. 49/51 is_refname_available(): query only possibly-conflicting referencesmhagger@alum.mit.edu, Dec 12, 2011
  77. 50/51 read_packed_refs(): keep track of the directory being worked inmhagger@alum.mit.edu, Dec 12, 2011
  78. 51/51 repack_without_ref(): call clear_packed_ref_cache()mhagger@alum.mit.edu, Dec 12, 2011
  79. Junio C HamanoDec 12, 2011

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.