1137668 - [pushes] use hglib to interact with mercurial repos

Status: RESOLVED FIXED

(Webtools Graveyard :: Elmo, defect)

Description

[Axel Hecht [:Pike]]

hglib is a nice level of abstraction for mercurial repos, and it'll help with the memory consumption of our current in-process usage of hg code.

I've started looking at this, and the biggest pity is the cross-repo diff code. Let's rip that out first, bug 1137667.

Comment 1

[Axel Hecht [:Pike]]

Attachment: use hglib for all the things but the diff view

Gregory, not sure if it makes sense for you to provide feedback on this patch. Probably more than anyone else.

My buglist for hglib is http://bz.selenic.com/buglist.cgi?bug_id=4510,4511,4644, so maybe that aspect is worth looking at?

Also, this doesn't do the full thing yet, there's one feature of the diff view like https://l10n.mozilla.org/source/diff/?from=0c54c4102de4&to=0300b60cd340&repo=releases%2Fl10n%2Fmozilla-aurora%2Fast where I support diffing between repos between changesets with a common ancestor. And piecing together the move and copy changes just proves to be really tedious. I'm hoping to resolve that by converting the local clones on elmo to unified repos, at which point that logic would just disappear.

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8602126 [details] [diff] [review]
use hglib for all the things but the diff view

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

Thank you for flagging me for review. I encourage people to flag me for reviews for anything relating to Mercurial.

The changes look good! Although, it's difficult to fully assess since I'm not familiar with the code and can't expand context in Splinter.

I saw your hglib bugs filed upstream. How urgent are fixes? If they are blocking this bug, I can probably patch hglib with a little effort.

