1162093 - Add a mercurial extension that handles pushing to try efficiently and most certainly without mq

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

The idea is to make an in-memory commit with a certain syntax, push it to try, and roll back. Try syntax itself would be generated somewhere else. The existing trychooser extensions would use this instead of mq.

I have the in-memory commit part working, but I may need help figuring out how to roll back (I'm probably using the api incorrectly, but simply aborting a transaction doesn't leave the repo in a good state).

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1162093/chmanchester

/r/8311 - Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

Pull down this commit:

hg pull -r 40fd0d60bd4cd2fc57de7a15606a14a638a56cdc https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 2

[Chris Manchester (limited bugmail, email directly)]

This is working pretty well. I'll add tests for uncommited file deletes and a few more cases (a comment around memctx makes me think they have special rules for making a commit).

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8602498 [details]
MozReview Request: bz://1162093/chmanchester

/r/8311 - Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

Pull down this commit:

hg pull -r 40fd0d60bd4cd2fc57de7a15606a14a638a56cdc https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 4

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8309/#review7131

::: hgext/push-to-try/__init__.py:37
(Diff revision 1)
> +        # And rollback to the previous state.
> +        tr.abort()
> +        lock.release()

Right, this part doesn't work when the uncommited change is a file deletion. I'll dig into that next week, feedback welcome for now.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8309/#review7133

> Right, this part doesn't work when the uncommited change is a file deletion. I'll dig into that next week, feedback welcome for now.

This was simple to resolve, but there may be some more cases to consider.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8602498 [details]
MozReview Request: bz://1162093/chmanchester

/r/8311 - Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

Pull down this commit:

hg pull -r 9d10d927b1f4dfeeb72db4ae798a78bccb01b4d9 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 7

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8311/#review7787

I like the work here. I wish existing trychooser extensions did this.

Speaking of existing trychooser extensions, now I guess we have N+1 try extensions. Perhaps you can refactor this code to be a library as part of pylib/mozhg. Then, we can import an existing trychooser extension into version-control-tools and make it not suck? I try not to ask for scope bloat. But, I feel that without try syntax selection, the utility of this code as an extension is a bit low. Do you have other plans not communicated in this current patch?

::: hgext/push-to-try/__init__.py:9
(Diff revision 2)
> +    msg = opts['message']
> +    url = opts['server']

Just add message=None server=None to the arguments list.

::: hgext/push-to-try/__init__.py:45
(Diff revision 2)
> +cmdtable = {
> +    "push-to-try": (
> +        push_to_try_command,
> +        [
> +            ('m', 'message', '', 'try message to use'),
> +            ('s', 'server', 'ssh://hg.mozilla.org/try', 'push destination'),
> +        ],
> +        'hg push-to-try -m MESSAGE [-s URL]',
> +    ),
> +}

I'm not sure where you grabbed this code from, but this is the ancient method for registering commands. Instead, use the @command decorator. Search for `@command` in hgext/.

::: hgext/push-to-try/__init__.py:41
(Diff revision 2)
> +        tr.abort()

tr may be None if the lock couldn't be acquired. Use the pattern at https://reviewboard-hg.mozilla.org/version-control-tools/file/f4eaf8be758c/pylib/mozhg/mozhg/rewrite.py#l285

::: hgext/push-to-try/tests/test-file-delete.t:5
(Diff revision 2)
> +  > qbackout = $TESTDIR/hgext/push-to-try

qbackout?!

::: hgext/push-to-try/tests/test-file-delete.t:40
(Diff revision 2)
> +  $ hg diff

Can you please throw an `hg status` in here? That will help double verify things are sane.

::: hgext/push-to-try/__init__.py:17
(Diff revision 2)
> +    status = repo.status()

It might be useful to print something when the working copy is dirty. That way the user realizes their uncommitted changes are being pushed.

::: hgext/push-to-try/__init__.py:13
(Diff revision 2)
> +        ui.write("STOP! A commit message with try syntax is required.")

Should we validate the commit message actually contains try syntax?

::: hgext/push-to-try/__init__.py:56
(Diff revision 2)
> +testedwith = '3.3.2'

Please move to near top of file, after imports but before any functions.

::: hgext/push-to-try/tests/test-clean-push.t:26
(Diff revision 2)
> +  transaction abort!

This output is confusing and scary.

We at least need a message around creating the try commit.

Ideally we would also suppress the harmless "transaction abort!" message. https://hg.mozilla.org/hgcustom/version-control-tools/file/52c1c20dfea2/hgext/reviewboard/client.py#l175 shows you how you can temporarily wrap the ui instance to filter certain output.

