<div dir="ltr">Hi guys,<div><br></div><div>Pull request sent (<a href="https://github.com/Subsurface-divelog/subsurface/pull/222">https://github.com/Subsurface-divelog/subsurface/pull/222</a>)</div><div>Sorry for the late reply, these days were quite busy with a bit of coding but mainly with my Instructor Training Course :)<div><br></div><div>Based on your recommendations I've added a subsurface_stat portability function. There I used the corresponding widechar implementation wstat() for windows (after utf8 to utf16 conversion) and simply called the normal stat() otherwise. It seems to work nicely.</div><div><br></div><div>From what I understand when reading msdn, stat() is actually expecting a path encoded with the codepoint that is defined in system settings so it can be many things but rarely utf8... Win32 only apps do not face issues since they would read this from user input or other win32 api that are already returning paths/stings encoded with that codepoint. In our case everything is converted to utf8 before being returned to the app level. At least this is my current understanding of the issue we faced.</div><div><br></div><div>Since I like to work with tests, I made some little changes to the testing code in order to be able to run cross compiled tests under windows too. And added a case that test specificaly our issue.</div><div>The changes I've made there are not yet providing a fully flavored solution (but it is a step forward, that allowed me to test without too much troubles).</div><div><br></div><div>What could come next (but I prefer to do this in a separate change) would be:</div><div>- Packaging of the tests together with dependencies and test data for windows run</div><div> (currently I went the hard way of manually copying what I needed)</div><div>- Making test execution not depending on user name. (All cloud part of TestGitStorage implementation uses default path based on user dir to execute tests.)</div><div>- Make test data user independent. (When I run the tests I get many: "didn't find picture entry for /home/hohndel/subsurface/dives<wbr>/images/PA102039.jpg" caused by hard coded path in the test cases)</div><div>- I still could not run TestPreferences.exe I get a strange missing dependency message (certainly explained by my manual copy of the dependencies ;))</div><div>- Some issue with recently added TestMerge and TestParse caused by a trailing \r in the value from written file</div><div><div> Actual (readin.takeFirst()) : "<divelog program='subsurface' version='3'>"</div><div> Expected (written.takeFirst()): "<divelog program='subsurface' version='3'>\r"</div></div><div><br></div><div>For now I got TestGitStorage to:</div><div>1. Reproduce the issue when the fix was not there. see log below, I cut off the missing image messages</div><div>2. Pass once the fix is included. see log below too</div><div><br></div><div>Have a look at the pull request, I will be happy to make any improvements or fixes to the change. (I will not be unhappy if you pull it in as is)</div><div><br></div><div>Cheers,</div><div><br></div><div>Jeremie</div><div><br></div><div>Before:</div><div><div>********* Start testing of TestGitStorage *********</div><div>Config: Using QtTest library 5.7.0, Qt 5.7.0 (i386-little_endian-ilp32 shared (dynamic) release build; by GCC 4.9.4)</div><div>PASS : TestGitStorage::initTestCase()</div><div>PASS : TestGitStorage::testSetup()</div><div>PASS : TestGitStorage::<wbr>testGitStorageLocal(ASCII path)</div><div>FAIL! : TestGitStorage::<wbr>testGitStorageLocal(Non ASCII path) Compared values are not the same</div><div> Actual (save_dives(qUtf8Printable(<wbr>testDirName + "[test]"))): -1</div><div> Expected (0) : 0</div><div>/home/jeremie/dev/subsurface-<wbr>win/subsurface/tests/<wbr>testgitstorage.cpp(81) : failure location</div><div>FAIL! : TestGitStorage::<wbr>testGitStorageCloud() Compared values are not the same</div><div> Actual (parse_file(qUtf8Printable(<wbr>cloudTestRepo))): -1</div><div> Expected (0) : 0</div><div>/home/jeremie/dev/subsurface-<wbr>win/subsurface/tests/<wbr>testgitstorage.cpp(107) : failure location</div><div>FAIL! : TestGitStorage::<wbr>testGitStorageCloudOfflineSync<wbr>() Compared values are not the same</div><div> Actual (parse_file(qUtf8Printable(<wbr>localCacheRepo))): -1</div><div> Expected (0) : 0</div><div>/home/jeremie/dev/subsurface-<wbr>win/subsurface/tests/<wbr>testgitstorage.cpp(129) : failure location</div><div>FAIL! : TestGitStorage::<wbr>testGitStorageCloudMerge() Compared values are not the same</div><div> Actual (parse_file(qUtf8Printable(<wbr>localCacheRepoSave))): -1</div><div> Expected (0) : 0</div><div>/home/jeremie/dev/subsurface-<wbr>win/subsurface/tests/<wbr>testgitstorage.cpp(179) : failure location</div><div>FAIL! : TestGitStorage::<wbr>testGitStorageCloudMerge2() Compared values are not the same</div><div> Actual (parse_file(qUtf8Printable(<wbr>localCacheRepo))): -1</div><div> Expected (0) : 0</div><div>/home/jeremie/dev/subsurface-<wbr>win/subsurface/tests/<wbr>testgitstorage.cpp(227) : failure location</div><div>FAIL! : TestGitStorage::<wbr>testGitStorageCloudMerge3() Compared values are not the same</div><div> Actual (parse_file(qUtf8Printable(<wbr>cloudTestRepo))): -1</div><div> Expected (0) : 0</div><div>/home/jeremie/dev/subsurface-<wbr>win/subsurface/tests/<wbr>testgitstorage.cpp(284) : failure location</div><div>PASS : TestGitStorage::<wbr>cleanupTestCase()</div><div>Totals: 4 passed, 6 failed, 0 skipped, 0 blacklisted, 20258ms</div><div>********* Finished testing of TestGitStorage *********</div></div><div><br></div><div>After:</div><div><div>********* Start testing of TestGitStorage *********</div><div>Config: Using QtTest library 5.7.0, Qt 5.7.0 (i386-little_endian-ilp32 shared (dynamic) release build; by GCC 4.9.4)</div><div>PASS : TestGitStorage::initTestCase()</div><div>PASS : TestGitStorage::testSetup()</div><div>PASS : TestGitStorage::<wbr>testGitStorageLocal(ASCII path)</div><div>PASS : TestGitStorage::<wbr>testGitStorageLocal(Non ASCII path)</div><div>PASS : TestGitStorage::<wbr>testGitStorageCloud()</div><div>PASS : TestGitStorage::<wbr>testGitStorageCloudOfflineSync<wbr>()</div><div>PASS : TestGitStorage::<wbr>testGitStorageCloudMerge()</div><div>PASS : TestGitStorage::<wbr>testGitStorageCloudMerge2()</div><div>PASS : TestGitStorage::<wbr>testGitStorageCloudMerge3()</div><div>PASS : TestGitStorage::<wbr>cleanupTestCase()</div><div>Totals: 10 passed, 0 failed, 0 skipped, 0 blacklisted, 125181ms</div><div>********* Finished testing of TestGitStorage *********</div></div><div><br></div><div> </div><div class="gmail_extra"><br><div class="gmail_quote">2017-02-21 0:47 GMT+07:00 Linus Torvalds <span dir="ltr"><<a href="mailto:torvalds@linux-foundation.org" target="_blank">torvalds@linux-foundation.org</a><wbr>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-m_4648693132985890796gmail-m_-8352999883872734387gmail-m_8808332365222267090gmail-">On Mon, Feb 20, 2017 at 9:41 AM, Dirk Hohndel <<a href="mailto:dirk@hohndel.org" target="_blank">dirk@hohndel.org</a>> wrote:<br>
><br>
> How about writing a wrapper function that can be called from C in<br>
> qthelper.cpp (we have quite a few of them there already) and have that do<br>
> something like<br>
><br>
> extern "C" bool dir_exists(const char *dir)<br>
> {<br>
> return QDir(dir).exists();<br>
> }<br>
<br>
</span>So we do actually want more than to check if it's a directory. We use<br>
"stat()" for two things in the git wrapper:<br>
<br>
- to check the existing cache:<br>
<br>
if it's a directorty, we uupdate it<br>
<br>
- if it's a non-directory,. we do warnings/cleanup<br>
<br>
- if it doesn't exist, we may create it.<br>
<br>
So that code actually wants at least a ternary return value, or just a<br>
regular stat() like it does now.<br>
<br>
Then in is_git_repository() we do indeed just want to know "does this<br>
directory exist".<br>
<br>
We *could* just use our existing "subsurface_open() + fstat()", which<br>
is already a pattern we use, but doing "open()" on a directory ends up<br>
being one of those things I could easily see not working under Windows<br>
either.<br>
<span class="gmail-m_4648693132985890796gmail-m_-8352999883872734387gmail-m_8808332365222267090gmail-HOEnZb"><font color="#888888"><br>
Linus<br>
</font></span></blockquote></div><br></div></div></div>