[PATCH 1 of 9 pager v2] pager: move pager-initiating code into core

Matt Harbison mharbison72 at gmail.com
Sun Feb 26 00:25:03 EST 2017


On Sat, 25 Feb 2017 23:32:12 -0500, Augie Fackler <raf at durin42.com> wrote:

>
>> On Feb 25, 2017, at 11:22 PM, Matt Harbison <mharbison72 at gmail.com>  
>> wrote:
>>
>> On Thu, 16 Feb 2017 11:59:10 -0500, Augie Fackler <raf at durin42.com>  
>> wrote:
>>
>>> # HG changeset patch
>>> # User Augie Fackler <augie at google.com>
>>> # Date 1487198871 18000
>>> #      Wed Feb 15 17:47:51 2017 -0500
>>> # Node ID cd19a77477bcd18d1f7ab4fa73ee0dbf3c7a4e46
>>> # Parent  791b4e846a7b9a0783440b9504585438777fe2d2
>>> pager: move pager-initiating code into core
>>>
>>> No functionality change.
>>>
>>> A previous version of this API had a category argument on
>>> ui.pager(). As I migrated the commands in core, I couldn't come up
>>> with good enough consistency in any categorization scheme so I just
>>> scrapped the whole idea. It may be worth revisiting in the future.
>>>
>>> diff --git a/hgext/pager.py b/hgext/pager.py
>>> --- a/hgext/pager.py
>>> +++ b/hgext/pager.py
>>> +        # TODO: add a "system defaults" config section so this default
>>> +        # of more(1) can be easily replaced with a global
>>> +        # configuration file. For example, on OS X the sane default is
>>> +        # less(1), not more(1), and on debian it's
>>> +        # sensible-pager(1). We should probably also give the system
>>> +        # default editor command similar treatment.
>>> +        envpager = encoding.environ.get('PAGER', 'more')
>>> +        pagercmd = self.config('pager', 'pager', envpager)
>>
>> Any thoughts on what this looks like?  It seems like it might be  
>> distantly(?) related to something I tried a couple years ago [1], and  
>> the discussions around it.
>
> It’s pretty different from the previous round in my mind: it’d be a  
> config section, only advertised for sysadmins and packagers,  
> specifically intended for setting more-reasonable defaults, e.g.:
>
> (debian)
> [systemdefaults]
> editor = sensible-editor
> pager = sensible-pager
>
> (macOS)
> [systemdefaults]
> editor = nano
> pager = less
>
> (FreeBSD)
> [systemdefaults]
> editor = vi
> pager = less
>
> etc.
>
>> Does it seem reasonable to move the above to a method, similar to  
>> ui.editor()?  I'd like to detect MSYS and default to `less`.  IDK if we  
>> can pull that off with just a global config file.  (The only way I see  
>> to detect MSYS is to look for 'MSYSTEM' in env.)
>
> In line with the above, I’d expect an mays-backed package to set  
> systemdefaults.pager to less (maybe that’s not a thing? I dunno.)

I'm not thinking of an msys package, rather my personal usage.  If I'm  
doing anything of any substance on the command line, I'll use msys.  But  
TortoiseHg launches cmd.exe with a simple Ctrl + Shift + T, and sometimes  
that's good enough.  Therefore, I can't just set a pager value in  
%HOME%/mercurial.ini.  The hg.exe built from source and the one bundled  
with TortoiseHg are each visible to msys and cmd.exe, depending on what  
directory I'm in.  So even if TortoiseHg picked one (and we ignore hg  
built from the dev repo), it may or may not use the best pager available  
without a little help.

> I’m extremely hesitant to add any kind of conditional logic in hgrc.

Agreed.  The conditional logic I'm thinking of is in python, similar to  
how plan9 sets its last-ditch editor to 'E' in ui.geteditor().  Based on  
the vision you've described, I don't see a list as useful.  And because I  
can't think of any other OS with such a bad pager (and a better one so  
tantalizingly close), maybe a one-off python conditional is OK?

> Do you know of any other systems that use this kind of configuration
> approach but don’t have a scripting language as their config language?

IDK what this means, so I'm gonna say no. :-)

> Note that systemdefaults (which is a name I came up with in the process  
> of writing this mail, and probably isn’t what we should keep) would  
> *not* overwrite environment variables, so if the user has EDITOR or  
> PAGER set, that takes precedence over systemdefaults - this new config  
> section is explicitly about helping packagers get the best possible “out  
> of the box” experience without having to patch hg (which the debian  
> package currently has to for sensible-editor).

Makes sense.  This would get rid of the special case editor for plan9, for  
packages anyway.  It wouldn't for hg built from source, but that could  
presumably be set in ~/.hgrc.

>> In theory, the `less` command in GnuWin32 can be installed and made  
>> visible to cmd.exe too.  I wonder if (on some platforms anyway), there  
>> should be a list of default choices, and the first one found in $PATH  
>> used.  ui.editor() doesn't make sure the editor is valid, so I figured  
>> keeping that kind of check out of ui was purposeful.  A bad editor will  
>> produce an error that tries to indicate the problem (though I missed it  
>> the first time in the OS error output):
>>
>>    $ HGEDITOR=xzy ../hg ci
>>    'xzy' is not recognized as an internal or external command,
>>    operable program or batch file.
>>    abort: edit failed: xzy exited with status 1
>>
>> But not so much for the pager:
>>
>>    hg>set PAGER=less
>>    hg>hg help filesets
>>    'less' is not recognized as an internal or external command,
>>    operable program or batch file.
>
> Could you please file a bug for this? It reproduces on OS X too, with  
> equally mystifying results (I’m on a slow connection right now, so email  
> is tolerable but https is not).

Done.

>> I'm not sure if the right thing here is to error out, or to treat the  
>> pager as disabled.  Especially if it is on by default, with an env knob  
>> that isn't as obvious as the example above.  So it seems like we might  
>> need some sort of check, whether or not we try a list of defaults.
>
> git disables the pager, with a warning that it couldn’t exec the pager.  
> We probably should do something similar, I can’t think of anything else  
> reasonable to do.

Seems reasonable.  Not sure if the warning should come before or after the  
wall of text, but it would be an improvement either way.

>> [1]  
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2015-March/067563.html


More information about the Mercurial-devel mailing list