::: hgext/push-to-try/tests/test-clean-push.t:40
(Diff revision 2)
> +  changeset:   1:???????????? (glob)

I'm guessing you are globbing here because of the time sneaking into the temporary commit?

We typically build backdoors into our code to facilitate testing. Typically we set a config option in the hgrc and look for it at run-time. It's the price we pay to be deterministic.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8311/#review8091

Thanks for the pointers here! The immediate motivation for this is to be used in bug 1149670, and if all goes well I was planning to modify existing trychooser extensions to consume it (in place of where they consume mq now). Turning this into a library sounds better, I'll look at that next.

> I'm not sure where you grabbed this code from, but this is the ancient method for registering commands. Instead, use the @command decorator. Search for `@command` in hgext/.

https://mercurial.selenic.com/wiki/WritingExtensions#Command_table

Comment 9

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/8311/#review8157

Looking at this again, I'm not sure factoring this into a library makes sense right now. Autoland, trychooser, and the "autotry" tool push a commit to try from the command line interface of mq, which this code as an extension can replace directly. Because I'll need this as an extension anyway for the 'autotry" tool, I think it's early to make this a library in v-c-t (the extension would be its only consumer).

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8602498 [details]
MozReview Request: bz://1162093/chmanchester

/r/8311 - Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

Pull down this commit:

hg pull -r eef5d4ef329d2dafb392cc55f90a98a59dc87a86 https://reviewboard-hg.mozilla.org/version-control-tools/

Comment 11

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8311/#review8553

Looks mostly good.

Please add a test that the executable bit of a modified file is preserved. (Hint: this test will fail in the current implementation).

::: hgext/push-to-try/README.md:1
(Diff revision 3)
> +*push-to-try*

Nit: we use rst throughout v-c-t because it integrates better with Sphinx, which powers https://mozilla-version-control-tools.readthedocs.org/en/latest/

::: hgext/push-to-try/__init__.py:26
(Diff revision 3)
> +    status = repo.status()
> +
> +    if status.modified + status.added + status.removed:
> +        ui.status('The following will be pushed to %s:\n' % server)
> +        commands.status(ui, repo)

Computing status is an expensive operation. I can't recall if it is cached at all when executed in the same process like this. You may want to measure this and create a TODO to only perform a single status call.

::: hgext/push-to-try/__init__.py:32
(Diff revision 3)
> +    def mk_memfilectx(repo, memctx, path):
> +        if path in status.removed:
> +            return
> +        data = cctx.filectx(path).data()
> +        return context.memfilectx(repo, path, data)

Sadly, this is too naive and it will drop file flags and copy information from files. Copy information is arguably not important. But file flags are. See pylib/mozhg/mozhg/rewrite.py:preservefilectx() for a callback that does the right thing.

Bonus points if you refactor preservefilectx instead of duplicating code.

::: hgext/push-to-try/__init__.py:65
(Diff revision 3)
> +    finally:
> +        # And rollback to the previous state.
> +        if tr:
> +            tr.abort()

try:
   ...
   tr.abort()
finally:
   tr.release()
   
The standard behavior is to have a tr.commit() at the end of the try block and tr.release() in the finally. Having the tr.abort() in the finally seems weird to me. Putting the tr.release() will no-op assuming the tr.abort() is called. But it reads easier, IMO.

::: hgext/push-to-try/__init__.py:64
(Diff revision 3)
> +        ui.status('Push complete\n')

Nit: lowercase "push", as Mercurial doesn't capitalize output (for whatever reason).

::: hgext/push-to-try/__init__.py:71
(Diff revision 3)
> +        ui.status("Temporary commit removed, repository restored\n")

Nit: lowercase

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1162093 - Add "push-to-try" from version-control-tools to the mercurial setup wizard prompt.;r=gps

Bug 1162093 - Add "push-to-try" from version-control-tools to the mercurial setup wizard prompt.;r=gps

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8614191 [details]
MozReview Request: Bug 1162093 - Add a mercurial extension to push any uncommited changes with a particular commit message.

https://reviewboard.mozilla.org/r/8311/#review8769

::: hgext/push-to-try/__init__.py:43
(Diff revisions 3 - 4)
> -            return
> +            return preserve_ctx(repo, memctx, path)

Please throw an explicit `return None` in here.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 8614197 [details]
MozReview Request: Bug 1162093 - Add "push-to-try" from version-control-tools to the mercurial setup wizard prompt.;r=gps

https://reviewboard.mozilla.org/r/9875/#review8771

This may bit rot with what I just landed to inbound. If so, it should be easy enough to rebase.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f96ac49e3bd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab80a6814ba

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dab80a6814ba