1288859 - Pushlog bulk insertion API should be tied to Mercurial transaction

Status: RESOLVED FIXED

(Developer Services :: Mercurial: Pushlog, defect)

Description

[Gregory Szorc [:gps]]

Patch coming up shortly.

Comment 1

[Gregory Szorc [:gps]]

Attachment: pushlog: teach _getconn() to associate with a Mercurial transaction (bug 1288859);

An upcoming commit will teach another function to tie its changes to
a Mercurial transaction. In preparation for this, we refactor the code
for associating a database connection/transaction with a Mercurial
transaction into _getconn().

No behavior should have changed. Test output seems to confirm this.

Review commit: https://reviewboard.mozilla.org/r/66596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66596/

Comment 2

[Gregory Szorc [:gps]]

Attachment: pushlog: drop compatibility with Mercurial <3.3 (bug 1288859);

While I was touching transactions code, I noticed this code to support
old Mercurial versions. Kill it.

Review commit: https://reviewboard.mozilla.org/r/66598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66598/

Comment 3

[Gregory Szorc [:gps]]

Attachment: pushlog: tie recordpushes() to a repo transaction (bug 1288859);

Before, recordpushes() always committed the database transaction. If
we called recordpushes() inside a Mercurial transaction and rolled back
Mercurial, the pushlog changes would persist. That's not desirable.

We now tie the pushlog transaction to the Mercurial transaction, just
like is done during recordpush().

We have to allow a transaction to be passed into the function because
the caller of recordpushes() in the pushlog extension itself (which
executes during `hg pull` code in exchange.py) appears to have the
repo in a weird state where self._transref() doesn't return a
transaction! I'm not sure what's going on here - I suspect repo
filtering foo. We can get a transaction reference by looking at the
trmanager instance on the pulloperation instance, so we do that.

Review commit: https://reviewboard.mozilla.org/r/66600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66600/

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8773971 [details]
pushlog: tie recordpushes() to a repo transaction (bug 1288859);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66600/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8773969 [details]
pushlog: teach _getconn() to associate with a Mercurial transaction (bug 1288859);

https://reviewboard.mozilla.org/r/66596/#review63922

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8773970 [details]
pushlog: drop compatibility with Mercurial <3.3 (bug 1288859);

https://reviewboard.mozilla.org/r/66598/#review63924

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8773971 [details]
pushlog: tie recordpushes() to a repo transaction (bug 1288859);

https://reviewboard.mozilla.org/r/66600/#review63928

I'm surprised this doesn't require a few tests changes, which suggests a lack of failure mode tests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1bfc74b3ef75
pushlog: teach _getconn() to associate with a Mercurial transaction ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/aeb68c2ca08a
pushlog: drop compatibility with Mercurial <3.3 ; r=glandium
https://hg.mozilla.org/hgcustom/version-control-tools/rev/cf8f74b3783c
pushlog: tie recordpushes() to a repo transaction ; r=glandium