Patchwork D3439: template filters: add commonprefix

login
register
mail settings
Submitter phabricator
Date May 6, 2018, 1:13 a.m.
Message ID <differential-rev-PHID-DREV-mxwcohnn3gpi574zm7nt-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/31265/
State Superseded
Headers show

Comments

phabricator - May 6, 2018, 1:13 a.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The commonprefix filter takes a list of files names like files() and
  returns the longest directory name common to all elements.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

AFFECTED FILES
  mercurial/templatefilters.py
  tests/test-template-filters.t

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - May 6, 2018, 12:28 p.m.
Can you add test for failure cases? such as

* `str|commonprefix`
* `int|commonprefix`
* nothing common (e.g. `["/foo", "bar"]`)
* empty list
* paths normalized by `os.path.normcase()`

`os.path.commonprefix()` isn't what we want, but it might provide some hints.
phabricator - May 6, 2018, 12:29 p.m.
yuja added a comment.


  Can you add test for failure cases? such as
  
  - `str|commonprefix`
  - `int|commonprefix`
  - nothing common (e.g. `["/foo", "bar"]`)
  - empty list
  - paths normalized by `os.path.normcase()`
  
  `os.path.commonprefix()` isn't what we want, but it might provide some hints.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: yuja, mercurial-devel
Yuya Nishihara - May 7, 2018, 1:08 p.m.
"`os.path.commonprefix()` isn't what we want, but it might provide some hints."

Perhaps `os.path.commonprefix()` could be used with conditional
fixup by `os.path.dirname()`?

```
prefix = os.path.commonprefix(files)
if prefix isn't a valid path:
    prefix = os.path.dirname(prefix)
```

This way, we can remove `normpath()` which has a side effect `s|/|\\|g`
on Windows.
phabricator - May 7, 2018, 1:09 p.m.
yuja added a comment.


  "`os.path.commonprefix()` isn't what we want, but it might provide some hints."
  
  Perhaps `os.path.commonprefix()` could be used with conditional
  fixup by `os.path.dirname()`?
  
    prefix = os.path.commonprefix(files)
    if prefix isn't a valid path:
        prefix = os.path.dirname(prefix)
  
  This way, we can remove `normpath()` which has a side effect `s|/|\\|g`
  on Windows.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - May 9, 2018, 5:51 p.m.
joerg.sonnenberger added a comment.


  {str|commonprefix} is not really interesting since it is naturally an iterable of text. The others are covered, the routine tries to optimize a couple of common cases as well now.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: yuja, mercurial-devel
phabricator - June 12, 2018, 9:20 p.m.
martinvonz added a comment.


  Do you think we should call it `commonpath()` or `commondir()` instead in case we want `commonprefix()` to work for any string in the future (and not care about path separators)?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel
phabricator - June 12, 2018, 9:46 p.m.
joerg.sonnenberger added a comment.


  In https://phab.mercurial-scm.org/D3439#58405, @martinvonz wrote:
  
  > Do you think we should call it `commonpath()` or `commondir()` instead in case we want `commonprefix()` to work for any string in the future (and not care about path separators)?
  
  
  `commondir()` would be fine as well. `commonpath()` doesn't feel better. I'm ambivalent about either name.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel
phabricator - June 12, 2018, 9:50 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3439#58410, @joerg.sonnenberger wrote:
  
  > In https://phab.mercurial-scm.org/D3439#58405, @martinvonz wrote:
  >
  > > Do you think we should call it `commonpath()` or `commondir()` instead in case we want `commonprefix()` to work for any string in the future (and not care about path separators)?
  >
  >
  > `commondir()` would be fine as well. `commonpath()` doesn't feel better. I'm ambivalent about either name.
  
  
  I'd vote for changing it then. Both because I think it might make it clearer that it's the common prefix of "foo/bar" and "food" is not "foo" and because, as I said, we might want such a function later and it would be unfortunate if the name was taken. Btw, I just checked and we already have `dirname()` and `stripdir()` functions that would match `commondir()`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel
Yuya Nishihara - June 13, 2018, 12:19 p.m.
>   > `commondir()` would be fine as well. `commonpath()` doesn't feel better. I'm ambivalent about either nam>
>   I'd vote for changing it then. Both because I think it might make it clearer that it's the common prefix of "foo/bar" and "food" is not "foo" and because, as I said, we might want such a function later and it would be unfortunate if the name was taken. Btw, I just checked and we already have `dirname()` and `stripdir()` functions that would match `commondir()`.

+1 for `commondir()`. It matches the actual behavior, which is the last filename
part is always stripped.
phabricator - June 13, 2018, 12:31 p.m.
yuja added a comment.


  >   > `commondir()` would be fine as well. `commonpath()` doesn't feel better. I'm ambivalent about either nam>
  >   I'd vote for changing it then. Both because I think it might make it clearer that it's the common prefix of "foo/bar" and "food" is not "foo" and because, as I said, we might want such a function later and it would be unfortunate if the name was taken. Btw, I just checked and we already have `dirname()` and `stripdir()` functions that would match `commondir()`.
  
  +1 for `commondir()`. It matches the actual behavior, which is the last filename
  part is always stripped.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3439

To: joerg.sonnenberger, #hg-reviewers
Cc: martinvonz, yuja, mercurial-devel

Patch

diff --git a/tests/test-template-filters.t b/tests/test-template-filters.t
new file mode 100644
--- /dev/null
+++ b/tests/test-template-filters.t
@@ -0,0 +1,4 @@ 
+  $ hg debugtemplate '{"foo/bar\nfoo/baz\n"|splitlines|commonprefix}\n'
+  foo
+  $ hg debugtemplate '{"foo/bar\nbar/baz"|splitlines|commonprefix}\n'
+  
diff --git a/mercurial/templatefilters.py b/mercurial/templatefilters.py
--- a/mercurial/templatefilters.py
+++ b/mercurial/templatefilters.py
@@ -99,6 +99,26 @@ 
     """
     return os.path.basename(path)
 
+@templatefilter('commonprefix')
+def commonprefix(filelist):
+    """List of text. Treats each list item as file name, and returns
+    the longest common directory prefix shared by all list items.
+    Returns the empty string if no common prefix exists.
+    For example, ["foo/bar/baz", "foo/baz/bar"] becomes "foo" and
+    ["foo/bar", "baz"] becomes "".
+    """
+    def common(a, b):
+        while a != b:
+            if len(a) > len(b):
+                a = os.path.dirname(a)
+            else:
+                b = os.path.dirname(b)
+        return a
+    if not filelist:
+        return ""
+    dirlist = [os.path.dirname(os.path.normpath(f)) for f in filelist]
+    return reduce(common, dirlist)
+
 @templatefilter('count')
 def count(i):
     """List or text. Returns the length as an integer."""