Thanks for the feedback. I've added four more patches to the end of
the series and updated the first two. My replies are below.
On Mon, 22 Jun 2009, Jakub Narebski wrote:
I'm not sure I would call the context of the bug numbers
"notifications". A good example of what I'm looking for is to
hyperlink "Resolves-bug: 1234" to
http://bugzilla.example.com/show_bug.cgi?bug_id=1234. I suppose this
could be seen as a notification of bug 1234's resolution, except that
I expect the more complex bugs (bugs, issues, and features) to require
several commits to resolve, each of which I'd like tagged with that
bug number, and so the bug would not really be resolved after the
first such commit.
I've rewritten the commit message paragraph as you suggested. Is it
now more inline with what you'd like to see?
So far, the biggest difficulty in generalizing the bugzilla hyperlink
feature has been the interaction between two or more commit tag
transformations -- how to structure them so they are composable but
decoupled.
The original patch still did the HTML escaping. I don't see much
value in implementing this transformation as a committag since it
wouldn't be useful to configure it, and I don't really see it making
the code more clear or concise. Did you have any particular reasons?
I'll exclude the mailing list references from the commit message since
they describe how I arrived at this set of changes rather than
describing the changes themselves.
That is the Bugtraq spec, although I had been looking at a text-only
version of it at the time. I can't seem to find the text-only
document, but I think that one explains it just as well.
Very well then, I'll leave everything related to the other git tools
for later.
Yes, it seems manageable to process the whole commit message at once
rather than line by line. I've made this change to support patch 4 of
this series.
This is a great example of the kind of decision I'd like to make
up-front to avoid breaking existing configs later:
gitweb.committag.<name>.<key> vs. committag.<name>.<key>. I'm not
very interested in adding the git-gui / gitk feature myself. Is
anyone actually interested in this feature? If not, perhaps it should
stay under gitweb.committag.<name>.<key>. And even if there is
interest in the git-gui / gitk features, perhaps it would make sense
to start with the gitweb-specific version of the config variable names
and once there is cross-tool support, keep those configs as overrides
for gitweb only?
Yes, I imagine there is a risk of abuse in allowing patterns to be
configurable. That's one reason why the site config can disallow
per-project configuration of regexes. Are you suggesting there be a
separate flag to allow override of regexes than to allow override of
other parameters? For example, with a separate flag for regexes,
repo.or.cz could allow you to configure the bug tracker URL but not
the bug regex. I've added very fine grain support for allowing only
some options to be overridden in patch 3 of this series.
Yes. I've removed the running commentary, since it seems to be in the
way.
Sounds as though there is no interest in changing the name to
something that more clearly describes the feature (asside from my own,
but I'd like a little more validation...). Let's stick with
committag.
I believe I've addressed your request by adding a comment that
applies to all tags. Because of the similarity of the tags, I don't
think it would be very useful to do this for each tag. Does the 'url'
option used on two of the tags need further explanation?
I liked 'override' close to the 'options' hash because that is what it
controls. The 'sub' key was not overridable per-project no matter how
you configure gitweb. Now it is, so maybe now 'override' could go
last. Or first. But really, I don't think the order matters much.
Ok, I'll use a neutral URL for the mantis default.
I'd rather use a regex-style or sprintf-style syntax that could be
used by all committags. I started with just an opaque string to
prepend to the bug ID because it was the simplest way to fulfill my
requirements, but it's certainly plausible that a BTS would want urls
like "example.com/bug/1234/detail" in which case the prepending
strategy isn't sufficient. Again, ideally we'd decide now whether to
use '$1' or '%s' so we don't break configs later. If the bugtraq
feature is implemented at some point, those config parameters can
still use the %BUGID% placeholder.
Actually, I think I started with a URL regex mentioned on-list and
then tweaked it until I thought it worked well enough.
Most of the regexes I found on the web had issues. They tended to be
too long because they were trying to precisely validate the URL or
because they were trying to parse all the different parts of the url,
or they did not deal with the trailing punctuation problem: I don't
want the period (".") to be highlighted in this context: "See
http://example.com/foo." I think it's a valid URL even with the
period, but typically the period will not be part of the URL. On the
other hand, "http://example.com/foo.html" should be completely
highlighted in spite of the period.
I'va added more schemes, but it's also possible to leave those as a
customization. In my organization, it would be very uncommon for
someone to follow a URL with any of those schemes. I'd be willing to
allow any scheme that matches "[a-z]+://". I don't care about stuff
like "data:literal+text", and "mailto:" is also less interesting to
me. (I'd rather add a committag to match the email address without a
"mailto:".) And then there's those pesky windows file sharing
thingies like '\\server\directory' that work like URLs in Internet
Explorer. I'd rather not include them in the URL committag... but a
site admin could certainly configure a committag for it. Here's a
list schemes for which KDE allegedly supports retreival of metadata:
bluetooth, fish, ftp, imap(s), invitation, iso, ldap(s), mac, mdns,
nfs, nntp(s), nxfish, obex, pop3(s), print, printdb, sdp, service,
sftp, slp, smb, smtp(s), webdav(s)
I wouldn't want to enumerate all of those, but, in addition to the
three you suggest, these look most useful to me: sftp, smb, webdav(s),
nfs. So really I think it comes down to whether we want to enumerate
them or just match any scheme, and risk matching something that was
not intended as a URL. And if we enumerate, how many do we want to
list.
What do you think of the new list of schemes in patch 1? I've
included several more than before.
The most important change is that the semicolon is removed. I
also made the dash optional. It wasn't clear to me whether this was
intended to match a header-style reference:
Message-Id: <asdf@example.com>
Or a casual mention:
In msgid <asdf@example.com>, John Doe says...
But I like the latter, and would like the former to still be
supported.
Thanks for the tip. Is there another common mail archive site
that allows looking up emails by message-id? If so, I'd love to
mention the url in the comment for that committag. I think we need to
strike a balance between having things work with minimal configuration
and avoiding the promotion of specific web sites that won't make sense
for most sites using a particular committag. So, unless there is a
whole slew of web sites that provide this feature, I'd suggest leaving
the specific URL in.
Yes, although if we decided it was sub-optimal link text, I'd be
happy to use a less generic mechanism.
I like the notion of names rather than numberic indices, but I'm
hesitant to require site admins to use unfamiliar Perl regex syntax to
configure committags. Of two admins I asked, neither could make sense
of the sample regex. If wouldn't want a site admin to *have* to learn
new syntax to configure regexes.
I chose to add that explanation in the section that describes the
configuration of committags rather than in this section that defines
the list of active committags. Sound ok?
I didn't understand how this would be very useful until I added
the signoff committag. The use case I see is that it allows the
gitweb distribution to adjust the base functionality, in this case by
pushing some functionality into a committag, without project owners
needing to reconfigure their repositories. Site admins don't need
this because they can use "push" or "unshift" to preserve the
distributed committags, right? The project owner should get the
distribution default, not the site default, when they write "default",
right? Are there other use cases you had in mind? A potential
implementation is in patch 6.
Thanks for pointing out the opportunity to fold the feature list
functions together.
Sounds like you're asking me to move the function call after the
function definition. I've made the change, but I'm also curious to
know why you have that preference. I sometimes find it less readable,
as in this case.
I thought the check for $git_dir would prevent loading it for
non-project-specific pages. Even so, I suppose we might load the
config on a page like the shortlog page that doesn't need it.
Are you saying I should be using other subroutines that already
exist in gitweb rather than implementing my own? Are you thinking of
git_get_project_config? If I understand correctly, it requires me to
enumerate all the config keys I want. I'd rather not require the
committags to have a predetermined set of possible config keys. I see
now that I could still call git_get_project_config to load data into
%config and access that directly... I didn't do it before because it
seems to violate some kind of abstraction.
Well... it means both not processing and HTML escaping. The string
refs will also not be escaped in the final processing step.
Sure. I chose @message_fragments as the less generic name for @list.
Hehe, that bit of code is mostly yours, which you posted on this list
ages ago. (: I suppose we should try to get your signoff into the
first patch of the series?
Ok, I guess I'll leave the subroutine prototype in then. I don't
understand all the implications, so if you think it'll work well
without the prototype, I'm happy to remove it. This is code
that I think you posted to the list.
I see there is some precedent for adding lib-foo.sh files. Perhaps it
would be appropriate to move parts of the no-errors test into a
lib-gitweb.sh? Like gitweb_init, most of gitweb_run, and maybe the
perl checks (which are that way for lib-git-svn.sh)? I'm all for
separating the tests. I don't like having to keep telling the test to
skip the first 85 cases when all I want to run is the committag stuff.
Actually, looks like Mark Rada already made a gitweb-lib.sh which was
exactly the same as the one I'd made except the name of the lib (vs.
lib-gitweb.sh) and the name of the file holding gitweb output. I've
rebased onto those changes.
Thanks for the tip, I'll use h=HEAD.
Would this mean people need to install some Mechanize stuff before
they can run the tests? I think I'd rather avoid the dependency as
long as grep seems to do the trick.
?
Marcel M. Cary (6):
gitweb: Hyperlink various committags in commit message with regex
gitweb: Add second-stage matching of bug IDs in bugzilla committag
gitweb: Allow finer-grained override controls for committags
gitweb: Allow committag pattern matches to span multiple lines
gitweb: Allow per-repository definition of new committags
gitweb: Add _defaults_ keyword for feature lists in project config
gitweb/INSTALL | 16 ++
gitweb/gitweb.perl | 404 +++++++++++++++++++++++++++++++++++++-----
t/t9502-gitweb-committags.sh | 309 ++++++++++++++++++++++++++++++++
3 files changed, 681 insertions(+), 48 deletions(-)
create mode 100755 t/t9502-gitweb-committags.sh
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
