This is an archive of the discontinued Mercurial Phabricator instance.

hgext: start building a library for simple hooks
ClosedPublic

Authored by joerg.sonnenberger on Sep 7 2019, 8:50 AM.

Details

Summary

Many workflows depend on hooks to enforce certain policies, e.g. to
prevent forced pushes. The Mercurial Guide includes some cases and
Google can help finding others, but it can save users a lot of time
if hg itself has a couple of examples for further customization.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pulkit added a subscriber: pulkit.Sep 7 2019, 12:20 PM

Thanks a lot for putting efforts here. I am +1 on this. I haven't looked at the code though, will try to review if no one else beats me to it.

I think we should go a step further and have some built-in hooks in Mercurial itself, like we do extensions. A good first implementation would be to define those hooks somewhere where they can be imported, add tests like they are standalone hooks. As a follow-up (read: slightly more work), we can define a new config syntax for using these hooks. e.g. builtin:reject_new_heads.

What do others think?

I like where this is going, but I wonder if these specific hooks can be written without being in-process Python hooks. Reason being that Python hooks use the unstable internals of hg and are semi-discouraged if they're avoidable.

Thoughts?

If you do stick with in-process for this, I definitely want a test included so we don't regress our own examples. ;)

I like where this is going, but I wonder if these specific hooks can be written without being in-process Python hooks. Reason being that Python hooks use the unstable internals of hg and are semi-discouraged if they're avoidable.

This is exactly why I think the Python hooks should be part of the modules shipped as part of the core distribution :)

baymax requested changes to this revision.Jan 24 2020, 12:32 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Jan 24 2020, 12:32 PM
joerg.sonnenberger retitled this revision from contrib: start building a library for simple hooks to hgext: start building a library for simple hooks.Feb 16 2020, 12:03 AM
joerg.sonnenberger updated this revision to Diff 20249.
indygreg added inline comments.Feb 28 2020, 12:50 AM
hgext/hooklib/__init__.py
7

Assuming this is related to the IRC page.

When the demand importer is active, an import will materialize a proxy module object: the code in that module won't get executed until an attribute of the module is accessed.

So anything relying on run-on-import side-effects may not work correctly. Since our configuration option registration mechanism relies on code running as a side-effect of module import, in order to ensure this code runs, you need to ensure the module is "loaded" by accessing any of its attributes.

In addition, the extension mechanism may be looking for a configtable attribute in the module in order to associate config options with extensions. If this isn't actually happening (I'm too lazy to look at the source code), then you won't need to assign to a local variable here: simply accessing changeset_published.configtable will be sufficient to trigger module execution and register the config options.

durin42 accepted this revision as: durin42.

I'm happy with this, modulo my one question for other maintainers. If anyone else is happy, please feel encouraged to push it.

hgext/hooklib/__init__.py
2

Should we mark this as (EXPERIMENTAL) or are we comfortable holding on to this long-term?

/cc @marmoute @martinvonz @indygreg @yuja

hgext/hooklib/__init__.py
2

I'm perfectly fine with adding such a marker and a comment that the functionality is meant for long-term support, but e.g. might migrate into the notify extension. One of the big goals here is to ensure in-tree testing after all.

martinvonz added inline comments.Feb 28 2020, 4:49 PM
hgext/hooklib/__init__.py
2

I'd prefer to mark it experimental to start with.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.