Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#11142 closed enhancement (fixed)

clean up sage/misc/hg.py

Reported by: jhpalmieri Owned by: jason
Priority: minor Milestone: sage-4.7.2
Component: misc Keywords: sd31
Cc: kini Merged in: sage-4.7.2.alpha3
Authors: John Palmieri Reviewers: Karl-Dieter Crisman, Keshav Kini
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #10594 Stopgaps:

Description (last modified by kini)

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.

Attachments (8)

trac_11142-sage-hg.patch (34.8 KB) - added by jhpalmieri 8 years ago.
main sage repo
trac_11142-delta.patch (4.8 KB) - added by jhpalmieri 8 years ago.
diff between old "v2" and new one, for reference only
trac_11142-delta-part2.patch (1.8 KB) - added by jhpalmieri 8 years ago.
another diff, applied on top of the other one (patch to "import_patch")
trac_1142-reviewer.patch (4.9 KB) - added by kcrisman 8 years ago.
Apply after v2 patch
trac_11142-sage-hg.v2.patch (56.0 KB) - added by jhpalmieri 8 years ago.
main Sage repo
trac_11142-delta-2to3.patch (508 bytes) - added by jhpalmieri 8 years ago.
diff between v2 and v3 (for reference only)
trac_11142-sage-hg.v3.patch (56.1 KB) - added by jhpalmieri 8 years ago.
main Sage repo
trac_11142-reviewer.patch (4.9 KB) - added by kini 8 years ago.
renamed trac_1142-reviewer.patch

Download all attachments as: .zip

Change History (43)

comment:1 Changed 9 years ago by jhpalmieri

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by 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.

comment:3 follow-up: Changed 8 years ago by kcrisman

  • Reviewers set to Karl-Dieter Crisman
  • Status changed from needs_review to needs_work

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.

comment:4 Changed 8 years ago by kcrisman

  • Keywords sd31 added

comment:5 in reply to: ↑ 3 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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?

comment:6 follow-up: Changed 8 years ago by 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.

comment:7 in reply to: ↑ 6 Changed 8 years ago by 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".

comment:8 Changed 8 years ago by 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.

Changed 8 years ago by jhpalmieri

main sage repo

comment:9 Changed 8 years ago by jhpalmieri

  • Dependencies set to #10594
  • Description modified (diff)

comment:10 follow-up: Changed 8 years ago by 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?

comment:11 Changed 8 years ago by 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.

comment:12 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by 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.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by 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.

comment:14 Changed 8 years ago by 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...

comment:15 Changed 8 years ago by 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.

comment:16 Changed 8 years ago by kcrisman

  • Status changed from needs_review to needs_work
  • Work issues set to pop\neq push and a couple minor documentation things

comment:17 Changed 8 years ago by 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.

comment:18 in reply to: ↑ 13 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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.

Changed 8 years ago by jhpalmieri

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

comment:19 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Work issues pop\neq push and a couple minor documentation things deleted

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

Changed 8 years ago by jhpalmieri

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

comment:20 Changed 8 years ago by 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!

Changed 8 years ago by kcrisman

Apply after v2 patch

comment:21 Changed 8 years ago by kcrisman

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

comment:22 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from needs_review to positive_review

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

    diff --git a/sage/misc/hg.py b/sage/misc/hg.py
    a b class HG: 
    527527               %b   basename of the exporting repository
    528528               %h   short-form changeset hash (12 bytes of hexadecimal)
    529529               %n   zero-padded sequence number, starting at 1
    530                %r  zero-padded changeset revision number
     530               %r   zero-padded changeset revision number
    531531
    532532        -  ``text`` - boolean (default False).  Setting this to be True
    533533           has the same effect as passing the "-a" option below.

Changed 8 years ago by jhpalmieri

main Sage repo

comment:23 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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.

comment:24 Changed 8 years ago by 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

    diff --git a/sage/misc/hg.py b/sage/misc/hg.py
    a b  
    336336
    337337            sage: hg_sage._warning() # random
    338338        """
    339         if not os.path.exists(os.path.join(os.environ['HOME'], '.hgrc')):
     339        from sage.plot.plot import DOCTEST_MODE
     340        if not os.path.exists(os.environ['HOME'] + '/.hgrc') and not DOCTEST_MODE:
    340341            print "\nWARNING:"
    341342            print "Make sure to create a ~/.hgrc file:"
    342343            print "-"*70

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

comment:25 Changed 8 years ago by kini

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

comment:26 Changed 8 years ago by kini

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

comment:27 Changed 8 years ago by jhpalmieri

kini: Sorry, that was a mistake.

comment:28 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Okay, here is a new patch.

Changed 8 years ago by jhpalmieri

diff between v2 and v3 (for reference only)

Changed 8 years ago by jhpalmieri

main Sage repo

comment:29 Changed 8 years ago by kini

  • Reviewers changed from Karl-Dieter Crisman to Karl-Dieter Crisman, Keshav Kini
  • Status changed from needs_review to positive_review

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

comment:30 Changed 8 years ago by leif

  • Description modified (diff)

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

comment:31 follow-up: Changed 8 years ago by 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.

comment:32 in reply to: ↑ 31 Changed 8 years ago by 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.

Changed 8 years ago by kini

renamed trac_1142-reviewer.patch

comment:33 Changed 8 years ago by kini

  • Description modified (diff)

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

comment:34 Changed 8 years ago by leif

  • Merged in set to sage-4.7.2.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:35 Changed 8 years ago by jdemeyer

See #11898 for a problem probably caused by this.

Note: See TracTickets for help on using tickets.