Merge hunk feature modifies file

[gotar]

Calling DiffMergeCurrentHunk (F5) causes left file to be saved instantly. Any forced exit at this time (either segv or some system error or power outage etc.) leaves modified file! Even discarding changes at legit exit doesn't work well as it restores file from backup created with current mtime, which is wrong as it confuses e.g. backup tools.

[angel_il]

and what do you suggest?
step-by-step...

[gotar]

Merging should be done on temporary file created instead of the backup (or better in memory, just like mcedit does). Swapping usage of original and backup/temporary file is the simplest solution.

[szaszg]

This ​patch make mcdiff use temp files for merge.

There is one problem:
Before the patch (when all changes made on the original files immediately), when we open files for edit (F4, F14) the file opened with correct syntax highlighting. Now, because mcdiffview open the temp files (in order to user see the actual state of the file) in some cases syntax highlighting does not works. Because some cases (e.g. for configure.ac) the extension of the file is not enough for mc to detect syntax rule :-(...
Solution:

• user can select proper syntax rule ;-(
• we can pass somehow the 'original' filename to mcedit (?)
• we can make a temp directory (in the temp directory :-), and save the temp file into this with original name ...

1. src/util.{h,c}
   • new function: mc_util_make_backup_if_possible_from() - like mc_util_make_backup_if_possible() just we can give the backup file name too
   • new function: mc_util_restore_from_backup_if_possible_to() - the pair of mc_util_restore_from_backup_if_possible() like before
   • changed functions: mc_util_make_backup_if_possible(), mc_util_restore_from_backup_if_possible() - just call ..._from() and ..._to()

2. diffviewer/internal.h
   • new struct member: orig_file - in WDiff we can save the original filename if do_merge_hunk() switch to temp file

3. diffviewer/ydiff.c
   • change function: do_merge_hunk() - create temp file before do the first merge (in any side)
   • change function: dview_save() - to handle temp file restoration and unlink
   • change function: dview_ok_to_exit() - to handle temp file restoration and unlink

[andrew_b]

Replying to szaszg:

There are some notes about the patch:

---

            GString *buff = g_string_new("");

Don't initialize variable by function. Split it to variable declaration and function call:

            GString *buff;

            buff = g_string_new("");

This isn't error itself, but our programming style.

---

            dview->file[n_merge] = g_strdup (vfs_path_to_str (temp_file_vpath));

There is a memory leak here. vfs_path_to_str() returns newly allocated string. g_strdup() is superfluous.

---

                    dview->file[n_merge] = g_strdup (buff->str);

Avoid extra string duplication. Use already created string:

                    dview->file[n_merge] = g_string_free (buff->str, FALSE);

Or, if you exactly need to duplicate of GString content, use g_strndup() to avoid extra strlen() call:

                    dview->file[n_merge] = g_strndup (buff->str, buff->len);

---

Thanks for your patches!

[szaszg]

mcdiff use temporary files for merge

[szaszg]

I updated the ​patch. I hope, now it is better ;-)

[andrew_b]

Replying to szaszg:

I updated the ​patch. I hope, now it is better ;-)

+            /* if no extension extension() return "" not NULL */
+            if (ext[0] != '\0')
+            {

There is a memory leak in case of ext[0] == '\0': buff is not free'd. Just move declaration of buff into this if block.
I think, GString is not needed here. char * and g_strconcat or g_strdup_printf are good to build file name.

[andrew_b]

+                dview->tempdir = g_build_filename (mc_tmpdir (), "mcdiffXXXXXX", (char *) NULL);
+                dview->tempdir = g_mkdtemp (dview->tempdir);

This is memory leak!

[szaszg]

use tmp dir for temp files with original filename

[szaszg]

I updated...
Question: for the "syntax highlighting" solution is the temp dir method acceptable?

[angel_il]

that do you want to highlight?

[szaszg]

Replying to angel_il:

that do you want to highlight?

After the first hunk merged, mcdiff uses temporary file, so if user open it (F4 or F14) mcedit search syntax rule for the temporary file name (or extension, or first line). If we create "ordinary" temp files the "syntax by filename or extension" fail (most of the syntax rule based on extension /sometimes the whole filename/, so if temp filename has not the same extension than the original filename, mcedit select "unknown" rule even if original filename is eg. configure.ac).
IMHO the best solution, if mcdiff create a temp directory and save temp files with the original file name here.