[MERGE] Add current-directory information to osutils.walkdirs

[John Arbash Meinel]

Robert Collins wrote:
> This adds the directory path-from-prefix and path-from-top information
> to the output of osutils.walkdirs, which makes it easier to use - less
> bookkeeping data is needed.
> 
> Cheers,
> Rob
> 
> 
> ------------------------------------------------------------------------
> 
> === modified file 'bzrlib/osutils.py'
> --- bzrlib/osutils.py	2006-07-13 19:58:22 +0000
> +++ bzrlib/osutils.py	2006-07-30 14:00:09 +0000
> @@ -875,13 +875,29 @@
>      to exclude some directories, they are then not descended into.
>      
>      The data yielded is of the form:
> -    [(relpath, basename, kind, lstat, path_from_top), ...]
> +    ((directory-relpath, directory-path-from-root),
> +    [(relpath, basename, kind, lstat), ...]),
> +     - directory-relpath is the containing dirs relpath from prefix
> +     - directory-path-from-root is the containing dirs path from /

^- This doesn't seem to be the absolute path. And the way you document
it here, makes it sound like it is.

So lets sort it out a bit...
'top' is the physical path to the directory (this may be relative from
cwd, or it may be an absolute path)
'prefix' is the fully qualified "relative" path, relative to some other
directory. (Such that if you have a branch, and you do walkdirs in a
subdir you can ask all dirs to be prefixed with the subdir)

So 'directory-relpath' is prefix + the relative path from top
And directory-relpath-from-root is the 'absolute path' which is just the
'top' + relative path.

I'm not sure the exact way to present this cleanly, but we probably need
something more clear.


> +     - relpath is the relative path within the subtree being walked.
> +     - basename is the basename of the path
> +     - kind is the kind of the file now. If unknonwn then the file is not
> +       present within the tree - but it may be recorded as versioned. See
> +       versioned_kind.

^- s/unknonwn/unknown/

> +     - lstat is the stat data *if* the file was statted.
> +     - planned, not implemented: 
> +       path_from_tree_root is the path from the root of the tree.
>  
>      :param prefix: Prefix the relpaths that are yielded with 'prefix'. This 
>          allows one to walk a subtree but get paths that are relative to a tree
>          rooted higher up.
>      :return: an iterator over the dirs.
>      """
> +    #TODO there is a bit of a smell where the results of the directory-
> +    # summary in this, and the path from the root, may not agree 
> +    # depending on top and prefix - i.e. ./foo and foo as a pair leads to
> +    # potentially confusing output. We should make this more robust - but
> +    # not at a speed cot. RBC 20060731
^- s/cot/cost/

>      lstat = os.lstat
>      pending = []
>      _directory = _directory_kind
> @@ -900,7 +916,7 @@
>              abspath = top + '/' + name
>              statvalue = lstat(abspath)

v- This line breaks all kinds of PEP8, and should probably be cleaned up.
>              dirblock.append ((relroot + name, name, file_kind_from_stat_mode(statvalue.st_mode), statvalue, abspath))
> -        yield dirblock
> +        yield (currentdir[0], top), dirblock
>          # push the user specified dirs from dirblock
>          for dir in reversed(dirblock):
>              if dir[2] == _directory:
> 

I'm +1 on the change.
I'm thinking we should also change one other thing, though. Because we
can get rid of an if/then inside the main loop if we do:

if prefix and not prefix.endswith('/'):
  prefix += '/'
if top and not top.endswith('/'):
  top += '/'
pending = [(prefix, top)]

while pending:
  dirblock = []
  relroot, top = pending.pop()
  for name in sorted(_listdir(top)):
    abspath = top + name
    statvalue = lstat(abspath)
    dirblock.append((relroot+name, name,
                     file_kind_from_stat_mode(statvalue.st_mode),
                     statvalue, abspath))
  yield dirblock
  for dir in reversed(dirblock):
    if dir[2] == _directory:
      pending.append((dir[0]+'/', dir[4]+'/'))

With this code, 'top' and 'prefix' are always are either empty, or end
in a slash, so we don't need to do one of the additions, and we don't
need the if/then.

I also think we might want to change the function from using
os.listdir() to using 'read_dir()' which hasn't been integrated into the
core of bzr (not even the python version).

Then when we have the extension available, walkdirs() doesn't even have
to stat every file.

Just a couple thoughts.

So even if you don't want to rework walkdirs just yet, I think we should
clarify the documentation.

Otherwise I'm +1 to how you changed the 'yield' value.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060730/a66a2429/attachment.pgp