Details

Reviewers

None

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

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

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

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.Fri, May 12, 10:21

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

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

valerio.bozzolan edited the summary of this revision. (Show Details)Fri, May 12, 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.Fri, May 12, 11:29

last fixes

Harbormaster completed remote builds in B399: Diff 722.Fri, May 12, 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.Thu, May 18, 10:16

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

Fix resolvex()

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

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

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

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

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 849

src/applications/config/controller/PhabricatorConfigConsoleController.php

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

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 849

src/applications/config/controller/PhabricatorConfigConsoleController.php

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

Revision Contents
Changeset List