I'll try to take a closer look at this some time this weekend. In the meantime, can you explain more why it didn't work for you? There are tests that cover this feature, so I'm wondering if some coverage is missing.

I'm not sure if this is intentional, but it appears that it will unconditionally override even .hg/hgrc configs if .arcconfig is present. Before it would only override the global config, load .arcconfig, and then load .hg/hgrc. I guess it's only a small benefit that you could re-target the phab instance without dirtying the repo, so I'm not strongly against the change.

One of the lines looks like it might be long-ish, so you might want to run ./run-tests --local test-phabricator.t test-check-* to make sure it's OK.

I finally got around to looking at why I needed this. It turned out to be pretty complicated, and although this patch would work for me, I'm sure there are other ways to work around this so it may not make sense to land.

At first, I thought it was simple: the wrapped loadhgrc is defined in an extension, and knowing what extensions exist requires loading the .hgrc config file, so of course it can't possibly work and everybody who is using it successfully must be, uh, liars or something!

But it appears that this loadhgrc is only used for loading a repository's .hg/hgrc, so the above logic is wrong.

Ok, here's where it gets weird. What I'm doing is making a "wrapper" extension mozphabricator.py that works by importing selected stuff from mercurial's in-tree hgext/phabricator.py, and then overriding and monkeypatching what I want to change. The problem is that phabricator.py hooks into the repo-specific .hgrc loading via @exthelper.exthelper().wrapfunction(localrepo, "loadhgrc"). When I thought using loadhgrc was just completely wrong, the implementation was trivial: just do from hgext.phabricator import reposetup and it just works.

Aha! Ok, the problem I was running into with the exthelper stuff is that it wasn't getting invoked, and it wasn't getting invoked because my extension did not set its uisetup at all. So the fix is very similar to the above: from hgext.phabricator import uisetup.

In D8524#134322, @mharbison72 wrote:

The documentation updates look good to me, but there are still some code changes that were probably not intentional, and the commit message probably needs an update.

I updated the commit message to "add .arcconfig to help messages and comments", which seems accurate.

It gets a little messy when uploading partial patches to code that I am patching

Are you simply amending your local commit and reposting it with phabsend? If not, I'm not sure what the state of it will be if someone imports it to land it.

Sorry, I said that wrong. Yes, I'm doing exactly what you're saying, it's just that the phabsend I'm using is coming from mozphabricator.py and I have a whole long stack of patches applied to it, and a corresponding stack of patches applied to phabricator.py, and I have to be sure to be in matching places in the stacks or things might get a little mangled (eg adding "Bug None - Bug None - Bug None -" to the beginning of the commit message).

Ok, the problem I was running into with the exthelper stuff is that it wasn't getting invoked, and it wasn't getting invoked because my extension did not set its uisetup at all. So the fix is very similar to the above: from hgext.phabricator import uisetup

I've got a similar extension that wraps the command itself and changes all reviewers to blocking reviewers (so that the first accept doesn't drop it out of everybody else's queue). I don't understand importing hgext.phabricator.uisetup though. You should have this somewhere near the top of your extension like phabricator.py does

eh = exthelper.exthelper()
...
uisetup = eh.finaluisetup

The underlying machinery simply looks for attributes on the module like uisetup(), reposetup(), etc, and calls them if present. And yeah, I get confused all the time about the differences between these hooks too.

Hm, you may be right. I hadn't needed exthelper for anything else in my wrapper yet, so I got rid of it entirely. (I was originally using it, but that meant when I did eg commands = eh.commands I wasn't "inheriting" the commands from phabricator.py. So I switched to from hgext.phabricator import commands and then modifying stuff as needed.)

Anyway, whether I do uisetup = eh.finaluisetup or from hgext.phabricator import uisetup doesn't matter too much; it ends up having the same effect. Given that phabricator.py already has an exthelper object instantiated with configuration on it that I want to use, it seems a little odd to make another one in my module. Perhaps a midway point would be from hgext.phabricator import eh and then commands = eh.commands etc.

In D8524#134326, @sfink wrote:

In D8524#134322, @mharbison72 wrote:

The documentation updates look good to me, but there are still some code changes that were probably not intentional, and the commit message probably needs an update.

I updated the commit message to "add .arcconfig to help messages and comments", which seems accurate.

I meant this, which is still in the summary section

Wrapping localrepo's loadhgrc() was not working for me because it is too late to wrap loadhgrc when the extension is loaded.

This is now the only code change (removing the explicit result variable and instead just using cfg to tell whether we updated any configuration.) This seems to better match the intent documented in mercurial/localrepo.py:

Returns a bool indicating whether any additional configs were loaded.

The only difference with the redundant result variable would be if you loaded in a .arcconfig that did not contain either of the two settings looked at here, and in that case it seems like it would be better to return False.
What do you think?

I understand now. Yeah this code is better. Usually we try to split unrelated documentation updates into a separate commit though, so can you hg split this (and get rid of the differential url in one of theme)?

In D8524#134327, @mharbison72 wrote:

In D8524#134326, @sfink wrote:

In D8524#134322, @mharbison72 wrote:

The documentation updates look good to me, but there are still some code changes that were probably not intentional, and the commit message probably needs an update.

I updated the commit message to "add .arcconfig to help messages and comments", which seems accurate.

I meant this, which is still in the summary section

Wrapping localrepo's loadhgrc() was not working for me because it is too late to wrap loadhgrc when the extension is loaded.

I agree that this needs to be changed.

Rest looks good to me, will push once commit description is updated.

This is now the only code change (removing the explicit result variable and instead just using cfg to tell whether we updated any configuration.) This seems to better match the intent documented in mercurial/localrepo.py:

Returns a bool indicating whether any additional configs were loaded.

The only difference with the redundant result variable would be if you loaded in a .arcconfig that did not contain either of the two settings looked at here, and in that case it seems like it would be better to return False.
What do you think?

I understand now. Yeah this code is better. Usually we try to split unrelated documentation updates into a separate commit though, so can you hg split this (and get rid of the differential url in one of theme)?