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

[Lex Trotman]

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.


> Events:
>  0 - CHANGED
>  1 - CHANGES_HINT (handled in Geany)
>  3 - CREATED
>
> It's natural that CREATED is reported, but it's still unclear why
> the first CHANGES_HINT is sent.  Going to investigate further…
>
>> > But I still don't understand why using g_file_set_contents or plain
>> > fwrite doesn't trigger the monitor??  I can't see anywhere in
>> > document.c where it muzzles the monitor whilst saving.
>> >
>>
>> Seems strange for me too.
>
> Actually g_file_replace_contents(), g_file_set_contents(), and fwrite()
> - are called in document.c:write_data_to_disk()
> - which is only called in document.c:save_doc()
> - which is only called in document_save_file()
> - which itself sets file_disk_status to FILE_IGNORE.
>
> So we ignore the first change hint despite of the function that is
> finally called.
>

Well spotted.

Cheers
Lex

PS I've sent a patch for an option to enable g_file_replace_contents
making backups, that could give other sequences of events since the
old file is now renamed before the temp file is renamed.

>>
>> Best regards,
>> Eugene.
> _______________________________________________
> Geany-devel mailing list
> Geany-devel at uvena.de
> http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
>