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

Re: [PATCH 08/41] packfile: abstract away hash constant values

From
brian m. carlson <sandals@crustytoothpaste.net>
Date
May 2, 2018, 00:11 UTC
Message-ID
<20180502001140.GH13217@genre.crustytoothpaste.net>
In-Reply-To
<20180501102243.GE15820@duynguyen.home>
On Tue, May 01, 2018 at 12:22:43PM +0200, Duy Nguyen wrote:
Show 9 quoted lines
> On Mon, Apr 23, 2018 at 11:39:18PM +0000, brian m. carlson wrote:
> > There are several instances of the constant 20 and 20-based values in
> > the packfile code.  Abstract away dependence on SHA-1 by using the
> > values from the_hash_algo instead.
> 
> While we're abstracting away 20. There's the only 20 left in
> sha1_file.c that should also be gone. But I guess you could do that
> later since you need to rename fill_sha1_path to
> fill_loose_object_path or something.
I'm already working on knocking those out.
Show 22 quoted lines
> > @@ -507,15 +509,15 @@ static int open_packed_git_1(struct packed_git *p)
> >  			     " while index indicates %"PRIu32" objects",
> >  			     p->pack_name, ntohl(hdr.hdr_entries),
> >  			     p->num_objects);
> > -	if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
> > +	if (lseek(p->pack_fd, p->pack_size - hashsz, SEEK_SET) == -1)
> >  		return error("end of packfile %s is unavailable", p->pack_name);
> > -	read_result = read_in_full(p->pack_fd, sha1, sizeof(sha1));
> > +	read_result = read_in_full(p->pack_fd, hash, hashsz);
> >  	if (read_result < 0)
> >  		return error_errno("error reading from %s", p->pack_name);
> > -	if (read_result != sizeof(sha1))
> > +	if (read_result != hashsz)
> >  		return error("packfile %s signature is unavailable", p->pack_name);
> > -	idx_sha1 = ((unsigned char *)p->index_data) + p->index_size - 40;
> > -	if (hashcmp(sha1, idx_sha1))
> > +	idx_hash = ((unsigned char *)p->index_data) + p->index_size - hashsz * 2;
> > +	if (hashcmp(hash, idx_hash))
> 
> Since the hash size is abstracted away, shouldn't this hashcmp become
> oidcmp? (which still does not do the right thing, but at least it's
> one less place to worry about)

Unfortunately, I can't, because it's not an object ID. I think the decision was made to not transform non-object ID hashes into struct object_id, which makes sense. I suppose we could have an equivalent struct hash or something for those other uses.

Show 12 quoted lines
> Same comment for other hashcmp in this patch.
> 
> > @@ -675,7 +677,8 @@ struct packed_git *add_packed_git(const char *path, size_t path_len, int local)
> >  	p->pack_size = st.st_size;
> >  	p->pack_local = local;
> >  	p->mtime = st.st_mtime;
> > -	if (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))
> > +	if (path_len < the_hash_algo->hexsz ||
> > +	    get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->sha1))
> 
> get_sha1_hex looks out of place when we start going with
> the_hash_algo. Maybe change to get_oid_hex() too.

I believe this is the pack hash, which isn't an object ID. I will transform it to be called something other than "sha1" and allocate more memory for it in a future series, though.

