This report is to do a few clean-ups in svn-apply: (1) Eliminate use of %copiedFiles (it's not necessary) (2) Address the FIXME to use exitStatus() in gitKnowsOfFile()
Created attachment 55638 [details] Proposed patch
Comment on attachment 55638 [details] Proposed patch Do we know this is covered properly with tests? I woudl have written this: + my $exitStatus = exitStatus($?); + return $exitStatus == 0; as just: return exitStatus($?) == 0; Then again, That should really use system instead of `` since it doesn't need/want the output.
(In reply to comment #2) Thanks, Eric! > (From update of attachment 55638 [details]) > Do we know this is covered properly with tests? Unfortunately, this area of svn-apply is not covered by unit tests. I did do some manual tests though. Now that you mention it, I'll also run "test-webkitpy --all" before landing to make sure. > I woudl have written this: > + my $exitStatus = exitStatus($?); > + return $exitStatus == 0; > > as just: > return exitStatus($?) == 0; Yes, thanks. I suppose I left it as it appears now because it makes it easier to later add a "print($exitStatus)" line for debug purposes.
Eeek! No tests!? Why should I r+ this then? :(((
(In reply to comment #4) > Eeek! No tests!? Why should I r+ this then? :((( That question seems rhetorical so I won't answer it. (Let me know if it's not.) In terms of tests, this change doesn't add to or change any functionality, so I didn't add any tests. Is it a requirement for patches that refactor non-unit-tested code to create tests? I was just trying to simplify the existing code without doing too much extra work. It looks like it could be a bigger project to add a decent test harness for testing higher-level Perl scm functionality (something I've made a lot of headway on, incidentally, via the parse-related subroutines in VCSUtils.pm and bug 34033 which was a keystone).
In general it's *strongly encouraged* to test code when touching it. In times when I've thought it would be a lot of work to unit test something in the python code I've found that it was less work than I thought, and totally worth the effort.
That said, my r+ still stands... I'm just trying to lay on the guilt real thick...
Comment on attachment 55638 [details] Proposed patch Clearing flags on attachment: 55638 Committed r59209: <http://trac.webkit.org/changeset/59209>
All reviewed patches have been landed. Closing bug.