Re: CIA hook for contrib/

[Petr Baudis <pasky@suse.cz>]

  Hi!

On Sat, Mar 27, 2010 at 06:26:32AM -0400, Eric Raymond wrote:
> Upon investigating further, I find that ciabot.sh seems to be a
> derivative of ciabot.pl, which is a dangling bit of the moribund
> Cogito project.  I have been unable to get responses from the authors
> of either ciabot.sh or ciabot.pl.

  Strange, I received no mail from you. When did you send it?

> A fixed and working version in shell is enclosed.

  It would have been easier to review if it was quotable. ;-)
(I have also looked at the current checked in version.)

  I'm happy that someone resurrected the hook script, thanks for that!
By the way, you seem to drop support for XML::RPC altogether while the
original ciabot.pl could use both. While XML::RPC requires another
dependency (not sure if plain Python installation can do it) and it can
time out in case of CIA server trouble (not sure how common these are
nowadays), the distinct advantage is that the commits will always end up
in correct order, while it seemed to be common that by mail, push of
multiple commits would reorder them randomly.

> # update: You have to call it once per merged commit:
> #
> #       refname=$1
> #       oldhead=$2
> #       newhead=$3
> #       for merged in $(git rev-list ${oldhead}..${newhead} | tac) ; do
> #               /path/to/ciabot.bash ${refname} ${merged}
                  ^^^^^^^^^^^^^^^^^^^^

Obsolete .bash reference.

> rev=$(git describe ${merged} 2>/dev/null)
> # ${merged:0:12} was the only bashism left in the 2008 version of this
> # script, according to checkbashisms.  Replace it with ${merged} here
> # because it was just a fallback anyway, and it's worth taking accepting
> # a longer fallback for faster execution and removing the bash deoendency.
> [ -z ${rev} ] && rev=${merged}

I personally find git-describe'd revspecs extremely ugly, unreadable and
less practical than plain hex ids (maybe I'm weird?), it would be really
nice to get a configuration choice between git describe and just:

	rev=$(echo "$merged" | cut -c 1-12)

(Not sure about the bashism rant since you can trivially just replace it
with the cut.)

> # This discards the part of the author's address after @.
> # Might be nice to ship the full email address, if not
> # for spammers' address harvesters - getting this wrong
> # would make the freenode #commits channel into harvester heaven.

The comment is not accurate since that code seems to be doing a lot of
other things as well.

I would personally prefer to have this configurable; I consider trying
to protect your e-mail address against harvesters is a lost fight anyway
and you'd be much better off just getting a good spam filter, rather
than making yours and others' life harder by trying to fight in vain.
But it's not too important for me since I can just disable this easily.

> rawcommit=$(git cat-file commit ${merged})
> author=$(echo "$rawcommit" | sed -n -e '/^author .*<\([^@]*\).*$/s--\1-p')
> logmessage=$(echo "$rawcommit" | sed -e '1,/^$/d' | head -n 1)
> logmessage=$(echo "$logmessage" | sed 's/\&/&amp\;/g; s/</&lt\;/g; s/>/&gt\;/g')
> ts=$(echo "$rawcommit" | sed -n -e '/^author .*> \([0-9]\+\).*$/s--\1-p')

Your life would be much easier in both the shell and python script if
you used something like:

	git log -1 '--pretty=format:%an <%ae>%n%at%n%s'

You would also get <ts> in the correct format, incl. timezone correction.

  Kind regards,

-- 
				Petr "Pasky" Baudis
http://pasky.or.cz/ | "Ars longa, vita brevis." -- Hippocrates