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

Re: [PATCH v2 08/51] is_dup_ref(): extract function from sort_ref_array()

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Dec 13, 2011, 04:35 UTC
Message-ID
<4EE6D61A.2030300@alum.mit.edu>
In-Reply-To
<7vvcplxq4r.fsf@alter.siamese.dyndns.org>
On 12/12/2011 11:33 PM, Junio C Hamano wrote:
Show 24 quoted lines
> mhagger@alum.mit.edu writes:
> 
>> +/*
>> + * Emit a warning and return true iff ref1 and ref2 have the same name
>> + * and the same sha1.  Die if they have the same name but different
>> + * sha1s.
>> + */
>> +static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2)
>> +{
>> +	if (!strcmp(ref1->name, ref2->name)) {
>> +		/* Duplicate name; make sure that the SHA1s match: */
>> +		if (hashcmp(ref1->sha1, ref2->sha1))
>> +			die("Duplicated ref, and SHA1s don't match: %s",
>> +			    ref1->name);
>> +		warning("Duplicated ref: %s", ref1->name);
>> +		return 1;
>> +	} else {
>> +		return 0;
>> +	}
>> +}
> 
> At this step, is_dup_ref() is only called from sort_ref_array() which in
> turn is only called on either a collection of loose or packed refs, so
> warning is warranted. IOW I do not see anything wrong with _this_ patch.
I agree.
> Later in the series, however, the collection of extra refs seems to be
> shoved into a phoney "direntry" and made to go through the same add_ref()
> machinery that uses find_containing_direntry() which in turn calls
> search_ref_dir() that smells like a definite no-no.
Correct.

I had remembered your explanation of the purpose of extra refs and also that they have unusual names, but I hadn't allowed for the possibility that multiple extra_refs can have the same name. The old code never sorted the extra refs and therefore never checked or eliminated duplicates. The new code does, as sorting is an intrinsic part of looking up references and reference subdirectories in the hierarchical cache. It is introduced along with the meat of the change in

    [PATCH v2 29/51] refs: store references hierarchically

So...the new code would die if two extra refs have the same name but different SHA1s, and emit a warning if they have the same name and the same SHA1s. (However, it is no problem for an extra ref to have the same name as a packed or loose ref.)

Is it *really* possible to have multiple extra_refs with the same name, or is this more of a hypothetical problem that requires more careful analysis?

If the former, then I will have to do some work. My approach would probably be to allow sorting without de-duplication; that way extra_refs can continue to share most of the code used for the other ref caches. It would be relatively easy to add such a fix on top of the patch series, where every ref_entry knows the ref_cache with which it is associated. It would be quite a bit more painful to massage such a fix through the whole patch series.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Junio C HamanoNext: Michael Haggerty
Message 15 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.