Jeff Epler <jepler@xxxxxxxxxxxxxx> writes: > I'm aware of no problems with this patch, and a number of people have > commented that it is useful to them. Hmmm, "didn't generate any discussion" does not mesh very well with "a number of people are happy". Which one should I trust? > * Documented that these are POSIX EREs; hopefully that's OK. I see > in CodingGuidelines that in git itself "a subset of BREs" are used, > so maybe even this is too much power. And hopefully tcl's > re_syntax really is close enough to an ERE superset that this isn't > a terrible lie about the initial implementation either. I think it is better to be honest and say these are fed to the native regexp engine of Tcl somewhere in the documentation. Declaring "these are POSIX EREs" invites a user to expect they truly are. When the pattern the user wrote triggers a strange Tcl extension to cause unexpected things to match, the documentation needs to help the user to understand why. I understand the longer-term wish to reuse these in gitweb and elsewhere, but it becomes even more important that it is clearly documented that these "regexp" are fed to native regexp engines of Tcl and Perl depending on the program that they are used in. Unless the documentation spells it out, the user will not be able to decide how to work the implementation around, avoiding constructs that would behave differently between Tcl and Perl. Doesn't tcl have/use pcre these days, by the way? If we envision that this will be shared with gitweb, perhaps using that might be a better option to reduce potential confusion. > * Added a Signed-Off-By, since I've had a number of positive feedbacks > and the only problems I've heard of (since patch v2) are the ones > related to 'eval' in git-web--browse. By the way, "This patch is good" does not have anything to do with signing off a patch. Paul wanted to keep gitk sources separately available from the rest of the git. After all, that is how gitk project started. Even after 5569bf9 (Do a cross-project merge of Paul Mackerras' gitk visualizer, 2005-06-22), we kept it so that git://git.kernel.org/pub/scm/gitk/gitk was the primary project to make changes to gitk, and git.git pulled from it (it is an assymmetric pull, as gitk cannot pull from git without contaminating its history with the changes to the rest of git). I do not know how motivated Paul is to keep gitk part separated in its own project these days. I do not think the /pub/scm/gitk/gitk repository has been re-populated yet. Assuming that it will eventually come back on-line, could you send the gitk part of this change to Paul (i.e. the diff header of your patch should read "diff --git a/gitk b/gitk") and another separate patch to Documentation/ part? Paul, if you are orphaning gitk, I do _not_ mind start taking patches that touch gitk myself directly into git tree. But I would still need reviewers who are motivated and interested in enhancing and maintaining gitk. > +linkify.<name>.regexp:: > + Specify a regular expression in the POSIX Extended Regular Expression > + syntax defining a class of strings to automatically convert to > + hyperlinks. This regular expression many not span multiple lines. > + You must also specify 'linkify.<name>.subst'. Saying "You must ..." without explicitly saying "why" is a bad style. If the reader already _knows_ the .regexp is used to supply captured substring to the corresponding .subst, then it is obvious that whenever you have .regexp you need a matching .subst, but that is not even explained here. How about this? A string that matches this regexp is converted to a hyperlink using the value of corresponding `linkify.<name>.subst` variable. The regular expression is passed to the regexp engine of Tcl (in gitk) or Perl (in gitweb). > +linkify.<name>.subst:: > + Specify a substitution that results in the target URL for the > + related regular expression. Back-references like '\1' refer > + to capturing groups in the associated regular expression. > + You must also specify 'linkify.<name>.regexp'. Likewise. A string matched the value of the corresponding `linkify.<name>.regexp` variable is rewritten to this URL. The value of this variable can contain back-references like `\1` to refer to capturing groups in the associated regular expression. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html