Don't error when trying to mirror or observe an empty repository
ClosedPublic
Actions

Authored by epriestley on Jan 24 2018, 3:24 AM.

Details

Reviewers

alexmv
amckinley

Maniphest Tasks

T5965: Mirroring an empty repository causes errors in the daemon log every minute.

Commits

rPd28ddc21a550: Don't error when trying to mirror or observe an empty repository

Summary

Fixes T5965.

Fixes two issues:

Observing an empty repository could write a warning to the log.
Mirroring an empty repository to a remote could fail.

For observing:

If newly-created with git init --bare, git ls-remote will
return the empty string. Properly return an empty set of refs, rather
than attempting to parse the single "line" that is produced by
splitting that on newlines:

[2018-01-23 18:47:00] ERROR 8: Undefined offset: 1 at [/phab_path/phabricator/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:405]
arcanist(head=master, ref.master=5634f8410176), phabricator(head=master, ref.master=12551a1055ce), phutil(head=master, ref.master=4755785517cf)
  #0 PhabricatorRepositoryPullEngine::loadGitRemoteRefs(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:343]
  #1 PhabricatorRepositoryPullEngine::executeGitUpdate() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:126]
  #2 PhabricatorRepositoryPullEngine::pullRepositoryWithLock() called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php:40]
  #3 PhabricatorRepositoryPullEngine::pullRepository() called at [<phabricator>/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php:59]
  ...

For mirroring:

git treats git push --mirror specially when a repository is empty. Detect this case by seeing if git for-each-ref --count 1 does anything. If the repository is empty, just bail.

Test Plan

Observed an empty and non-empty repository.
Mirrored an empty and non-empty repository.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

alexmv created this revision.Jan 24 2018, 3:24 AM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJan 24 2018, 3:24 AM

Herald added a subscriber: epriestley. · View Herald Transcript

Harbormaster completed remote builds in B19164: Diff 45381.Jan 24 2018, 3:25 AM

alexmv requested review of this revision.Jan 24 2018, 3:25 AM

I suspect this does not work. I think the command probably returns a single newline -- not the empty string -- and that the strlen() test fails because $stdout actually has length 1?

The behavior of phutil_split_lines() here is sort of problematic, and I think we've hit very similar issues elsewhere. If the input is "" or "\n", it returns array(""). I think the behavior is reasonably correct from a pure string handling point of view, but doesn't make sense if we know that the input is command output.

A possible fix might be to introduce phutil_split_command_lines() (ideally with some cleaner name) which (a) always discards newlines without requiring a flag and (b) returns array() when a command produces no output or a single newline. It would otherwise work like phutil_split_lines().

Alternatively, test strlen(rtrim($stdout)).

(If it's easier for you to just have me grab this and drag it the rest of the way, let me know -- I can test it pretty easily and you may not be able to in your environment. But I don't want to steal your internet points.)

This revision now requires changes to proceed.Jan 24 2018, 4:07 PM

Herald added a subscriber: Korvin. · View Herald TranscriptJan 24 2018, 4:07 PM

reasonably correct from a pure string handling point of view

Well, I think "\n" -> array("") is pretty reasonable. "" -> array("") is kind of goofy and maybe we could change the behavior of phutil_split_lines() in that case.

Please commandeer -- I don't have a trivial way to test this at current.

Pray be chivalrous, and ignore the quiet sobbing into my handkerchief, broken-hearted, over the internet points that I shall thus not glean.

This is also probably the same as T5965, although that's old and describes a different error. Getting a clean test case here should resolve that, in any case.

epriestley planned changes to this revision.Jan 24 2018, 8:29 PM

epriestley added a task: T5965: Mirroring an empty repository causes errors in the daemon log every minute. .

Oh, you're observing an empty repository here, not "mirroring" one in the particular sense that we use the term in Diffusion. So this isn't especially related to T5965, although maybe I'll fix that too since I went through the work to set it up.