* Avoid a race in the git rcs_commit function, by not assuming HEAD will

  stay the same for the duration of the function.

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.
        # 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})$/) {
                        $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
        #
        # 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'} };
        return if !defined $ci;
 
        my @changed_pages = map { $_->{'file'} } @{ $ci->{'details'} };


@@ -451,6 +451,8 @@ sub rcs_notify () { #{{{
                $message = join "\n", @{ $ci->{'comment'} };
        }
 
                $message = join "\n", @{ $ci->{'comment'} };
        }
 

+       my $sha1 = $ci->{'commit'};
+

        require IkiWiki::UserInfo;
        send_commit_mails(
                sub {
        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.
     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
 
 
 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.