Re: [Outreachy PATCH] range-diff: use the repository from the diff options
flat view
From: Pablo Sabater <hidden>
Date: 2026-10-29 01:02:47
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Okhuomon Ajayi <redacted> --- 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