-
Notifications
You must be signed in to change notification settings - Fork 133
Improve error detection & handling when invoking SVN #247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Move the call to plugin_push_current() to be before the first call to svn_binary() to ensure that svn_binary()'s calls to plugin_config_get() retrieve the options from SourceSVN's config options rather than any plugin inheriting from SourceSVN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bright-tools many thanks for this. Here's a couple minor review comments to start with; I'll try to run a few tests here later on.
SourceSVN/SourceSVN.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the param names to the PHPDoc block, i.e.
@param string $p_cmd Command and any parameters
SourceSVN/lang/strings_english.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error messages are very similar - is it really worth having 2 different strings ?
@dregad , thanks for the review!
I hopefully fixed the PHPDoc block
With regard to the errors, I've tried to refine them (the redundant parameter on one of the probably did not help). Intent is that the first error is triggered in the case that proc_open() fails (i.e. not possible to execute SVN), while the other error is triggered if the SVN process returns to stderr. It seemed like a distinction worth making, but let me know what you think.
SourceSVN/SourceSVN.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed the $ in front of the param name 😉
WRT the error messages, I think its good following your latest commit.
Argh! You can tell how much I've used PHPDoc mark-up, can't you? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested OK (no real-life conditions though - just using a local SVN repo created for this purpose)
@bright-tools Thanks for your contribution !
As a side note and if you don't mind, I would prefer when you submit pull requests, that you rebase your branch on top of upstream master, instead of merging it (that's in the interest of keeping a clean git history). Thanks !
Noted, will do!
In addition to pr mantisbt-plugins#247, e02794f
As discussed on #241, try and be a bit more robust when executing the SVN command.
@dregad, I tried to keep my implementation close to the work that you did in the hope it would ease the review process. Please take a look & let me know if you want any changes prior to merge. I've done some basic testing.