clean up sage/misc/hg.py

[jhpalmieri]

The attached patch cleans up hg.py in a few ways:

• it adds a few doctests, although many of them are marked "not tested", since otherwise it would try to import or export non-existent patches, etc.

• the function "pager" is rewritten, to use the Mercurial pager extension. This way, any highlighting is preserved when you use "hg_sage.diff()" or "hg_sage.log()" with the color extension enabled.

• the function "color" was added, to disable the Mercurial color extension in the notebook. Otherwise it produces some bad output: try putting

  [extensions]
  color =
  

  in your .hgrc file, modify some files in the Sage library, and type "hg_sage.status()" in the command line vs. the notebook.

---

Apply

1. trac_11142-sage-hg.v3.patch​
2. trac_11142-reviewer.patch​

to the Sage library.

[kcrisman]

This looks pretty good overall, though I haven't had time to review it quite properly yet. I really like this idea, thanks for doing it.

I would add a couple useful options on the revert and the patch, that one can use options='--all' with revert and rollback (instead of having to know the file name) and importing patch with options='--no-commit' which is VERY useful until one learns how to use queues.

[kcrisman]

Yup, you're right that it depends on adding the pager stuff to work (though not to apply). Overall this looks good.

Okay, I also have a question. Why the debug=False? You did a nice job with this and the pager, but shouldn't we print out the commands? Or is that just annoying to see? Just wondering.

'Needs work' for adding the very important options in the documentation to revert, rollback, and import. If we are going to update doc at all, this should happen.

[jhpalmieri]

Replying to kcrisman:

Okay, I also have a question. Why the debug=False? You did a nice job with this and the pager, but shouldn't we print out the commands? Or is that just annoying to see? Just wondering.

I thought it was little annoying to see. Typical output might be

cd "/Applications/sage/devel/sage" && hg diff   --config pager.pager="LESS='FSRX' less"

But I've changed it to True.

'Needs work' for adding the very important options in the documentation to revert, rollback, and import. If we are going to update doc at all, this should happen.

Okay, I've taken care of this.

Now, changing the pager as I've done changes the current behavior in several ways (from the command line, not the notebook: the notebook should behave the same way):

• (the option F says to quit the pager if everything fits on one page; this doesn't change the current behavior, but see option X below.)
• the option S says to truncate long lines rather than wrap them. Currently we wrap them, as in

  changeset:   15816:11586fb87f83
  user:        John Cremona <john.cremona@gmail.com> and Robert Bradshaw <robertwb@math.washington.ed
  u>
  date:        Fri Apr 22 15:50:26 2011 +0100
  summary:     Trac #11220: #11220: implement CM j-invariants and orders for quadratic fields
  

  Note lines 2-3. With the S option, line 3 would be omitted. Opinions?
• the option R says to preserve any color information in the output, so output from hg_sage.diff() would be colorized nicely. This is one of the points of this ticket, so I definitely want to keep this.
• the option X says to not clear the screen when you quit the pager. I happen to like this. However, if this is off but option F is on, then any output containing less than a screen displays briefly, and then the screen gets cleared, so it's gone. This is not Sage-specific: it reproduces the behavior when you run "hg diff" from the shell, with pager = LESS='FSR' less in the [pager] section of your .hgrc file.

My suggestion: we use either "FSRX" or "FRX", and probably the second of these is better. That's what the latest version of the patch uses.

Another question: should we try to have a way to configure the pager options? We can accept keyword arguments into the various hg commands and then pass them to the pager command, or we can do it like the way latex options are set. Is it worth it?

[kini]

Personally I don't like the F option (unpredictable behavior), X option (if I wanted to read the last screen of the paged stuff, I wouldn't have quit less), or S option (information loss), but that's just me.

ipython's native pager (for help lookups, etc.) acts most like FR, fwiw.

[jhpalmieri]

Replying to kini:

Personally I don't like the F option (unpredictable behavior), X option (if I wanted to read the last screen of the paged stuff, I wouldn't have quit less), or S option (information loss), but that's just me.

If I look at a docstring (which displays with a pager), sometimes I want to refer to the docstring when typing the next command, and then it's really frustrating that it went away when I typed "q" to quit the pager program. The same can happen with hg commands, although not as frequently, at least for me. However, it can also be nice to restore what was there before I started reading the docstring. We could add an option for any hg command which uses the pager: by default, it will open with "R", but with the option, it will open with "FRX".

[jhpalmieri]

Here are two more patches. One replaces the old patch, and this just changes the options for "less" to "R" instead of "FRX". The second adds lots of methods for Mercurial queues, and it brings doctest coverage up to 97% -- I can't figure out how to write a helpful and/or meaningful doctest for get_remote_file. So the second is more ambitious. If you think it belongs on another ticket, we can just work with the first version.

[jhpalmieri]

main sage repo

[kcrisman]

Hey John, I really like the second patch, so much good (if tedious) work on a lot of things. Have not yet tested it, and can't tonight, though it applies fine.

