Fix inconsistent VCS checkin return values

It is not possible to transport the different error/abort/success conditions
of a VCS checkin through a simple empty or nonempty string. Therefore it was
no wonder that the return values were used inconsistently for different VCS.
Now the log and return status are decoupled, and all VCS are handled
consistently. This is a prerequisite for proper locking detection of the
upcoming rename command.

src/LyXVC.cpp

@@ -173,14 +173,13 @@ bool LyXVC::registrer()
 }
 
 
-string LyXVC::checkIn()
+LyXVC::CommandResult LyXVC::checkIn(string & log)
 {
 	LYXERR(Debug::LYXVC, "LyXVC: checkIn");
 	if (!vcs)
-		return string();
+		return ErrorBefore;
 	docstring empty(_("(no log message)"));
 	docstring response;
-	string log;
 	bool ok = true;
 	if (vcs->isCheckInWithConfirmation())
 		ok = Alert::askForText(response, _("LyX VC: Log Message"));
@@ -189,15 +188,11 @@ string LyXVC::checkIn()
 			response = empty;
 		//shell collisions
 		response = subst(response, from_ascii("\""), from_ascii("\\\""));
-		log = vcs->checkIn(to_utf8(response));
-
-		// Reserve empty string for cancel button
-		if (log.empty())
-			log = to_utf8(empty);
+		return vcs->checkIn(to_utf8(response), log);
 	} else {
 		LYXERR(Debug::LYXVC, "LyXVC: user cancelled");
+		return Cancelled;
 	}
-	return log;
 }
 
 
@@ -270,9 +265,13 @@ string LyXVC::toggleReadOnly()
 	case VCS::UNLOCKED:
 		LYXERR(Debug::LYXVC, "LyXVC: toggle to locked");
 		return checkOut();
-	case VCS::LOCKED:
+	case VCS::LOCKED: {
 		LYXERR(Debug::LYXVC, "LyXVC: toggle to unlocked");
-		return checkIn();
+		string log;
+		if (checkIn(log) != Success)
+			return string();
+		return log;
+	}
 	case VCS::NOLOCKING:
 		break;
 	}

src/LyXVC.h

@@ -39,6 +39,13 @@ class Buffer;
 */
 class LyXVC {
 public:
+	/// Return status of a command
+	enum CommandResult {
+		Cancelled,    ///< command was cancelled
+		ErrorBefore,  ///< error before executing command
+		ErrorCommand, ///< error while executing command
+		Success       ///< command was executed successfully
+	};
 	///
 	LyXVC();
 	///
@@ -77,8 +84,9 @@ public:
 	// by the next multiple messages on the top of the processed dispatch
 	// machinery.
 
-	/// Unlock and commit changes. Returns log.
-	std::string checkIn();
+	/// Unlock and commit changes.
+	/// \p log is non-empty on success and may be empty on failure.
+	CommandResult checkIn(std::string & log);
 	/// Does the current VC support this operation?
 	bool checkInEnabled() const;
 

src/VCBackend.cpp

@@ -233,12 +233,15 @@ void RCS::registrer(string const & msg)
 }
 
 
-string RCS::checkIn(string const & msg)
+LyXVC::CommandResult RCS::checkIn(string const & msg, string & log)
 {
 	int ret = doVCCommand("ci -q -u -m\"" + msg + "\" "
 		    + quoteName(onlyFileName(owner_->absFileName())),
 		    FileName(owner_->filePath()));
-	return ret ? string() : "RCS: Proceeded";
+	if (ret)
+		return LyXVC::ErrorCommand;
+	log = "RCS: Proceeded";
+	return LyXVC::Success;
 }
 
 
@@ -757,22 +760,27 @@ string CVS::scanLogFile(FileName const & f, string & status)
 	ifs.close();
 	return string();
 }
