Paths

Table of Contentst

Differential D25738

Avoid RuntimeException on "Skip past this commit" when commit still importing
Needs ReviewPublic
Actions

Authored by aklapper on Jul 22 2024, 15:25.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15888: RuntimeException using "Skip past this commit" to access a commit still importing: "reset() expects parameter 1 to be array, null given"

Summary

For whatever reasons, imports of commits in Diffusion can get stuck, displaying Still Importing... This commit is still importing. Changes will be visible once the import finishes.

Using the "Skip past this commit" button in a file in Diffusion trying to access the past of such a non-imported commit, Phabricator throws a RuntimeException.

Avoid throwing the RuntimeException by handling the situation more gracefully: When the SQL query in loadCommitSequence() does not return a result, $path_change is null. Thus do not crash trying to reset($path_change) but return null, and handle null accordingly in loadOldFilename() which is the only caller of loadCommitSequence().

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]

Closes T15888

Test Plan

Unknown. Ideally, find a problematic commit still importing in Diffusion and try to access its past via "Skip past this commit" from a future revision of a file affected by the problematic commit. Practically, read the code in the class DiffusionRenameHistoryQuery.

Diff Detail

Repository

rP Phorge

Branch

gitBlameResetException

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1511
Build 1511: arc lint + arc unit

Event Timeline

aklapper created this revision.Jul 22 2024, 15:25

Owners added a reviewer: O1: Blessed Committers.Jul 22 2024, 15:25

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJul 22 2024, 15:25

aklapper requested review of this revision.Jul 22 2024, 15:25

Harbormaster completed remote builds in B1448: Diff 2260.Jul 22 2024, 15:25

aklapper edited the test plan for this revision. (Show Details)Jul 22 2024, 15:26

valerio.bozzolan added inline comments.Aug 9 2024, 06:41

src/applications/diffusion/query/DiffusionRenameHistoryQuery.php
111–112	With an early return null, we can change less lines (Non-blocking tip. I still need to reproduce and test)

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.

This revision now requires changes to proceed.Aug 9 2024, 09:59

Thanks for finding a way to reproduce! I still fail to find the right timing here to produce a testcase, so this is another untested shot in the dark.

Idea is to pass null up from DiffusionRenameHistoryQuery into DiffusionBrowseController and in this case show a similar error as the already existing ones. Regarding tinkering with the return values of these two methods, there are no other calls to these methods.

Harbormaster completed remote builds in B1511: Diff 2351.Aug 11 2024, 09:35

aklapper marked an inline comment as done.Aug 11 2024, 09:37

Stumbled upon https://we.phorge.it/rP95e179d9a4f31b8c5c2cbb4db8f7fa9f2d3867d6 which also mentions