Page MenuHomePhabricator

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

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

Details

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

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

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.

epriestley edited reviewers, added: alexmv; removed: epriestley.

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.

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.

Ah! Yes, sorry for not using the right term.

epriestley retitled this revision from Don't error when trying to mirror an empty repository to Don't error when trying to mirror or observe an empty repository.
epriestley edited the summary of this revision. (Show Details)
epriestley edited the test plan for this revision. (Show Details)
epriestley edited reviewers, added: amckinley; removed: Blessed Reviewers.
  • Fix both mirroring and observing; make some attempt to test changes.
This revision is now accepted and ready to land.Jan 24 2018, 11:49 PM
This revision was automatically updated to reflect the committed changes.