Re: [PATCH 12/12] hash: stop depending on `the_repository` in `null_oid()`
- From
Justin Tobler <jltobler@gmail.com>
- Date
- Mar 4, 2025, 20:16 UTC
- Message-ID
- <yl7t44tp5zhygppxrusdpiehjydncv3b6bmxhp2hgzk6xjwvbr@cjpzvpwo4vxk>
- In-Reply-To
- <20250303-b4-pks-objects-without-the-repository-v1-12-c5dd43f2476e@pks.im>
On 25/03/03 09:47AM, Patrick Steinhardt wrote:
Show 36 quoted lines
> The `null_oid()` function returns the object ID that only consists of > zeroes. Naturally, this ID also depends on the hash algorithm used, as > the number of zeroes is different between SHA1 and SHA256. Consequently, > the function returns the hash-algorithm-specific null object ID. > > This is currently done by depending on `the_hash_algo`, which implicitly > makes us depend on `the_repository`. Refactor the function to instead > pass in the hash algorithm for which we want to retrieve the null object > ID. Adapt callsites accordingly by passing in `the_repository`, thus > bubbling up the dependency on that global variable by one layer. > > There are a couple of trivial exceptions for subsystems that already got > rid of `the_repository`. These subsystems instead use the repository > that is available via the calling context: > > - "builtin/grep.c" > - "grep.c" > - "refs/debug.c" > > There is also a single non-trivial exception with "diff-no-index.c". > Here we know that we may not have a repository initialized at all, so we > cannot rely on `the_repository`. Instead, we adapt `diff_no_index()` to > get a `struct git_hash_algo` as parameter. The only caller is located in > "builtin/diff.c", where we know to call `repo_set_hash_algo()` in case > we're running outside of a Git repository. Consequently, it is fine to > continue passing `the_repository->hash_algo` even in this case. > > This means that we could in theory just not bother about this edge case > at all and just use `the_repository` in "diff-no-index.c". But doing so > would feel misdesigned. > > Remove the `USE_THE_REPOSITORY_VARIABLE` preprocessor define in > "hash.c". > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > ---
[snip]
Show 5 quoted lines
> -int diff_no_index(struct rev_info *revs, > - int implicit_no_index, > - int argc, const char **argv) > +int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop, > + int implicit_no_index, int argc, const char **argv)
As mentioned in the commit message, diff_no_index() may not have a repository initialized, so the git_hash_algo should be explicitly passed.
Makes sense.
Show 12 quoted lines
> {
> int i, no_index;
> int ret = 1;
> @@ -354,7 +354,7 @@ int diff_no_index(struct rev_info *revs,
> setup_diff_pager(&revs->diffopt);
> revs->diffopt.flags.exit_with_status = 1;
>
> - if (queue_diff(&revs->diffopt, paths[0], paths[1], 0))
> + if (queue_diff(&revs->diffopt, algop, paths[0], paths[1], 0))
> goto out;
> diff_set_mnemonic_prefix(&revs->diffopt, "1/", "2/");
> diffcore_std(&revs->diffopt);[snip]
The other changes in this patch largely consist of just updating null_oid() and its call sites to recieve the git_hash_algo explicitly.
Looks good to me.
-Justin