10167 unable to check out illumos-gate on Windows Reviewed by: Reviewed by: Approved by:

Review Request #2185 — Created July 16, 2019 and submitted

10167 unable to check out illumos-gate on Windows Reviewed by: Reviewed by: Approved by:

Test build in progress. git pbhck verified by hand -- this includes special test cases to keep us from needing to fix this in the future.

  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
  1. Ship It!
  2. usr/src/tools/scripts/git-pbchk.py (Diff revision 1)

    Have you tested this with toold built both against python2 and python3?

    1. No, I will endeavor to do so.

    2. Well, nuts. It doesn't work under python2.7, because of split and differences between str and unicode.

      Is operation under python 2.7 required?

    3. It is now; there were bugs.

  3. usr/src/tools/scripts/git-pbchk.py (Diff revision 1)

    Rather than duplicating the entire gen_files() here, I would add an extra parameter to gen_files().

    Something like

    def gen_files(root, parent, paths, exclude, filter=None):
        if filter is None:
            filter = lambda x: os.path.isfile(x)
        def gen(select=None):
            if (filter(path) and not empty and
                    select(path) and not exclude(abspath)):
                    yield path

    Then gen_links can be implemented as:

    def gen_links(root, parent, paths, exclude)
        return gen_files(root, parent, paths, exclude, lambda x: os.path.islink(x))

    or the call to gen_links() could just be replaced with this.

    1. Oh, that's a good idea. In all fairness, its been a long time since I've messed with Python, and I'm sure some of what I've done isn't terrifically pythonic.

  4. usr/src/tools/scripts/git-pbchk.py (Diff revision 1)

    This is not necessary, only symlinks will be returned from the flist() generator.

  5. usr/src/tools/scripts/git-pbchk.py (Diff revision 1)

    Have you tried using a pre-compiled regexp for this (and the previous test)? It should be significantly faster for large changesets.

    1. Nope. It may be, but I wouldn't expect it to make much difference. Even if it's a few hundred msec (which would be large) I doubt anyone is likely to ever notice. This isn't terrifically hot code. But I will investigate if it improves readability.

    2. Because all of these are special characters, I am concerned that that a regular expression is likely to be less readable.

      It might be faster to reverse the loop though, and iterate over each character of the path, rather than characters in the specials array. I could conceivably make the specials a string as well, which might be faster than using an array; it would be terser -- not sure about readability.

      I've settled on the following, which I strongly suspect will compare favorably with any regular expression. (The only way to do this faster than I can think of would be to have the characters used as an array index -- it's possible that a regular expression compiler will generate such a lookup array, but I sort of doubt it.) At any rate, I think readability trumps a tiny performance improvement.

      def haswinspecial(name):
         specials = '<>:"\\|?*'
         for c in name:
             if c in specials:
                 return True
         return False
    3. As you say, it is probably not worth optimising this much and the new version is more readable, thanks.

      I was, however, interested so I tried:

      import re
      specials = re.compile(r'[<>:"\\|?*]')
      def haswinspecial(name):
              return re.search(specials, name) is not None

      and this is twice as fast on an 80-character test pathname (I'm not saying to change it).

  2. usr/src/tools/scripts/git-pbchk.py (Diff revision 2)

    This should be moved up out of this loop. It does not need to be done each iteration.

  1. Ship It!
  1. Ship It!
Review request changed

Status: Closed (submitted)