Last Comment Bug 794580 - mach command to configure Mercurial
: mach command to configure Mercurial
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: mach (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Gregory Szorc [:gps]
:
Mentors:
: 774143 (view as bug list)
Depends on: 894197 902529 909867 909937 930988
Blocks: 899818
  Show dependency treegraph
 
Reported: 2012-09-26 12:14 PDT by Gregory Szorc [:gps]
Modified: 2013-10-28 10:22 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch against m-i (1.80 KB, patch)
2012-09-30 14:33 PDT, Nick Alexander :nalexander
no flags Details | Diff | Splinter Review
Patch against m-i, part 2 (14.20 KB, patch)
2012-09-30 14:33 PDT, Nick Alexander :nalexander
no flags Details | Diff | Splinter Review
Patch against m-i, part 3 (4.77 KB, patch)
2012-09-30 14:34 PDT, Nick Alexander :nalexander
no flags Details | Diff | Splinter Review
Part 2: Create |mach mercurial-configure| (4.71 KB, patch)
2013-06-22 18:43 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Part 1: Define state directory in mach context object; (5.57 KB, patch)
2013-06-24 16:39 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
mach mercurial-setup (5.60 KB, patch)
2013-07-15 20:20 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
mach mercurial-setup (5.83 KB, patch)
2013-07-15 20:25 PDT, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
mach mercurial-setup (17.71 KB, patch)
2013-07-19 16:18 PDT, Gregory Szorc [:gps]
nalexander: review+
Details | Diff | Splinter Review
mach mercurial-setup (17.54 KB, patch)
2013-07-29 14:21 PDT, Gregory Szorc [:gps]
nalexander: review+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-09-26 12:14:41 PDT
One of the hard parts of the initial developer experience is configuring your source control system to conform with Mozilla's standards. Unified diffs, 8 lines of context, proper author information, etc.

I think it would be awesome if we had a tool that could do this all for you.

My idea is there would be a mach command to configure your ~/.hgrc ~/.gitconfig (or the equivalent in your source tree - I'm pretty sure both Mercurial and Git inherit configs like that).

We could have the tool set up a handy set of default configs that work for 95% of people. In the case of Mercurial, we could even enable useful extensions, like mqext, progress, rebase, graphlog, etc. We could add the host fingerprint for hg.mozilla.org. For both Mercurial and Git, we could enable color output, proper PAGER integration, etc.

There's a lot we can do here. We should figure out a minimal useful shipping feature, ship it, and then iterate with additional improvements.
Comment 1 Gary Kwong [:gkw] [:nth10sd] 2012-09-26 23:54:17 PDT
> host fingerprint for hg.mozilla.org. For both Mercurial and Git, we could
> enable color output, proper PAGER integration, etc.

We should test if enabling color output works properly with MSYS in MozillaBuild. Last I tried I had issues, but they have gone away.
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-09-27 05:21:33 PDT
I couldn't get colorized pagered output to work right in hg on MozillaBuild. Do you have a config that works? I have it working nicely on Linux and Mac:
https://blog.mozilla.org/ted/2012/08/20/prettier-mercurial-output/
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2012-09-27 09:51:33 PDT
> We should test if enabling color output works properly with MSYS in
> MozillaBuild. Last I tried I had issues, but they have gone away.

I should have said they *might* have gone away. I guess I may have disabled colorized output specifically for Windows too. :-/
Comment 4 Nick Alexander :nalexander 2012-09-30 14:33:02 PDT
Created attachment 666381 [details] [diff] [review]
Patch against m-i
Comment 5 Nick Alexander :nalexander 2012-09-30 14:33:30 PDT
Created attachment 666382 [details] [diff] [review]
Patch against m-i, part 2
Comment 6 Nick Alexander :nalexander 2012-09-30 14:34:08 PDT
Created attachment 666383 [details] [diff] [review]
Patch against m-i, part 3
Comment 7 Nick Alexander :nalexander 2012-09-30 14:35:35 PDT
Hey!  Have a first cut at some of this.  Adds mach.ini [user] section with name and email settings, and then uses those to populate $TOPDIR/.hg/hgrc and $TOPDIR/.git/config.

This could go in many different directions, so I thought I'd put something out there.
Comment 8 Gregory Szorc [:gps] 2012-09-30 15:20:29 PDT
\o/ These patches make me happy.

I haven't looked at them thoroughly, but the general approach looks great!

One thing that worries me is the amount of low-level config file management code. In the case of Mercurial, it would be *very* tempting to import the Mercurial Python modules and do config file manipulation with the actual Mercurial APIs. 

In the case of Git, we may consider importing Dulwich (a Python implementation of Git) or a similar Git wrapper into the tree.

Or, we should consider invoking |hg config| and/or |git config| to query/manipulate the config files.

Something else to consider is how this will fit in with the overall mach workflow. I don't want to bloat scope on you, but I think it would be a cool idea if mach had a concept of bootstrap/initial setup. When you run mach, it ensures the "environment" is proper. If not, it "forces" you into running some commands (like version control configuration) to ensure everything is proper. You probably don't need to work on this. But, it's something to think about while you implement this.
Comment 9 Nick Alexander :nalexander 2012-09-30 18:02:48 PDT
(In reply to Gregory Szorc [:gps] from comment #8)
> \o/ These patches make me happy.

Huzzah!

> I haven't looked at them thoroughly, but the general approach looks great!
> 
> One thing that worries me is the amount of low-level config file management
> code. In the case of Mercurial, it would be *very* tempting to import the
> Mercurial Python modules and do config file manipulation with the actual
> Mercurial APIs.
> 
> In the case of Git, we may consider importing Dulwich (a Python
> implementation of Git) or a similar Git wrapper into the tree.

I wondered about doing exactly this, but how much of PyPy do we intend to ship in mozilla-central?  At the moment it looks like "not a lot".

> Or, we should consider invoking |hg config| and/or |git config| to
> query/manipulate the config files.

I did consider using |git config| and |hg config| to get *existing* user information for mach.ini, but didn't really consider using the command line interfaces for getting and setting the actual details.  I think this is an excellent idea and will move on it when I get a moment.
 
> Something else to consider is how this will fit in with the overall mach
> workflow. I don't want to bloat scope on you, but I think it would be a cool
> idea if mach had a concept of bootstrap/initial setup. When you run mach, it
> ensures the "environment" is proper. If not, it "forces" you into running
> some commands (like version control configuration) to ensure everything is
> proper. You probably don't need to work on this. But, it's something to
> think about while you implement this.

I kept Bug 774136 in mind while hacking this together.  The code I posted assumes that mach.ini has a few user settings, and I assume that the mach bootstrap would interrogate the environment/query the user for them.  I have added |pyrepl| to python/ and started a little firstrun package to experiment in this direction.
Comment 10 Gregory Szorc [:gps] 2012-09-30 18:10:20 PDT
(In reply to Nick Alexander :nalexander from comment #9)

> I wondered about doing exactly this, but how much of PyPy do we intend to
> ship in mozilla-central?  At the moment it looks like "not a lot".

Adding a package to mozilla-central is simple if it conforms to the following:

A) is MPL-compatible. Even non-MPL compatible licenses (like GPL) can be added - we just need to be a little more careful about how we use them)
B) Works on all Tier 1 platforms (and preferably all platforms Python runs on)
C) Doesn't contain Python C extensions (we can still add packages with C extensions, we just can't require them for the build... yet)
D) Works on our build infra

