[Geany-Devel] Simplifying file saving options

Lex Trotman elextr at xxxxx
Wed Aug 19 10:43:38 UTC 2015


On 19 August 2015 at 19:48, Jiří Techet <techet at gmail.com> wrote:
> Hi,
>
> after some time spent debugging
>
> https://github.com/geany/geany/issues/605
>
> I think the file saving options are a bit confusing and misleading and
> should be simplified.

Agree the usability is poor with several options that interact with each other.

>
> 1. Both g_file_set_contents() and g_file_replace_contents() do absolutely
> the same thing (saving to temporary file, then renaming the file),

No they are not the same, set_contents does not work on file systems
where rename is not available or is not allowed over existing files
(windows), and it creates a new file with new file metadata (eg
permissions), whereas replace_contents is much more complex than set,
it tries to compensate for these issues but can do several copies of
the data during the attempt, which is very costly on slow remote
filesystems and has a bug (see below).

> the only
> difference is the first uses POSIX calls while the second GIO. The
> use_gio_unsafe_file_saving name is very confusing because it's not unsafe in
> any way.

It is unsafe due to a long standing bug where a failure deletes the
copy of the original data without copying it back to the original
file, so you are left with a truncated original file and no backup of
the original data.  You still have the new data in the Geany buffer of
course.  But it regularly has caught people out if they closed the
buffer after a failure assuming they had the original data safe.

>
> 2. It is now possible to set
>
> use_gio_unsafe_file_saving=true
> use_atomic_file_saving=true
>
> and to users it's unclear which of the options will be actually used (it's
> the use_atomic_file_saving case because it's checked first in the code).

Yeah, an enum not a set of bools would be better, IIUC the current
system grew all the options over time rather than being planned.  For
sure cleaning it up would be good.

>
> 3. The fallback "ordinary" file saving when
>
> use_gio_unsafe_file_saving=false
> use_atomic_file_saving=false
>
> doesn't bring any benefit compared to the two above

It just overwrites the file, nothing else, one data transfer, fast, no
rename, works on *all* filesystems, keeps the files metadata.  Whilst
its not in any way "safe" its got the best performance.

- it's similar to
> use_atomic_file_saving=true in using POSIX calls, it's just a bit less safe
> because it writes directly to the file without the renaming. The GIO
> g_file_replace_contents() recreates the file's permissions/ownership
> correctly

Yes, but its part of the complexity I mentioned above, if it can't
re-create the metadata on a new file or it can't do the rename then it
copies the original file data, truncates the original file and writes
to it directly, so it keeps the metadata.  It is supposed to put the
original data back on a failure to be as safe as the rename system,
but it doesn't, which is where the "unsafe" comes in.

If this has been fixed in a version of GIO that matches the oldest
Glib/GTK we use then the set_contents can be removed and
replace_contents used in its place, but I am not aware of it having
been done.

> in
>
> https://github.com/GNOME/glib/blob/master/gio/glocalfileoutputstream.c#L734
>
>
> I would suggest removing the file saving option mentioned in (3) above and
> have just a single settings option like
>
> use_gio_file_operations
>
> Opinions?

Whilst the usability can definitely be improved, each of the three
options has different characteristics and fits a different use-case.
Since I only ever use reliable local native filesystems I would be
happy with the simple overwrite method, but I acknowledge that others
have differing needs.

>
> Cheers,
>
> Jiri
>
>
> _______________________________________________
> Devel mailing list
> Devel at lists.geany.org
> https://lists.geany.org/cgi-bin/mailman/listinfo/devel
>


More information about the Devel mailing list