Server error on user-specific PR diff and a pruned commit

[zakjan]

Description

This is an edge case happening with the following conditions:

• an open PR with an assigned reviewer
• the reviewer interacted with the PR, Gitea stores user details about the interaction linked to a particular commit
• the author force-pushed to the PR
• git GC has run, causing a commit from the time of the interaction to be pruned

Observed behavior:

• the reviewer who interacted with the PR can't open PR Files Changed tab
• all other users can open it fine

Steps to replicate:

• In terminal
  • Run Gitea with basic docker-compose from https://docs.gitea.io/en-us/install-with-docker/#basics
• In browser
  • Install Gitea with SQLite database, admin user author
• In terminal
  • Shutdown Gitea
  • Configure git GC to prune unreachable objects immediately, append to gitea/conf/app.ini:

  [git]
  GC_ARGS = "--prune=now"
  

  • Run Gitea again
• In browser, logged as author
  • Add SSH key
  • Create repo repo with initialized main branch, user reviewer, add him to repo collaborators
• In terminal
  • Clone repo
  • Checkout branch feature, edit a file, commit, push
• In browser, logged as author
  • Create a PR for feature branch, assign reviewer as reviewer
• In browser, logged as reviewer
  • Open the PR Files Changed tab
  • Mark the changed file as Viewed
• In terminal
  • Edit the file, amend the first commit, force-push
• In browser, logged as author
  • Site Administration > Maintenance Operations > Garbage collect all repositories > Run
  • Confirm that the first commit was pruned - opening it returns 404, this is expected
• In browser, logged as reviewer
  • Open the PR, Files Changed tab - opening returns 500
  • The following error is logged:

  gitea  | 2022/10/07 10:15:40 ...ers/web/repo/pull.go:715:ViewPullFiles() [E] [633fe02b] SyncAndGetUserSpecificDiff: exit status 128 - fatal: Invalid revision range 722a5b2dcdb1560ecfa0306c2492d1c5c7fd2a4e..47703d0b1e66ddf52827ff9596dc80f4de860089
  gitea  | 	 - fatal: Invalid revision range 722a5b2dcdb1560ecfa0306c2492d1c5c7fd2a4e..47703d0b1e66ddf52827ff9596dc80f4de860089
  

  • 722a5b… is the pruned commit, 47703d… is the amended commit

Expected behavior:

• PR Files Changed tab can be opened by anyone

Workaround:

• close the PR and reopen as a new PR

Gitea Version

1.17.2

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

https://gist.github.com/zakjan/3e3c3574ac9df73d2e2bf0e717cd685d

Screenshots

Git Version

2.36.2

Operating System

macOS

How are you running Gitea?

Docker

Database

SQLite

[delvh]

Ok, yeah, I have a pretty good guess of what's going wrong:

services/gitdiff/gitdiff.go:1239:   changedFiles, err := gitRepo.GetFilesChangedBetween(review.CommitSHA, latestCommit)

probably throws this error simply because Git is asked to display the changes between the two commits, and can't find the first one.
The solution will be to catch the error and assume all files changed (makes the most sense, since it can't find the first commit, so you don't know what the original state was).
Does our implementation throw a specific error for that case, or do I have to compare error messages?

[zakjan]

How about to pre-emptively check if review.CommitSHA exists here?

gitea/services/gitdiff/gitdiff.go

Lines 1227 to 1230 in b59b0ca

	review, err := pull_model.GetNewestReviewState(ctx, userID, pull.ID)
	if err != nil || review == nil || review.UpdatedFiles == nil {
	return diff, err
	}

Then we'd not need to check for a specific error later.

[delvh]

The problem is: That's impossible*.
All we know is the current commit SHA when you made your review, which is simply a string.
We don't care/ know if it points to an actual commit, or if the SHA is an explicit Not a commit SHA and even more text. Lorem Ipsum… for some reason.
It is simply passed down to git, and git then complains.
The only way to check it beforehand would be to explicitly ask git if that commit exists, which is way overkill as that will spawn a complete process before returning a simple boolean.

[zakjan]

Right, I understand there is an effort to limit calls to git. I'm not sure what kind of git errors does Gitea recognize, so that it would need to differentiate between them. Possibly all errors from the user-specific diff could be catched, so that opening a user-independent diff works, and all files are considered as changed?