In general, if a package conforms to the above and provides a useful feature [that saves people time], then we can add it to mozilla-central with very few questions asked. If in doubt, just ask.
Comment 11 Gregory Szorc [:gps] 2012-09-30 18:29:41 PDT
*** Bug 774143 has been marked as a duplicate of this bug. ***
Comment 12 Ted Mielczarek [:ted.mielczarek] 2012-10-01 07:02:23 PDT
(In reply to Gregory Szorc [:gps] from comment #8)
> One thing that worries me is the amount of low-level config file management
> code. In the case of Mercurial, it would be *very* tempting to import the
> Mercurial Python modules and do config file manipulation with the actual
> Mercurial APIs. 

Note that you can't do this on Windows because the Mercurial we ship there bundles its own Python, so the modules for it aren't installed into the system Python.
Comment 13 :Ehsan Akhgari 2012-10-01 09:52:57 PDT
(In reply to comment #8)
> Or, we should consider invoking |hg config| and/or |git config| to
> query/manipulate the config files.

I think that would be the best option.  I'd try to avoid using Mercurial internals or Dulwich it at all possible, because they would both increase the maintenance cost.
Comment 14 Gregory Szorc [:gps] 2012-10-03 22:20:29 PDT
Sorry for not reviewing the code yet. I just wanted to say bug 794509 bitrots the argument parser integration. Basically, commands and their arguments are now Python decorators. And, the modules defining commands can now live anywhere in the source tree. As long as you have a mach/commands directory under a sys.path that mach is aware of (either from the virtualenv or from mach's fake virtualenv (see the source for the main mach script)), Python modules will be automatically loaded by mach and subcommands defined in them will be automatically discovered and registered with mach. Yay!
Comment 15 Gregory Szorc [:gps] 2012-10-09 12:56:05 PDT
Bug 799262 bit rots how module loading is done. All for the better, IMO.

Also, the new code probably shouldn't belong in python/mozbuild. Instead, we should create a new Python package/directory for this. python/mozversioncontrol perhaps? I'm open to better names.

That won't stop me from doing a proper review, however. But, if you could provide updated patches, that would be nice.
Comment 16 Nick Alexander :nalexander 2012-10-09 13:14:19 PDT
(In reply to Gregory Szorc [:gps] from comment #15)
> Bug 799262 bit rots how module loading is done. All for the better, IMO.
> 
> Also, the new code probably shouldn't belong in python/mozbuild. Instead, we
> should create a new Python package/directory for this.
> python/mozversioncontrol perhaps? I'm open to better names.

Originally, I had it in python/versioncontrol.  Happy to prefix with moz.
 
> That won't stop me from doing a proper review, however. But, if you could
> provide updated patches, that would be nice.

Let's leave review on this until I get time to post new patches.
Comment 17 Gregory Szorc [:gps] 2012-11-13 21:51:39 PST
Comment on attachment 666381 [details] [diff] [review]
Patch against m-i

Putting review on hold per last comment on bug. I hope we check this in someday...
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2012-12-28 14:30:22 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> I couldn't get colorized pagered output to work right in hg on MozillaBuild.
> Do you have a config that works? I have it working nicely on Linux and Mac:
> https://blog.mozilla.org/ted/2012/08/20/prettier-mercurial-output/

Mossop got it working, and I confirm that his method works!

http://www.oxymoronical.com/blog/2012/12/Making-Git-play-nice-on-Windows

I added:

[extensions]
color =

[color]
mode = win32

to my .hgrc and in the same place created a .profile file in the same location with the following line:

export PATH="/c/Program Files/Mercurial":$PATH

(I stripped out the git stuff as I didn't need it (yet), but the full line can be found in his blogpost)

and viola! colourised output on Windows.
Comment 19 Gary Kwong [:gkw] [:nth10sd] 2012-12-28 15:23:57 PST
> to my .hgrc and in the same place created a .profile file in the same
> location with the following line:

This .profile might be optional, if you don't have Mercurial installed separately in your Program Files directory.
Comment 20 Dave Townsend [:mossop] 2012-12-28 15:27:59 PST
(In reply to Gary Kwong [:gkw] from comment #19)
> > to my .hgrc and in the same place created a .profile file in the same
> > location with the following line:
> 
> This .profile might be optional, if you don't have Mercurial installed
> separately in your Program Files directory.

It is. Last I checked the mercurial in MozillaBuild didn't support win32 colouring though. That may well have changed.
Comment 21 Ted Mielczarek [:ted.mielczarek] 2013-01-02 08:51:21 PST
Ah is it just a matter of Hg 1.9.1 being a little too old?
Comment 22 Gregory Szorc [:gps] 2013-06-22 18:43:06 PDT
Created attachment 766374 [details] [diff] [review]
Part 2: Create |mach mercurial-configure|

It just downloads some extensions right now. We can expand scope over
time.
Comment 23 Gregory Szorc [:gps] 2013-06-24 16:39:46 PDT
Created attachment 766959 [details] [diff] [review]
Part 1: Define state directory in mach context object;

I accidentally uploaded this patch to bug 784580. jhammel was kind
enough to review it there.
Comment 24 Gregory Szorc [:gps] 2013-07-15 20:20:34 PDT
Created attachment 776139 [details] [diff] [review]
mach mercurial-setup

Nick: Since you and I are on the same page regarding making Mercurial
work better, I figured I'd ask you for review. AFAIK, /tools is a
shared, multi-module directory and the barrier to entry is "is it
useful." So, I think it's fine if you review this.

The current patch just downloads some Mercurial extensions and provides
instructions for enabling them. I'd like to follow up with a mode to
automatically enable these extensions. However, that requires editing
the .hgrc file. That will likely have to wait until configobj is added
to the tree (unless we want to do it real hackily).

There are of course other config options we could enable. Again, I think
it's best to wait until we have a mechanism for editing config files.
You have to start somewhere.
Comment 25 Gregory Szorc [:gps] 2013-07-15 20:25:44 PDT
Created attachment 776140 [details] [diff] [review]
mach mercurial-setup

Now touching a file after command invocation so a future enhancement can
stat the file and look at the mtime to determine if an extensions
refresh is recommended.
Comment 26 Gregory Szorc [:gps] 2013-07-16 14:16:38 PDT
Comment on attachment 776140 [details] [diff] [review]
mach mercurial-setup

I'll put up a new patch that modifies config files automatically. It's simple enough to get there...
Comment 27 Nick Alexander :nalexander 2013-07-16 14:19:44 PDT
Comment on attachment 776140 [details] [diff] [review]
mach mercurial-setup

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

::: build/mach_bootstrap.py
@@ +63,5 @@
>      'python/mozbuild/mozbuild/frontend/mach_commands.py',
>      'testing/marionette/mach_commands.py',
>      'testing/mochitest/mach_commands.py',
>      'testing/xpcshell/mach_commands.py',
> +    'tools/version-control-setup/mach_commands.py',

nit: Above, we put mozbuild/mach_commands.py before mozbuild/frontend/mach_commands.py.

::: tools/version-control-setup/mach_commands.py
@@ +84,5 @@
> +
> +        print('=' * 80)
> +        print('Ensuring trychooser extension is up to date...')
> +        try:
> +            vc.update_git_repo(git,

I'd like you to factor out |update_git_repo| like |update_hg_repo|, and to make these top-level on VersionControlCommands.  Eventually, we'll want to install |moz-git-tools| for git as well, and that command will want the functionality.

@@ +95,5 @@
> +        print(MERCURIAL_EXTENSION_INFO.format(prefix=ext_dir))
> +
> +        # Touch a file so we can periodically prompt to update extensions.
> +        state_path = os.path.join(self._context.state_dir,
> +            '.hgsetup.lastcheck')

nit: make the filename and the function name parallel.

::: tools/version-control-setup/vcssetup/versioncontrol.py
@@ +7,5 @@
> +import os
> +import subprocess
> +
> +
> +# The logic here is far from robust. Improvements are welcome.

Yeah, this is going to be interesting in the wild.
Comment 28 Gregory Szorc [:gps] 2013-07-19 16:18:42 PDT
Created attachment 778713 [details] [diff] [review]
mach mercurial-setup

Now that configobj is in the tree, we can perform some nifty hgrc
rewriting.

This version of the patch is better than the last in almost every way.

When you run |mach mercurial-setup| a wizard will take over and ensure:

* ui.username is defined
* Recommended [diff] settings are used
* Host fingerprints for hg.mozilla.org and bugzilla.mozilla.org are
  present
* Prompts for built-in mq, color, and progress extensions
* Prompts for 3rd party trychooser, bzexport, mqext, and qimportbz
  extensions

At the end, it even shows you a diff against the old hgrc before asking
whether to write out changes.

There is code to touch a file in the state directory at the end of the
wizard. In a future patch (possibly another bug), I'd like mach to nag
the user if it's been a while since the wizard has executed. That way,
if the wizard gains new functionality or extensions are updated, people
won't drift out of date.

Flagging Nick for review because why not. Flagging jdm for review
because I hear he likes contributor engagement and Python and this seems
like an obvious contributor engagement win since it will easily tackle a
lot of hurdles around configuring Mercurial.
Comment 29 :Ehsan Akhgari 2013-07-19 16:25:20 PDT
Should we also advertize the qbackout extension?  I ave found it immensely helpful.
Comment 30 Gregory Szorc [:gps] 2013-07-19 17:04:59 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #29)
> Should we also advertize the qbackout extension?  I ave found it immensely
> helpful.

Want to upload a part 2?
Comment 31 Gregory Szorc [:gps] 2013-07-19 17:07:19 PDT
Someone can file another bug for Git.
Comment 32 Jeff Hammel 2013-07-20 22:40:04 PDT
> In a future patch (possibly another bug), I'd like mach to nag
> the user if it's been a while since the wizard has executed. That
> way,
> if the wizard gains new functionality or extensions are updated,
> people
> won't drift out of date.

As long as we're thinking wishfully, I'd suggest nagging based on feature updates (however you want to track them; e.g. internal version #?) vs time (or maybe that's what you meant)
Comment 33 Gregory Szorc [:gps] 2013-07-20 22:45:22 PDT
(In reply to Jeff Hammel [:jhammel] from comment #32)
> > In a future patch (possibly another bug), I'd like mach to nag
> > the user if it's been a while since the wizard has executed. That
> > way,
> > if the wizard gains new functionality or extensions are updated,
> > people
> > won't drift out of date.
> 
> As long as we're thinking wishfully, I'd suggest nagging based on feature
> updates (however you want to track them; e.g. internal version #?) vs time
> (or maybe that's what you meant)

Probably both. We likely want to prompt to update if new functionality is added to the wizard. Since we're pulling external repositories, we want to prompt periodically (say once every week or two) to ensure those are up to date.
Comment 34 :Ehsan Akhgari 2013-07-22 10:59:55 PDT
(In reply to comment #30)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #29)
> > Should we also advertize the qbackout extension?  I ave found it immensely
> > helpful.
> 
> Want to upload a part 2?

Not really, I don't expect to use this code myself.  :-)
Comment 35 Gregory Szorc [:gps] 2013-07-22 15:15:19 PDT
Comment on attachment 778713 [details] [diff] [review]
mach mercurial-setup

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

::: python/mozversioncontrol/mozversioncontrol/repoupdate.py
@@ +9,5 @@
> +
> +
> +# The logic here is far from robust. Improvements are welcome.
> +
> +def update_hg_repo(hg, repo, path, revision='default'):

I just noticed we now have build/util/hg.py in the tree as of bug 763903. We should use this code somehow instead of reinventing it here.
Comment 36 Nick Alexander :nalexander 2013-07-24 08:28:21 PDT
Comment on attachment 778713 [details] [diff] [review]
mach mercurial-setup

gps: I'm at a work week and forgot to comment as such.  I expect to review this Friday this week (July 26th).
Comment 37 Nick Alexander :nalexander 2013-07-25 15:21:12 PDT
Comment on attachment 778713 [details] [diff] [review]
mach mercurial-setup

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

One general comment: let's include links to each repo and/or documentation page in each extension stanza.  I imagine devs will be curious about what each extension does and we should make that discovery process as easy as possible.  Otherwise, r+ with removing trychooser and considering nits!

::: python/mozversioncontrol/mozversioncontrol/repoupdate.py
@@ +9,5 @@
> +
> +
> +# The logic here is far from robust. Improvements are welcome.
> +
> +def update_hg_repo(hg, repo, path, revision='default'):

I agree.  Looking at that code, it's only used in a few places and I don't see any tests.  I'd rather see this land, and then test and unify with that code as a follow up.

::: tools/mercurial/hgsetup/config.py
@@ +63,5 @@
> +
> +        username = '%s <%s>' % (name, email)
> +        existing = self._c['ui'].get('username', '')
> +
> +        if existing == username:

Is this necessary?  The assignment below shouldn't hurt.  Does this avoid a write?  Is there a reason ConfigObj doesn't just ignore this update?

@@ +99,5 @@
> +
> +        if d.get('showfunc', None) != '1':
> +            return False
> +
> +        if d.get('unified', None) != '8':

What happens if the user has other diff settings?  We should verify that the user has *exactly* the recommended settings.

@@ +108,5 @@
> +    def ensure_recommended_diff_settings(self):
> +        if 'diff' not in self._c:
> +            self._c['diff'] = {}
> +
> +        d = self._c['diff']

Can this function be merged with the one above?  Perhaps updating the dict in place and returning a flag on update, or returning None or a new dict, or...

::: tools/mercurial/hgsetup/wizard.py
@@ +47,5 @@
> +'''.strip()
> +
> +BAD_DIFF_SETTINGS = '''
> +You don't have the recommended settings for producing patches. If you upload a
> +patch to Mozilla, it may be rejected because you aren't using an appropriate

This is true, but a little negative.  Could we reword to say something positive (not "reject") and unambiguous (not "may")?  Like "Mozilla developers produce patches in a standard format, but your Mercurial is not configured to produce patches in that format."?

@@ +121,5 @@
> +                print('')
> +
> +        update_bzexport = 'bzexport' in active
> +        if 'bzexport' not in active:
> +            if self._prompt_yn('Would you like a |hg bzexport| command to '

I don't think new developers will shorten Bugzilla to bz automatically.  Can we provide a line of context?

@@ +140,5 @@
> +                'Ensuring bzexport extension is up to date...')
> +
> +        update_trychooser = 'trychooser' in active
> +        if 'trychooser' not in active:
> +            if self._prompt_yn("Would you like a |hg try| command to easily "

New developers can't access try.  And configuring |hg try| requires correct paths (and SSH keys).  I'd like to see this as part of a separate follow-up ticket, aimed not at new developers but new committers.

@@ +170,5 @@
> +        if 'mq' in active:
> +            update_mqext = 'mqext' in active
> +            if 'mqext' not in active:
> +                if self._prompt_yn("Would you like your mq repository to "
> +                    "automatically commit changes as you modify patches"):

As someone who doesn't use mqext, I don't understand "automatically commit".  Can we improve the description?

@@ +219,5 @@
> +        diff = list(difflib.unified_diff(old_lines, new_lines,
> +            'hgrc.old', 'hgrc.new'))
> +
> +        if len(diff):
> +            print('Your Mercurial config file needs updated. I can do this '

nit: "needs updating" or "needs to be updated".

@@ +232,5 @@
> +                with open(config_path, 'wt') as fh:
> +                    c.write(fh)
> +                print('Wrote changes to %s.' % config_path)
> +            else:
> +                print('WARNING: hgrc changes not written!')

nit: I don't see scare-caps helping here; colour would be better.  If we think lots of people are going to get this far but hit no by accident, let's arrange to leave the new file somewhere and tell the user.

@@ +268,5 @@
> +    def _prompt_yn(self, msg):
> +        print('%s? [Y/n]' % msg)
> +
> +        while True:
> +            choice = raw_input().lower()

.strip()?

@@ +279,5 @@
> +
> +            if choice in ('n', 'no'):
> +                return False
> +
> +            print('Must reply with 1 of {yes, no, y, no}.')

nit: I find numerals in prose very odd.  Prefer 'one'.

::: tools/mercurial/mach_commands.py
@@ +30,5 @@
> +        result = wizard.run(os.path.expanduser('~/.hgrc'))
> +
> +        # Touch a file so we can periodically prompt to update extensions.
> +        state_path = os.path.join(self._context.state_dir,
> +            '.hgsetup.lastcheck')

Again: why is this not parallel to |mercurial*| throughout the source?
Comment 38 Ted Mielczarek [:ted.mielczarek] 2013-07-29 04:53:08 PDT
Someone should check that this does the right thing on Windows. The color/pager settings are tricky to get right there, and I configured Mercurial in MozillaBuild to work fairly sanely in the most recent version.
Comment 39 Gregory Szorc [:gps] 2013-07-29 14:18:11 PDT
(In reply to Nick Alexander :nalexander from comment #37)
> ::: tools/mercurial/hgsetup/config.py
> @@ +63,5 @@
> > +
> > +        username = '%s <%s>' % (name, email)
> > +        existing = self._c['ui'].get('username', '')
> > +
> > +        if existing == username:
> 
> Is this necessary?  The assignment below shouldn't hurt.  Does this avoid a
> write?  Is there a reason ConfigObj doesn't just ignore this update?

This was left over from an early version of the patch where I tracked the "dirty" status of the config and only attempted to write out if things changed. Now that we show a diff of the changes and prompt before writing out, I don't think it is necessary.

> @@ +99,5 @@
> > +
> > +        if d.get('showfunc', None) != '1':
> > +            return False
> > +
> > +        if d.get('unified', None) != '8':
> 
> What happens if the user has other diff settings?  We should verify that the
> user has *exactly* the recommended settings.

This is a double-edged sword, I think. I'd like to do what you say. However, I worry that there could be extensions supplementing [diff] settings and we wouldn't want to wipe those out!

> 
> ::: tools/mercurial/mach_commands.py
> @@ +30,5 @@
> > +        result = wizard.run(os.path.expanduser('~/.hgrc'))
> > +
> > +        # Touch a file so we can periodically prompt to update extensions.
> > +        state_path = os.path.join(self._context.state_dir,
> > +            '.hgsetup.lastcheck')
> 
> Again: why is this not parallel to |mercurial*| throughout the source?

?
Comment 40 Gregory Szorc [:gps] 2013-07-29 14:21:57 PDT
Created attachment 782771 [details] [diff] [review]
mach mercurial-setup

I think I hit most of the points of feedback. I don't believe anything
should be too contentious - but just want to be sure.
Comment 41 Nick Alexander :nalexander 2013-07-29 14:22:28 PDT
(In reply to Gregory Szorc [:gps] from comment #39)
> (In reply to Nick Alexander :nalexander from comment #37)
> > ::: tools/mercurial/hgsetup/config.py
> > @@ +63,5 @@
> > > +
> > > +        username = '%s <%s>' % (name, email)
> > > +        existing = self._c['ui'].get('username', '')
> > > +
> > > +        if existing == username:
> > 
> > Is this necessary?  The assignment below shouldn't hurt.  Does this avoid a
> > write?  Is there a reason ConfigObj doesn't just ignore this update?
> 
> This was left over from an early version of the patch where I tracked the
> "dirty" status of the config and only attempted to write out if things
> changed. Now that we show a diff of the changes and prompt before writing
> out, I don't think it is necessary.
> 
> > @@ +99,5 @@
> > > +
> > > +        if d.get('showfunc', None) != '1':
> > > +            return False
> > > +
> > > +        if d.get('unified', None) != '8':
> > 
> > What happens if the user has other diff settings?  We should verify that the
> > user has *exactly* the recommended settings.
> 
> This is a double-edged sword, I think. I'd like to do what you say. However,
> I worry that there could be extensions supplementing [diff] settings and we
> wouldn't want to wipe those out!

If you have extensions supplementing [diff] settings, you're squarely outside of the "mach command to configure mercurial" camp.  I think we should make this a tool to get new devs started and not a tool to keep existing devs up to date.  (My put: experienced devs won't want us writing .hgrc, ever.)

> > ::: tools/mercurial/mach_commands.py
> > @@ +30,5 @@
> > > +        result = wizard.run(os.path.expanduser('~/.hgrc'))
> > > +
> > > +        # Touch a file so we can periodically prompt to update extensions.
> > > +        state_path = os.path.join(self._context.state_dir,
> > > +            '.hgsetup.lastcheck')
> > 
> > Again: why is this not parallel to |mercurial*| throughout the source?
> 
> ?

There's this weird thing where half of mercurial references are to mercurial, and half are to hg.  The command is |mercurial-*|, but the code (and this file) say |.hg*|.  Let's just chose one and make future grep and mxr searching easy.
Comment 42 Nick Alexander :nalexander 2013-07-29 14:50:56 PDT
Comment on attachment 782771 [details] [diff] [review]
mach mercurial-setup

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

lgtm!

::: tools/mercurial/hgsetup/config.py
@@ +60,5 @@
> +        """
> +        if 'ui' not in self._c:
> +            self._c['ui'] = {}
> +
> +        username = '%s <%s>' % (name, email)

nit: strip()?
Comment 43 Gregory Szorc [:gps] 2013-07-29 17:02:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/57bf6621279b

Thanks a bunch for the review, Nick.
Comment 44 neil@parkwaycc.co.uk 2013-07-30 03:48:48 PDT
Note that if you're on Windows and want to use Mercurial in colour but are not using something like rxvt which natively handles ANSI colour codes then you'll probably also want to configure the pager extension to send Mercurial's colour codes to MSYS's less -FR which will interpret the colour codes for you. You'll need to ensure that pager is enabled for at least all the commands with colour enabled.
Comment 45 :Gijs Kruitbosch (Gone July 28 - Aug 11) 2013-07-30 03:51:37 PDT
Alright, seeing as you asked for it and I'm not sure if this is a bug or not, mercurial-setup suggested to change my .hgrc with:

-post-try = hg phase --force --draft "mq()"
+post-try = 'hg phase --force --draft "mq()"'


Why?
Comment 46 Ryan VanderMeulen [:RyanVM] 2013-07-30 10:22:22 PDT
https://hg.mozilla.org/mozilla-central/rev/57bf6621279b
Comment 47 Gregory Szorc [:gps] 2013-07-30 15:56:26 PDT
(In reply to :Gijs Kruitbosch from comment #45)
> Alright, seeing as you asked for it and I'm not sure if this is a bug or
> not, mercurial-setup suggested to change my .hgrc with:
> 
> -post-try = hg phase --force --draft "mq()"
> +post-try = 'hg phase --force --draft "mq()"'
> 
> 
> Why?

configobj probably wants to quote values with spaces by default. I have a hunch that if we passed list_values=False to the ConfigObj instance it wouldn't do this. I'll file a followup...

Note You need to log in before you can comment on or make changes to this bug.