-	
-	
-string CVS::checkIn(string const & msg)
+
+
+LyXVC::CommandResult CVS::checkIn(string const & msg, string & log)
 {
 	CvsStatus status = getStatus();
 	switch (status) {
 	case UpToDate:
 		if (vcstatus != NOLOCKING)
-			unedit();
-		return "CVS: Proceeded";
+			if (unedit())
+				return LyXVC::ErrorCommand;
+		log = "CVS: Proceeded";
+		return LyXVC::Success;
 	case LocallyModified:
 	case LocallyAdded: {
 		int rc = doVCCommand("cvs -q commit -m \"" + msg + "\" "
 			+ getTarget(File),
 		    FileName(owner_->filePath()));
-		return rc ? string() : "CVS: Proceeded";
+		if (rc)
+			return LyXVC::ErrorCommand;
+		log = "CVS: Proceeded";
+		return LyXVC::Success;
 	}
 	case NeedsMerge:
 	case NeedsCheckout:
@@ -787,7 +795,7 @@ string CVS::checkIn(string const & msg)
 				toString(status)));
 		break;
 	}
-	return string();
+	return LyXVC::ErrorBefore;
 }
 
 
@@ -1158,31 +1166,41 @@ void SVN::registrer(string const & /*msg*/)
 }
 
 
-string SVN::checkIn(string const & msg)
+LyXVC::CommandResult SVN::checkIn(string const & msg, string & log)
 {
 	FileName tmpf = FileName::tempName("lyxvcout");
 	if (tmpf.empty()){
 		LYXERR(Debug::LYXVC, "Could not generate logfile " << tmpf);
-		return N_("Error: Could not generate logfile.");
+		log = N_("Error: Could not generate logfile.");
+		return LyXVC::ErrorBefore;
 	}
 
-	doVCCommand("svn commit -m \"" + msg + "\" "
-		    + quoteName(onlyFileName(owner_->absFileName()))
-		    + " > " + quoteName(tmpf.toFilesystemEncoding()),
-		    FileName(owner_->filePath()));
+	ostringstream os;
+	os << "svn commit -m \"" << msg << '"';
+	os << ' ' << quoteName(onlyFileName(owner_->absFileName()));
+	os << " > " << quoteName(tmpf.toFilesystemEncoding());
+	LyXVC::CommandResult ret =
+		doVCCommand(os.str(), FileName(owner_->filePath())) ?
+			LyXVC::ErrorCommand : LyXVC::Success;
 
-	string log;
 	string res = scanLogFile(tmpf, log);
-	if (!res.empty())
+	if (!res.empty()) {
 		frontend::Alert::error(_("Revision control error."),
 				_("Error when committing to repository.\n"
 				"You have to manually resolve the problem.\n"
 				"LyX will reopen the document after you press OK."));
+		ret = LyXVC::ErrorCommand;
+	}
 	else
-		fileLock(false, tmpf, log);
+		if (!fileLock(false, tmpf, log))
+			ret = LyXVC::ErrorCommand;
 
 	tmpf.erase();
-	return log.empty() ? string() : "SVN: " + log;
+	if (!log.empty())
+		log.insert(0, "SVN: ");
+	if (ret == LyXVC::Success && log.empty())
+		log = "SVN: Proceeded";
+	return ret;
 }
 
 
@@ -1247,10 +1265,10 @@ string SVN::scanLogFile(FileName const & f, string & status)
 }
 
 
-void SVN::fileLock(bool lock, FileName const & tmpf, string &status)
+bool SVN::fileLock(bool lock, FileName const & tmpf, string &status)
 {
 	if (!locked_mode_ || (isLocked() == lock))
-		return;
+		return true;
 
 	string const arg = lock ? "lock " : "unlock ";
 	doVCCommand("svn "+ arg + quoteName(onlyFileName(owner_->absFileName()))
@@ -1266,16 +1284,20 @@ void SVN::fileLock(bool lock, FileName const & tmpf, string &status)
 	}
 	ifs.close();
 
-	if (!isLocked() && lock)
+	if (isLocked() == lock)
+		return true;
+
+	if (lock)
 		frontend::Alert::error(_("Revision control error."),
 			_("Error while acquiring write lock.\n"
 			"Another user is most probably editing\n"
 			"the current document now!\n"
 			"Also check the access to the repository."));
-	if (isLocked() && !lock)
+	else
 		frontend::Alert::error(_("Revision control error."),
 			_("Error while releasing write lock.\n"
 			"Check the access to the repository."));
+	return false;
 }
 
 

