[Geany-devel] GIO file monitor - Re: Changed file saving implementation for systems with GIO

[Eugene Arshinov]

On Mon, 15 Nov 2010 09:41:53 +1100%
Lex Trotman <elextr at gmail.com> wrote:

> On 15 November 2010 03:29, Eugene Arshinov <earshinov at gmail.com>
> wrote:
> > Some results after preliminary investigation:
> >
> > On Sun, 14 Nov 2010 14:25:54 +0300%
> > Eugene Arshinov <earshinov at gmail.com> wrote:
> >
> >> On Sun, 14 Nov 2010 22:02:31 +1100%
> >> Lex Trotman <elextr at gmail.com> wrote:
> >>
> >> > On 14 November 2010 21:37, Eugene Arshinov <earshinov at gmail.com>
> >> > wrote:
> >> > > On Sun, 14 Nov 2010 21:24:39 +1100%
> >> > > Lex Trotman <elextr at gmail.com> wrote:
> >> > >
> >> > >> On 14 November 2010 20:31, Eugene Arshinov
> >> > >> <earshinov at gmail.com> wrote:
> >> > >> > Hi.
> >> > >> >
> >> > >> > I don't know whether it was this change which caused this,
> >> > >> > but after I updated recently to r5395 and turned on the
> >> > >> > #define in document.c which controls using GIO file
> >> > >> > monitor, each time I save a document (I only use local
> >> > >> > filesystems) I get a dialog telling me the file was
> >> > >> > changed. In debug output coming from
> >> > >> > document.c:monitor_file_changed_cb() I see that CHANGE
> >> > >> > notification is sent twice after a file is saved.  Maybe
> >> > >> > it's g_file_replace_contents() which cause this,
> >> > >>
> >> > >> Possibly, g_file_replace_contents writes to the temp file and
> >> > >> then renames the temp file to the old file, but why two??.
> >> > >>
> >> > >> The interesting thing is why doesn't any change to the file
> >> > >> trigger the monitor no matter how its written??  Why does it
> >> > >> only happen for GIO IO??
> >> > >
> >> > > I was not fully correct.  There were 2 CHANGE notifications
> >> > > and 1 "unknown" notification between them.  Here is the output
> >> > > I get after opening, changing and saving a file:
> >> > >
> >> > > Geany-INFO: /tmp/temp.xml : XML (UTF-8)
> >> > > Geany-INFO: /tmp/temp.xml : XML (UTF-8)
> >> > > Geany-INFO: monitor_file_changed_cb: event: CHANGED; status:
> >> > > IGNORE -> OK
> >> > > Geany-INFO: monitor_file_changed_cb: event: UNKNOWN; status:
> >> > > OK -> OK Geany-INFO: monitor_file_changed_cb: event: CHANGED;
> >> > > status: OK -> OK
> >> > >
> >> >
> >> > Since what is called change here is actually last_change_hint,
> >> > you could actually get it several times because its only
> >> > "probably" the last change.
> >>
> >> Yes, and it also means that the original implementation, which
> >> ignores exactly one hint callback receives, is wrong.  The
> >> comments near the #define actually tell that GIO based file
> >> monitoring is not completely stable, but when I ran an older
> >> version of Geany (maybe rev. 5380) with the #define turned on,
> >> saving seemed to work fine. Though there is a high probability
> >> that I'm wrong because I used that version only for testing new
> >> patches (i.e., rarely).  Anyway, seems that I need to find a
> >> version where saving began to "fail".
> >
> > Okay, it still fails at r5380.  The change is caused by
> > g_file_replace_contents() which was introduced before than that.
> >
> >>
> >> > The unknown could be move/rename delete or just plain
> >> > change (probably *not* last??).
> >> >
> >> > > Note that with my patch the output is different from trunk, but
> >> > > things should be clear.  I'll investigate the unknown
> >> > > notification a bit later today, maybe it is caused by the
> >> > > rename.
> >> >
> >> > I'm not sure how g_file_monitor would report an atomic rename of
> >> > a temp file over the file we are monitoring??  The directory
> >> > entry is changed and the old inode and data deleted if this was
> >> > the last hard link.
> >> >
> >> > You would have to test it or look at the source.
> >> >
> >>
> >> Yes, there's a need for experiments.
> >>
> >
> > r5395
> >  Geany-INFO: monitor_file_changed_cb: event: 1 previous file
> > status: 2
> 
> Probably the change of atime, though I would have expected that to be
> an attribute change event.
> 
> >  Geany-Message: monitor_file_changed_cb: FILE_CHANGED
> >  Geany-INFO: monitor_file_changed_cb: event: 3 previous file
> > status: 0
> 
> Create when the rename is done, OK
> 
> >  Geany-INFO: monitor_file_changed_cb: event: 1 previous file
> > status: 0 Geany-Message: monitor_file_changed_cb: FILE_CHANGED
> 
> Thats another change of atime when the file descriptor is closed (on
> Unix its closed *after* the rename).
> 
> >
> > r5065 (before g_file_replace_contents())
> >  Geany-INFO: monitor_file_changed_cb: event: 0 previous file
> > status: 2 Geany-INFO: monitor_file_changed_cb: event: 1 previous
> > file status: 2 Geany-Message: monitor_file_changed_cb: FILE_CHANGED
> >
> 
> And with safe file saving (ie g_file_set_contents) it just gets:
> 
> Geany-INFO: monitor_file_changed_cb: event: 3 previous file status: 2
> 
> Because it only does the rename it doesn't do the other opens and
> closes.
> 
> Its not the Glib version, it happens for me on 2.24.1 as well.
> 
> The above looks to me like Glib is working properly, Geany just needs
> to learn to ignore changes of its own making better.
> 
> Your mtime patch looks sensible to me, since we really don't want to
> reload unless the content changed.  We don't care about changes to
> metadata since we don't use it.  Except I suggest:
> 
> changed = doc->priv->mtime < st.st_mtime;
> 
> be != instead of < in case someone restores an old version using a
> tool that also restores the mtime.
> 

Agree, but then we also need to change a similar check in
document_check_disk_status():

	else if (doc->priv->mtime < st.st_mtime)
	{
		monitor_reload_file(doc);
		doc->priv->mtime = st.st_mtime;
		ret = TRUE;
	}

Best regards,
Eugene.