We seem to be talking in the wrong bug log. My mistake - hand't noticed the CC and responded to the wrong bug.
-- Antti-Juhani Kaijanaho, Jyväskylä, Finland http://antti-juhani.kaijanaho.fi/newblog/ http://www.flickr.com/photos/antti-juhani/
--- Begin Message ---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 z...@{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 _______________________________________________ Dctrl-tools-devel mailing list [email protected] http://lists.alioth.debian.org/mailman/listinfo/dctrl-tools-devel
--- End Message ---
