Hacktoberfest 2026: the issues maintainers tagged for October, open and beginner-friendly. Browse Hacktoberfest issues

Dynamic Review: an incremental review with an ancestor as previous commit becomes a full review

Open
  • Is something wrong?
#306 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
35/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
git, typescript
Domain
backend

Research direction

Start in the handler that catches ObjectUnavailable from tracked_tree(previous) and sets previous = None, then follow the previous_reviewed_sha not in store.objects check that skips the fetch after the filter tree:0 ancestry walk. Reproduce with the history base -> c1 -> c2 and an incremental claim, using the integration tests that run against real upload-pack. Done means the ancestor case reports comparison == "incremental" with the right previous_diff, while the refused-server, unknown-commit and sibling cases keep their current behaviour.

Written by the indexing model from the issue text.

Description

area: runner type: bug

Found while reviewing PR #300 (review finding F4). Pre-existing: it is not caused by #296.

Problem

An incremental review whose previous reviewed commit is an ancestor of the head, the usual case when new commits are pushed on top, silently becomes a full review. The commits-only ancestry walk (filter tree:0) already stores the previous commit, so if previous_reviewed_sha not in store.objects skips the fetch. tracked_tree(previous) then finds no tree, raises ObjectUnavailable, and the handler turns that into previous = None: comparison is full and previous_diff is empty.

Reproduced on the code before and after #296 with the history base -> c1 -> c2, previous = c1, base branch = base, claim incremental. The existing incremental tests only use a previous commit that is not an ancestor, so none of them covers this.

Design

  • Decide whether the previous commit's tree is present, not whether the commit is: fetch the previous commit when its tree is absent (a depth-1 fetch with the size filter), then read the tree.
  • Keep the existing fallback (previous commit explicitly refused by the server → full mode with the prior state kept).
  • Add a test with a linear history.

Test cases

ID kind level case expected
T01 regression integration (real upload-pack) previous = ancestor of head comparison == "incremental", previous_diff holds the change since the previous commit; red before the fix
T02 regression integration previous = sibling of head unchanged: incremental
T03 behavior integration previous commit refused by the server full mode, prior state kept
T04 behavior integration previous commit unknown to the repository full mode
T05 behavior unit (checkout) the tree of the previous commit is absent but the commit is present the fetch happens once
Dominant language
TypeScript
Stars
0
Forks
1
Avg merge
1h 18m
Merged PRs (30d)
186

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from ExpidetGames/code-reviewer

All issues in ExpidetGames/code-reviewer

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.