[PATCH 1 of 2] ui: add normpathfn() to get a conversion function for ui.slash

[Patrick Mézard]

Le 20/08/12 01:52, Mads Kiilerich a écrit :
> Patrick Mezard wrote, On 08/18/2012 04:48 PM:
>> # HG changeset patch
>> # User Patrick Mezard <patrick at mezard.eu>
>> # Date 1345146027 -7200
>> # Node ID d5e4db676f3058b883426cb9a9a8fda3cdce145a
>> # Parent  a10f7eeb2588ae469b996288b0d2554ccbe409da
>> ui: add normpathfn() to get a conversion function for ui.slash
> 
> I agree with the general direction of this.
> 
> 'normpath' does a lot of normalization - much more than ui.slash should control according to its documentation. I think it would be better to introduce 'normslash' functionality which either would do nothing or replace '\' with '/' and nothing more.

OK, I can do that in a followup.

>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -2438,9 +2438,7 @@
>>       items = list(repo.walk(m))
>>       if not items:
>>           return
>> -    f = lambda fn: fn
>> -    if ui.configbool('ui', 'slash') and os.sep != '/':
>> -        f = lambda fn: util.normpath(fn)
>> +    f = ui.normpathfn()[0]
>>       fmt = 'f  %%-%ds  %%-%ds  %%s' % (
>>           max([len(abs) for abs in items]),
>>           max([len(m.rel(abs)) for abs in items]))
>> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
>> --- a/mercurial/dirstate.py
>> +++ b/mercurial/dirstate.py
>> @@ -132,8 +132,11 @@
>>           return ignore.ignore(self._root, files, self._ui.warn)
>>         @propertycache
>> -    def _slash(self):
>> -        return self._ui.configbool('ui', 'slash') and os.sep != '/'
>> +    def _slashfn(self):
> 
> I would call this _normslashfn.

OK.

> 
>> +        fn, normalizing = self._ui.normpathfn()
>> +        if normalizing:
>> +            return fn
>> +        return None
>>         @propertycache
>>       def _checklink(self):
>> @@ -201,8 +204,8 @@
>>           if cwd is None:
>>               cwd = self.getcwd()
>>           path = util.pathto(self._root, cwd, f)
>> -        if self._slash:
>> -            return util.normpath(path)
>> +        if self._slashfn:
>> +            return self._slashfn(path)
> 
> (It would have been a bit simpler here if util.pathto just emitted the right separater ... but that is a different story.)
> 
>>           return path
>>         def __getitem__(self, key):
>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>> --- a/mercurial/ui.py
>> +++ b/mercurial/ui.py
>> @@ -759,3 +759,13 @@
>>           ui.write(ui.label(s, 'label')).
>>           '''
>>           return msg
>> +
>> +    def normpathfn(self):
> 
> I would call this normslashfn.

OK.

> 
>> +        '''Return (fn, normalizing) where fn is identity if normalizing
>> +        is False, or a function taking a path and converting its native
>> +        separators to forward slashes. The behaviour is driven by
>> +        ui.slash.
>> +        '''
>> +        if self.configbool('ui', 'slash') and os.sep != '/':
>> +            return util.normpath, True
> 
> I would use a simple replace which is much cheaper than a full normpath. But yes, that is an unrelated change ;-)
> 
>> +        return (lambda p: p), False
> 
> The pattern of returning both a function and a flag indicating whether the function is trivial is IMO not pretty.
> 
> This is all about how Mercurial displays paths in its output. Will this "ever" be used in code paths where the possible overhead of running each filename through a trivial lambda function matters?

I did that to preserve dirstate.pathto() behaviour. I agree this is ugly. Basically I wanted the default behaviour to return an identity function but be able to pass None, and I was reluctant to have a lambda/function as default argument, but I will just do that instead.

> 
> If so: Wouldn't it be cleaner to return either a function or None - and leave it to the caller to add (what corresponds to) 'or lambda p: p' where necessary?

--
Patrick Mézard