From: Phillip Wood Date: Thu, 13 Jun 2024 10:01:35 GMT Subject: Re: [PATCH v2 08/20] hash: require hash algorithm in `empty_tree_oid_hex()` Message-ID: <66763b5a-3ea6-481f-b4d3-5fad76f5da0c@gmail.com> In-Reply-To: <4858cca25fe9e57c984fc3181fe8498d0b7222b0.1718259125.git.ps@pks.im> On 13/06/2024 07:14, Patrick Steinhardt wrote: > The `empty_tree_oid_hex()` function use `the_repository` to derive the > hash function that shall be used. Require callers to pass in the hash > algorithm to get rid of this implicit dependency. Many of these call sites already have a repository instance available so don't need to use "the_repository". I haven't checked but with these changes it might be possible to remove some of these files from the next patch. I've only really looked at this patch in this series as I was just checking for changes to the sequencer code. As for the series as a whole I think adding USE_THE_REPOSITORY_VARIABLE is a good direction. > While at it, remove the unused `empty_blob_oid_hex()` function. > > Signed-off-by: Patrick Steinhardt > --- > add-interactive.c | 2 +- > add-patch.c | 2 +- > builtin/merge.c | 3 ++- > builtin/receive-pack.c | 2 +- > hash-ll.h | 3 +-- > object-file.c | 10 ++-------- > sequencer.c | 2 +- > submodule.c | 6 +++--- > wt-status.c | 4 ++-- > 9 files changed, 14 insertions(+), 20 deletions(-) > > diff --git a/add-interactive.c b/add-interactive.c > index b5d6cd689a..a0961096cd 100644 > --- a/add-interactive.c > +++ b/add-interactive.c > @@ -557,7 +557,7 @@ static int get_modified_files(struct repository *r, > s.skip_unseen = filter && i; > > opt.def = is_initial ? > - empty_tree_oid_hex() : oid_to_hex(&head_oid); > + empty_tree_oid_hex(the_repository->hash_algo) : oid_to_hex(&head_oid); The hunk fragment shows that we already have a struct repository instance in this function which we should use instead of "the_repository" > diff --git a/add-patch.c b/add-patch.c > index 814de57c4a..86181770f2 100644 > --- a/add-patch.c > +++ b/add-patch.c > @@ -420,7 +420,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) > /* could be on an unborn branch */ > !strcmp("HEAD", s->revision) && > repo_get_oid(the_repository, "HEAD", &oid) ? > - empty_tree_oid_hex() : s->revision); > + empty_tree_oid_hex(the_repository->hash_algo) : s->revision); It's not obvious from this hunk but there is a repository instance in "s->s->r" which we should use instead of "the_repository" > diff --git a/sequencer.c b/sequencer.c > index 68d62a12ff..823691e379 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2263,7 +2263,7 @@ static int do_pick_commit(struct repository *r, > unborn = 1; > } else if (unborn) > oidcpy(&head, the_hash_algo->empty_tree); > - if (index_differs_from(r, unborn ? empty_tree_oid_hex() : "HEAD", > + if (index_differs_from(r, unborn ? empty_tree_oid_hex(the_repository->hash_algo) : "HEAD", The hunk fragment shows that we already have a struct repository instance in "r" which we should use instead of "the_repository" here. > diff --git a/wt-status.c b/wt-status.c > index ff4be071ca..5051f5e599 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -641,7 +641,7 @@ static void wt_status_collect_changes_index(struct wt_status *s) > > repo_init_revisions(s->repo, &rev, NULL); > memset(&opt, 0, sizeof(opt)); > - opt.def = s->is_initial ? empty_tree_oid_hex() : s->reference; > + opt.def = s->is_initial ? empty_tree_oid_hex(the_repository->hash_algo) : s->reference; The context line above shows us that we have a repository instance available so we should use "s->repo" instead of "the_repository" > @@ -1136,7 +1136,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s) > rev.diffopt.ita_invisible_in_index = 1; > > memset(&opt, 0, sizeof(opt)); > - opt.def = s->is_initial ? empty_tree_oid_hex() : s->reference; > + opt.def = s->is_initial ? empty_tree_oid_hex(the_repository->hash_algo) : s->reference; We should use "s->repo" here as well Best Wishes Phillip