#1913 Wrong Timestamps in Generaged Patch

[Liisachan]

version 2.13.7 Unicode, Windows XP SP3

For example, let's say we have two files, 20080101.utf16 and 20080101.utf8
whose time stamps are 2008-01-01; and we also have another two files,
20090101.utf16 and 20090101.utf8 whose time stamps are 2009-01-01.

If we do Tools - Generate Patch for .utf8, the patch says
--- path to/20080101.utf8 Tue Jan 01 00:00:00 2008
+++ path to/20090101.utf8 Thu Jan 01 00:00:00 2009
showing the correct time stamps. But if we do that between .utf16 files,
the patch says:
--- path to/20080101.utf16 Thu May 14 01:28:46 2009
+++ path to/20090101.utf16 Thu May 14 01:28:46 2009
not showing the right time stamps, probably showing the time stamps
of the temporary files it created just now.

To avoid these wrong timestamps and the inconsistent behavior,
WinMerge should remember the original timestamps and synchronize the timestamps
of the temporary files.
Currently I don't have MFC and can't test if I'm guessing right,
but maybe we can do so in
DiffFileData::Filepath_Transform (or it might be FileTransform_Prediffing)
like this:

DiffFileData.cpp
@@ -196,6 +235,10 @@
{
BOOL bMayOverwrite = FALSE; // temp variable set each time it is used

+ // Save the original timestamps
+ FILETIME ftCreated, ftModified;
+ BOOL bFileTimeSaved = GetFileTime(filepath, &ftCreated, &ftModified);
+
if (fpenc.encoding.m_unicoding && fpenc.encoding.m_unicoding != ucr::UCS2LE)
{
// second step : normalize Unicode to OLECHAR (most of time, do nothing) (OLECHAR = UCS-2LE in Windows)
@@ -235,6 +278,13 @@
if (!FileTransform_ToUTF8(filepathTransformed, bMayOverwrite))
return false;
}
+
+ // Synchronize timestamps
+ if (bFileTimeSaved && lstrcmpi(filepathTransformed.c_str(), filepath.c_str()) != 0)
+ {
+ SetFileTime(filepathTransformed, &ftCreated, &ftModified);
+ }
+
return true;
}

GetFileTime and SetFileTime are obvious wrappers for API32 functions
with the same names. They are in DiffFileData_test.cpp attached.

ps. Help->Configuration... now gives me "Cannot open file path\to\WinMerge.txt" error.
I'm sure this was working before. I can't attach WinMerge.txt because of that.
Also, that error message seems to be in a fake MessageBox.
If you emulate MessageBox, you should also emulate Ctrl+C behavior (allowing
the user to easily copy the text in the MessageBox).

[Liisachan]

20080101.utf8/16 20090101.utf8/16 DiffFileData.diff

[Kimmo Varis]

> Help->Configuration... now gives me "Cannot open file
> path\to\WinMerge.txt" error.

Hmm. Which Windows version? I changed the file to be created to My folders -folder instead of temp folder. I've tested it in Win XP and Vista.

> Also, that error message seems to be in a fake MessageBox.
No, it is normal messagebox (by MFC).

[Kimmo Varis]

> Hmm. Which Windows version?
Oops, ignore this, it is mentioned in first line of your description. But I wonder why writing to my folders fails...

[Kimmo Varis]

Ok, the problem with config log is it fails to write the file if the My Documents/WinMerge folder does not exist. I've committed a fix that creates the folder first.

You can create the folder yourself and log file gets created there.

And yes, I happen to mix My Documents -folder to My Folders constantly since I use that folder so rarely..

[Liisachan]

Ok, I get now WinMerge.txt by following your instructions,
which I'll attach, though the settings have nothing to do with
file time stamps.

>> Also, that error message seems to be in a fake MessageBox.
> No, it is normal messagebox (by MFC).
Then maybe MFC's messagebox doesn't support Ctrl+C.
Didn't know that. Either that or WinMerge is hooking keyboard input
and eating Ctrl+C, but I don't think that's the case.

[Liisachan]

WinMerge.txt

[Kimmo Varis]

Yep, I think the original bug is pretty clear. And I remember there was earlier bug where this was already discussed. But like many other bugs I've just lost track of them... Now that I have more time in coming months for development I hope I get more bugs resolved.

I'll look at your proposed changes tomorrow.

[Liisachan]