::: apps/pushes/repo_fixtures.py
@@ +6,4 @@
>  '''
>  
>  import os
> +import os.path

You can remove this line: `import os` will make os.path available automagicaly.

@@ +27,5 @@
>      def file(cls, repo, path, contents):
> +        path = repo.pathto(path)
> +        open(path, 'w').write(contents)
> +        if repo.status(include=[path], all=True)[0][0] == '?':
> +            repo.add(path)

With a little refactoring, you could probably use `hg commit -A` to auto add untracked files.

::: requirements/prod.txt
@@ +56,4 @@
>  # sha256: UvCV-GFHuwtfYjva-VJVLReOu8XqyI2RsvWR09IsO4A
>  funfactory==2.3.0
>  
> +#python-hglib==1.5

I think 1.6 is available. You should probably use that.

Comment 3

[Gregory Szorc [:gps]]

(In reply to Axel Hecht [:Pike] from comment #1)
> Created attachment 8602126 [details] [diff] [review]
> use hglib for all the things but the diff view
> 
> Gregory, not sure if it makes sense for you to provide feedback on this
> patch. Probably more than anyone else.
> 
> My buglist for hglib is
> http://bz.selenic.com/buglist.cgi?bug_id=4510,4511,4644, so maybe that
> aspect is worth looking at?
> 
> Also, this doesn't do the full thing yet, there's one feature of the diff
> view like
> https://l10n.mozilla.org/source/diff/
> ?from=0c54c4102de4&to=0300b60cd340&repo=releases%2Fl10n%2Fmozilla-
> aurora%2Fast where I support diffing between repos between changesets with a
> common ancestor. And piecing together the move and copy changes just proves
> to be really tedious. I'm hoping to resolve that by converting the local
> clones on elmo to unified repos, at which point that logic would just
> disappear.

If repos share a common ancestor, they are the same repo. Easiest way to diff is to `hg pull` to a common repository and do the diffing that way.

Comment 4

[Axel Hecht [:Pike]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8602126 [details] [diff] [review]
> use hglib for all the things but the diff view
> 
> Review of attachment 8602126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for flagging me for review. I encourage people to flag me for
> reviews for anything relating to Mercurial.
> 
> The changes look good! Although, it's difficult to fully assess since I'm
> not familiar with the code and can't expand context in Splinter.

This being in webdev land, this is on git. I don't suspect you invest much energy in reviewboard for git?

> I saw your hglib bugs filed upstream. How urgent are fixes? If they are
> blocking this bug, I can probably patch hglib with a little effort.

Yeah, they're blocking. The install location is actually from a tarball on my bitbucket fork. Which is also why the version number is a bit of a lie in the requirements. I'd love to see those upstream, and I guess Matt, too. Just couldn't figure out the technicalities of the mailing list patch bomb stuff.

<...>

> @@ +27,5 @@
> >      def file(cls, repo, path, contents):
> > +        path = repo.pathto(path)
> > +        open(path, 'w').write(contents)
> > +        if repo.status(include=[path], all=True)[0][0] == '?':
> > +            repo.add(path)
> 
> With a little refactoring, you could probably use `hg commit -A` to auto add
> untracked files.

I'll ponder over this a bit. I like how an explicit add wouldn't touch things like .orig files or so. Need to look at that to see if that matters.

(In reply to Gregory Szorc [:gps] from comment #3)
> If repos share a common ancestor, they are the same repo. Easiest way to
> diff is to `hg pull` to a common repository and do the diffing that way.

Right, but I can't make the 'default' branch in my local clones ambiguous as it's referenced as a fallback in some places in my automation. I also on occasion use it for URL params, when I want to show a diff between an old version and whatever's current on aurora, for example. So I'll need some sort of port of firefoxtree first, and then make the automation use those labels. I'd love to find a timewindow to chat with you on how to best do that, some of the design principles of your addon won't work for l10n. Like when you tell by the root commit which repo you're in. That'd be unwieldy for 100 l10n repos, and growing.

Comment 5

[Axel Hecht [:Pike]]

Addressed the review comments and pushed a rebased branch to https://github.com/mozilla/elmo/compare/develop...Pike:feature/hgclient-no-diff.

Gregory, regarding the modifications to hglib, yeah, I'll need those. Could you help getting them uplifted?

I've uplifted them ontop of 1.6, which was pretty invasive due to the py3 support adding a ton of wallpaper for a lot of strings. No idea if I got things right, tbh :-/ . If you could help, I've pushed the current state of the three patches to https://bitbucket.org/pike/python-hglib/commits/all?search=c82671b%3A%3A

Comment 6

[Gregory Szorc [:gps]]

I left some comments on Bitbucket.

I think patches 2 and 3 are not very contentious and should be accepted easily. Patch 1 (pathto) may meet resistance, as I'm not sure the utility of it as part of the library. But it shouldn't hurt to submit it and see what happens.

Comment 7

[Axel Hecht [:Pike]]

Gotta get back to looking at those comments, flagging myself for that.

Comment 8

[Axel Hecht [:Pike]]

OK, I think I addressed Gregory's comments, rebased and pushed to bitbucket

https://bitbucket.org/pike/python-hglib/commits/all?search=fb2dee4..4278a1b has the commits.

I made an extra commit for dropping the old python versions, so that that's extra clear.

Gregory, can you take another look and help shepherd those in upstream? Thanks.

Comment 9

[Gervase Markham [:gerv]]

gps: ping? This bug is on the path to resolving bug 1123812, which has been open far too long.

Gerv

Comment 10

[Gervase Markham [:gerv]]

gps: reping?

Gerv

Comment 11

[Gregory Szorc [:gps]]

I'm starting to send these patches upstream. I'll clear the needinfo and close this bug once everything is accepted upstream.

If you have any additional feature requests, let me know in this bug.

Comment 12

[Axel Hecht [:Pike]]

Thanks. I'm looking at https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-July/085923.html, and I assume the log() is to address the problem of evolve cutting holes into the changeset numbers?

What's the perf impact on a repo like mozilla-central?

Though I would need to get off of such a code path anyway, as I want to replicate firefox-unified, and in that repo, the code path doesn't make sense to begin with. 

Seems like https://github.com/Pike/elmo/blob/7e8b0bb863578bb371abc0825dd207c5317bc9fd/apps/pushes/management/commands/fixrepos.py#L50 is bad with that patch?

Comment 13

[Gregory Szorc [:gps]]

Clearing my needinfo queue before I disappear for ~1 month. Please badger me about this when I return.

Comment 14

[[github robot]]

Commits pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/6eadd86e5207ae7ec0ee80d3d70fb872bdc1ef94
bug 1137668, use hglib to interact with mercurial repos, r=gps

I monkey-patched hglib to include
https://bz.mercurial-scm.org/show_bug.cgi?id=4510. Not great,
but still.

I will fix the diff view once we can work with unified repos, the
cross-repository file moves etc logic is tedious to replicate without
mercurial code.

https://github.com/mozilla/elmo/commit/3cb0571ef89588ccc4fd8eed94e522b4b2d33153
bug 1137668, remove --rebase option to localrepos command

As long as we can't get https://bz.mercurial-scm.org/show_bug.cgi?id=4644
landed in hglib, don't support --rebase.

https://github.com/mozilla/elmo/commit/ceee994370989b4db497e9305f20ef812b270f82
bug 1137668, make json-changesets not use raw mercurial

https://github.com/mozilla/elmo/commit/8f32e297dfc13358cf85d4af80b7ded350863a55
Merge bug 1137668 into develop

Comment 15

[[github robot]]

Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/737a1328285a89d595bfabb29b2b900762fbdb37
bug 1137668, follow-up to add virtualenv/bin to PATH, to pick up local hg.

Comment 16

[Axel Hecht [:Pike]]

Most of this landed, but not all usage of plain mercurial is gone.

https://bz.mercurial-scm.org/show_bug.cgi?id=4644, rebase on pull, I ignored, and just removed the feature from the django command.

As mentioned in the commit message, I monkeypatched in pathto, https://bz.mercurial-scm.org/show_bug.cgi?id=4510, for I can't land that.

The last one was IndexError, which is now a KeyError upstream, and I made the code work with that, so that ticket's actually gone on my list.

Note that a10n also was converted in https://github.com/Pike/a10n/commit/08ecddb0a89e30bb6b10203a7d78d966470efa82.

Left to do is still bug 1137667.

Comment 17

[Axel Hecht [:Pike]]

All dependencies and side stories have been fixed now, elmo and it's supporting infrastructure are now on vanilla hglib, no more mercurial imports.