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'); | ||||
valerio.bozzolanAuthorUnsubmitted 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(); | list($err, $stdout, $stderr) = $remote_future->resolve(); | ||||
| if ($err) { | if ($err) { | ||||
| // Very probably this is a "detected dubious ownership" | |||||
| // Better to report it, than just ignoring it. | |||||
| // https://we.phorge.it/T15282 | |||||
Not Done Inline Actionsredundant comment avivey: redundant comment | |||||
| phlog(sprintf( | |||||
| 'Webserver user cannot list remotes. Command: %s Exit: %d Error: %s', | |||||
| $remote_future->getCommand(), | |||||
| $err, | |||||
| $stderr)); | |||||
| // If this fails for whatever reason, just move on. | // If this fails for whatever reason, just move on. | ||||
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(); | list($err, $stdout, $stderr) = $future->resolve(); | ||||
| if (!$err) { | if (!$err) { | ||||
| list($hash, $epoch) = explode(' ', $stdout); | list($hash, $epoch) = explode(' ', $stdout); | ||||
| } else { | } else { | ||||
| // Very probably this is a "detected dubious ownership" | |||||
| // Better to report it, than just ignoring it. | |||||
| // https://we.phorge.it/T15282 | |||||
| phlog(sprintf( | |||||
| 'Webserver user cannot open git log. Command: %s Exit: %d Error: %s', | |||||
| $future->getCommand(), | |||||
| $err, | |||||
| $stderr)); | |||||
| $hash = null; | $hash = null; | ||||
| $epoch = null; | $epoch = null; | ||||
| } | } | ||||
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); | ||||
| ▲ Show 20 Lines • Show All 59 Lines • ▼ Show 20 Lines | private function newBinaryVersionTable() { | ||||
| 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); | ||||
| } | } | ||||
| } | } | ||||
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. | |||||
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… | |||||
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… | |||||
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… | |||||
Note that the command is only about git log, so it have sense to have error logs mentioning that fact