On Sat, Jun 27, 2009 at 3:03 AM, Junio C Hamano<gitster@xxxxxxxxx> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@xxxxxxxxx> writes:
>
>> foreach my $line (@$log) {
>> - if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|acked[ \-]by[ :]|cc[ :])/i) {
>> + if ($line =~ m/^ *(signed[ \-]off[ \-]by[ :]|(?:trivially[ \-])?acked[ \-]by[ :]|cc[ :])/i) {
>
> Wouldn't it make more sense to do something like:
>
> if ($line =~ m/^[-a-z]+: (.*)$/i && looks_like_a_name_and_email($1)) {
> ... do the avatar thing ...
> }
>
> and limit this processing only to the last run of no-blank lines?
I've been thinking about doing something like this, especially since
the next iteration this patch also adds 'Looks-fine-to-me-by' (spearce
really _loves_ customizing its signoffs lines during reviews 8-D).
The main difference between the two sections of the code is that the
original code allows whitespace instead of dashes in the signoff
lead-in, and the lead-in itself is OPTIONALLY terminated by a colon.
If we can lose this, then ^\s*[-a-z]+: followed by what looks like a
name and email can be a detector. An alternative would be to keep the
current signoff detector as-is and ADD the generic one, possibly under
the condition that we are already parsing signoffs.
The idea of accepting those lines as signoffs only at the end of the
commit log is also a good idea, but has the problem of requiring some
form of look-ahead. Two possible logics:
(1) when something that looks like a signoff is met, stash it away,
and keep collecting signoff-lookalikes and empty lines until either a
non-signoff-lookalike (in which case the previous lines are NOT
signoff) or the log end is met (in which case we print them as signoff
lines).
(2) same as before, but two or more consecutive signoff-lookalikes are
a signoff block regardless of what follows them.
Which one should I go with?
--
Giuseppe "Oblomov" Bilotta
--
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