src/VCBackend.h

@@ -39,8 +39,10 @@ public:
 
 	/// register a file for version control
 	virtual void registrer(std::string const & msg) = 0;
-	/// check in the current revision, returns log
-	virtual std::string checkIn(std::string const & msg) = 0;
+	/// check in the current revision.
+        /// \p log is non-empty on success and may be empty on failure.
+	virtual LyXVC::CommandResult
+	checkIn(std::string const & msg, std::string & log) = 0;
 	/// can this operation be processed in the current VCS?
 	virtual bool checkInEnabled() = 0;
 	/// should a log message provided for next checkin?
@@ -140,7 +142,8 @@ public:
 
 	virtual void registrer(std::string const & msg);
 
-	virtual std::string checkIn(std::string const & msg);
+	virtual LyXVC::CommandResult
+	checkIn(std::string const & msg, std::string & log);
 
 	virtual bool checkInEnabled();
 
@@ -214,7 +217,8 @@ public:
 
 	virtual void registrer(std::string const & msg);
 
-	virtual std::string checkIn(std::string const & msg);
+	virtual LyXVC::CommandResult
+	checkIn(std::string const & msg, std::string & log);
 
 	virtual bool checkInEnabled();
 
@@ -343,7 +347,8 @@ public:
 
 	virtual void registrer(std::string const & msg);
 
-	virtual std::string checkIn(std::string const & msg);
+	virtual LyXVC::CommandResult
+	checkIn(std::string const & msg, std::string & log);
 
 	virtual bool checkInEnabled();
 
@@ -392,7 +397,7 @@ protected:
 	/// is the loaded file locked?
 	bool isLocked() const;
 	/// acquire/release write lock for the current file
-	void fileLock(bool lock, support::FileName const & tmpf, std::string & status);
+	bool fileLock(bool lock, support::FileName const & tmpf, std::string & status);
 
 private:
 	/// is the loaded file under locking policy?

src/frontends/qt4/GuiView.cpp

@@ -2255,7 +2255,7 @@ bool GuiView::renameBuffer(Buffer & b, docstring const & newname)
 	// trying to overwrite ourselves, which is fine.)
 	if (theBufferList().exists(fname) && fname != oldname
 		  && theBufferList().getBuffer(fname) != &b) {
-		docstring const text = 
+		docstring const text =
 			bformat(_("The file\n%1$s\nis already open in your current session.\n"
 		            "Please close it before attempting to overwrite it.\n"
 		            "Do you want to choose a new filename?"),
@@ -2366,7 +2366,8 @@ bool GuiView::exportBufferAs(Buffer & b)
 }
 
 
-bool GuiView::saveBuffer(Buffer & b) {
+bool GuiView::saveBuffer(Buffer & b)
+{
 	return saveBuffer(b, FileName());
 }
 
@@ -2377,7 +2378,7 @@ bool GuiView::saveBuffer(Buffer & b, FileName const & fn)
 		return true;
 
 	if (fn.empty() && b.isUnnamed())
-			return renameBuffer(b, docstring());
+		return renameBuffer(b, docstring());
 
 	bool success;
 	if (fn.empty())
@@ -2812,8 +2813,14 @@ void GuiView::dispatchVC(FuncRequest const & cmd, DispatchResult & dr)
 		if (!buffer || !ensureBufferClean(buffer))
 			break;
 		if (buffer->lyxvc().inUse() && !buffer->isReadonly()) {
-			dr.setMessage(buffer->lyxvc().checkIn());
-			if (!dr.message().empty())
+			string log;
+			LyXVC::CommandResult ret = buffer->lyxvc().checkIn(log);
+			dr.setMessage(log);
+			// Only skip reloading if the checkin was cancelled or
+			// an error occured before the real checkin VCS command
+			// was executed, since the VCS might have changed the
+			// file even if it could not checkin successfully.
+			if (ret == LyXVC::ErrorCommand || ret == LyXVC::Success)
 				reloadBuffer(*buffer);
 		}
 		break;