diff options
| -rw-r--r-- | IkiWiki/Rcs/git.pm | 14 | ||||
| -rw-r--r-- | debian/changelog | 4 | ||||
| -rw-r--r-- | doc/bugs/git_mail_notification_race.mdwn | 40 |
3 files changed, 51 insertions, 7 deletions
diff --git a/IkiWiki/Rcs/git.pm b/IkiWiki/Rcs/git.pm index 84e7d74e5..b5562f0ed 100644 --- 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 3817f142a..325a61de0 100644 --- 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 index 000000000..c77eaf7cd --- /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. |