Ticket #2510 (new defect)

Opened 8 years ago

Last modified 3 years ago

Merge hunk feature modifies file

Reported by: gotar Owned by:
Priority: major Milestone: Future Releases
Component: mcdiff Version: 4.7.5
Keywords: Cc:
Blocked By: Blocking:
Branch state: no branch Votes for changeset:

Description

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.

Attachments

mc.2510.diff (9.5 KB) - added by szaszg 6 years ago.
mcdiff use temporary files for merge
mc.2510_2.diff (10.1 KB) - added by szaszg 6 years ago.
use tmp dir for temp files with original filename

Change History

comment:1 Changed 8 years ago by angel_il

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

comment:2 Changed 8 years ago by 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.

comment:3 Changed 7 years ago by gotar

  • Priority changed from minor to major
  • Branch state set to no branch

comment:4 follow-up: ↓ 5 Changed 6 years ago by 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()
  1. diffviewer/internal.h
    • new struct member: orig_file - in WDiff we can save the original filename if do_merge_hunk() switch to temp file
  1. 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

comment:5 in reply to: ↑ 4 Changed 6 years ago by 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!

Changed 6 years ago by szaszg

mcdiff use temporary files for merge

comment:6 follow-up: ↓ 7 Changed 6 years ago by szaszg

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

comment:7 in reply to: ↑ 6 Changed 6 years ago by 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.

comment:8 Changed 6 years ago by andrew_b

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

This is memory leak!

Changed 6 years ago by szaszg

use tmp dir for temp files with original filename

comment:9 Changed 6 years ago by szaszg

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

comment:10 follow-up: ↓ 11 Changed 5 years ago by angel_il

that do you want to highlight?

comment:11 in reply to: ↑ 10 Changed 5 years ago by 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.

comment:12 Changed 3 years ago by andrew_b

  • Milestone changed from 4.8 to Future Releases
Note: See TracTickets for help on using tickets.