#12058 closed defect (fixed)
Mercurial should not enable pager by default
Reported by: | jdemeyer | Owned by: | tbd |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.8 |
Component: | packages: standard | Keywords: | |
Cc: | Merged in: | sage-4.8.alpha3 | |
Authors: | Jeroen Demeyer | Reviewers: | Volker Braun |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description
The Mercurial spkg (#10594) created a file $SAGE_LOCAL/etc/mercurial/hgrc
enabling the "pager" extension. This is very bad for scripts using Mercurial and should be disabled.
Attachments (1)
Change History (19)
comment:1 follow-up: ↓ 3 Changed 11 years ago by
- Status changed from new to needs_info
comment:2 Changed 11 years ago by
As Volker pointed out on sage-devel, an even better way to avoid this is to set the HGPLAIN
environment variable.
comment:3 in reply to: ↑ 1 ; follow-up: ↓ 4 Changed 11 years ago by
Replying to kini:
Please explain what is very bad about this. Any problems caused by the pager extension being enabled in
$SAGE_LOCAL/etc/mercurial/hgrc
would equally be caused by the pager extension being enabled in$HOME/.hgrc
, which you will agree is not under our control.
True, but $SAGE_LOCAL/etc/mercurial/hgrc
is not under my control. I personally do not want a pager to be enabled, so I do not enable the pager extension in my .hgrc
. It's true that I have the environment variable PAGER
set, but that is used for many things (like e.g. EDITOR
), so I don't want to disable it just for Sage.
Even if we did set a pager, it is set to only apply to the commands annotate, cat, diff, log, glog, and qdiff. I question why one would need to use these in a script in the first place, as they are by nature interactive commands.
There are 6 occurences of hg diff
in the $SAGE_ROOT/local/bin/*
scripts, so I disagree.
Wrong, this was introduced in #10594. It is the Mercurial spkg which writes $SAGE_LOCAL/etc/mercurial/hgrc
. On #11121 I have made a comment that I did not want the pager extension to be enabled by default.
As Volker pointed out on sage-devel, an even better way to avoid this is to set the HGPLAIN environment variable.
True, but I think that is an orthogonal issue. Even if we do use HGPLAIN
in scripts, I think we still should not enable the pager extension by default.
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 11 years ago by
Replying to jdemeyer:
Even if we did set a pager, it is set to only apply to the commands annotate, cat, diff, log, glog, and qdiff. I question why one would need to use these in a script in the first place, as they are by nature interactive commands.
There are 6 occurences of
hg diff
in the$SAGE_ROOT/local/bin/*
scripts, so I disagree.
Yes, and all six of them are mysterious and seem unnecessary to me.
Wrong, this was introduced in #10594. It is the Mercurial spkg which writes
$SAGE_LOCAL/etc/mercurial/hgrc
. On #11121 I have made a comment that I did not want the pager extension to be enabled by default.
The spkg on #10594 fixes #10594, #11121, and #11120 in three separate commits to the mercurial repository in the spkg. There is zero discussion of pagers on the trac ticket for #10594, whereas there is substantial discussion of it on #11121. So I think it's fair to say that this was introduced by #11121, and not by #10594. But this is immaterial I suppose.
As Volker pointed out on sage-devel, an even better way to avoid this is to set the HGPLAIN environment variable.
True, but I think that is an orthogonal issue. Even if we do use
HGPLAIN
in scripts, I think we still should not enable the pager extension by default.
Do you propose to also revert #11142, which explicitly uses the pager extension?
True, but
$SAGE_LOCAL/etc/mercurial/hgrc
is not under my control. I personally do not want a pager to be enabled, so I do not enable the pager extension in my.hgrc
. It's true that I have the environment variablePAGER
set, but that is used for many things (like e.g.EDITOR
), so I don't want to disable it just for Sage.
Just to be clear, I think it was a bad idea, from an overall software design perspective, to entangle Mercurial with Sage, create wrappers for it in the Sage library, etc. in the first place. Maybe it was even a mistake to bundle Mercurial at all. But one of the main goals behind the way Sage is set up is to lower the barrier of entry and make things easier for beginner developers, and I think part of that goal involves making sage -hg
as user-friendly as possible. The functionality implemented by jhpalmieri at #11142 is part of this and requires the pager extension. So too is making sage -hg
understand your $PAGER
variable and show things with ANSI colors.
In short, it seems to me that sage -hg
is meant to be an easy tool for those who don't care about fine tuning their Mercurial. To my mind, Sage is a mathematical software distribution, and the real reason that Mercurial is distributed with it is not that it gives the user a Mercurial installation, but rather that it 1) allows Sage to use Mercurial to do various things with its scripts; and 2) allows you to use Mercurial from the command line or from the Sage prompt to do some minimal amount of actions that are necessary to contribute to Sage development. For more advanced things, or if you desire more control, you are free to use your own system Mercurial in whatever way you like - I certainly don't use sage -hg
myself in interactive shells. And if we are using $HGPLAIN
in scripts, the only objection remaining, as far as I can see, is the lack of control over the behavior of sage -hg
on the command line, which I don't think is a legitimate issue, assuming the above is true.
Also, I don't think this even really needs mentioning, but if there is really someone out there who cares so much, they can manually turn off the pager extension in their ~/.hgrc
by setting pager = !
. But I gather you are not one of these people, as you say on sage-devel that you don't use sage -hg
anyway.
Now, maybe you are of the opinion that all what I said above is totally wrong, and that Sage is supposed to be shipping Mercurial as a standalone application, just like it ships GAP, Python, etc., and that you should be able to use sage -hg
just like you would use hg
. Honestly, I could agree with that. That would make more sense with Burcin's prefix system, for example, which I hope one day Sage will eventually ship with.
But in that case most of #11121 doesn't make any sense anyway and most of $SAGE_LOCAL/etc/mercurial/hgrc
should be deleted (except the first two lines). What do you think? I am fine with that, if that's what people generally agree upon. In that case I would request that you start rejecting patches that are not in git format, though, as that was the original (and important) purpose of #11121. Luckily the patches on #11121 (i.e. not the commit in the spkg) mostly solve that problem.
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 13 Changed 11 years ago by
Replying to kini:
Replying to jdemeyer:
Even if we did set a pager, it is set to only apply to the commands annotate, cat, diff, log, glog, and qdiff. I question why one would need to use these in a script in the first place, as they are by nature interactive commands.
There are 6 occurences of
hg diff
in the$SAGE_ROOT/local/bin/*
scripts, so I disagree.Yes, and all six of them are mysterious and seem unnecessary to me.
Maybe you are right, but I do think that hg diff
can be useful in scripts sometimes, and hg log
also.
True, but I think that is an orthogonal issue. Even if we do use
HGPLAIN
in scripts, I think we still should not enable the pager extension by default.Do you propose to also revert #11142, which explicitly uses the pager extension?
No, I think using the pager within Sage is fine, analogously to how "is_prime?
" runs a pager. In Sage, there is no simple way to explicitly run a pager (in the shell, it's easy to type "hg diff |less
")
Just to be clear, I think it was a bad idea, from an overall software design perspective, to entangle Mercurial with Sage, create wrappers for it in the Sage library, etc. in the first place. Maybe it was even a mistake to bundle Mercurial at all.
I don't think it's a bad idea to bundle Mercurial. I actually like how the Sage scripts like sage-sdist
use Mercurial.
But one of the main goals behind the way Sage is set up is to lower the barrier of entry and make things easier for beginner developers, and I think part of that goal involves making
sage -hg
as user-friendly as possible.
Okay, agreed.
But in that case most of #11121 doesn't make any sense anyway and most of
$SAGE_LOCAL/etc/mercurial/hgrc
should be deleted (except the first two lines). What do you think? I am fine with that, if that's what people generally agree upon. In that case I would request that you start rejecting patches that are not in git format, though, as that was the original (and important) purpose of #11121. Luckily the patches on #11121 (i.e. not the commit in the spkg) mostly solve that problem.
Well, I think the "git" default is good to have, also the encoding being utf-8 (through HGENCODING
set in sage-env
). I never argued about that, only about the pager.
comment:6 Changed 11 years ago by
I changed my mind about HGPLAIN
: maybe it is the best solution.
comment:7 follow-up: ↓ 8 Changed 11 years ago by
If you call mercurial from within sage ('hg-sage' and friends) to diff then we pass --config pager.pager=...
in sage/misc/hg.py
. So whether or not the pager is enabled by default in the hgrc
is just a matter of taste.
My vote would be:
- Use
HGPLAIN
in scripts if necessary. - Keep the pager extension to intentionally break scripts that forget to set
HGPLAIN
.
comment:8 in reply to: ↑ 7 Changed 11 years ago by
Replying to vbraun:
If you call mercurial from within sage ('hg-sage' and friends) to diff then we pass
--config pager.pager=...
insage/misc/hg.py
. So whether or not the pager is enabled by default in thehgrc
is just a matter of taste.
The point is that --config pager.pager=...
won't work unless the pager extension is enabled. As far as I can tell there is no way to enable an extension on the fly from a command line call to Mercurial, which makes sense since extensions can have a rather deep effect on Mercurial's operation. In the hgrc
, no pager program is defined, which is a different matter (and is already what I think we all agree is better than defining a pager program there).
My vote would be:
- Use
HGPLAIN
in scripts if necessary.- Keep the pager extension to intentionally break scripts that forget to set
HGPLAIN
.
Sounds great to me. Keep in mind that the pager extension will not break scripts that forget to set $HGPLAIN
unless $PAGER
is also set while the script is run, or pager.pager
is set to something in some hgrc
somewhere.
Changed 11 years ago by
comment:9 follow-up: ↓ 12 Changed 11 years ago by
Proof-of-concept patch using HGPLAIN
. Note that I replaced sage -hg
by hg
in all scripts, hoping that SAGE_ROOT/local/bin
is in the path.
comment:10 Changed 11 years ago by
- Status changed from needs_info to needs_review
comment:11 Changed 11 years ago by
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
Looks good to me!
comment:12 in reply to: ↑ 9 Changed 11 years ago by
Replying to jdemeyer:
Note that I replaced
sage -hg
byhg
in all scripts, hoping thatSAGE_ROOT/local/bin
is in the path.
This is a good idea. If it doesn't work then something is broken and needs to be fixed.
comment:13 in reply to: ↑ 5 Changed 11 years ago by
Replying to jdemeyer:
Well, I think the "git" default is good to have
Sure, me too, but it also takes control away from the user, just like turning the pager extension on. The patches on #11121 which are not in the spkg cause sage to put .hg/.hgrc
files in the Sage mercurial repositories themselves. This means that sage -hg
will produce git-style patches when operating on a Sage repository, but not by default when operating on any other random repository on your system. So it's a more "vanilla" approach to leave those patches from #11121 intact and remove the git stuff from $SAGE_LOCAL/etc/mercurial/hgrc
. Just saying, in case we eventually decide to roll back #11121 and rewrite #11142.
(The first two lines of $SAGE_LOCAL/etc/mercurial/hgrc
are still necessary to make the previously mentioned .hgrc
files be accepted by a Mercurial running under a different user from whoever installed the Sage repos - another indication that maybe version control should not be integrated into the installation of a package, but should rather be managed by the user in user-owned directories, but that's another story.)
comment:14 Changed 11 years ago by
- Merged in set to sage-4.8.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:15 Changed 10 years ago by
BAD BAD BAD news for this on systems with nothing installed. Here is what I see with 4.7.2.
changeset: 16189:9e29a3d84c48 tag: tip user: Jeroen Demeyer <jdemeyer@cage.ugent.be> date: Sat Oct 29 12:50:17 2011 +0000 summary: 4.7.2 changeset: 16188:0f6304794835 user: Jeroen Demeyer <jdemeyer@cage.ugent.be> date: Sat Oct 29 09:36:26 2011 +0000 summary: Added tag 4.7.2 for changeset 55f2efa29bab changeset: 16187:55f2efa29bab tag: 4.7.2 user: Jeroen Demeyer <jdemeyer@cage.ugent.be> date: Sat Oct 29 09:36:25 2011 +0000 summary: Added tag 4.7.2.rc1 for changeset acc90fc8ee5d changeset: 16186:acc90fc8ee5d tag: 4.7.2.rc1 user: Jeroen Demeyer <jdemeyer@cage.ugent.be> date: Sat Oct 29 09:36:22 2011 +0000 summary: Added tag 4.7.2.rc0 for changeset af169997d6c8 :
But with 4.8.alpha5 (i.e., after this patch):
changeset: 3:6ee487e4d475 user: tornaria@math.utexas.edu date: Sat Feb 11 21:39:44 2006 +0000 summary: [project @ patch to sage-1.0.0.1] changeset: 2:a572d77184a3 user: tornaria@math.utexas.edu date: Sat Feb 11 21:36:43 2006 +0000 summary: [project @ patch to sage-1.0.0] changeset: 1:bf5417bc8931 user: tornaria@math.utexas.edu date: Sat Feb 11 21:33:33 2006 +0000 summary: [project @ patch to sage-0.10.13] changeset: 0:039f6310c6fe user: tornaria@math.utexas.edu date: Sat Feb 11 01:13:08 2006 +0000 summary: [project @ original sage-0.10.12]
after a lot of scrolling. No surprise, perhaps.
The whole point of hg_sage
is for it to be EASY for people to use. Now it is not. I'm not quite sure what happened in this ticket, and in general the changes of late with Mercurial and paging seemed ok, but this did something very different.
comment:16 Changed 10 years ago by
Oh, I think I see. See #12288 for a followup.
comment:17 follow-up: ↓ 18 Changed 10 years ago by
I wouldn't want to revert this ticket, because it would imply reverting #10594. Sage could unset the environment variable HGPLAIN or explicitly run less. This would be a fairly easy fix. I don't have the energy to write or review such a patch myself though.
Please explain what is very bad about this. Any problems caused by the pager extension being enabled in
$SAGE_LOCAL/etc/mercurial/hgrc
would equally be caused by the pager extension being enabled in$HOME/.hgrc
, which you will agree is not under our control. Furthermore we do not actually set a pager, so by default the pager extension doesn't even do anything, other than enable the command line option--pager=
. Even if we did set a pager, it is set to only apply to the commandsannotate
,cat
,diff
,log
,glog
, andqdiff
. I question why one would need to use these in a script in the first place, as they are by nature interactive commands. And even if for some reason you must use them, you can always run them with--pager=off
(and indeed you should do this anyway).This was introduced by #11121, not #10594 - please see the comments there for more discussion about this. See also #11142.