• Does hg_sage.revert() really just revert all files? I always needed options="--all", but maybe I don't any more.
• What happens if a user does not have queues enabled in their .hgrc but tries to use the queue commands? (My understanding is that #11121 doesn't really affect this patch/ticket, correct?)
• I haven't tried hg_sage.root but I assume this was enabled when the root directory finally got a repository?

[kini]

#11121 is actually resolved (more or less) by the spkg at #10594, as is #11120. #10594 is a dependency of this ticket. So, in effect, queues will be enabled.

[kcrisman]

• Does hg_sage.revert() really just revert all files? I always needed options="--all", but maybe I don't any more.

Yeah, this does not work.

sage: hg_sage.revert()
cd "/Users/.../sage-4.7.1/devel/sage" && hg revert 
abort: no files or directories specified; use --all to revert the whole repo

This is even after the patch and hg upgrade. We definitely need to have documentation for this. Ideally, we'd even catch it in hg_sage.revert itself. It takes a while to figure out you need options="--all" as a newbie.

Or we could change the behavior so that it does this.

• What happens if a user does not have queues enabled in their .hgrc but tries to use the queue commands? (My understanding is that #11121 doesn't really affect this patch/ticket, correct?)

Ok, Keshav, as long as you vouch for this. Changing my hgrc didn't seem to do anything (the desired behavior), but maybe the repository already had hg initialized with mq or something...

• I haven't tried hg_sage.root but I assume this was enabled when the root directory finally got a repository?

Yes. Cool! Now I can edit the README more effectively :)

Current patch (with R, not FRX) does indeed clear the screen after a short view in the pager is quit. I thought this was not the current behavior, but apparently I get that with my 4.7 and 4.4.4 installs as well, so I'm a bit confused.

Otherwise we just need to test a lot of this stuff, but it looks good.

[jhpalmieri]

Replying to kcrisman:

• Does hg_sage.revert() really just revert all files? I always needed options="--all", but maybe I don't any more.

Yeah, this does not work.

I actually think it's a bit dangerous to be able to revert without specifying "--all", so I think we should just fix the documentation, not change the behavior. I'll do this soon.

• What happens if a user does not have queues enabled in their .hgrc but tries to use the queue commands? (My understanding is that #11121 doesn't really affect this patch/ticket, correct?)

Ok, Keshav, as long as you vouch for this. Changing my hgrc didn't seem to do anything (the desired behavior), but maybe the repository already had hg initialized with mq or something...

As Keshav says, the latest Mercurial spkg turns on queues in SAGE_ROOT/local/etc/mercurial/hgrc, so this is not an issue. (This is why #10594 is a prerequisite for this ticket.)

• I haven't tried hg_sage.root but I assume this was enabled when the root directory finally got a repository?

Yes. Cool! Now I can edit the README more effectively :)

Right, ticket #9433, in particular trac_9433-sage-repo.2.patch, added this.

[kcrisman]

I actually think it's a bit dangerous to be able to revert without specifying "--all", so I think we should just fix the documentation, not change the behavior. I'll do this soon.

Can you at that time also make sure to document the (very useful) --no-commit option for import_patch?

Still testing...

[kcrisman]

Here's a BIG problem, but very easy to fix:

sage: hg_sage.qpop()
cd "/Users/.../sage-4.7.1/devel/sage" && hg qpush 
applying trac_11142-sage-hg.patch

Oops! That's the only one like that I found in the queues stuff, though.

[kcrisman]

But otherwise debug and verbose function properly, I tried lots of commands with various options, and it seems to work exactly the same as hg * from my command line (which uses the HG in Sage, of course). Correctly accesses internet. Passes tests. So I think that the issues in 'work issues' are probably all that needs to be fixed.

Nice work! Now that I know how to use queues, I might end up being even more productive having them in the command line Sage. Thanks.

[jhpalmieri]

Re the qpush/qpop thing: I'm just like trying to keep people on their toes.

Re the documentation: I wonder if Mercurial itself used to revert all files when there were no arguments given: the rest of the docstring seems to have been taken from the help message printed by "hg help revert". I've now replaced it with the current version of that message, edited slightly to use Sage hg_sage.... syntax instead of hg ... shell syntax.

There is more work to be done, perhaps: commands like "hg status" can take many options when executed from the shell, but the Sage implementation doesn't include any of them. If someone feels like opening up a follow-up ticket to implement things like that, they could. Also, it wouldn't hurt to make sure that our docstrings are in line with Mercurial's current docstrings. Since using Mercurial from the shell is pleasant enough, I would give these tasks a pretty low priority, as long our our docstrings don't say things which are completely wrong, like your catch with revert.

[jhpalmieri]

diff between old "v2" and new one, for reference only

[jhpalmieri]

Oops, forgot "--no-commit". Now it's there, too.

[jhpalmieri]

another diff, applied on top of the other one (patch to "import_patch")

[kcrisman]

There are a bunch of things that didn't seem to look so good in the documentation. Then I went ahead and made various other minor changes in a reviewer patch. You can pick and choose, or just say positive review. I tried to actually build the changes, but my Sage is funny sometimes and doesn't recognize changed files for the docbuild.

The only ones I would insist on are

-        To see the changes in this file since revision 10000:
+        To see the changes in this file since revision 10000::

and the stuff about emailing William with a bundle, which is totally outdated in several ways.

I also wonder if you wanted to use :: for the various options for the hg commands. It looks nice, but in other places (like plot documentation) we just use the Sphinx subitem syntax, I think, so maybe we shouldn't make those look like examples. I am okay with either one, just wanted to ask about this.

Positive review on the updates and patch, though. I'm already using it!

[kcrisman]

Apply after v2 patch

[kcrisman]

I'm sorry about the name. It's for 11142, of course.

[jhpalmieri]

The changes look good to me. Thanks for cleaning that up.

The nice thing about the "::" for the option lists is that it uses a fixed-width font, so the hyphens line up, etc. Speaking of which, I'm putting up a new version of my patch which just adds a missing space in one of these option lists:

sage/misc/hg.py

@@ -527 +527 @@
                %b   basename of the exporting repository
                %h   short-form changeset hash (12 bytes of hexadecimal)
                %n   zero-padded sequence number, starting at 1
-               %r  zero-padded changeset revision number
+               %r   zero-padded changeset revision number
 
         -  ``text`` - boolean (default False).  Setting this to be True
            has the same effect as passing the "-a" option below.

[jhpalmieri]

main Sage repo

[jdemeyer]

I get several doctest failures of the form

sage -t  -force_lib devel/sage/sage/misc/hg.py
**********************************************************************
File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage-main/sage/misc/hg.py", line 783:
    sage: hg_sage.rename('sage/misc/hg.py', 'sage/misc/hgnew.py', options='--dry-run')
Expected:
    Moving sage/misc/hg.py --> sage/misc/hgnew.py
    cd ... && hg mv --dry-run "sage/misc/hg.py" "sage/misc/hgnew.py"
Got:
    Moving sage/misc/hg.py --> sage/misc/hgnew.py
    <BLANKLINE>
    WARNING:
    Make sure to create a ~/.hgrc file:
    ----------------------------------------------------------------------
    [ui]
    username = William Stein <wstein@gmail.com>
    ----------------------------------------------------------------------
    <BLANKLINE>
    <BLANKLINE>
    cd "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage" && hg mv --dry-run "sage/misc/hg.py" "sage/misc/hgnew.py"
**********************************************************************

I believe the doctests should be written in such a way that they do not require a .hgrc file. Also, are the messages

Adding file module_list.pyc
cd "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.2.alpha2/devel/sage" && hg mv --dry-run "sage/misc/hg.py" "sage/misc/hgnew.py"

and

WARNING:
Make sure to create a ~/.hgrc file:
----------------------------------------------------------------------
[ui]
username = William Stein <wstein@gmail.com>
----------------------------------------------------------------------

written to stdout or stderr? The second one certainly should be written to stderr. If the first is written to stdout, the problem can be solved by capturing only stdout.

[jhpalmieri]

The warning about the .hgrc file is printed by the _warning method, not by Mercurial itself. What do you think about not printing that warning if doctesting? I'm considering this patch:

sage/misc/hg.py

@@ -336 +336 @@
 
             sage: hg_sage._warning() # random
         """
-        if not os.path.exists(os.path.join(os.environ['HOME'], '.hgrc')):
+        from sage.plot.plot import DOCTEST_MODE
+        if not os.path.exists(os.environ['HOME'] + '/.hgrc') and not DOCTEST_MODE:
             print "\nWARNING:"
             print "Make sure to create a ~/.hgrc file:"
             print "-"*70

With this, all tests pass even if I don't have a .hgrc file.

[kini]

Good idea. But why on earth is DOCTEST_MODE in sage.plot.plot?

[kini]

Er, also, why change the os.path.join to string concatenation?

[jhpalmieri]

kini: Sorry, that was a mistake.

[jhpalmieri]

Okay, here is a new patch.

[jhpalmieri]

diff between v2 and v3 (for reference only)

[jhpalmieri]

main Sage repo

[kini]

All doctests pass. Looks good! Positive review from me.

[leif]

It would certainly be better to reupload a patch with a misspelled name, i.e., ticket number.

[kini]

I don't think it matters - the filename of the patch isn't stored anywhere in the Mercurial history and the commit message itself is accurate.

[leif]

Replying to kini:

I don't think it matters - the filename of the patch isn't stored anywhere in the Mercurial history and the commit message itself is accurate.

Well, there's sometimes a small period of time between when a patch gets uploaded and when it gets merged.

[kini]

renamed trac_1142-reviewer.patch

[kini]

If it helps you with release managing, why not :)

[jdemeyer]

See #11898 for a problem probably caused by this.