Bug 38862 – svn-apply: Eliminate the "copiedFiles" hash

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

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()

[Chris Jerdonek]

Created attachment 55638 [details]
Proposed patch

[Eric Seidel (no email)]

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.

[Chris Jerdonek]

(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.

[Eric Seidel (no email)]

Eeek!  No tests!?  Why should I r+ this then? :(((

[Chris Jerdonek]

(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).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

That said, my r+ still stands... I'm just trying to lay on the guilt real thick...

[Chris Jerdonek]

Comment on attachment 55638 [details]
Proposed patch

Clearing flags on attachment: 55638

Committed r59209: <http://trac.webkit.org/changeset/59209>

[Chris Jerdonek]

All reviewed patches have been landed.  Closing bug.