[Review - not yet] Re: [ITP] tree

[Igor Pechtchanski]

On Thu, 18 Dec 2003, Stipe Tolj wrote:
Re: [ITP] tree: Recursive directory listing program that produces a depth indented listing of files

> Hi list,
>
> this is a quick one. Canonical homepage is:
>
>   http://mama.indstate.edu/users/ice/tree/
>
> --setup.hint--
> sdesc: "Recursive directory listing program that produces a depth
> indented listing of files"
> category: Utils
> requires: cygwin
> ldesc: "Tree is a recursive directory listing program that produces
> a depth indented listing of files, which is colorized ala dircolors
> if the LS_COLORS environment variable is set and output is to tty."
> --setup.hint--
>
> --cut--
> #!/bin/bash
>
> mkdir -p tree
> cd tree
>
> wget http://cygwin.dev.wapme.net/packages/tolj/cygwin/release/tree/tree-1.4-1-src.tar.bz2
> wget http://cygwin.dev.wapme.net/packages/tolj/cygwin/release/tree/tree-1.4-1.md5sum
> wget http://cygwin.dev.wapme.net/packages/tolj/cygwin/release/tree/tree-1.4-1.tar.bz2
> wget http://cygwin.dev.wapme.net/packages/tolj/cygwin/release/tree/setup.hint
> --cut--
>
> Please review and vote for upload.
>
> Stipe

Sounds interesting.  I'll vote for this.

Now for the review:

1) The documentation is still in /usr/doc and /usr/man...  Any plans on
   moving it to /usr/share/{doc,man}?

2) This uses method 1 packaging.  AFAIU, the patch in CYGWIN-PATCHES
   should still be a forward patch.  Yours is reverse.

3) Any particular reason you have the make and install logs in
   CYGWIN-PATCHES?  Also, the make log contains a warning that looks
   suspicious, and the install log shows that "make install" will install
   files directly into /usr/bin, rather than to a subdirectory.  Also,
   "make install" will not put the Cygwin-specific README in the right
   directory.

4) The file list in the Cygwin-specific README mentions /usr/bin/tree, and
   not /usr/bin/tree.exe, which, IMO, is misleading.

5) There are no port notes in the Cygwin-specific README, even though
   there are some user-visible changes in the patch, such as changing the
   prefix to /usr and removing the "-s" linker flag (any particular reason
   why you did that?)

6) I don't see a "strip" step anywhere in the installation process
   (although the executable in the binary tarball is stripped).  That's
   actually what the -s linker flag does...

7) There is a bug in the printing of inode numbers, since ino_t is 64-bit
   in Cygwin now, and it's printed using "%ld" (tree.c:515).

8) There is a bug that the file size is declared as u_long, and will be
   truncated for large files.  Why not simply change it to off_t?

I think this is it.  A lot of the above is very easy to fix, so we'll wait
for a new installment soon, eh? ;-)
	Igor
-- 
				http://cs.nyu.edu/~pechtcha/
      |\      _,,,---,,_		pechtcha@cs.nyu.edu
ZZZzz /,`.-'`'    -.  ;-;;,_		igor@watson.ibm.com
     |,4-  ) )-,_. ,\ (  `'-'		Igor Pechtchanski, Ph.D.
    '---''(_/--'  `-'\_) fL	a.k.a JaguaR-R-R-r-r-r-.-.-.  Meow!

"I have since come to realize that being between your mentor and his route
to the bathroom is a major career booster."  -- Patrick Naughton