This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3439

templatefilters: add commonprefix
ClosedPublic

Authored by joerg.sonnenberger on May 5 2018, 9:13 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG56dd15178190: templatefilters: add commonprefix
Summary

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

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

yuja added a subscriber: yuja.May 6 2018, 8:29 AM

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.

yuja added a comment.May 7 2018, 9:09 AM

"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.

joerg.sonnenberger retitled this revision from template filters: add commonprefix to templatefilters: add commonprefix.May 8 2018, 11:18 AM
joerg.sonnenberger updated this revision to Diff 8549.
yuja added a comment.May 9 2018, 8:46 AM

Please add tests of edge cases.

  • str|commonprefix
  • infinite loop (e.g. ["/foo", "bar"])
  • exact match (e.g. ["/foo"] and ["/foo", "/foo"])
  • empty list

os.path.commonprefix() will provide some hints to avoid comparison of
all list elements by using min()/max().

joerg.sonnenberger added a comment.May 9 2018, 1:51 PM

{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.

yuja added a comment.May 10 2018, 8:40 AM

+ $ hg debugtemplate '{"foo/bar\nfoo/bar\n"|splitlines|commonprefix}\n'
+ foo

should be "foo/bar"

+ $ hg debugtemplate '{"/foo/bar\n/foo/bar\n"|splitlines|commonprefix}\n'
+ foo

should be "/foo/bar"

+ $ hg debugtemplate '{"/foo\n/foo\n"|splitlines|commonprefix}\n'
+

should e "/foo"

FWIW, I doubt if scanning all elements with the seen set is faster than
calling min()/max() to pick the farthest pair because Python is slow.

{str|commonprefix} is not really interesting since it is naturally an
iterable of text.

I think it's better to error out, but that could be addressed later.

Closed by commit rHG56dd15178190: templatefilters: add commonprefix (authored by joerg.sonnenberger). · Explain WhyJun 1 2018, 2:00 AM
This revision was automatically updated to reflect the committed changes.
yuja added a comment.Jun 2 2018, 11:35 PM

For the record, I still believe the commonprefix of a pair of identical paths
should be the path itself as it is the longest sub-component of these paths.

+ $ hg debugtemplate '{"foo/bar\nfoo/bar\n"|splitlines|commonprefix}\n'
+ foo

should be "foo/bar"

And removing the leading slash is awkward.

+ $ hg debugtemplate '{"/foo/bar\n/foo/bar\n"|splitlines|commonprefix}\n'
+ foo

should be "/foo/bar"

martinvonz added a subscriber: martinvonz.Jun 12 2018, 5:20 PM

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)?

joerg.sonnenberger added a comment.Jun 12 2018, 5:46 PM
In 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.

martinvonz added a comment.Jun 12 2018, 5:50 PM
In D3439#58410, @joerg.sonnenberger wrote:
In 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().

yuja added a comment.Jun 13 2018, 8:31 AM
> `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.

Revision Contents
Changeset List

Diff 8549

mercurial/templatefilters.py

@templatefilter('basename', intype=bytes) @templatefilter('basename', intype=bytes)
def basename(path): def basename(path):
"""Any text. Treats the text as a path, and returns the last """Any text. Treats the text as a path, and returns the last
component of the path after splitting by the path separator. component of the path after splitting by the path separator.
For example, "foo/bar/baz" becomes "baz" and "foo/bar//" becomes "". For example, "foo/bar/baz" becomes "baz" and "foo/bar//" becomes "".
""" """
return os.path.basename(path) 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.
The list items are not normalized, i.e. "foo/../bar" is handled as
file "bar" in the directory "foo/..".
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
try:
if not filelist:
return ""
dirlist = [os.path.dirname(f) for f in filelist]
return reduce(common, dirlist)
except TypeError:
raise error.ParseError(_('argument is not a list of text'))
@templatefilter('count') @templatefilter('count')
def count(i): def count(i):
"""List or text. Returns the length as an integer.""" """List or text. Returns the length as an integer."""
try: try:
return len(i) return len(i)
except TypeError: except TypeError:
raise error.ParseError(_('not countable')) raise error.ParseError(_('not countable'))

tests/test-template-filters.t

  • This file was added.
$ hg debugtemplate '{"foo/bar\nfoo/baz\n"|splitlines|commonprefix}\n'
foo
$ hg debugtemplate '{"foo/bar\nbar/baz"|splitlines|commonprefix}\n'
$ hg debugtemplate '{"foo/../bar\nfoo/bar"|splitlines|commonprefix}\n'
foo
$ hg init
$ hg log -r null -T '{rev|commonprefix}'
hg: parse error: argument is not a list of text
(template filter 'commonprefix' is not compatible with keyword 'rev')
[255]