⚙ D26259 Fixes svn plugin issuing "mkfifo" on Windows.

nikolaik requested review of this revision.Dec 28 2019, 4:30 PM

nikolaik created this revision.

Harbormaster completed remote builds in B20349: Diff 72302.Dec 28 2019, 4:30 PM

nikolaik added a reviewer: Dolphin.Dec 28 2019, 4:31 PM

nikolaik added reviewers: elvisangelaccio, meven, ngraham.Jan 7 2020, 1:38 PM

meven requested changes to this revision.Jan 7 2020, 2:57 PM

meven added inline comments.

svn/CMakeLists.txt
24 ↗	(On Diff #72302)	Remove
svn/fileviewsvnplugin.cpp
339 ↗	(On Diff #72302)	No need for exceptions here, if you throw it yourself, you can can call what you put in your catch catch directly here, and then return

This revision now requires changes to proceed.Jan 7 2020, 2:57 PM

Updated in response to review comments: no exceptions.

Can we make sure to clean the temp file ?

In D26259#590136, @meven wrote:

Can we make sure to clean the temp file ?

Just read doc of QTemporaryFile, this is already taken care of.

svn/fileviewsvnplugin.cpp
312	You don't need a pointer I think `QTemporaryFile file(tmpFileNameTemplate, process);`

nikolaik added inline comments.Jan 8 2020, 10:34 AM

svn/fileviewsvnplugin.cpp
312	The QTemporaryFile object will be destroyed right after function ends not when QProcess got destroyed. At that time comparator process could not even start to loading it (I see it on my machine). I think we should destroy temporary file when comparator process no longer needs it. So as long as comparator process is running the file must be there. The best way to achive this is through the `QTemporaryFile *file = new QTemporaryFile(tmpFileNameTemplate, process);`

Seems fine to me, please wait for others to have some time to review.
I will accept it eventually.

svn/fileviewsvnplugin.cpp
312	Thanks

elvisangelaccio requested changes to this revision.Jan 12 2020, 6:32 PM

elvisangelaccio added inline comments.

svn/fileviewsvnplugin.cpp
311	Please use `QStringLiteral("%1/%2.XXXXXX").arg(...)` instead. No need to use QDir::separator().
315	No space before/after ()
315	I'd just show a generic "Could not show local SVN changes.". Maybe we could print a qWarning() saying that the temporary file could not be created (which is unlikely though...).
330	"Could not show local SVN changes: svn diff failed".
343	"Could not show local SVN changes: could not start kompare". This would also help in the case kompare is not installed.

This revision now requires changes to proceed.Jan 12 2020, 6:32 PM

Updated in response to review comments.

nikolaik marked 8 inline comments as done.Jan 17 2020, 10:13 AM

nikolaik added inline comments.

svn/fileviewsvnplugin.cpp
315	Don't know if we should add qWarning() only here. Maybe we should add it on every error in this function or nowhere. Anyway i am ready to make all the required corrections.

I noticed the following warning if I close dolphin while kompare is still running:

QProcess: Destroyed while process ("kompare") is still running.

We should find a way to close kompare gracefully in this case, or even keep it running if possible (which is the current behavior).

svn/fileviewsvnplugin.cpp
315	It's okay, it's never gonna happen anyway...

This revision now requires changes to proceed.Jan 19 2020, 9:51 PM

Use startDetached instead.

Thanks for comments.
Updated: now we are using startDetached() because one may close Dolphin but need kompare running.
Provided solution has a flaw: temporary files will be deleted on Dolphin close. So we make an assumption: when the file got deleted diff program should already load it and no longer needs it.
This flaw seems insignificant.
Also patch keeps current behavior of kompare running after Dolphin close intact.

nikolaik marked 3 inline comments as done.Jan 23 2020, 1:23 PM

elvisangelaccio added inline comments.Jan 25 2020, 11:26 AM

svn/fileviewsvnplugin.cpp
311 ↗	(On Diff #72302)	Please write "when the file gets deleted kompare has already loaded it and no longer needs it."
336 ↗	(On Diff #72302)	Please call it `started` and make it `const`.

Updated in response to review comments.

nikolaik marked 2 inline comments as done.Jan 25 2020, 12:02 PM

LGTM now.

@meven What about you?

meven accepted this revision.Jan 26 2020, 8:14 PM

This revision is now accepted and ready to land.Jan 26 2020, 8:14 PM

Patch doesn't seem to apply, can you rebase it on master please?

svn/fileviewsvnplugin.cpp
345 ↗	(On Diff #72302)	This `return` is redundant since there is nothing after.

Removed redundant return statement, feature branch rebased on master.

nikolaik marked an inline comment as done.Jan 27 2020, 7:27 AM

Still does not apply for me.

One more try: applied changes directly to master and upload diff.

nikolaik edited the summary of this revision. (Show Details)Feb 3 2020, 7:33 AM

ping @elvisangelaccio

The patch applies fine on master.
I think you can merge.

meven accepted this revision.Feb 14 2020, 6:12 PM

I wish i could but i don't have developer account. Need invitation or something i guess.

I can merge this for you, I just need your email, I already know your name thanks to phabricator but it does not let me see your actual commit for some reason and your email that's in there.

nkrasheninnikov@yandex.ru
Thanks

Closed by commit R449:0df2e299f415: Fixes svn plugin issuing "mkfifo" on Windows. (authored by nikolaik, committed by meven). · Explain WhyFeb 18 2020, 8:27 AM

This revision was automatically updated to reflect the committed changes.

			Path	Packages
M			svn/CMakeLists.txt (2 lines)
M			svn/fileviewsvnplugin.cpp (43 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	72302	163dc0c		Dec 28 2019, 4:30 PM	★	★
Diff 2	73014			Jan 7 2020, 7:48 PM	★	★
Diff 3	73370			Jan 13 2020, 7:34 AM	★	★
Diff 4	74226			Jan 23 2020, 1:12 PM	★	★
Diff 5	74356			Jan 25 2020, 12:00 PM	★	★
Diff 6	74407			Jan 27 2020, 7:25 AM	★	★
Diff 7	74889			Feb 3 2020, 7:31 AM	★	★
Diff 8	75884	40adb35	R449:0df2e299f415411eef36327c8d8d4933766a94d5	Feb 18 2020, 8:27 AM	★	★

Fixes svn plugin issuing "mkfifo" on Windows.
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 73014

svn/CMakeLists.txt

svn/fileviewsvnplugin.cpp

Fixes svn plugin issuing "mkfifo" on Windows.ClosedPublicActions