Date: Sun, 25 Feb 2001 16:16:33 -0500 From: Garrett Rooney <rooneg@electricjellyfish.net> To: Maxim Sobolev <sobomax@freebsd.org> Cc: ports@freebsd.org, jhk@freebsd.org, gad@freebsd.org, reg@freebsd.org Subject: Re: [patch] which package functionality for pkg_info Message-ID: <20010225161633.C94657@electricjellyfish.net> In-Reply-To: <200102252059.f1PKxte31663@vic.sabbo.net>; from sobomax@freebsd.org on Sun, Feb 25, 2001 at 10:59:18PM %2B0200 References: <20010225141537.A94657@electricjellyfish.net> <200102252059.f1PKxte31663@vic.sabbo.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Feb 25, 2001 at 10:59:18PM +0200, Maxim Sobolev wrote:
> >
> > comments welcome as always.
>
> This time only cosmetics, more comprehensive review I'll try to do
> tomorrow.
thanks a lot!
> > char *CheckPkg = NULL;
> > +char *CheckFile = NULL;
>
> Please be carefull with spaces vs. tabs. Get an editor with space
> highlighting and reviw all your patch.
oops, i thought i had gotten that one...
> > +/* comparison to see if two paths we're on matches the one we're looking for. */
>
> Please break up long lines like the above to the several lines.
done.
> > +static int
> > +cmp_path(const char *target, const char *current, const char *cwd) {
>
> Please read style(9) for the conventions used in FreeBSD code.
> Particularly, function should be in the form:
>
> void
> foo(int bar)
> {
> ...
> }
ok.
> > + strncpy(temp, cwd, cwd_len);
> > + strncat(temp, "/", 1);
> > + strncat(temp, current, current_len);
>
> Use strlcpy(3) and sprlcat(3) instead of strncpy/strncat - the former
> are considerably less error-prone.
cool! i had never seen those before.
> > + if (!strncmp(target, fixed_path, strlen(target)))
> > + rval = 1;
> > + else
> > + rval = 0;
>
> Again, please make indentation consistent with the rest of code.
the way it is now is a 4 space indent, with tabs used wherever there are two
levels of indent, which is what the rest of the file seems to do. is there
something else i should be doing?
> > + FTS *ftsp;
> > + FTSENT *entp;
>
> Please check if you can leverage yet-to-be added matchinstalled()
> function (see my recent patches in the freebsd-ports list) to get names
> of installed packages instead of rolling your own fts(3) loop.
i'll take a look at it.
> > -.Op Fl cdDfgGiIkLmopqrRsvx
> > +.Op Fl cdDfgGiIkLmopqrRsvWx
> > .Op Fl e Ar package
> > .Op Fl l Ar prefix
> > .Op Fl t Ar template
> > +.Op Fl W Ar filename
>
> You don't have to manifest `W' twice. Please remove it from the
> `cdDfgGiIkLmopqrRsvWx'.
oops. thanks.
--
garrett rooney Unix was not designed to stop you from
rooneg@electricjellyfish.net doing stupid things, because that would
http://electricjellyfish.net/ stop you from doing clever things.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-ports" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010225161633.C94657>