Page MenuHomePhorge

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

Authored by valerio.bozzolan on Apr 29 2023, 22:34.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 31, 06:25
Unknown Object (File)
Tue, May 23, 20:36
Unknown Object (File)
Tue, May 23, 17:52
Unknown Object (File)
Tue, May 23, 13:02
Unknown Object (File)
Thu, May 18, 06:42
Unknown Object (File)
Wed, May 17, 10:27
Unknown Object (File)
Wed, May 17, 04:52
Unknown Object (File)
Tue, May 16, 19:47

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_2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 484
Build 484: 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)