Bug: TWiki::Store::saveAttachment locking is broken

I think the logic in TWiki::Store::saveAttachment is broken, For some reason this doesn't cause problems when it is used in bin/upload, but when used outside that context, it fails with the message

can't lock an attachment at /usr/local/twiki/lib/TWiki/Store/RcsFile.pm line 421.

TWiki::Store::saveAttachment takes a flag $doUnlock as a parameter, but if we examine how it is used it doesn't do what it says:

sub saveAttachment
{
    my( $web, $topic, $text, $saveCmd, $attachment, $dontLogSave, $doUnlock, $dontNotify, $theComment, $theTmpFilename,
        $forceDate) = @_;

    my $topicHandler = _getTopicHandler( $web, $topic, $attachment );
    my $error = $topicHandler->addRevision( $theTmpFilename, $theComment, $TWiki::userName );
    $topicHandler->setLock( ! $doUnlock );

    return $error;
}

So $doUnlock should more properly be called $lockOrUnlock or somesuch. I think the intention is to do this:

   $topicHandler->setLock( 0 ) if ( $doUnlock );

Im my case I really don't want to unlock anything, because I will be attaching further files whilst holding the topic locked.

I see this code repeated in other places as well, is this a generic bug or am I misunderstanding something?

Test case

None readily available, found during development of addon script (see MailToTWikiAddOnDev)

Environment

-- AlanBurlison - 23 Jan 2003

Follow up

Officially you should not use undocumented functions in TWiki because your add-on or Plugin might break in the future. Granted, we need to enhance the TWikiFuncModule to handle file attachments.

The setLock handling is correct. If you do not want to unlock the topic simply re-lock it several times.

Fix record

• Allow topic lock when uploading a file
• Removed topic lock warning (because of above spec change)
• Removed $filename parameter from sub isAsciiDefault(), it was called without it.

In TWikiAlphaRelease and at TWiki.org.

-- PeterThoeny - 30 Jan 2003

Thanks for the fix - it was the warning message that was tripping me up. FYI, the only other unofficial functions I needed to write the MailToTWikiAddOn were:

• TWiki::initialize
• TWiki::isWikiName

-- AlanBurlison - 30 Jan 2003