1118619 - Port pushlog extension to Mercurial 3.3

Status: RESOLVED FIXED

(Developer Services :: Mercurial: Pushlog, defect)

Description

[Gregory Szorc [:gps]]

The pushlog extension isn't working in Mercurial 3.3 (current @) due to failures in transaction monkeypatching and refactored transaction code in exchange.pulloperation. It will need to be ported to the new APIs.

Comment 1

[Gregory Szorc [:gps]]

Attachment: pushlog: wrap exchange._pullobsolete ()

Mercurial 3.3 refactored the transactions API and removed our previous
hook point: exchange.pushop.closetransaction. The pull code in both
versions is essentially:

  _pullbookmarks()
  _pullobsolete()
  closetransaction()

This patch changes the hook point from inside and before
closetransaction to inside and after _pullobsolete.

The extension still fails to write new data properly in 3.3 due to
failures in transaction monkeypatching. This will be fixed in a
subsequent patch.

Comment 2

[Gregory Szorc [:gps]]

Attachment: pushlog: port transaction monkeypatching to Mercurial 3.3 ()

Mercurial 3.3 changed the transaction API. Gone are the .after and
.onabort hook points. Added are some add* APIs for registering
callbacks.

This patch ports the pushlog transaction monkeypatching code to use both
forms of the API.

Sadly, the new Mercurial APIs still do not provide a mechanism to
register a callback which will be called in all transaction termination
scenarios. Specifically, in the case of abort/rollback, no mechanism is
provided to register callbacks. So, we fall back to monkeypatching
._abort on the transaction instance.

Our rollback code has essentially moved from 4 lines into
transaction._abort() into the first thing. The lines being skipped over
mostly involve closing files related to the transaction. They should
have no relevance to us rolling back the SQLite transaction.

Comment 3

[Gregory Szorc [:gps]]

marmoute: the second patch demonstrates that the new add* callbacks in transaction API needs expansion. Specifically, we need an addabort() hook point so we can handle rollbacks properly. I'm able to work around it (by monkeypatching). But it's super annoying.

I doubt I can come up with an in-Mercurial consumer of this API. Normally that means you won't take the patch. Will you make an exception in this case?

Comment 4

[Gregory Szorc [:gps]]

I went ahead and submitted a patch upstream to complete the API. We'll see what happens.

http://www.selenic.com/pipermail/mercurial-devel/2015-January/065255.html

glandium: it might be best to hold off on review of the 2nd patch until we hear back.

Comment 5

[Pierre-Yves David]

I will probably take the addabort patch even withtou internal user because it make sense and improve consistency. We probably need to find a way it to have it tested by the core test suite to prevent regression on it.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/33d60d5e4095

Comment 7

[Gregory Szorc [:gps]]

Attachment: pushlog: port transaction monkeypatching to Mercurial 3.3 ()

Mercurial 3.3 changed the transaction API. Gone are the .after and
.onabort hook points. Added are some add* APIs for registering
callbacks.

This patch ports the pushlog transaction monkeypatching code to use both
forms of the API.

Our rollback code has essentially moved from 4 lines into
transaction._abort() into the first thing. The lines being skipped over
mostly involve closing files related to the transaction. They should
have no relevance to us rolling back the SQLite transaction.

The location of the "onabort" callbacks in Mercurial 3.4 is after
Mercurial begins the rollback process. As a result, the order of log
output is slightly different between versions. This will eventually
break tests once failures from @ fail the test harness (they are
currently ignored). We will have to tackle this problem later. I'm
punting on it for now because a solution is non-trivial.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8546330 [details] [diff] [review]
pushlog: port transaction monkeypatching to Mercurial 3.3 ()

Review of attachment 8546330 [details] [diff] [review]:
-----------------------------------------------------------------

> The location of the "onabort" callbacks in Mercurial 3.4

You mean 3.3.

> We will have to tackle this problem later. I'm punting on it for now because a solution is non-trivial.

I'd argue that supporting 3.3, which is not even released, can be tackled later too. And that problem at the same time...
OTOH, as I see it, you can just hook transaction._playback on older versions, instead of onabort, and you'd end up at the same level as the addabort with 3.3.

::: hgext/pushlog/__init__.py
@@ +226,2 @@
>          def abort():
> +            self.repo.ui.warn('rolling back pushlog\n')

You could just call onabort(None) and avoid the duplication.

Comment 9

[Mike Hommey [:glandium]]

Also, it would be very much better if it were possible to run some tests without having to setup docker.

Comment 10

[Gregory Szorc [:gps]]

Attachment: pushlog: port transaction monkeypatching to Mercurial 3.3

Mercurial 3.3 changed the transaction API. Gone are the .after and
.onabort hook points. Added are some add* APIs for registering
callbacks.

This patch ports the pushlog transaction monkeypatching code to use both
forms of the API.

Our rollback code has essentially moved from 4 lines into
transaction._abort()'s execution to the first operation. The lines being
skipped over mostly involve closing files related to the transaction. They
should have no relevance to us rolling back the SQLite transaction.

The location of the "onabort" callbacks in Mercurial 3.4 is after
Mercurial begins the rollback process. As a result, the order of log
output is slightly different between versions. The new output is
more logical (abort transaction then print pushlog rollback message).

To maintain consistent output between 3.2 and 3.3, we've monkeypatched
transaction._playback on 3.2 to print the pushlog rollback message.
There may be a code path where the pushlog doesn't actually get rolled
back when this function is called. However, no harm should come of it.
The plan is to delete this 3.2 support code once 3.3 is deployed to
production.

Comment 11

[Gregory Szorc [:gps]]

Attachment: pushlog: don't save backup from strip

Mercurial 3.3 uses slightly different strip backup bundle filenames
in some scenarios. This was causing test output to vary between 3.2
and 3.3. Since the output from strip is irrelevant for this test, we
just don't produce a backup bundle any more. The test output is thus
consistent between 3.2 and 3.3.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8558633 [details] [diff] [review]
pushlog: port transaction monkeypatching to Mercurial 3.3

Review of attachment 8558633 [details] [diff] [review]:
-----------------------------------------------------------------

> The location of the "onabort" callbacks in Mercurial 3.4

You still mean 3.3.

::: hgext/pushlog/__init__.py
@@ +588,2 @@
>  
> +    # Only for 3.3 support.

for < 3.3 support

::: hgext/pushlog/tests/hghave
@@ +3,5 @@
> +import os
> +
> +HERE = os.path.abspath(os.path.dirname(__file__))
> +REPO_ROOT = os.path.join(HERE, '..', '..', '..')
> +execfile(os.path.join(REPO_ROOT, 'testing', 'hghave.py'))

What is this for? It seems like a spurious hg add.

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f300b6d0a6db
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a1729f81ac6f