Stephan Witt wrote:
> > 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.
sorry is starts to go beyond my head, cf end of the mail...
> >> --- 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.
i meant that there is currently else part in code below (not seen in patch)
which
triggers cancel message, just in case no diff was found.
> >> 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.
the problem is how can various rcs/svn/cvs version react on empty message
as parameter. compared to this few cpu cycles are negligible.
> >> @@ -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?
either ask for isClean or not touch revert at all
> >> + 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.
not let it as it is. it was kind of joke.
> 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.
yes its ugly hack and i did it because there was no reasonable way how to pass
return info to user at the moment i implemented (this should be better now).
there are many things which can be improved inside revision control code
implementation however i want recall our original settlement - do some changes
inside CVS but dont touch API at this stage of development. i dont have time to
reimplement things and test all possible usescases again before public release
of 2.0.
you wanted to fix that bug that we ask for log even if only lock (or something
like that in cvs) is to be relased, ok, but please dont go beyond that.
pavel
