Permanent Import Failure on a commit that removes a .gitignore'd .DS_Store


#1

Hello. I ran into a bug where a commit could not be imported.

Observed Behavior:

  • A user on our self-hosted phabricator instance has committed a .DS_Store file to a diff
  • An astute reviewer asked to add .DS_Store files to .gitignore and accepted the Diff
  • The user updated the .gitignore as requested, removed the .DS_Store files and landed the revision right away without updating the diff
  • After that, in phabricator, the commit was stuck at the Importing state, and the job responsible for importing this commit was failing continuously

Following the troubleshooting guide, I got the following when running ./bin/repository reparse --importing --trace <commit id>:

[2018-06-01 07:29:16] EXCEPTION: (CommandException) Command failed with error #128!
COMMAND
git cat-file blob '232906cde731b718ac5ac510f53bd627b1451971':'src/python/.DS_Store'

STDOUT
(empty)

STDERR
fatal: Not a valid object name 232906cde731b718ac5ac510f53bd627b1451971:src/python/.DS_Store
 at [<phutil>/src/future/exec/ExecFuture.php:369]
arcanist(head=master, ref.master=73f5afd44110), phabricator(head=master, ref.master=37a03402bc44), phutil(head=master, ref.master=1ad42491e44a)
  #0 ExecFuture::resolvex() called at [<phutil>/src/filesystem/linesofalarge/LinesOfALargeExecFuture.php:108]
  #1 LinesOfALargeExecFuture::readMore() called at [<phutil>/src/filesystem/linesofalarge/LinesOfALarge.php:199]
  #2 LinesOfALarge::next() called at [<phutil>/src/filesystem/linesofalarge/LinesOfALarge.php:111]
  #3 LinesOfALarge::rewind() called at [<phabricator>/src/applications/files/uploadsource/PhabricatorFileUploadSource.php:103]
  #4 PhabricatorFileUploadSource::readFileData() called at [<phabricator>/src/applications/files/uploadsource/PhabricatorFileUploadSource.php:146]
  #5 PhabricatorFileUploadSource::shouldChunkFile() called at [<phabricator>/src/applications/files/uploadsource/PhabricatorFileUploadSource.php:75]
  #6 PhabricatorFileUploadSource::uploadFile() called at [<phabricator>/src/applications/diffusion/query/DiffusionFileFutureQuery.php:114]
  #7 DiffusionFileFutureQuery::executeQuery() called at [<phabricator>/src/applications/diffusion/query/DiffusionQuery.php:96]
  #8 DiffusionQuery::execute() called at [<phabricator>/src/applications/diffusion/query/DiffusionFileFutureQuery.php:60]
  #9 DiffusionFileFutureQuery::respondToConduitRequest(ConduitAPIRequest) called at [<phabricator>/src/applications/diffusion/conduit/DiffusionFileContentQueryConduitAPIMethod.php:29]
  #10 DiffusionFileContentQueryConduitAPIMethod::getResult(ConduitAPIRequest) called at [<phabricator>/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php:158]
  #11 DiffusionQueryConduitAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:123]
  #12 ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
  #13 ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
  #14 ConduitCall::execute() called at [<phabricator>/src/applications/diffusion/query/DiffusionQuery.php:82]
  #15 DiffusionQuery::callConduitWithDiffusionRequest(PhabricatorUser, DiffusionGitRequest, string, array) called at [<phabricator>/src/applications/differential/engine/DifferentialDiffExtractionEngine.php:187]
  #16 DifferentialDiffExtractionEngine::isDiffChangedBeforeCommit(PhabricatorRepositoryCommit, DifferentialDiff, DifferentialDiff) called at [<phabricator>/src/applications/differential/engine/DifferentialDiffExtractionEngine.php:255]
  #17 DifferentialDiffExtractionEngine::updateRevisionWithCommit(DifferentialRevision, PhabricatorRepositoryCommit, array, PhabricatorDaemonContentSource) called at [<phabricator>/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:247]
  #18 PhabricatorRepositoryCommitMessageParserWorker::updateCommitData(DiffusionCommitRef) called at [<phabricator>/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php:42]
  #19 PhabricatorRepositoryCommitMessageParserWorker::parseCommit(PhabricatorRepository, PhabricatorRepositoryCommit) called at [<phabricator>/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php:51]
  #20 PhabricatorRepositoryCommitParserWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:123]
  #21 PhabricatorWorker::executeTask() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementReparseWorkflow.php:325]
  #22 PhabricatorRepositoryManagementReparseWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:441]
  #23 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:333]
  #24 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/repository/manage_repositories.php:22]

We are using redpointgame’s phabricator Docker image (https://github.com/RedpointGames/phabricator). I updated to the latest version, and it did not help.

Workaround
We were able to work around the issue in the following way:

  • reopen the diff (it was closed manually following the failed import and failed auto-close)
  • checkout the repo to the parent of the faulty commit
  • arc patch the diff
  • remove the .DS_Store file
  • update the diff

At this point the commit was imported successfully, which seems to hint at a problem in the import logic when a binary file exists in a diff but not in the related commit, or vice-versa.

Expected Behavior:
Import the commit successfully and close the revision

Phabricator Version:

  • phabricator ee32c186dd822b90826514ff8fc61740ebc8bd68 (May 1 2018)
  • arcanist a604548101025875de20a9c263df3790fea425b3 (Apr 27 2018)
  • phutil 20eff1c8d14f08f05ef72828fa379e871d29662c (Apr 13 2018)

Reproduction Steps:
Follow the same steps as our user:

  • Create a diff with a new .DS_Store file (seen as binary), Accept the revision
  • Remove said file
  • land the revision

I’m not sure about the reproduceability but still want to put this out there in the open in case someone else runs into this issue.

I’m more than happy to provide more details if required.

Kudos to the team, you are doing a wonderful job!