OK. Full immersion mode 🌈

🔴 After this change, it's true that we skip that error, but we then introduce a new behavior: we redirect to a different commit (its parent). By itself, showing a different commit may be problematic; moreover, this may lead to new errors.

I don't know next steps here 🤔 I will just share how I arrived at this conclusion...

Preamble: I was able to create a borked repository in this way: create a new one, activate, push a long history, and then play with start/stop on phd. So, on your borked repo, just surf the default branch.

I started visiting a file at a specific commit (1911f82d0e25e0321548e497d01dca08dd3a482c):

http://phorge.localhost/diffusion/2/browse/master/gradle.properties;1911f82d0e25e0321548e497d01dca08dd3a482c

Then, I tried the Skip Past This Commit link. Note the destination commit is 0c361f3621e6. This is a problematic commit.

If I click, this is the destination:

http://phorge.localhost/diffusion/2/browse/master/gradle.properties;1911f82d0e25e0321548e497d01dca08dd3a482c?before=0c361f3621e67631aecc4ef11c7511b15a40d749

Before This Change

Before this patch, while visiting 0c361f3621e6 I remain here:

http://phorge.localhost/diffusion/2/browse/master/gradle.properties;1911f82d0e25e0321548e497d01dca08dd3a482c?before=0c361f3621e67631aecc4ef11c7511b15a40d749

EXCEPTION: (RuntimeException) reset() expects parameter 1 to be array, null given at [<arcanist>/src/error/PhutilErrorHandler.php:273]
  #0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer, array) called at [<arcanist>/src/error/PhutilErrorHandler.php:273]
  #1 <#2> reset(NULL) called at [<phorge>/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php:100]

After this Change

After this patch, while visiting 0c361f3621e6, instead, I'm redirected to its parent commit (?), that is:

http://phorge.localhost/diffusion/2/browse/master/gradle.properties;d177bbec48bd6dd1d87b48e3d418b2bdcd3f225f

At this point, I may see new confusing errors, like this one:

Command failed with error #128!
COMMAND
git cat-file -t -- d177bbec48bd6dd1d87b48e3d418b2bdcd3f225f:<the file i was trying to visit>
STDOUT
(empty)

STDERR
fatal: path '<the file i was trying to visit>' does not exist in 'd177bbec48bd6dd1d87b48e3d418b2bdcd3f225f'

Expected Behavior

After this patch, while visiting 0c361f3621e6, probably we should see something like this (like, an error message telling something like "Import Problem"):

The screenshot happens while visiting commits like:

http://phorge.localhost/diffusion/2/browse/master/gradle.properties;0c361f3621e67631aecc4ef11c7511b15a40d749?follow=created

This is just an indication about the possibility to show custom error messages for import issues.

Proposed error message:

Unable to continue tracing the history of this file because this commit was not imported correctly.