From owner-freebsd-ports  Sun Feb 25 13:25:51 2001
Delivered-To: freebsd-ports@freebsd.org
Received: from blizzard.sabbo.net (blizzard.sabbo.net [193.193.218.18])
	by hub.freebsd.org (Postfix) with ESMTP
	id 346E137B65D; Sun, 25 Feb 2001 13:25:08 -0800 (PST)
	(envelope-from max@vic.sabbo.net)
Received: from vic.sabbo.net (root@vic.sabbo.net [193.193.218.112])
	by blizzard.sabbo.net (8.10.1/8.10.1) with ESMTP id f1PLP2v13571;
	Sun, 25 Feb 2001 23:25:03 +0200
Received: (from max@localhost)
	by vic.sabbo.net (8.11.2/8.11.2) id f1PLPH132145;
	Sun, 25 Feb 2001 23:25:17 +0200 (EET)
	(envelope-from sobomax@FreeBSD.org)
From: Maxim Sobolev <sobomax@freebsd.org>
Message-Id: <200102252125.f1PLPH132145@vic.sabbo.net>
Subject: Re: [patch] which package functionality for pkg_info
To: rooneg@electricjellyfish.net (Garrett Rooney)
Date: Sun, 25 Feb 2001 23:25:17 +0200 (EET)
Cc: sobomax@freebsd.org (Maxim Sobolev), ports@freebsd.org,
	jhk@freebsd.org, gad@freebsd.org, reg@freebsd.org
In-Reply-To: <20010225161633.C94657@electricjellyfish.net> from "Garrett Rooney" at Feb 25, 2001 04:16:33 PM
X-Mailer: ELM [version 2.5 PL3]
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: owner-freebsd-ports@FreeBSD.ORG
Precedence: bulk
X-Loop: FreeBSD.org

> 
> 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!

Don't even mention it! ;)

> 
> > >  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.

BTW, in the example above sprinf(3) or asprintf(3) could be even more
applicable.

> > > +    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?

Identation-wise it should be OK if so. If you haven't yet, pleae read
style(9) manpage to get an idea of how your code should be formatter.

> 
> > > +    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.

No problem.

-Maxim

To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-ports" in the body of the message