Re: Another patch for 1.6.x
Stephan Witt wrote: I have another patch in my local branch checkout... It fixes a warning and a potential bug. OK. Jürgen
Re: r35620
Uwe Stöhr wrote: Jürgen, can this also go to branch? Yes. Jürgen
Re: r35575 - lyx-devel/trunk/src
Stephan Witt wrote: The owner_ variable points to the buffer, of course. Most of the time VC implementation uses the filename of the buffer. But I cannot see what sense the file_ variable of the VC instance has. intuitively the meaning is clear - when possible use owner. when buffer might/does not exist use filename. there are two cases i can imagine where buffer does not exist - calling scanMaster before loading the file and when we reload the document. reload has been refactorized during 2.0svn development and iirc buffer structure is not destructed now. whether scanMaster is actually called before buffer creation i would need to check. otherwise i wouldnt see need for file_. One can drop it - or use it consequently - I'd say. i would need to dig into the code if you want some specific info. If possible... that would be nice. i quickly went through sources and saw also few usage of file_ outside scanMaster in svn routines which looks inconsitent and owner_ would be more appropriate. pavel
Re: r35575 - lyx-devel/trunk/src
Pavel Sanda wrote: i quickly went through sources and saw also few usage of file_ outside scanMaster in svn routines which looks inconsitent and owner_ would be more appropriate. no as i see it again they are part of scanMaster so the question really is whether this routine can be called without existing buffer... pavel
Re: r35575 - lyx-devel/trunk/src
Am 15.10.2010 um 15:08 schrieb Pavel Sanda: Pavel Sanda wrote: i quickly went through sources and saw also few usage of file_ outside scanMaster in svn routines which looks inconsitent and owner_ would be more appropriate. no as i see it again they are part of scanMaster so the question really is whether this routine can be called without existing buffer... AFAICS not. scanMaster() gets called from constructors only. The constructors are in LyXVC::registrer() which gets called from buffer and LyXVC::file_found_hook() - called when buffer file is readable. So I think file_ is used as internal state to transfer the constructor parameter to scanMaster. Since it's not really broken - I'll refrain from fixing it. But in general owner_-fileName() should be used instead of file_. Another question: when the working copy has no diff to repository the check in operation isn't sensible. But it's expensive to test this. So I'd like to test it before prompting for the log message and then doing the noop check in. Does this sound sensible? The proper solution would be to refactor the diff operation and use it for that and for repoUpdate in the check for it and prompt when a real change was made. If you're fine with that I'll prepare a patch otherwise I have to copy the diff test code to CVS::checkIn. Stephan
Re: r35575 - lyx-devel/trunk/src
Stephan Witt wrote: But in general owner_-fileName() should be used instead of file_. yes Another question: when the working copy has no diff to repository the check in operation isn't sensible. But it's expensive to test this. So I'd like to test it before prompting for the log message and then doing the noop check in. Does this sound sensible? this is already bug #chrrm (bugzilla is down). i was about to fix it few months back but there was some catch (hard to remember, maybe because checkin can release locks only or something like that), so i postponed it ;) i agree this should be fixed but dont have time recheck that all other rcs/svn usecases are not affected, so this testing would be up to you. pavel
Re: r35575 - lyx-devel/trunk/src
Pavel Sanda wrote: this is already bug #chrrm (bugzilla is down). #6396
Re: r35575 - lyx-devel/trunk/src
Am 15.10.2010 um 18:00 schrieb Pavel Sanda: Pavel Sanda wrote: this is already bug #chrrm (bugzilla is down). #6396 Ok, thanks. Stephan
Re: Another patch for 1.6.x
Stephan Witt wrote: > I have another patch in my local branch checkout... > It fixes a warning and a potential bug. OK. Jürgen
Re: r35620
Uwe Stöhr wrote: > Jürgen, > > can this also go to branch? Yes. Jürgen
Re: r35575 - lyx-devel/trunk/src
Stephan Witt wrote: > The owner_ variable "points" to the buffer, of course. > Most of the time VC implementation uses the filename of the buffer. > But I cannot see what sense the file_ variable of the VC instance has. intuitively the meaning is clear - when possible use owner. when buffer might/does not exist use filename. there are two cases i can imagine where buffer does not exist - calling scanMaster before loading the file and when we reload the document. reload has been refactorized during 2.0svn development and iirc buffer structure is not destructed now. whether scanMaster is actually called before buffer creation i would need to check. otherwise i wouldnt see need for file_. > One can drop it - or use it consequently - I'd say. > > > i would need to dig into the code if you want some > > specific info. > > If possible... that would be nice. i quickly went through sources and saw also few usage of file_ outside scanMaster in svn routines which looks inconsitent and owner_ would be more appropriate. pavel
Re: r35575 - lyx-devel/trunk/src
Pavel Sanda wrote: > i quickly went through sources and saw also few usage of file_ outside > scanMaster in svn routines which looks inconsitent and owner_ would be > more appropriate. no as i see it again they are part of scanMaster so the question really is whether this routine can be called without existing buffer... pavel
Re: r35575 - lyx-devel/trunk/src
Am 15.10.2010 um 15:08 schrieb Pavel Sanda: > Pavel Sanda wrote: >> i quickly went through sources and saw also few usage of file_ outside >> scanMaster in svn routines which looks inconsitent and owner_ would be >> more appropriate. > > no as i see it again they are part of scanMaster so the question really is > whether > this routine can be called without existing buffer... AFAICS not. scanMaster() gets called from constructors only. The constructors are in LyXVC::registrer() which gets called from buffer and LyXVC::file_found_hook() - called when buffer file is readable. So I think file_ is used as internal state to transfer the constructor parameter to scanMaster. Since it's not really broken - I'll refrain from fixing it. But in general owner_->fileName() should be used instead of file_. Another question: when the working copy has no diff to repository the check in operation isn't sensible. But it's expensive to test this. So I'd like to test it before prompting for the log message and then doing the noop check in. Does this sound sensible? The proper solution would be to refactor the diff operation and use it for that and for repoUpdate in the check for it and prompt when a real change was made. If you're fine with that I'll prepare a patch otherwise I have to copy the diff test code to CVS::checkIn. Stephan
Re: r35575 - lyx-devel/trunk/src
Stephan Witt wrote: > But in general owner_->fileName() should be used instead of file_. yes > Another question: when the working copy has no diff to repository > the check in operation isn't sensible. But it's expensive to test > this. So I'd like to test it before prompting for the log message > and then doing the noop check in. Does this sound sensible? this is already bug #chrrm (bugzilla is down). i was about to fix it few months back but there was some catch (hard to remember, maybe because checkin can release locks only or something like that), so i postponed it ;) i agree this should be fixed but dont have time recheck that all other rcs/svn usecases are not affected, so this testing would be up to you. pavel
Re: r35575 - lyx-devel/trunk/src
Pavel Sanda wrote: > this is already bug #chrrm (bugzilla is down). #6396
Re: r35575 - lyx-devel/trunk/src
Am 15.10.2010 um 18:00 schrieb Pavel Sanda: > Pavel Sanda wrote: >> this is already bug #chrrm (bugzilla is down). > > #6396 Ok, thanks. Stephan
