[PATCH 0 of 1 V2] checking for portable filenames

[Adrian Buehlmann]

On 2011-04-19 04:21, Matt Mackall wrote:
> By the way, people interested in the layering issues should look at
> this:
> 
> http://www.selenic.com/blog/?p=626

Since you bring this up here, I'd like to respond again onto your original
decision, which deemed my original patch a layering violation.

I am obeying that decision since you are the architect in charge of
mercurial, but I still think it would be ok to have functions in util that
call things on ui objects (which you rejected as a layering violation).

I'm referring to your message

On 2011-04-16 17:49, Matt Mackall wrote:
> On Sat, 2011-04-16 at 14:35 +0200, Adrian Buehlmann wrote:
> 
>> diff --git a/mercurial/util.py b/mercurial/util.py
>> --- a/mercurial/util.py
>> +++ b/mercurial/util.py
>> @@ -544,6 +544,23 @@
>>              return _("filename ends with '%s', which is not allowed "
>>                       "on Windows") % t
>>  
>> +def checkportable(ui, f):
> 
> I'm afraid that's a definite layering violation. We'll have to think of
> somewhere to put a helper function.

I don't want to lecture you on design, but let me just point out that I
haven't added any new import in the util module on my original change for
util, which was:

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -544,6 +544,23 @@
             return _("filename ends with '%s', which is not allowed "
                      "on Windows") % t
 
+def checkportable(ui, f):
+    '''Check if filename f is portable and warn or abort depending on config'''
+    checkfilename(f)
+    val = ui.config('ui', 'portablefilenames', 'warn')
+    lval = val.lower()
+    abort = os.name == 'nt' or lval == 'abort'
+    bval = parsebool(val)
+    if abort or lval == 'warn' or bval:
+        msg = checkwinfilename(f)
+        if msg:
+            if abort:
+                raise Abort("%s: %r" % (msg, f))
+            ui.warn(_("warning: %s: %r\n") % (msg, f))
+    elif bval is None and lval != 'ignore':
+        raise error.ConfigError(
+            _("ui.portablefilenames value is invalid ('%s')") % val)
+

I could argue that this piece of code depends on an abstract, opaque interface
of ui objects, using a contract that an ui object has:

- a member function config, which returns a config string, given section
  and config option name
- a function warn

So this piece of code would depend on that abstract interface of ui objects, but
not on the specific implementation of ui objects.

In C++-ish (my home turf), this would be:

  class ui_interface
  {
  public:
    virtual string config(const string& section, const string& name) = 0
    virtual warn(const string& msg) = 0
  };

which could be put in a header "ui_interface.h" and which wouldn't #include "ui.h".
"ui.cpp" *would* #include "ui_interface.h" and implement ui_interface by deriving
from it.

util's checkportable function would then take an ui_interface param (in C++-ish):

  void checkportable(ui_interface& ui, const string& f)
  {
    ..
  }

thus depending on "ui_interface.h" and not on "ui.h" nor "ui.cpp". It would only
depend on the fact that there's *some* kind of implementation of ui_interface.

So the module dependency graph would have:

                     util
                      ¦
                     ui_interface

with ui_interface being a pure abstract module (containing only declarations of
abstract interfaces). Which is not the same as the (truly bad)

                     util
                      ¦
                     ui

Robert C. Martin calls this the dependency inversion principle [1].

Of course, it can be done in Java and C# as well (and most definitely others too).

In Python's duck typing system, this abstract interface is implicit. It doesn't
need to be declared as in C++, but it's not as explicit then as in C++. 

I still think it would be great if we could use ui objects in module util in that
manner in mercurial.

[1] http://www.objectmentor.com/resources/articles/dip.pdf