@kimmov
Actually It's me who posted that old bug report in 2006 (originally in 2005):
[1198614] Incorrect path to UTF8/16 file shown in generated patch
http://sourceforge.net/tracker/index.php?func=detail&aid=1198614&group_id=13216&atid=113216

The above [ 1198614 ] log may be helpful to see the whole picture,
the true nature of the problem: most of its symptoms were fixed, one by one,
but the treatment caused another problem. In old days, we had WRONG paths,
paths to temp files there, and CORRECT time stamps of those wrong paths.

I posted this again because it was apparently forgotten,
and I've been constantly annoyed by these wrong time stamps.
The problem is especially confusing when it is not clear from the names of the files
compared which one of the 2 is older and which is the updated one.
Especially if you accidentally flipped (swapped) +++ and --- in the wrong way,
which actually happens more often than it should,
because Explorer's behavior (which file comes first in the command line if
you select 2 files?) is not intuitive.
If the time stamps were correctly shown, probably you could tell "Uh oh +++ is older.
This is the wrong way around." by just one look. Currently you can't tell that
without actually checking the diff closely, and even if you do so, which is the updated one?
is not always obvious.
It's inconvenient anyway that the diff doesn't tell you
"You compared the file updated on yyyy-mm-dd
and another file last update on yyyy-mm-dd".
Perhaps programmers usually don't use UTF-16, but UTF-16 is used often,
sometimes even exclusively, in some other activities: for example in .reg file.

[Kimmo Varis]

Thanks for the link for the other item. It is OK to post again, and one can even send me e-mail if it looks like I've forgotten something. Since I constantly forgot items - partly because sf.net tracker is such pain to use.

I've been busy with other items past days, but I'll assign this to myself so I remember to look at it tomorrow.

[Liisachan]

OK glad to hear that :)
After reading some more, I found that you already have m_mtime
in CHexMergeView::LoadFile,
m_mtime = GetLastWriteTime(h);
So maybe you just can call SetLastWriteTime whenever a temp
file is created.

[Kimmo Varis]

I haven't forgotten this again. :) But I've been finding a good place to add this timestamp fixing code. Currently I'm adding it to CDiffWrapper::WritePatchFile() function. So it will be only run when we are creating patches. As I said before I really don't want this addition to take time in normal compares where it really should not matter. I hope to have a tested patch in few hours...

[Kimmo Varis]

Patch fixing timestamps

[Kimmo Varis]

Attached a patch (wrong_patch_timestamps.patch) fixing this bug. It was actually pretty simple when I found the correct place. Just needed to update timestamps to files stat infos read earlier.

I'll apply this to 2.12 branch too.

[Liisachan]

Thanks so much! This should fix the problem in practical sense :D
Strictly speaking, the real correct place is right after reading the original file,
because the results of _tstat64 are volatile, could change any moment by 100ns.
1) Open the original file exclusively (lock) so no one can modify it.
2) Read it + Read its time stamp. <-- Here
3) Close (unlock) the original file.
4) Translate UTF-16 to UTF-8

Either that or never unlock the original file until the patch is generated.

If the file is huge, converting encoding and writing the temp file could take a few seconds,
and something like this may happen.
Yesterday: huge.utf16 last modified
Today 00:00: user asks WinMerge to generate a patch for huge.utf16
00:01 WinMerge loads huge.utf16 to memory.
00:03 user suddenly updates huge.utf16 behind WinMerge's back
00:05 WinMerge finally created temp.utf8
WinMerge's temp file is a translated copy of yesterday's version of huge.utf16,
but if it calls _stat for the original file now, the result is today 00:03,
so the "corrected" timestamp is actually wrong.
But this race condition is rather theoretical.
A sane user would never update their file like that.

[Kimmo Varis]

Thanks for pointing that possibility. I think this is actually totally another class of problem - what if the file gets changed after user has opened it in compare - or even selected in the create patch -dialog. The patch gets created from files in the disk, not "old" files that were visible in the file compare. Maybe user is comparing live log files and they got updated while user was looking at them in file compare.

In that case I think all we can do (with current implementation) is to check filestamps and if they have changes show an informative message to user...

[Kimmo Varis]

I committed my patch to SVN trunk:
Completed: At revision: 6782

Will commit to 2.12 branch in few days...

[Liisachan]

2.13.8 Unicode tested.
Looks like it's fixed--finally :D

[Kimmo Varis]

Thanks for testing!

Committed the patch to 2.12 branch:
Completed: At revision: 6808

So the bug will be fixed in 2.12.4 release (happening within two weeks).

And closing now as fixed.