Page MenuHomePhorge

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

Authored by valerio.bozzolan on Apr 29 2023, 22:34.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 22:18
Unknown Object (File)
Tue, Mar 26, 21:24

Details

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
Branch
arcpatch-D25148
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 295
Build 295: arc lint + arc unit

Event Timeline

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

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

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

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.

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.

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

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

create a very useful "tip generator"

@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 edited the test plan for this revision. (Show Details)

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

src/applications/config/controller/PhabricatorConfigConsoleController.php
197

redundant comment

204

this comment is also redundant (and wrong) now.

288

redundant comment

368

We generally prefer non-static methods wherever possible - just a style thing.

384

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).

This revision is now accepted and ready to land.Jun 10 2023, 10:12
src/applications/config/controller/PhabricatorConfigConsoleController.php
371

Does git return translated error messages? Is there an identifiable error code for this error we could check instead?

384

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?

src/applications/config/controller/PhabricatorConfigConsoleController.php
371

@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 added inline comments.
src/applications/config/controller/PhabricatorConfigConsoleController.php
384

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.