Ticket #2510 (new defect)
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
Change History
comment:2 Changed 14 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 13 years ago by gotar
- Priority changed from minor to major
- Branch state set to no branch
comment:4 follow-up: ↓ 5 Changed 11 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 ...
- 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()
- diffviewer/internal.h
- new struct member: orig_file - in WDiff we can save the original filename if do_merge_hunk() switch to temp file
- 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 11 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!
comment:6 follow-up: ↓ 7 Changed 11 years ago by szaszg
I updated the patch. I hope, now it is better ;-)
comment:7 in reply to: ↑ 6 Changed 11 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 11 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 11 years ago by szaszg
- Attachment mc.2510_2.diff added
use tmp dir for temp files with original filename
comment:9 Changed 11 years ago by szaszg
I updated...
Question: for the "syntax highlighting" solution is the temp dir method acceptable?
comment:10 follow-up: ↓ 11 Changed 11 years ago by angel_il
that do you want to highlight?
comment:11 in reply to: ↑ 10 Changed 11 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.
and what do you suggest?
step-by-step...