On Jan 12, 2008 11:17 PM, Shawn Walker <[EMAIL PROTECTED]> wrote:
> On Jan 9, 2008 10:25 PM, Mike Gerdts <[EMAIL PROTECTED]> 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).
> Other than that, this looks great. The code was easy to follow, and
> quite elegant in my view.
>
> I've been following the discussions on this particular change for some
> time now, and feel your are to be commended for this contributions.
>
> I'm really looking forward to never having to set MANPATH again.
Thanks for the review and kind words.
Mike
--
Mike Gerdts
http://mgerdts.blogspot.com/
_______________________________________________
indiana-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/indiana-discuss
