Merge branch 'master' of ssh://git.kitenet.net/srv/git/ikiwiki.info

diff --git a/IkiWiki/Rcs/git.pm b/IkiWiki/Rcs/git.pm
index 84e7d74e58631d0b0930cceaa9722ba52f9f4473..b5562f0edd236c9674ebca35368bf3b512a7cddc 100644 (file)
--- a/IkiWiki/Rcs/git.pm
+++ b/IkiWiki/Rcs/git.pm
@@ -165,7 +165,10 @@ sub _parse_diff_tree ($@) { #{{{
        # Identification lines for the commit.
        while (my $line = shift @{ $dt_ref }) {
                # Regexps are semi-stolen from gitweb.cgi.
-               if ($line =~ m/^tree ([0-9a-fA-F]{40})$/) {
+               if ($line =~ m/^commit ([0-9a-fA-F]{40})$/) {
+                       $ci{'commit'} = $1;
+               }
+               elsif ($line =~ m/^tree ([0-9a-fA-F]{40})$/) {
                        $ci{'tree'} = $1;
                }
                elsif ($line =~ m/^parent ([0-9a-fA-F]{40})$/) {
@@ -431,12 +434,9 @@ sub rcs_notify () { #{{{
        #
        # Here, we rely on a simple fact: we can extract all parts of the
        # notification content by parsing the "HEAD" commit (which also
-       # triggers a refresh of IkiWiki pages) and we can obtain the diff
-       # by comparing HEAD and HEAD^ (the previous commit).
-
-       my $sha1 = 'HEAD'; # the commit which triggers this action
+       # triggers a refresh of IkiWiki pages).
 
-       my $ci = git_commit_info($sha1);
+       my $ci = git_commit_info('HEAD');
        return if !defined $ci;
 
        my @changed_pages = map { $_->{'file'} } @{ $ci->{'details'} };
@@ -451,6 +451,8 @@ sub rcs_notify () { #{{{
                $message = join "\n", @{ $ci->{'comment'} };
        }
 
+       my $sha1 = $ci->{'commit'};
+
        require IkiWiki::UserInfo;
        send_commit_mails(
                sub {

diff --git a/debian/changelog b/debian/changelog
index 3817f142aabe4345c4da007de5a7ddc6e2818a64..325a61de0108b7760d1b68b4f36cc6b300deabaf 100644 (file)
--- a/debian/changelog
+++ b/debian/changelog
@@ -5,8 +5,10 @@ ikiwiki (2.12) UNRELEASED; urgency=low
     page name to be expired and reused for several distinct guids. When this
     happened, the expiry code counted each past guid that had used that page
     name as a currently existing page, and thus expired too many pages.
+  * Avoid a race in the git rcs_commit function, by not assuming HEAD will
+    stay the same for the duration of the function.
 
- -- Joey Hess <joeyh@debian.org>  Tue, 30 Oct 2007 22:47:36 -0400
+ -- Joey Hess <joeyh@debian.org>  Wed, 31 Oct 2007 17:11:12 -0400
 
 ikiwiki (2.11) unstable; urgency=low
 

diff --git a/doc/bugs/git_mail_notification_race.mdwn b/doc/bugs/git_mail_notification_race.mdwn
new file mode 100644 (file)
index 0000000..c77eaf7
--- /dev/null
+++ b/doc/bugs/git_mail_notification_race.mdwn
@@ -0,0 +1,40 @@
+I was suprised to receive two mails from ikiwiki about one web edit:
+
+ 1   F Oct 30 To joey+ikiwiki update of ikiwiki's plugins/contrib/gallery.mdwn by http://arpitjain11.myopenid.com/
+ 1   F Oct 30 To joey+ikiwiki update of ikiwiki's plugins/contrib/gallery.mdwn by http://arpitjain11.myopenid.com/
+
+The first of these had the correct diff for the changes made by the web
+ edit (00259020061577316895370ee04cf00b634db98a).
+
+But the second had a diff for modifications I made to ikiwiki code
+around the same time (2a6e353c205a6c2c8b8e2eaf85fe9c585c1af0cd).
+
+I'm now fairly sure a race is involved. Note that the name of the modified
+page is correct, while the diff was not. The git rcs_commit code first
+gets the list of modified page(s) and then gets the diff, and apparently my
+other commit happened in the meantime, so HEAD changed.
+
+Seems that the rcs_notify for git assumes that HEAD is the commit
+that triggered the ikiwiki run, and that it won't change while the function is
+running. Both assumptions are bad. Git does no locking and another commit
+can come in at any time.
+
+Notice that the equivilant code for subversion is careful to look at $REV.
+git's post-update hook doesn't have an equivilant of $REV. Its post-receive hook
+does, but I'm not sure if that's an appropriate hook for ikiwiki to be using
+for this. Switching existing wikis to use a different hook would be tricky..
+
+I've avoided part of the race; now the code does:
+
+1. Get commit info for HEAD commit.
+2. Extract file list from that.
+3. Extract sha1 of commit from it too.
+4. git diff sha1^ sha1
+
+Still seems like there can be a race if another commit comes in before step #1.
+In this case, two post-receive hooks could run at the same time, and both
+see the same sha1 as HEAD, so two diffs might be sent for second commit and no
+diff for the first commit.
+
+Ikiwiki's own locking prevents this from happenning if both commits are web
+edits. At least one of the two commits has to be a non-web commit.