Details

Reviewers

avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15243: The /config/ page should log git errors instead of silently ignore them

Commits

rP83edbffd521d: Config page: add lovely git-related error messages in standard error log

Summary

Premise: the Config page runs git commands. Spoiler: they can fail.

Before this change errors were just suppressed and ignored.

After this change you get at least a log line. Also, you get a tip for a very specific well-known error affecting recent git.

Probably suppressing stuff was fine in the moment git worked here. But nowadays
git doesn't work so easily here, since it introduced very weird additional
configurations in order for a repository to be indicated as "safe" or not.

Error suppression was a problem there, because understanding the error with
"future objects" is not trivial for most users. Really.

After this change, these errors are beautifully mentioned in the standard log
of your webserver, to the best of our communication ability.

This is a cute example of a new log line:

Cannot identify the version of the phorge repository because the webserver does not trust it (more info on Task https://we.phorge.it/T15282).
Try this system resolution:
sudo git config --system --add safe.directory /var/www/phorge

Another:

Cannot identify the version of the phorge repository because the webserver does not trust it (more info on Task https://we.phorge.it/T15282).
Try this system resolution:
sudo git config --system --add safe.directory /var/www/arcanist

Incidentally, these specific errors probably afflict your Phorge/Phabricator, and now
you have some useful resolution tips. You are welcome!

You can also join T15282 to discuss your specific case.

Closes T15243

Test Plan

visit the /config/ page: does it worked before? it still works now
visit the /config/ page without /etc/gitconfig: you may still see "Unknown" as version - but, you finally get something in the log (instead of nothing)
visit the /config/ page after following your log messages: now you should see the library versions! yeeh

Additional tests:

manually sabotage the command "git log" replacing with "gitfooolog" and visit /config page: see the unexpected 'gitfooolog command not found' log line
manually sabotage the command "git remove" replacing with "gitremotelog" and visit /config/ page: see the unexpected 'gitremotelog command not found' log line

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

valerio.bozzolan created this revision.Apr 29 2023, 22:34

Owners added a reviewer: O1: Blessed Committers.Apr 29 2023, 22:34

Herald added subscribers: Cigaryno, Matthew, tobiaswiese, speck. · View Herald TranscriptApr 29 2023, 22:34

valerio.bozzolan requested review of this revision.Apr 29 2023, 22:34

Harbormaster completed remote builds in B293: Diff 559.Apr 29 2023, 22:34

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 29 2023, 22:36

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 29 2023, 22:40

valerio.bozzolan mentioned this in D25149: Config page: add $HOME to allow a gitconfig and help on "dubious ownership".Apr 29 2023, 23:44

clarify that the problematic user is the one related to the webserver

Harbormaster completed remote builds in B295: Diff 561.Apr 29 2023, 23:49

valerio.bozzolan edited the summary of this revision. (Show Details)Apr 29 2023, 23:50

Hi @aklapper it seems you are affected by this problem. Feel free to try this patch with:

arc patch D25148

Visiting the /config/ page should generate more useful error log lines in your PHP FPM error log. Please share them.

In case, if you really have extra time (edited: but also extra tie was OK), you may also want to try the related proposed fix for your case:

D25149

valerio.bozzolan mentioned this in Z1: Phorge.Apr 30 2023, 10:21

valerio.bozzolan added inline comments.Apr 30 2023, 10:26

src/applications/config/controller/PhabricatorConfigConsoleController.php
157	Note that the command is only about git log, so it have sense to have error logs mentioning that fact
161	Note that the command is only about git log, so it have sense to have error logs mentioning that fact

In D25148#4653, @valerio.bozzolan wrote:

Hi @aklapper it seems you are affected by this problem. Feel free to try this patch
[...]
Visiting the /config/ page should generate more useful error log lines in your PHP FPM error log. Please share them.

Went to http://phorge.localhost/config/ and under Version Information it shows Unknown for both Phorge and Arcanist.
Ran arc patch D25148
Reloaded http://phorge.localhost/config/
Error log is in P9

In case, if you really have extra time (edited: but also extra tie was OK), you may also want to try the related proposed fix for your case:

D25149

After applying D25149 working around some PHP8 exceptions, no changes - still showing Unknown here.

In D25148#5566, @aklapper wrote:

After applying D25149 working around some PHP8 exceptions, no changes - still showing Unknown here.

Yeah! Now, don't move

I'm not pretty sure about the $HOME of your webserver user in Fedora, but AFTER all of this shit, now you can finally fix as indicated in the commit message of D25149.

The fix is something like "become www-data, and run git config"

# create the gitconfig for www-data user (I think this is your home of your www-data user)
sudo touch           /var/www/.gitconfig
sudo chown www-data: /var/www/.gitconfig

# mark these repositories as safe (update to your repository pathname)
sudo -u www-data git config --global --add safe.directory /var/www/html/phorge/phorge
sudo -u www-data git config --global --add safe.directory /var/www/html/phorge/arcanist

# harden again
sudo chown root:     /var/www/.gitconfig

If this works, it's definitely worth to land the mentioned patches, and mention this git-stuff in the Phorge installation notes. Otherwise, Phorge itself cannot fix, since the Phorge has not enough permissions to do that, and an human is needed.

Hi @deadalnix :D Nice to meet a Blessed Committers

Can I ask what do you think about this patch? Any comment is welcome

Thank you so much 🙏

I noticed that it exists a ExecFuture#resolvex() method that is the exact replacement of ExecFuture#resolve() but throws a CommandException.

@avivey I should try to use that, right?

use resolve() → resolvex() that natively throws a nice exception message

Harbormaster completed remote builds in B397: Diff 720.May 12 2023, 10:21

valerio.bozzolan edited the summary of this revision. (Show Details)May 12 2023, 10:31

valerio.bozzolan edited the test plan for this revision. (Show Details)

valerio.bozzolan edited the summary of this revision. (Show Details)May 12 2023, 11:28

valerio.bozzolan edited the test plan for this revision. (Show Details)

create a very useful "tip generator"

Harbormaster completed remote builds in B398: Diff 721.May 12 2023, 11:29

last fixes

Harbormaster completed remote builds in B399: Diff 722.May 12 2023, 11:32

@aklapper Very probably, with this patch, now the log shows a resolution message with a fix to your issue

Feel free to follow the test plan. You can also eventually add yourself as reviewer and approve this, if you like this (premising we still need O1 approval).

valerio.bozzolan mentioned this in Wikimedia Hackaton 2023 Athens Code Sprint.May 18 2023, 10:16

valerio.bozzolan mentioned this in T15282: Fix/avoid/simplify similar fatal: detected dubious ownership in repository at '/var/www/phorge'.May 23 2023, 09:51

Fix resolvex()

Harbormaster completed remote builds in B484: Diff 849.May 30 2023, 10:12

valerio.bozzolan edited the summary of this revision. (Show Details)May 30 2023, 10:21

valerio.bozzolan edited the test plan for this revision. (Show Details)

A few style comments, but otherwise it's ready.

src/applications/config/controller/PhabricatorConfigConsoleController.php
192–195	this comment is also redundant (and wrong) now.
195	redundant comment
269	redundant comment
350	We generally prefer non-static methods wherever possible - just a style thing.
366	This is one of the rare places where I think it's find to use `sudo` in the docs (and suggest system-wide changes): It greatly simplifies the doc It's sort-of safe, because it anyways involves code that's running on the server and has access to pretty much anything important anyway We generally assume the Phorge server is dedicated, and shouldn't be used for anything else. 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).

improving stuff

Harbormaster completed remote builds in B524: Diff 911.Jun 10 2023, 10:02

avivey accepted this revision.Jun 10 2023, 10:12

This revision is now accepted and ready to land.Jun 10 2023, 10:12

Closed by commit rP83edbffd521d: Config page: add lovely git-related error messages in standard error log. · Explain WhyJun 10 2023, 11:21

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rP83edbffd521d: Config page: add lovely git-related error messages in standard error log.

speck added inline comments.Jun 10 2023, 14:31

src/applications/config/controller/PhabricatorConfigConsoleController.php
353	Does git return translated error messages? Is there an identifiable error code for this error we could check instead?
366	Evan 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?

valerio.bozzolan added inline comments.Jun 10 2023, 15:19

src/applications/config/controller/PhabricatorConfigConsoleController.php
353	@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 marked an inline comment as done.Jun 10 2023, 15:23

valerio.bozzolan added inline comments.

src/applications/config/controller/PhabricatorConfigConsoleController.php
366	It 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.

Config page: add lovely git-related error messages in standard error log
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 913

src/applications/config/controller/PhabricatorConfigConsoleController.php

Config page: add lovely git-related error messages in standard error logClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 913

src/applications/config/controller/PhabricatorConfigConsoleController.php

Config page: add lovely git-related error messages in standard error log
ClosedPublic
Actions

Revision Contents
Changeset List