Am 22.10.2010 um 02:11 schrieb Pavel Sanda:
> Stephan Witt wrote:
>> I made a new patch to implement getDiff() and use it to avoid the query for
>> log message
>> before checkIn() is done and the confirmation on revert when no diff is
>> found.
>
> thanks for your patience, i went closely through the patch now and generally
> liked the approach.
>
>> +FileName const CVS::getDiff(OperationMode opmode)
>> +{
>> + FileName tmpf = FileName::tempName("lyxvcout");
>> + if (tmpf.empty()) {
>> + LYXERR(Debug::LYXVC, "Could not generate logfile " << tmpf);
>> + return tmpf;
>> + }
>> +
>> + doVCCommand("cvs diff " + getTarget(opmode)
>> + + " > " + quoteName(tmpf.toFilesystemEncoding()),
>> + FileName(owner_->filePath()), false);
>> + return tmpf;
>> +}
>
> the error case is suspicious. if tempName fails or "cvs diff" fails how you
> detect
> this on a higher stage? cmiiw but if something fails you identify it with
> having
> empty diff, which looks wrong, even more if you release lock automatically in
> such
> a case...
Your right. I'll try to come up with a better solution.
BTW, I copied the tmpf allocation code sequence.
I think the debug message does not need to print the empty tmpf.
But I've found another way to solve the same goal - using "cvs status".
Checking for the diff is not good enough.
While doing some "stress test" with checkIn() I found the error message when
merge is needed
"Something's wrong with cvs commit" not acceptable. Then I tried to change that
and failed to
solve it because of the stderr output of the VC command is lost silently. So I
came to the
idea to implement getStatus() and use the result accordingly.
The result would be one out of
* uptodate
* locally modified
* needs checkout
* needs merge
* no cvs file
The checkInWithMessage() implementation would be then
{ return getStatus() == LocallyModified; }
And the checkIn() would do a switch on the getStatus() and
raise more descriptive error messages according the status.
>
>> +int CVS::update(OperationMode opmode, FileName const tmpf)
>
> unless you want to mix this with repoupdate why is opmode here?
Yes. That's the plan.
>> + // should a log message provided for next checkin?
>> + virtual bool checkInWithMessage() { return true; }
>
>> + // should a confirmation before revert requested?
>> + virtual bool revertWithConfirmation() { return true; }
>
> i would change naming, so its clear what the function really does.
> for example isCheckinWithConfirmation...
Ok.
>
>> private:
>> support::FileName file_;
>> // revision number from scanMaster
>> std::string version_;
>> /// The user currently keeping the lock on the VC file.
>> std::string locker_;
>> +
>> + virtual std::string const getTarget(OperationMode opmode);
>> + virtual support::FileName const getDiff(OperationMode opmode);
>> + virtual int edit();
>> + virtual int unedit();
>> + virtual int update(OperationMode opmode, support::FileName const tmpf);
>> + virtual bool const hasDiff(OperationMode opmode);
>> + virtual bool const hasDiff() { return hasDiff(File); }
>> };
>
> comments missing
Ok.
>
>> --- src/LyXVC.cpp (Revision 35732)
>> +++ src/LyXVC.cpp (Arbeitskopie)
>> @@ -163,9 +163,10 @@
>> docstring empty(_("(no log message)"));
>> docstring response;
>> string log;
>> - bool ok = Alert::askForText(response, _("LyX VC: Log Message"));
>> + bool dolog = vcs->checkInWithMessage();
>> + bool ok = !dolog || Alert::askForText(response, _("LyX VC: Log
>> Message"));
>
> hmm, but if !dolog then user automatically gets "cancel" message?
You mean the user should have the option to cancel the operation if the file is
up-to-date?
Seems reasonable.
>
>> if (ok) {
>> - if (response.empty())
>> + if (dolog && response.empty())
>> response = empty;
>
> why response=empty harms in !nodolog?
It doesn't harm. It wastes CPU cycles to set response when !nodolog.
>
>> @@ -212,8 +213,9 @@
>> docstring text = bformat(_("Reverting to the stored version of the "
>> "document %1$s will lose all current
>> changes.\n\n"
>> "Do you want to revert to the older version?"),
>> file);
>> - int const ret = Alert::prompt(_("Revert to stored version of
>> document?"),
>> - text, 0, 1, _("&Revert"), _("&Cancel"));
>> + bool const doask = vcs->revertWithConfirmation();
>
> i would need to check whats above this function in guiview but isn't possible
> that we will skip
> reverting in case the document is edited but not saved?
Yes, so it is. Then reverting should be guarded with revertEnabled() too?
>
>> + int const ret = doask ? Alert::prompt(_("Revert to stored version of
>> document?"),
>> + text, 0, 1, _("&Revert"), _("&Cancel")) : 0;
>
> correspondingly to your previous coding style one would expect ret = doask &&
> prompt ; ;)
I don't think so. The first is bool and the second is int.
But I can change the style to be more verbose if you like.
E. g.
if (vcs->checkInWithMessage()) {
log = vcs->checkIn(to_utf8(empty));
// Reserve empty string for cancel button
if (log.empty())
log = to_utf8(empty);
} else if (Alert::askForText(response, _("LyX VC: Log Message"))) {
if (response.empty())
response = empty;
log = vcs->checkIn(to_utf8(response));
// Reserve empty string for cancel button
if (log.empty())
log = to_utf8(empty);
} else {
LYXERR(Debug::LYXVC, "LyXVC: user cancelled");
}
BTW, I have the problem that returning strings as result code is strange.
And SVN::checkOut() implementation returns the a non empty string on error
when the temporary log file name cannot be generated.
Stephan
