[indiana-discuss] Fwd: Code Review: PSARC/2007/688 manwhich: Deriving MANPATH from PATH

[Shawn Walker]

On Jan 13, 2008 1:46 PM, Mike Gerdts <mgerdts at gmail.com> wrote:
> On Jan 12, 2008 11:17 PM, Shawn Walker <swalker at opensolaris.org> wrote:
> > On Jan 9, 2008 10:25 PM, Mike Gerdts <mgerdts at gmail.com> wrote:
> > > I've made some enhancements to man(1), partly inspired by the default
> > > PATH in the October developer preview release.  Aside from updating
> > > the copyright year, I believe the code is ready for putback.  So far I
> > > have had little luck with the folks at opensolaris-code to get code
> > > reviews[1].  If your priorities are more aligned to have a more
> > > friendly man(1) in the next Indiana release, please take a look and
> > > get me your feedback.
> >
> > http://cr.opensolaris.org/~mgerdts/manpath-from-path/usr/src/cmd/man/src/man.c.html
> >
> > The comments for the various BMP flags at lines 175-177 have better
> > comments at lines 593-600.
> >
> > In particular, the comment for BMP_FALLBACK_MANDIR at line 177 is the
> > same used for BMP_APPEND_DIR at 176, which seems wrong somehow.
>
> Yep.  There was some evolution in this area and some of the ugliness
> got left behind.  It seems to me to be best to put the most useful
> comments around line 175 say something to the effect of "see BMP_*
> flag definitions above for valid flags."
>
> >
> > 538          * "man -p" with out operands
> > s/with out/without/
>
> OK.
>
> > 3191         *p = '\0';
> >
> > The above seems either redundant or unnecessary given what is already
> > being done at lines 3166-3177.
>
> This is necessary, as described below the code block.
>
> 3162         /*
> 3163          * Advance to end of buffer, strip trailing /'s then remove last
> 3164          * directory component.
> 3165          */
> 3166         for ( p = mand; *p != '\0'; p++);
> 3167         for ( ; p > mand && *p == '/' ; p-- );
> 3168         for ( ; p > mand && *p != '/' ; p-- );
> 3169         if ( p == mand && *p == '.' ) {
> 3170                 if ( realpath("..", mand) == NULL ) {
> 3171                         free(mand);
> 3172                         return(NULL);
> 3173                 }
> 3174                 for ( ; *p != '\0' ; p++);
> 3175         } else {
> 3176                 *p = '\0';
> 3177         }
> 3178
> 3179         if ( strlcat(mand, "/man", PATH_MAX) >= PATH_MAX ) {
> 3180                 free(mand);
> 3181                 return(NULL);
> 3182         }
> 3183
> 3184         if ( stat(mand, &sb) == 0 ) {
> 3185                 return(mand);
> 3186         }
> 3187
> 3188         /*
> 3189          * Strip the /man off and try /share/man
> 3190          */
> 3191         *p = '\0';
> 3192         if ( strlcat(mand, "/share/man", PATH_MAX) >= PATH_MAX ) {
>
> Lines 3166 - 3177 cause p to point to null character at the end of a
> string (mand) containing the directory name.  At line 3179 the '\0'
> moves to the right 4 positions.  If <mand>/man doesn't exist
> (3184-3186), it needs to shorten mand (strip off "/man") (3191) before
> appending "/share/man" (3192).

Ah, that makes sense now. Sorry, I was misreading what is there.

Now that I understand what it's doing better, I agree.

+1 from me

Cheers,
-- 
Shawn Walker, Software and Systems Analyst
http://binarycrusader.blogspot.com/

"To err is human -- and to blame it on a computer is even more so." -
Robert Orben