# mercurial module needs review and feedback! **URL:** \ **Category:** Ansible Project\ **Created:** [January 13, 2013, 2:03am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200 "2013-01-13T02:03:41Z")\ **Posts on this page:** 20\ **Page:** 2
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 27, 2013, 4:49am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/21 "2013-01-27T04:49:47Z")
Thanks to you guys. 1. Yeah. Well, it’s actually quite a catch. hg clean requires purge to be enabled. But, the clean you proposed is hg update -C which discard any modified yet uncommitted changes. You are probably right that people will hate hg purge (oh I forgot to add files such and such). 2. I think the patch should be in ansible/ansible. 3. Let’s propose this: - clean (requires purge enabled, and we can do this ad-hoc on behalf of the user) - force (hg update -C) You think that’s a good idea? Yeukhon ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 27, 2013, 4:50am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/22 "2013-01-27T04:50:34Z")
That’s just basically adding another option. All the code is in there as we observe. John ---
**Author:** ![Michael\_DeHaan](https://avatars.discourse-cdn.com/v4/letter/m/c4cdca/32.png) [@Michael\_DeHaan](https://forum.ansible.com/u/Michael_DeHaan)\ **Post date:** [January 27, 2013, 2:42pm UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/23 "2013-01-27T14:42:20Z")
Yes, patches go to ansible/ansible. (Note: We prefer discussion on list and pull requests to RFE tickets and discussion in github, just for ease of not having them pile up, and more people can see discussions here.) ---
**Author:** ![Joel\_Roller](https://avatars.discourse-cdn.com/v4/letter/j/90ced4/32.png) [@Joel\_Roller](https://forum.ansible.com/u/Joel_Roller)\ **Post date:** [January 28, 2013, 4:28am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/24 "2013-01-28T04:28:15Z")
Few more notes from light testing and reading the code: 1. The state=absent|present|latest option seems both redundant with the revision option and the file module in general. state=absent has a bug where it calls rmtree only if the path does not exist, and returns changed either way. This could be done by the file module directly. state=present runs clone if it needs to or reports unchanged. It appears that this is distinguished from the “latest” option by the fact that it never does a pull other than an initial clone. state=latest doesn’t work if the destination doesn’t exist, and returns changed no matter what The whole thing seems overly complicated compared to the git module, for example. It’d be nice if a user could just specify the repo, dest, and let the hg module work out the details. Doing an hg pull (git fetch) between specified repo/dest shouldn’t be dangerous. The logic would be simpler to just say if exists, hg pull else hg clone. 1. hg verify gets called with every command. I worry about the hg verify being expensive in the case of large repositories. I’ve never run this before, but perhaps I’m not paranoid enough. hg verify: This will perform an extensive check of the repository’s integrity, validating the hashes and checksums of each entry in the changelog, manifest, and tracked files, as well as the integrity of their crosslinks and indices. ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 28, 2013, 4:40am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/25 "2013-01-28T04:40:37Z")
All of them are valid. 1. Wow. No one caught that bug! Thanks for that notice! Indeed. It should be if os.path.exists . 2. Somehow, I thought git and svn had states. Now I think about it, you are probably right. The initial pleasure of having states in the hg module is, well, I don’t need to call file module. If a few more lines can do all the job, it’s cool with me. I have big repo. verify is okay compared to hg clone. That’s 10 times slower than verify from my own experience. I have raised my own concern above about these health checks. Here is the revision from what I gather: repo, dest, clean, force, revision If any hg returns ‘.hg not found’, then it’s not a repo, return error. ---
**Author:** ![Joel\_Roller](https://avatars.discourse-cdn.com/v4/letter/j/90ced4/32.png) [@Joel\_Roller](https://forum.ansible.com/u/Joel_Roller)\ **Post date:** [January 28, 2013, 4:55am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/26 "2013-01-28T04:55:23Z")
> > 1. Somehow, I thought git and svn had states. > Now I think about it, you are probably right. The initial pleasure of having states in the hg module is, well, I don’t need to call file module. If a few more lines can do all the job, it’s cool with me. > > I have big repo. verify is okay compared to hg clone. That’s 10 times slower than verify from my own experience. I have raised my own concern above about these health checks. Verify could be ram and CPU bound if everything is cached, but it could also require a machine to dig a lot of history off the disk. Once everything is local and hot, everything should be fast. Just saying that rehashing the entire history is not a normal operation. > Here is the revision from what I gather: > > repo, dest, clean, force, revision I’d personally prefer clean to use the mercurial clean option, and force to be an alias to clean, because it mirrors what the git force command does. It’d be nice to use the word “purge” somewhere if you’re planning on purging. > If any hg returns ‘.hg not found’, then it’s not a repo, return error. Yes, and I wouldn’t try to hard to duplicate error checking that mercurial already does. If hg succeeds, it probably did the right thing, otherwise just give us hg’s error. -joel ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 28, 2013, 5:00am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/27 "2013-01-28T05:00:39Z")
Yeah. They all sound reasonable. I think Michael wouldn’t mind revision in a day or two. I will push a revised tomorrow. Well, actually, isn’t hg clean == hg purge? hg update -C seems to be git module’s force, based on description, so wouldn’t we have clean ===\> hg clean (which is also purge) force ===\> hg update -C since clean requires purge to be enabled, it will be off by defaul. force option is going to be yes by default ---
**Author:** ![Michael\_DeHaan](https://avatars.discourse-cdn.com/v4/letter/m/c4cdca/32.png) [@Michael\_DeHaan](https://forum.ansible.com/u/Michael_DeHaan)\ **Post date:** [January 28, 2013, 5:06am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/28 "2013-01-28T05:06:56Z")
> Few more notes from light testing and reading the code: > > 1. The state=absent|present|latest option seems both redundant with the > revision option and the file module in general. I doubt many people will use absent, so I'm fine with removing this. We should strive to make things look as close to this as possible, for consistency: [http://ansible.cc/docs/modules.html#git](http://ansible.cc/docs/modules.html#git) > 2. hg verify gets called with every command. I worry about the hg verify > being expensive in the case of large repositories. I've never run this > before, but perhaps I'm not paranoid enough. hg just strikes me as slower than git in general, but I've never run hg verify on a repository in a year plus of working at an hg shop. It seems we could remove this. ---
**Author:** ![Michael\_DeHaan](https://avatars.discourse-cdn.com/v4/letter/m/c4cdca/32.png) [@Michael\_DeHaan](https://forum.ansible.com/u/Michael_DeHaan)\ **Post date:** [January 28, 2013, 5:07am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/29 "2013-01-28T05:07:57Z")
Also, you have 4 days before we get to the point where we do not want to change what the options are. If we can get this done before then (release), that's good, else we risk changing module signatures later. Generally this is something we would like to avoid. ---
**Author:** ![Joel\_Roller](https://avatars.discourse-cdn.com/v4/letter/j/90ced4/32.png) [@Joel\_Roller](https://forum.ansible.com/u/Joel_Roller)\ **Post date:** [January 28, 2013, 5:10am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/30 "2013-01-28T05:10:51Z")
Hah, oh wow, looks like we got our signals crossed. I’ve been using “clean” to mean the “–clean” option to hg update, ie: update working directory (or switch revisions) -C --clean discard uncommitted changes (no backup) I didn’t realize the purge extension provided an alias providing a “hg clean” command. I blame mercurial for that. Maybe I should switch to git. 🙂 I do think that this sort of behavior should be the default, I just thought disabling it is a nice option. The purge extension is another thing I’ve never had to use before! -joel ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 28, 2013, 5:13am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/31 "2013-01-28T05:13:46Z")
Yes. It’s a big big issue with the naming. Well, actually, people new to git always say git’s naming convention sucks. lol I guess this is the only case where hg sucks with name. clean, purge, Let’s just finalzie this mess I created, which I apologize but glad we have feedback. options: repo, dest, clean, force(which does hg update -C) force is defaults to yes as git does, but clean is no because it requires editing hgrc file. I will rely on hg error as much as possible. John ---
**Author:** ![Joel\_Roller](https://avatars.discourse-cdn.com/v4/letter/j/90ced4/32.png) [@Joel\_Roller](https://forum.ansible.com/u/Joel_Roller)\ **Post date:** [January 28, 2013, 5:24am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/32 "2013-01-28T05:24:00Z")
Maybe can we just change the option “clean” to be “purge”? That is the actual command name for purging. “clean” is stated as an alias. I see mercurial tell me about “–clean” option every time I try to update a dirty working directory that “crosses branches”. I wouldn’t want other users to be caught in this same confusion. ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 28, 2013, 5:40am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/33 "2013-01-28T05:40:04Z")
Yes. Sure. I assume people would use hg clean more often. Let’s pt that in doc. ---
**Author:** ![Brad\_Olson](https://avatars.discourse-cdn.com/v4/letter/b/779978/32.png) [@Brad\_Olson](https://forum.ansible.com/u/Brad_Olson)\ **Post date:** [January 28, 2013, 11:04pm UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/34 "2013-01-28T23:04:23Z")
Lurking the conversation. Sorry I contribute this month, guys. Nice work. Good discussion. Not clear on how you came down on the clean option, but IMHO I think the hg module should either 1) run without extensions (in other words, no purge), or 2) install the needed extensions. FWIW, ‘hg up -C’ accomplishes what a purge does, but ‘hg purge’ does less than ‘hg up -C’. Sorry if I haven’t absorbed all the nuances of the discussion. Thanks for the work, Joel & Yeukhon! --Bradobro ---
**Author:** ![Ahmad\_Khayyat](https://avatars.discourse-cdn.com/v4/letter/a/858c86/32.png) [@Ahmad\_Khayyat](https://forum.ansible.com/u/Ahmad_Khayyat)\ **Post date:** [January 28, 2013, 11:36pm UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/35 "2013-01-28T23:36:15Z")
I agree that “clean” is confusing: it could mean “update --clean” or “clean” (in the presence of the purge extension). So, I suggest not to use it at all. The git module uses “force”. The built-in behavior of mercurial that is closest to what “force” does is “update --clean”, which resets all tracked files to the parent changeset state, leaving untracked files intact. Users would expect the built-in behavior. I wouldn’t expect the behavior of “purge”, knowing that it is an extension that needs to be enabled. Then, “purge” can still be added to to delete untracked files, without causing any confusion. “purge” has no other meaning. In summary: “force”: “update --clean” “purge”: “purge” (extension) ---
**Author:** ![Michael\_DeHaan](https://avatars.discourse-cdn.com/v4/letter/m/c4cdca/32.png) [@Michael\_DeHaan](https://forum.ansible.com/u/Michael_DeHaan)\ **Post date:** [January 29, 2013, 2:26am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/36 "2013-01-29T02:26:37Z")
Extensions should not be the default, and I like keeping it having the same "API" as the git module, so let's make it like the git one. purge can be there but should yell if the extension isn't there. ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 29, 2013, 2:33am UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/37 "2013-01-29T02:33:17Z")
Let me revisit the summary above that I made last night: repo dest purge =\> default to no, requires extension. it;s an option (hg clean == hg purge, we can be explicit here called this option purge rather than clean) force =\> default to yes like git, which discard uncommited changes through hg update -C I think this is the best we could do. John ---
**Author:** ![Michael\_DeHaan](https://avatars.discourse-cdn.com/v4/letter/m/c4cdca/32.png) [@Michael\_DeHaan](https://forum.ansible.com/u/Michael_DeHaan)\ **Post date:** [January 29, 2013, 1:37pm UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/38 "2013-01-29T13:37:39Z")
+1 make it happen! ---
**Author:** ![John\_Wong](https://avatars.discourse-cdn.com/v4/letter/j/6f9a4e/32.png) [@John\_Wong](https://forum.ansible.com/u/John_Wong)\ **Post date:** [January 29, 2013, 10:38pm UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/39 "2013-01-29T22:38:18Z")
Please review it on my fork: [https://github.com/yeukhon/ansible/blob/devel/library/hg](https://github.com/yeukhon/ansible/blob/devel/library/hg) I tried to follow git module implementation as much as possible. Have tested with all options possible. Here is log with -vv double verbose 😃 [https://gist.github.com/4668576](https://gist.github.com/4668576) ---
**Author:** ![Michael\_DeHaan](https://avatars.discourse-cdn.com/v4/letter/m/c4cdca/32.png) [@Michael\_DeHaan](https://forum.ansible.com/u/Michael_DeHaan)\ **Post date:** [January 29, 2013, 11:11pm UTC](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200/40 "2013-01-29T23:11:50Z")
Just use "%s foo" % (vars) instead of the ".format" stuff please. It's cleaner, and also 2.4 compatible. I'll leave testing to those with more needs for hg usage. [Previous page](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200.md?page=1) [Next page](https://forum.ansible.com/t/mercurial-module-needs-review-and-feedback/12200.md?page=3)