Show 8 quoted lines
> > @@ -1678,10 +1683,10 @@ int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32
> >  
> >  	index_lookup = index_fanout + 4 * 256;
> >  	if (p->index_version == 1) {
> > -		index_lookup_width = 24;
> > +		index_lookup_width = hashsz + 4;
> 
> You did good research to spot this 24 constant ;-)
Thanks.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Previous: Duy NguyenNext: Duy Nguyen
Message 71 of 76 in “object_id part 13”
  1. 00/41 object_id part 13brian m. carlson, Apr 23, 2018
  2. 01/41 cache: add a function to read an object ID from a bufferbrian m. carlson, Apr 23, 2018
  3. Martin ÅgrenApr 24, 2018
  4. Duy NguyenMay 1, 2018
  5. brian m. carlsonMay 1, 2018
  6. 02/41 server-info: remove unused members from struct pack_infobrian m. carlson, Apr 23, 2018
  7. Martin ÅgrenApr 24, 2018
  8. Duy NguyenMay 1, 2018
  9. 03/41 Remove unused member in struct object_contextbrian m. carlson, Apr 23, 2018
  10. Duy NguyenMay 1, 2018
  11. 04/41 packfile: remove unused member from struct pack_entrybrian m. carlson, Apr 23, 2018
  12. Duy NguyenMay 1, 2018
  13. 06/41 sha1_file: convert freshen functions to object_idbrian m. carlson, Apr 23, 2018
  14. 07/41 packfile: convert find_pack_entry to object_idbrian m. carlson, Apr 23, 2018
  15. 09/41 pack-objects: abstract away hash algorithmbrian m. carlson, Apr 23, 2018
  16. Duy NguyenMay 1, 2018
  17. 17/41 pack-redundant: convert linked lists to use struct object_idbrian m. carlson, Apr 23, 2018
  18. 16/41 Update struct index_state to use struct object_idbrian m. carlson, Apr 23, 2018
  19. 22/41 revision: replace use of hard-coded constantsbrian m. carlson, Apr 23, 2018
  20. 21/41 http: eliminate hard-coded constantsbrian m. carlson, Apr 23, 2018
  21. Martin ÅgrenApr 24, 2018
  22. Junio C HamanoApr 24, 2018
  23. brian m. carlsonApr 25, 2018
  24. 20/41 dir: convert struct untracked_cache_dir to object_idbrian m. carlson, Apr 23, 2018
  25. 25/41 builtin/receive-pack: avoid hard-coded constants for push certsbrian m. carlson, Apr 23, 2018
  26. Martin ÅgrenApr 24, 2018
  27. brian m. carlsonApr 25, 2018
  28. Martin ÅgrenApr 25, 2018
  29. 28/41 merge: convert empty tree constant to the_hash_algobrian m. carlson, Apr 23, 2018
  30. 36/41 sequencer: use the_hash_algo for empty tree object IDbrian m. carlson, Apr 23, 2018
  31. 34/41 sha1_file: convert cached object code to struct object_idbrian m. carlson, Apr 23, 2018
  32. 30/41 submodule: convert several uses of EMPTY_TREE_SHA1_HEXbrian m. carlson, Apr 23, 2018
  33. 40/41 add--interactive: compute the empty tree valuebrian m. carlson, Apr 23, 2018
  34. 39/41 Update shell scripts to compute empty tree object IDbrian m. carlson, Apr 23, 2018
  35. Duy NguyenMay 1, 2018
  36. brian m. carlsonMay 4, 2018
  37. 38/41 sha1_file: only expose empty object constants through git_hash_algobrian m. carlson, Apr 23, 2018
  38. 37/41 dir: use the_hash_algo for empty blob object IDbrian m. carlson, Apr 23, 2018
  39. 41/41 merge-one-file: compute empty blob object IDbrian m. carlson, Apr 23, 2018
  40. SZEDER GáborApr 24, 2018
  41. brian m. carlsonApr 24, 2018
  42. 35/41 cache-tree: use is_empty_tree_oidbrian m. carlson, Apr 23, 2018
  43. 33/41 builtin/reset: convert use of EMPTY_TREE_SHA1_BINbrian m. carlson, Apr 23, 2018
  44. 31/41 wt-status: convert two uses of EMPTY_TREE_SHA1_HEXbrian m. carlson, Apr 23, 2018
  45. Martin ÅgrenApr 24, 2018
  46. brian m. carlsonMay 1, 2018
  47. 32/41 builtin/receive-pack: convert one use of EMPTY_TREE_SHA1_HEXbrian m. carlson, Apr 23, 2018
  48. 29/41 sequencer: convert one use of EMPTY_TREE_SHA1_HEXbrian m. carlson, Apr 23, 2018
  49. 27/41 builtin/merge: switch tree functions to use object_idbrian m. carlson, Apr 23, 2018
  50. 23/41 upload-pack: replace use of several hard-coded constantsbrian m. carlson, Apr 23, 2018
  51. Simon RuderichApr 24, 2018
  52. 26/41 builtin/am: convert uses of EMPTY_TREE_SHA1_BIN to the_hash_algobrian m. carlson, Apr 23, 2018
  53. 13/41 fsck: convert static functions to struct object_idbrian m. carlson, Apr 23, 2018
  54. 24/41 diff: specify abbreviation size in terms of the_hash_algobrian m. carlson, Apr 23, 2018
  55. 19/41 commit: convert uses of get_sha1_hex to get_oid_hexbrian m. carlson, Apr 23, 2018
  56. 05/41 packfile: convert has_sha1_pack to object_idbrian m. carlson, Apr 23, 2018
  57. 18/41 index-pack: abstract away hash function constantbrian m. carlson, Apr 23, 2018
  58. Martin ÅgrenApr 24, 2018
  59. brian m. carlsonApr 24, 2018
  60. Martin ÅgrenApr 25, 2018
  61. Duy NguyenApr 26, 2018
  62. brian m. carlsonApr 27, 2018
  63. Duy NguyenApr 28, 2018
  64. 15/41 split-index: convert struct split_index to object_idbrian m. carlson, Apr 23, 2018
  65. 14/41 submodule-config: convert structures to object_idbrian m. carlson, Apr 23, 2018
  66. 12/41 tree-walk: convert get_tree_entry_follow_symlinks to object_idbrian m. carlson, Apr 23, 2018
  67. 11/41 tree-walk: avoid hard-coded 20 constantbrian m. carlson, Apr 23, 2018
  68. 10/41 pack-redundant: abstract away hash algorithmbrian m. carlson, Apr 23, 2018
  69. 08/41 packfile: abstract away hash constant valuesbrian m. carlson, Apr 23, 2018
  70. Duy NguyenMay 1, 2018
  71. brian m. carlsonMay 2, 2018
  72. Duy NguyenMay 2, 2018
  73. brian m. carlsonMay 2, 2018
  74. Duy NguyenApr 30, 2018
  75. brian m. carlsonApr 30, 2018
  76. Duy NguyenMay 1, 2018

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.