Open Bug 1539412 Opened 6 months ago Updated 6 months ago

add the ability to blacklist mercurial extensions to moz-phab

Categories

(Conduit :: moz-phab, enhancement, P3)

enhancement

Tracking

(Not tracked)

People

(Reporter: valentin, Unassigned)

Details

(Keywords: conduit-triaged)

So I recently installed the bookbinder mercurial extension again and noticed that when uploading a stack with moz-phab submit, the commits were actually submitted in reverse order.

[1] https://bitbucket.org/halbersa/bookbinder

I use the extension to get the equivalent of hg qapplied:

[extensions]
histedit = 
rebase = 
share =
purge = 
evolve = ~/.mozbuild/evolve/hgext3rd/evolve
blackbox = 
firefoxtree = ~/.mozbuild/version-control-tools/hgext/firefoxtree
clang-format = ~/.mozbuild/version-control-tools/hgext/clang-format
format-source = ~/.mozbuild/version-control-tools/hgext/format-source
push-to-try = ~/.mozbuild/version-control-tools/hgext/push-to-try
mq = !
bookbinder = ~/workspace/bookbinder

[alias]
blog = log --rev 'feature(tip)'

I noticed the problem because sometimes the command errors out like this:

abort: this rebase will cause divergences from: d2f53839e4b1
(to force the rebase please set experimental.evolution.allowdivergence=True)
CalledProcessError: Command '['hg', '--config', 'extensions.rebase=', 'rebase', '--source', 'd2f53839e4b15187fe1eb85beea053ed2acca8b3', '--dest', '3bb3381ff56d00d1807db7c42b8b4e434dc07407']' returned non-zero exit status 255

or

abort: source and destination form a cycle
CalledProcessError: Command '['hg', '--config', 'extensions.rebase=', 'rebase', '--source', '66b5f646a592adf8205b2c0e2d034e7006cbae7a', '--dest', '1bffc850d56e655bdb47cba90aad4e23e94177cd']' returned non-zero exit status 255

The errors don't happen without the extension.
I'm not sure how moz-phab works, and if this is a bug in the extension causing subtle differences in how it outputs the history or in how moz-phab is using that history.

mercurial extensions have the ability to change pretty well any aspect of how mercurial works; it isn't surprising that this sort of thing happens unfortunately.

in bookbinder's case they override log's output to reverse the ordering, which is exciting.
https://bitbucket.org/halbersa/bookbinder/src/40ddfc43535fea55f71e592b19b40f6ba82155a8/bookbinder.py?at=default&fileviewer=file-view-default#bookbinder.py-156

moz-phab used to use a whitelist of hg extensions to avoid this, however we ran into issues where some useful extensions were not being loaded (especially experimental extensions or those relating to performance).

so now moz-phab loads all configured extensions, with a --safe-mode switch to fall back to the whitelisting approach. until this bug is resolved you'll have to pass --safe-mode to moz-phab submit in order for things to work.

looks like we'll need to add the ability to blacklist an extension:
hg --config extensions.bookbinder=! ..

Keywords: conduit-triaged
Priority: -- → P2

Commented on that issue. Basically I need to add log to the reversewrap in order to match the behaviour of mercurial. I.e, without reversing, hg log -r "ancestors(.)" -T "{rev}\n" results in:

0
1
2
3
...

Fwiw, I use bookbinder + moz-phab without issue (unless moz-phab only very recently stopped using a whitelist and I haven't upgraded yet).

One possible thing to try is to set HGPLAIN when running moz-phab submit? Just a shot in the dark, haven't tested or anything.

Valentin, might also help to know what version of mercurial you're using.

Flags: needinfo?(valentin.gosu)

(In reply to Andrew Halberstadt [:ahal] from comment #3)

One possible thing to try is to set HGPLAIN when running moz-phab submit? Just a shot in the dark, haven't tested or anything.

HGPLAIN impacts encoding, verbosity, localisation; it doesn't override extensions.

(In reply to Andrew Halberstadt [:ahal] from comment #4)

Valentin, might also help to know what version of mercurial you're using.

Mercurial Distributed SCM (version 4.5.3)

Flags: needinfo?(valentin.gosu)

From the issue:

I think I see the issue. Here is mercurial's logic for handling log: https://www.mercurial-scm.org/repo/hg/file/stable/mercurial/logcmdutil.py#l693

So essentially if you pass in -r <revset> mercurial does not reverse the output. But if you don't pass in -r <revset>, it does reverse the output.

Bookbinder only operates when you pass in -r, so that is why I needed to reverse in order to match the default behaviour. The bug here is that I'm reversing all revsets, whether or not bookbinder actually expanded them or not. I should only reverse revsets that bookbinder expands.

(In reply to Byron Jones ‹:glob› 🎈 from comment #5)

HGPLAIN impacts encoding, verbosity, localisation; it doesn't override extensions.

It does impact bookbinder:
https://bitbucket.org/halbersa/bookbinder/src/40ddfc43535fea55f71e592b19b40f6ba82155a8/bookbinder.py?at=default&fileviewer=file-view-default#lines-51

But I think it would be irrelevant in this particular case.

Note: I'm still confused about why I'm seeing moz-phab submit revisions in the right order. Valentin and I must have something different in our local hg configs.

I think I pushed a fix to bookbinder, though I'm not sure as I wasn't able to reproduce your issue. Can you try updating bookbinder and pushing to review again?

Flags: needinfo?(valentin.gosu)

(In reply to Andrew Halberstadt [:ahal] from comment #9)

I think I pushed a fix to bookbinder, though I'm not sure as I wasn't able to reproduce your issue. Can you try updating bookbinder and pushing to review again?

moz-phab seems to work correctly now.
hg blog does list the commits in reverse order compared to hg log -l 4 but this is a lot better than submitting them in reverse order :)

Thanks a lot for the fix.

Flags: needinfo?(valentin.gosu)

I'm not sure what hg blog is, but if that's a custom alias using -r, yes it will start logging in the reverse order as before (which is what mercurial does by design). You can re-align it by wrapping whatever your revset is with reverse(<revset>).

Oh I see it in your ~/.hgrc, yes you'll need to change it to reverse(feature(tip)).

(In reply to Andrew Halberstadt [:ahal] from comment #12)

Oh I see it in your ~/.hgrc, yes you'll need to change it to reverse(feature(tip)).

Thanks! It's perfect now.

thanks for the super quick fix ahal!

as it might be useful in future i'll make this bug about a generic blacklisting feature.

might be best implemented as a configuration option so users can self-manage (as opposed to a hard-coded list in moz-phab).

Priority: P2 → P3
Summary: moz-phab submit uploads stacks in reverse order with the bookbinder extension → add the ability to blacklist mercurial extensions to moz-phab
You need to log in before you can comment on or make changes to this bug.