Last modified: 2014-02-28 13:52:27 UTC
CodeRepository::getDiff() returns the diff as a string, or a numeric constant if it is unable to get the diff for any reason. The constant describes the reason the diff failed. Currently the function can't tell the difference between an SVN/connection failure and an empty diff, and so returns self::DIFFRESULT_NoDataReturned in both cases. The code should be updated so that it can tell the difference and return (e.g.) DIFFRESULT_ConnectionError if there was a connection failure, and DIFFRESULT_NoDataReturned if the diff is genuinely empty. Brion had some suggestions about how this might be implemented, in http://www.mediawiki.org/wiki/Special:Code/MediaWiki/80822#c13364. Here is the relevant excerpt: Re: the "can't tell difference between connection failure and empty diff", I think we can remedy that in the SubversionAdaptor classes: * SubversionAdaptorPecl looks like it should throw an exception on failure, so already good? * SubversionAdaptorShell should check the return code; a failure should give a non-zero return which we can parley into a nice exception ;) * SubversionProxy should throw an exception on a bad return, but that could depend on whether the proxy actually returns an HTTP error code on error
Actually, thinking about it, I'm not sure what a blank diff actually means. Can it represent an error condition, or does it always mean just that there is no diff text to show - e.g. that there were only moves/renames/deletions and no content changes. If it can mean either, is there any way we can differentiate between error conditions and valid blank diffs? Either way, we should probably be returning an empty string rather than a constant for genuinely blank diffs, as this is not an error condition!
And following onto that, when we should really be caching the errors if they are actually empty diffs etc
We should modify the way we store the diff in the DB, so that an empty string is stored for genuine empty diffs, but that NULL is stored for error conditions. NULL values should be retried when running svnImport, whilst blank values should be skipped as they are valid. We could maybe even go one step further and store the error code in the DB (if we can guarantee that this won't conflict with a valid diff string, which I think we can). If we do this then it will give better reporting in the interface, and we can decide whether to retry in a more sensible manner (e.g. DIFFRESULT_TooManyPaths probably isn't worth a retry, unless $wgCodeReviewMaxDiffPaths has changed).
Assigning this to Sam. Sam, please feel free to downgrade the priority and unassign yourself if you think you shouldn't have this one.
Changing default assignee per IRC
