From 8aa0cafbd30f1d2df149e32b22b581a5d86afdeb Mon Sep 17 00:00:00 2001 From: Bryce Kwon Date: Sat, 31 Oct 2026 14:11:56 -0700 Subject: Look up a request's commit in one place The diff page now names a revision that is no commit the way the request wrote it, as the other pages already did, rather than as the full id it resolved to. The helpers sit beside the error page in ui-shared.c, since shared.c prints nothing. --- source/ui-blame.c | 10 ++-------- source/ui-commit.c | 10 ++-------- source/ui-diff.c | 26 ++++++-------------------- source/ui-patch.c | 39 ++------------------------------------- source/ui-shared.c | 27 +++++++++++++++++++++++++++ source/ui-shared.h | 14 ++++++++++++++ source/ui-snapshot.c | 8 +------- source/ui-tree.c | 10 ++-------- tests/t0107-diff.sh | 10 ++++++++++ 9 files changed, 66 insertions(+), 88 deletions(-) diff --git a/source/ui-blame.c b/source/ui-blame.c index 2e281cf..28fd16f 100644 --- a/source/ui-blame.c +++ b/source/ui-blame.c @@ -388,15 +388,9 @@ void cgit_print_blame(void) if (!rev) rev = ctx.qry.head; - if (repo_get_oid(the_repository, rev, &oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", rev); + commit = cgit_get_commit(rev, &oid); + if (!commit) return; - } - commit = lookup_commit_reference(the_repository, &oid); - if (!commit || repo_parse_commit(the_repository, commit)) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", rev); - return; - } walk.rev = rev; walk.match_baselen = path_items.match ? basedir_len(path_items.match) : -1; diff --git a/source/ui-commit.c b/source/ui-commit.c index 9b2f8b9..c290a2c 100644 --- a/source/ui-commit.c +++ b/source/ui-commit.c @@ -251,15 +251,9 @@ void cgit_print_commit(char *hex, const char *prefix) if (!hex) hex = ctx.qry.head; - if (repo_get_oid(the_repository, hex, &oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", hex); + commit = cgit_get_commit(hex, &oid); + if (!commit) return; - } - commit = lookup_commit_reference(the_repository, &oid); - if (!commit) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", hex); - return; - } info = cgit_parse_commit(commit); format_display_notes(&commit->object.oid, ¬es, PAGE_ENCODING, 1); diff --git a/source/ui-diff.c b/source/ui-diff.c index 736cdaf..51df73d 100644 --- a/source/ui-diff.c +++ b/source/ui-diff.c @@ -604,31 +604,17 @@ void cgit_print_diff(const char *new_rev, const char *old_rev, const char *prefi if (!new_rev) new_rev = ctx.qry.head; - if (repo_get_oid(the_repository, new_rev, new_rev_oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", new_rev); + new_commit = cgit_get_commit(new_rev, new_rev_oid); + if (!new_commit || cgit_get_old_oid(old_rev, new_commit, old_rev_oid)) return; - } - new_commit = lookup_commit_reference(the_repository, new_rev_oid); - if (!new_commit || repo_parse_commit(the_repository, new_commit)) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", oid_to_hex(new_rev_oid)); - return; - } new_tree_oid = get_commit_tree_oid(new_commit); - if (old_rev) { - if (repo_get_oid(the_repository, old_rev, old_rev_oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", old_rev); - return; - } - } else if (new_commit->parents && new_commit->parents->item) { - oidcpy(old_rev_oid, &new_commit->parents->item->object.oid); - } else { - oidclr(old_rev_oid, the_repository->hash_algo); - } - if (!is_null_oid(old_rev_oid)) { + // A named old side is a commit already, so only a first parent + // missing from the repository fails here, and it has no name + // but its id. old_commit = lookup_commit_reference(the_repository, old_rev_oid); - if (!old_commit || repo_parse_commit(the_repository, old_commit)) { + if (!old_commit) { cgit_print_error_page(404, "Not Found", "Not a commit: %s", oid_to_hex(old_rev_oid)); return; } diff --git a/source/ui-patch.c b/source/ui-patch.c index 9cf61c0..763776a 100644 --- a/source/ui-patch.c +++ b/source/ui-patch.c @@ -16,42 +16,6 @@ // Room for two hex object ids, the two dots between them, and the null byte. #define REV_RANGE_LEN (2 * GIT_MAX_HEXSZ + 3) -/* - * A root commit has no parent, so the old end is left null and the caller asks - * for that one commit instead. - */ -static int resolve_range(const char *new_rev, const char *old_rev, - struct object_id *new_oid, struct object_id *old_oid) -{ - struct commit *commit; - - if (repo_get_oid(the_repository, new_rev, new_oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", new_rev); - return -1; - } - commit = lookup_commit_reference(the_repository, new_oid); - if (!commit) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", new_rev); - return -1; - } - - if (old_rev) { - if (repo_get_oid(the_repository, old_rev, old_oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", old_rev); - return -1; - } - if (!lookup_commit_reference(the_repository, old_oid)) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", old_rev); - return -1; - } - } else if (commit->parents && commit->parents->item) { - oidcpy(old_oid, &commit->parents->item->object.oid); - } else { - oidclr(old_oid, the_repository->hash_algo); - } - return 0; -} - void cgit_print_patch(const char *new_rev, const char *old_rev, const char *prefix) { struct rev_info rev; @@ -71,7 +35,8 @@ void cgit_print_patch(const char *new_rev, const char *old_rev, const char *pref return; } - if (resolve_range(new_rev, old_rev, &new_oid, &old_oid)) + commit = cgit_get_commit(new_rev, &new_oid); + if (!commit || cgit_get_old_oid(old_rev, commit, &old_oid)) return; if (is_null_oid(&old_oid)) { diff --git a/source/ui-shared.c b/source/ui-shared.c index 5f5dcca..b34861d 100644 --- a/source/ui-shared.c +++ b/source/ui-shared.c @@ -1499,6 +1499,33 @@ void cgit_print_blob_too_large(size_t size) size / 1024, ctx.cfg.max_blob_size); } +struct commit *cgit_get_commit(const char *rev, struct object_id *oid) +{ + struct commit *commit; + + if (repo_get_oid(the_repository, rev, oid)) { + cgit_print_error_page(404, "Not Found", "Bad object id: %s", rev); + return NULL; + } + // lookup_commit_reference parses the commit, and fails on one git + // cannot parse. + commit = lookup_commit_reference(the_repository, oid); + if (!commit) + cgit_print_error_page(404, "Not Found", "Not a commit: %s", rev); + return commit; +} + +int cgit_get_old_oid(const char *old_rev, const struct commit *commit, struct object_id *oid) +{ + if (old_rev) + return cgit_get_commit(old_rev, oid) ? 0 : -1; + if (commit->parents && commit->parents->item) + oidcpy(oid, &commit->parents->item->object.oid); + else + oidclr(oid, the_repository->hash_algo); + return 0; +} + static void add_clone_urls(void (*fn)(const char *), char *urls, char *suffix) { struct strbuf **url_list = strbuf_split_str(urls, ' ', 0); diff --git a/source/ui-shared.h b/source/ui-shared.h index fb02d7b..871dba0 100644 --- a/source/ui-shared.h +++ b/source/ui-shared.h @@ -121,6 +121,20 @@ extern void cgit_print_error_page(int code, const char *msg, const char *fmt, .. * Answer with the error page for a blob of size bytes past max-blob-size. */ extern void cgit_print_blob_too_large(size_t size); + +/* + * Resolve rev to the parsed commit it names, peeling a tag, with oid set to + * what rev itself names. Returns NULL once the 404 page naming rev is out. + */ +extern struct commit *cgit_get_commit(const char *rev, struct object_id *oid); + +/* + * Fill oid with the older end of a comparison with commit, which is what + * old_rev names when it is given, else commit's first parent, else the null + * id for a root commit. Returns -1 once the 404 page for old_rev is out. + */ +extern int cgit_get_old_oid(const char *old_rev, const struct commit *commit, struct object_id *oid); + extern void cgit_print_filemode(unsigned short mode); extern void cgit_print_snapshot_links(const struct cgit_repo *repo, const char *ref, const char *separator); diff --git a/source/ui-snapshot.c b/source/ui-snapshot.c index 2fa121f..333bce6 100644 --- a/source/ui-snapshot.c +++ b/source/ui-snapshot.c @@ -224,14 +224,8 @@ static void send_snapshot(const struct cgit_snapshot_format *format, const char { struct object_id oid; - if (repo_get_oid(the_repository, hex, &oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", hex); + if (!cgit_get_commit(hex, &oid)) return; - } - if (!lookup_commit_reference(the_repository, &oid)) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", hex); - return; - } // A compressor that cannot run only shows up as a broken pipe once // the tar is under way, so it is looked for while a status can still // say so. diff --git a/source/ui-tree.c b/source/ui-tree.c index 1c29e30..300207f 100644 --- a/source/ui-tree.c +++ b/source/ui-tree.c @@ -486,15 +486,9 @@ void cgit_print_tree(const char *rev, char *path) if (!rev) rev = ctx.qry.head; - if (repo_get_oid(the_repository, rev, &oid)) { - cgit_print_error_page(404, "Not Found", "Bad object id: %s", rev); + commit = cgit_get_commit(rev, &oid); + if (!commit) return; - } - commit = lookup_commit_reference(the_repository, &oid); - if (!commit || repo_parse_commit(the_repository, commit)) { - cgit_print_error_page(404, "Not Found", "Not a commit: %s", rev); - return; - } walk.rev = rev; diff --git a/tests/t0107-diff.sh b/tests/t0107-diff.sh index f058246..96e37f5 100755 --- a/tests/t0107-diff.sh +++ b/tests/t0107-diff.sh @@ -22,6 +22,16 @@ test_expect_success 'find added line' ' grep "
+5
" tmp ' +# A revision that names no commit is quoted the way the request wrote it, as +# the commit and patch pages quote it, on either side of the diff. +test_expect_success 'a tree on either side is named as the request gave it' ' + tree=$(git -C repos/foo rev-parse --short HEAD^{tree}) && + cgit_query "url=foo/diff/&id=$tree" >tmp && + grep "
Not a commit: $tree
" tmp && + cgit_query "url=foo/diff/&id2=$tree" >tmp && + grep "
Not a commit: $tree
" tmp +' + # git writes a hunk length only when the hunk spans more than one line, so the # header for a single line reads "@@ -1 +1 @@" with no comma in it. Requiring # that comma left those hunks numbered from zero, which dropped the line -- cgit