Changeset View
Standalone View
src/applications/config/controller/PhabricatorConfigConsoleController.php
| Show First 20 Lines • Show All 148 Lines • ▼ Show 20 Lines | private function loadVersions(PhabricatorUser $viewer) { | ||||
| $log_futures = array(); | $log_futures = array(); | ||||
| $remote_futures = array(); | $remote_futures = array(); | ||||
| foreach ($specs as $lib) { | foreach ($specs as $lib) { | ||||
| $root = dirname(phutil_get_library_root($lib)); | $root = dirname(phutil_get_library_root($lib)); | ||||
| $log_command = csprintf( | $log_command = csprintf( | ||||
| 'git log --format=%s -n 1 --', | 'git log --format=%s -n 1 --', | ||||
valerio.bozzolan: Note that the command is only about git log, so it have sense to have error logs mentioning… | |||||
| '%H %ct'); | '%H %ct'); | ||||
| $remote_command = csprintf( | $remote_command = csprintf( | ||||
| 'git remote -v'); | 'git remote -v'); | ||||
Done Inline ActionsNote that the command is only about git log, so it have sense to have error logs mentioning that fact valerio.bozzolan: Note that the command is only about git log, so it have sense to have error logs mentioning… | |||||
| $log_futures[$lib] = id(new ExecFuture('%C', $log_command)) | $log_futures[$lib] = id(new ExecFuture('%C', $log_command)) | ||||
| ->setCWD($root); | ->setCWD($root); | ||||
| $remote_futures[$lib] = id(new ExecFuture('%C', $remote_command)) | $remote_futures[$lib] = id(new ExecFuture('%C', $remote_command)) | ||||
| ->setCWD($root); | ->setCWD($root); | ||||
| } | } | ||||
| Show All 14 Lines | $upstream_pattern = | ||||
| )). | )). | ||||
| ')'; | ')'; | ||||
| $upstream_futures = array(); | $upstream_futures = array(); | ||||
| $lib_upstreams = array(); | $lib_upstreams = array(); | ||||
| foreach ($specs as $lib) { | foreach ($specs as $lib) { | ||||
| $remote_future = $remote_futures[$lib]; | $remote_future = $remote_futures[$lib]; | ||||
| list($err, $stdout) = $remote_future->resolve(); | try { | ||||
| if ($err) { | list($stdout, $err) = $remote_future->resolvex(); | ||||
| // If this fails for whatever reason, just move on. | } catch (CommandException $e) { | ||||
| $this->logGitErrorWithPotentialTips($e, $lib); | |||||
Not Done Inline Actionsredundant comment avivey: redundant comment | |||||
Not Done Inline Actionsthis comment is also redundant (and wrong) now. avivey: this comment is also redundant (and wrong) now. | |||||
| continue; | continue; | ||||
| } | } | ||||
| // These look like this, with a tab separating the first two fields: | // These look like this, with a tab separating the first two fields: | ||||
| // remote-name http://remote.uri/ (push) | // remote-name http://remote.uri/ (push) | ||||
| $upstreams = array(); | $upstreams = array(); | ||||
| ▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines | private function loadVersions(PhabricatorUser $viewer) { | ||||
| if ($upstream_futures) { | if ($upstream_futures) { | ||||
| id(new FutureIterator($upstream_futures)) | id(new FutureIterator($upstream_futures)) | ||||
| ->resolveAll(); | ->resolveAll(); | ||||
| } | } | ||||
| $results = array(); | $results = array(); | ||||
| foreach ($log_futures as $lib => $future) { | foreach ($log_futures as $lib => $future) { | ||||
| list($err, $stdout) = $future->resolve(); | try { | ||||
| if (!$err) { | list($stdout, $err) = $future->resolvex(); | ||||
| list($hash, $epoch) = explode(' ', $stdout); | list($hash, $epoch) = explode(' ', $stdout); | ||||
| } else { | } catch (CommandException $e) { | ||||
| $hash = null; | $hash = null; | ||||
| $epoch = null; | $epoch = null; | ||||
| $this->logGitErrorWithPotentialTips($e, $lib); | |||||
| } | } | ||||
Not Done Inline Actionsredundant comment avivey: redundant comment | |||||
| $result = array( | $result = array( | ||||
| 'hash' => $hash, | 'hash' => $hash, | ||||
| 'epoch' => $epoch, | 'epoch' => $epoch, | ||||
| 'upstream' => null, | 'upstream' => null, | ||||
| 'branchpoint' => null, | 'branchpoint' => null, | ||||
| ); | ); | ||||
| $upstream_future = idx($upstream_futures, $lib); | $upstream_future = idx($upstream_futures, $lib); | ||||
| if ($upstream_future) { | if ($upstream_future) { | ||||
| list($err, $stdout) = $upstream_future->resolve(); | list($stdout, $err) = $upstream_future->resolvex(); | ||||
| if (!$err) { | if (!$err) { | ||||
| $branchpoint = trim($stdout); | $branchpoint = trim($stdout); | ||||
| if (strlen($branchpoint)) { | if (strlen($branchpoint)) { | ||||
| // We only list a branchpoint if it differs from HEAD. | // We only list a branchpoint if it differs from HEAD. | ||||
| if ($branchpoint != $hash) { | if ($branchpoint != $hash) { | ||||
| $result['upstream'] = $lib_upstreams[$lib]; | $result['upstream'] = $lib_upstreams[$lib]; | ||||
| $result['branchpoint'] = trim($stdout); | $result['branchpoint'] = trim($stdout); | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 48 Lines • ▼ Show 20 Lines | $table_view = id(new AphrontTableView($rows)) | ||||
| )); | )); | ||||
| return id(new PHUIObjectBoxView()) | return id(new PHUIObjectBoxView()) | ||||
| ->setHeaderText(pht('Other Version Information')) | ->setHeaderText(pht('Other Version Information')) | ||||
| ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) | ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) | ||||
| ->appendChild($table_view); | ->appendChild($table_view); | ||||
| } | } | ||||
| /** | |||||
| * Help in better troubleshooting git errors. | |||||
| * @param CommandException $e Exception | |||||
| * @param string $lib Library name involved | |||||
| */ | |||||
| private function logGitErrorWithPotentialTips($e, $lib) { | |||||
Not Done Inline ActionsWe generally prefer non-static methods wherever possible - just a style thing. avivey: We generally prefer non-static methods wherever possible - just a style thing. | |||||
| // First, detect this specific error message related to [safe] stuff. | |||||
| $expected_error_msg_part = 'detected dubious ownership in repository'; | |||||
speckUnsubmitted Not Done Inline ActionsDoes git return translated error messages? Is there an identifiable error code for this error we could check instead? speck: Does git return translated error messages? Is there an identifiable error code for this error… | |||||
valerio.bozzolanAuthorUnsubmitted Done Inline Actions@speck Nice question! Yep it supports i18n but premising that ExecFuture does not expose any environment variable AFAIK, so git does not have any way to detect what the desired language should be (LANG, LC_ALL and other things). So it seems to me it should fallback to the default, English. It's interesting because that is another good reason to do not expose all traditional environment variables. Also, it seems there is not any specific error code for this. It exits with status 128 that is just the boring "unexpected error" class. valerio.bozzolan: @speck Nice question! Yep it supports i18n but premising that `ExecFuture` does not expose any… | |||||
| $stderr = $e->getStderr(); | |||||
| if (strpos($stderr, $expected_error_msg_part) !== false) { | |||||
| // Found! Let's show a nice resolution tip. | |||||
| // Complete path of the problematic repository. | |||||
| $lib_root = dirname(phutil_get_library_root($lib)); | |||||
| phlog(pht( | |||||
| "Cannot identify the version of the %s repository because ". | |||||
| "the webserver does not trust it (more info on Task %s).\n". | |||||
| "Try this system resolution:\n". | |||||
| "sudo git config --system --add safe.directory %s", | |||||
Done Inline ActionsThis is one of the rare places where I think it's find to use sudo in the docs (and suggest system-wide changes):
however, we should keep in mind that "3rd party extensions" also go through this path, and our trust in those is limited (they still have lots of access just because they are running in the server process though). avivey: This is one of the rare places where I think it's find to use `sudo` in the docs (and suggest… | |||||
Not Done Inline ActionsEvan did call out that logs are largely intended for us during development and not something for end users. Instead catching this invalid state ahead of time and reporting it in the interface would be a better option, though I’m not sure there’s a dedicated “setup issues” type of page for specific repos, but maybe it’s appropriate? speck: Evan did call out that logs are largely intended for us during development and not something… | |||||
Done Inline ActionsIt would also probably make sense to show the error in the /config/ interface, but I have not been able to do it intelligently. Nope, a setup check is not available at the moment. Yep, I agree a setup check would be useful to proactively help to fix this. valerio.bozzolan: It would also probably make sense to show the error in the /config/ interface, but I have not… | |||||
| $lib, | |||||
| 'https://we.phorge.it/T15282', | |||||
| $lib_root)); | |||||
| } else { | |||||
| // Otherwise show a generic error message | |||||
| phlog($e); | |||||
| } | |||||
| } | |||||
| } | } | ||||
Note that the command is only about git log, so it have sense to have error logs mentioning that fact