Vincent van Ravesteijn wrote:
> I had a look at the locking code (because of bug #6433), but I have some
> questions.
thank you for looking on that bug, it would be problem to debug/test it here.
> Typo ? :
>
> void SVN::scanMaster()
> {
> locker_.clear();
> vcstatus = NOLOCKING;
> if (checkLockMode()) {
> if (isLocked()) {
> locker_ = "Locked";
> vcstatus = LOCKED;
> } else {
> locker_ = "Unlocked";
> - vcstatus = LOCKED;
> + vcstatus = UNLOCKED;
> }
> }
> }
maybe not typo, just that this assignment belongs one step up
in 'if' hierarchy. i will check it later.
> > /// The user currently keeping the lock on the VC file.
> > std::string locker_;
>
> If the locker_ string contains the user, I would say that the variable
> should be named such that it indicates that it is a user name. But then I
> see:
>
> > locker_ = "Unlocked";
>
> This is a strange user.
>
> > locker_ = "Locked";
>
> This is also a strange user, but moreover who did lock it then ?
>
> > locker_.clear();
>
> What does it mean when locker_ is empty .. ? then the file is not under
> locking policy. But why don't we check for "vcstatus == NOLOCKING" then ?
> It's a bit strange that we keep this variable and when we want to know this
> info, we check for an empty user id.
>
> Furthermore, locker() or locker_ is nowhere used except in the combination
> "locker().empty()". Why do we record the user name then ? Or is this
> unfinished business ?
the whole svn code started as a copy paste from rcs routines and these are
leftovers,
not unfinished business. maybe i was just afraid to kill the locker_ at the
time, not
to break too much things in one step... probably to be killed now.
> > LyXVC.h:
> >/// Returns the version number.
> >std::string const versionString() const;
> >
> >VCBackend.h
> >
> >/// return the current version description
> >virtual std::string const versionString() const = 0;
> >/// return the current version
> >std::string const & version() const { return version_; }
> >
> >virtual std::string const versionString() const {
> > return "RCS: " + version_;
> >}
>
> What is a version, version number, version description ? If the only use of
> versionString() is to add the "RCS: " part, then I'd rather write this as
> VCS::vcName() + " " + VCS::version().
ask Lars for the original intention ;)
> >virtual std::string checkIn(std::string const & msg) = 0;
> >virtual std::string checkOut() = 0;
> >virtual std::string repoUpdate() = 0;
> >virtual std::string lockingToggle() = 0;
>
> I don't understand why these return strings. That's at least very
> unintuitive. Why don't we call Buffer::message(string) to show the status
> message.
iirc its an ugly hack to postpone displaying of the messages. i have tried your
proposal and anything directly written into message() get immediately
overwritten by the next multiple messages on the top of the processed dispatch
machinery.
pavel
