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
Michael Haggerty <mhagger@alum.mit.edu>
Date
Feb 10, 2012, 14:51 UTC
Message-ID
<4F352F03.2030104@alum.mit.edu>
In-Reply-To
<4F158E99.2020906@alum.mit.edu>
On 01/17/2012 04:07 PM, Michael Haggerty wrote:
Show 32 quoted lines
> On 12/14/2011 12:24 AM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>> ...  But there are so many calls to the
>>> for_each_ref*() family of functions that I wasn't able to determine
>>> exactly which should allow for extra refs and which shouldn't.
>>
>> Only the ones that follow add_extra_ref() in the control flow.
>>
>> builtin/clone.c adds them in setup_reference() to deal with --reference.
>> The objects known to be complete in these repositories we borrow from
>> need to be marked complete on our end (i.e. clone does not have to fetch)
>> and transport_fetch_refs() that eventually goes to fetch_refs_via_pack()
>> that calls fetch_pack() uses this information. All three for_each_ref()
>> calls in builtin/fetch-pack.c are about "what are the objects that we know
>> are complete?" and needs to pay attention to extra refs.
>>
>> Having said that, I have a slight suspicion that you might be able to
>> eliminate this one in clone.  setup_reference() adds the reference
>> repository to the $GIT_DIR/objects/info/alternates, and the fetch logic
>> already knows to account for the refs in alternate repositories via
>> insert_alternate_refs() callchain.
> 
> If I comment out the call from add_one_reference() to add_extra_ref()
> then I get a single failure, in t5700:
> 
> not ok - 8 fetched no objects
> #	! grep "^want" "$U"
> 
> So your suspicion does not seem to be borne out (at least not in the
> naivest form).
> 
> Still studying...

I finally had some time to get back to this puzzle. It took me a while to narrow down the problem, and I still don't understand it entirely.

It seems like Junio should be right that setup_reference() doesn't need to add the alternate references to extra_refs. Indeed, if I remove the call to add_extra_ref(), I see the alternate references being added to ref_list via insert_one_alternate_ref(). However, in the context of t5700, clone nevertheless sends "want" lines for one of those references and test 8 fails. Something is screwy.

So how do the extra_refs prevent the "want" lines from being emitted? It turns out that when the alternate refs *are* added as extra_refs, then find_common() is not called at all. do_fetch_pack() calls everything_local(), which returns true, and do_fetch_pack() skips over the call to find_common().

So ISTM that there are two problems:

First problem: everything_local() seems to be either broken or used incorrectly. I can't decide which because I don't know what its semantics are *supposed* to be.

If everything_local() is trying to check that the references are all in the local repository itself, then it is incorrect for clone to enter alternates into extra_refs because everything_local() then mistakes them for local.

If everything_local() is trying to check that the references are in the local repository plus alternates, then it is incorrect that everything_local() doesn't consider alternate references in its determination. My guess is that this is the case, and that something like the following might be the fix:

Show 22 quoted lines
> ----------------------------- builtin/fetch-pack.c -----------------------------
> index 9500f35..4257a8d 100644
> @@ -581,6 +581,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
>  	*refs = newlist;
>  }
>  
> +static void mark_alternate_complete(const struct ref *ref, void *unused)
> +{
> +	mark_complete(NULL, ref->old_sha1, 0, NULL);
> +}
> +
>  static int everything_local(struct ref **refs, int nr_match, char **match)
>  {
>  	struct ref *ref;
> @@ -609,6 +614,7 @@ static int everything_local(struct ref **refs, int nr_match, char **match)
>  
>  	if (!args.depth) {
>  		for_each_ref(mark_complete, NULL);
> +		for_each_alternate_ref(mark_alternate_complete, NULL);
>  		if (cutoff)
>  			mark_recent_complete_commits(cutoff);
>  	}

With this patch, then the full test suite passes even if I take out the code that adds the alternate refs to extra_refs.

Second problem: it seems like the presence of alternate refs is not suppressing the "want" lines for those refs in all cases. Strangely, in the case of t5700, adding the two alternate refs to ref_list insert_one_alternate_ref() causes the "want" line for one of them to be suppressed, but not for the other.

Specifically: (without the above patch) I commented out the call to
add_extra_ref() in clone.c:add_one_reference(), then ran t5700 through
step 8 then aborted.  insert_one_alternate_ref() was called four times:

insert_one_alternate_ref(ccc25a1f9655742174c93f48f616bea8ad0bc6ff) insert_one_alternate_ref(ccc25a1f9655742174c93f48f616bea8ad0bc6ff) insert_one_alternate_ref(5355551c5a927a2b6349505ada2da4bb702c0a49) insert_one_alternate_ref(5355551c5a927a2b6349505ada2da4bb702c0a49)

(The duplication here seems strange.) However, UPLOAD_LOG still contains "want" lines (2 of them!) for one of the commits:

#S #E #S #S #E want 5355551c5a927a2b6349505ada2da4bb702c0a49 multi_ack_detailed side-band-64k thin-pack ofs-delta want 5355551c5a927a2b6349505ada2da4bb702c0a49 #E

The "5355551c" object corresponds to refs/remotes/origin/master in the alternate object store:

$ (cd B; git for-each-ref) ccc25a1f9655742174c93f48f616bea8ad0bc6ff commit refs/heads/master 5355551c5a927a2b6349505ada2da4bb702c0a49 commit refs/remotes/origin/HEAD 5355551c5a927a2b6349505ada2da4bb702c0a49 commit refs/remotes/origin/master

It seems to me that even in the absence of short-circuiting due to everything_local() returning true, the presence of the alternate refs should be suppressing the "want" lines for those references.

I have some patches that seem to work (and get rid of extra_refs entirely, hurrah!), but I don't want to submit them until I understand what the correct behavior is *supposed* to be.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Michael HaggertyNext: Jeff King
Message 51 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.