Re: [Outreachy PATCH] range-diff: use the repository from the diff options

["Pablo Sabater" <pabloosabaterr@gmail.com>]

On Wed Oct 28, 2026 at 9:51 PM WET, Okhuomon Ajayi wrote:

Hi!

I think that the title, "range-diff: use the repository from the diff
options" can be confusing for someone that doesn't know that diffopt has
a repo field (as it happened to me when reading this).

Also, when talking about variables in our code, using their names makes
it easier to know which code is referred to.

Does this work for you as title?:

range-diff: use diffopt->repo instead of the_repository

> output_pair_header() and patch_diff() already receive the diff
> options, but use the_repository and the_hash_algo. Use the

This first sentence reads a bit strange. But apart from that, I miss a
reason about why we want this [1].

> repository from diffopt instead, and pass only the hash algorithm
> to get_filespec(), as that is all it needs.

"and pass only" reads as if now get_filespec() only receives the hash
algo, but it receives the hash algo on top of what it was already
receiving.

Also, saying that this is related to the_repository replacement because
the_hash_algo comes from the_repository would be nice.

>
> read_patches(), output() and is_range_diff_range() still use
> the_repository, so USE_THE_REPOSITORY_VARIABLE stays.

Nice to know. But I think that it would read better [2] if you used the
verb at the start, like:

Keep USE_THE_REPOSITORY_VARIABLE because read_patches(), output() and
is_range_diff_range() still use the_repository.

I recommend reading Documentation/SubmittingPatches for this kind of
things. I left at the bottom two links.

>
> Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail.com>
> ---
>  range-diff.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index 8e2dd2eb19..3f56a4f71a 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -418,7 +418,7 @@ static void output_pair_header(struct diff_options *diffopt,
>  
>  	if (!dashes->len)
>  		strbuf_addchars(dashes, '-',
> -				strlen(repo_find_unique_abbrev(the_repository, oid, abbrev)));
> +				strlen(repo_find_unique_abbrev(diffopt->repo, oid, abbrev)));
>  
>  	if (!b_util) {
>  		color = color_old;
> @@ -440,7 +440,7 @@ static void output_pair_header(struct diff_options *diffopt,
>  		strbuf_addf(buf, "%*s:  %s ", patch_no_width, "-", dashes->buf);
>  	else
>  		strbuf_addf(buf, "%*d:  %s ", patch_no_width, a_util->i + 1,
> -			    repo_find_unique_abbrev(the_repository, &a_util->oid, abbrev));
> +			    repo_find_unique_abbrev(diffopt->repo, &a_util->oid, abbrev));
>  
>  	if (status == '!')
>  		strbuf_addf(buf, "%s%s", color_reset, color);
> @@ -452,9 +452,9 @@ static void output_pair_header(struct diff_options *diffopt,
>  		strbuf_addf(buf, " %*s:  %s", patch_no_width, "-", dashes->buf);
>  	else
>  		strbuf_addf(buf, " %*d:  %s", patch_no_width, b_util->i + 1,
> -			    repo_find_unique_abbrev(the_repository, &b_util->oid, abbrev));
> +			    repo_find_unique_abbrev(diffopt->repo, &b_util->oid, abbrev));
>  
> -	commit = lookup_commit_reference(the_repository, oid);
> +	commit = lookup_commit_reference(diffopt->repo, oid);
>  	if (commit) {
>  		if (status == '!')
>  			strbuf_addf(buf, "%s%s", color_reset, color);
> @@ -474,11 +474,12 @@ static struct userdiff_driver section_headers = {
>  	},
>  };
>  
> -static struct diff_filespec *get_filespec(const char *name, const char *p)
> +static struct diff_filespec *get_filespec(const struct git_hash_algo *algop,
> +					  const char *name, const char *p)
>  {
>  	struct diff_filespec *spec = alloc_filespec(name);
>  
> -	fill_filespec(spec, null_oid(the_hash_algo), 0, 0100644);
> +	fill_filespec(spec, null_oid(algop), 0, 0100644);
>  	spec->data = (char *)p;
>  	spec->size = strlen(p);
>  	spec->should_munmap = 0;
> @@ -492,7 +493,8 @@ static void patch_diff(const char *a, const char *b,
>  		       struct diff_options *diffopt)
>  {
>  	diff_queue(&diff_queued_diff,
> -		   get_filespec("a", a), get_filespec("b", b));
> +		   get_filespec(diffopt->repo->hash_algo, "a", a),
> +		   get_filespec(diffopt->repo->hash_algo, "b", b));
>  
>  	diffcore_std(diffopt);
>  	diff_flush(diffopt);
>
> base-commit: 8d1b215c01512b9658d4525c54ff9b9245f9a7e8

The code looks good to me.
Passes make DEVELOPER=1 and git clang-format --diff HEAD~

[1]: https://git-scm.com/docs/SubmittingPatches#meaningful-message
[2]: https://git-scm.com/docs/SubmittingPatches#imperative-mood

Good job,
Pablo