[Dctrl-tools-devel] Bug#383921: Bug#383921: Bug#476861: dctrl-tools: option to match against package-name boundaries

Stefano Zacchiroli zack at debian.org
Fri Feb 27 08:23:26 UTC 2009


Hi Antti-Juhani, thanks for your feedback.

On Wed, Feb 25, 2009 at 07:45:39PM +0200, Antti-Juhani Kaijanaho wrote:
> I do think this is a bit inelegant, but all other avenues for implementation
> that I can think of are much more complex.
> 
> In other words, I think your approach is sound.

Fair enough :-|

> > I've changed a bit the regular expression boundaries wrt yours, mines
> > are:
> > 
> > 	#define RE_PKG_BEGIN	"(^| )"
> > 	#define RE_PKG_END	"([, \\(]|$)"
> > 
> > with the rationale that whitespaces, according to policy, do not
> > necessarily appear between package names and optional version
> > requirements; hence also '(' can denote the end of package name.
> 
> Would it make sense to instead rule out any character that would be
> a part of a package name?

Uhm, I don't think so, because that set of characters is not
"symmetric", e.g. a package cannot start with a -, but can contain it
and other similar details. You can of course come up with a proper
regexp, but I found delimiters more appropriate for this specific
case.

> > Please consider applying the attached patch.
> 
> Since you are a DD, you can commit it yourself when it's ready.
> (Please read debian/README, first.)

Thanks for the point, I overlooked that. I'll ping the bug report with
the final patch version before pushing anyhow.

> You haven't signed off the patch.  Please read
>   http://git.debian.org/?p=collab-maint/dctrl-tools.git;a=blob_plain;f=dco-1.1.txt;hb=dco
> and if you can certify that, please add a Signed-Off-By line at the
> end of the patch description (see debian/README).

Thanks, will do.

> > +	char * regex_pat = NULL;
> > +	int regex_patlen = atom->patlen + 30;
> 
> Where does the magic number 30 come from?  The combined length of the two regexps?
> If so, I would prefer making that explicit, as in:
<snip>
> ... Ah, I see you've already done this.  I would prefer combining
> the two code patches, though.

Yes, I did that in my branch, but did not want to bother again the bug
log readers before feedback (as sworn) :-) Sure, I'll squash all my
changes into a single patch and push it.

> >  	if (atom->mode == M_REGEX || atom->mode == M_EREGEX) {
> > +		regex_pat = calloc(1, regex_patlen);
> 
> Do you use the property that the returned memory is zeroed out?

Yes. Will add an explicit comment about that; since you wondered, I
guess others can wonder too.

> > +.IP "\-\-whole\-pkg"
> > +Do an extended regular expression match on whole package names,
> > +assuming the syntax of inter-package relationship fields such as
> > +Depends, Recommends, ... When this flag is given you should not worry
> > +about sub-package names such as "libpcre3" also matching
> > +"libpcre3-dev". This flag implies \-e.
> 
> I'd prefer allowing us to later drop the -e connection, so should say
> "currently implies" or similar.

Fair enough.  I've a question on this however.

When I first hacked up the patch, I did not get that those flags were
specific of grep-dctrl pattern "particles" (i.e., that --whole-pkg
should be repeated on different sub-patterns). Now that I'm aware of
it, I think it does make sense, what's your take on it?

Nevertheless, that "implication" is currently annoying for the user
because if you specify both -e and --whole-pkg you will get an
error. What would be your preferred fix for this?

Cheers.

-- 
Stefano Zacchiroli -o- PhD in Computer Science \ PostDoc @ Univ. Paris 7
zack@{upsilon.cc,pps.jussieu.fr,debian.org} -<>- http://upsilon.cc/zack/
Dietro un grande uomo c'è ..|  .  |. Et ne m'en veux pas si je te tutoie
sempre uno zaino ...........| ..: |.... Je dis tu à tous ceux que j'aime





More information about the Dctrl-tools-devel mailing list