38862 2010-05-10 12:53:07 -0700 svn-apply: Eliminate the "copiedFiles" hash 2010-05-11 22:10:57 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 cjerdonek cjerdonek cjerdonek dbates eric oldest_to_newest 223467 0 cjerdonek 2010-05-10 12:53:07 -0700 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() 223658 1 55638 cjerdonek 2010-05-10 18:55:55 -0700 Created attachment 55638 Proposed patch 223671 2 55638 eric 2010-05-10 19:06:29 -0700 Comment on attachment 55638 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. 223692 3 cjerdonek 2010-05-10 19:38:51 -0700 (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. 223694 4 eric 2010-05-10 19:45:18 -0700 Eeek! No tests!? Why should I r+ this then? :((( 224276 5 cjerdonek 2010-05-11 17:18:19 -0700 (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). 224360 6 eric 2010-05-11 20:24:49 -0700 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. 224361 7 eric 2010-05-11 20:25:12 -0700 That said, my r+ still stands... I'm just trying to lay on the guilt real thick... 224403 8 55638 cjerdonek 2010-05-11 22:10:50 -0700 Comment on attachment 55638 Proposed patch Clearing flags on attachment: 55638 Committed r59209: <http://trac.webkit.org/changeset/59209> 224404 9 cjerdonek 2010-05-11 22:10:57 -0700 All reviewed patches have been landed. Closing bug. 55638 2010-05-10 18:55:55 -0700 2010-05-11 22:10:50 -0700 Proposed patch _patch-38862-1.diff text/plain 2286 cjerdonek ZGlmZiAtLWdpdCBhL1dlYktpdFRvb2xzL0NoYW5nZUxvZyBiL1dlYktpdFRvb2xzL0NoYW5nZUxv ZwppbmRleCA2ZTBmMmI0Li5kYzBiNTM2IDEwMDY0NAotLS0gYS9XZWJLaXRUb29scy9DaGFuZ2VM b2cKKysrIGIvV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMDUt MTAgIENocmlzIEplcmRvbmVrICA8Y2plcmRvbmVrQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRml4ZWQgdHdvIEZJWE1FJ3MgaW4g c3ZuLWFwcGx5OiBlbGltaW5hdGVkIHRoZSB1bm5lY2Vzc2FyeSAlY29waWVkRmlsZXMKKyAgICAg ICAgdmFyaWFibGUgYW5kIGNoYW5nZWQgZ2l0S25vd3NPZkZpbGUoKSB0byB1c2UgZXhpdFN0YXR1 cygpLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0z ODg2MgorCisgICAgICAgICogU2NyaXB0cy9zdm4tYXBwbHk6CisKIDIwMTAtMDUtMTAgIERpcmsg UHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRXJp YyBTZWlkZWwuCmRpZmYgLS1naXQgYS9XZWJLaXRUb29scy9TY3JpcHRzL3N2bi1hcHBseSBiL1dl YktpdFRvb2xzL1NjcmlwdHMvc3ZuLWFwcGx5CmluZGV4IGY0ZTFkMzguLjhlMWE2NWQgMTAwNzU1 Ci0tLSBhL1dlYktpdFRvb2xzL1NjcmlwdHMvc3ZuLWFwcGx5CisrKyBiL1dlYktpdFRvb2xzL1Nj cmlwdHMvc3ZuLWFwcGx5CkBAIC0xMjIsNyArMTIyLDYgQEAgbXkgJGdsb2JhbEV4aXRTdGF0dXMg PSAwOwogbXkgJHJlcG9zaXRvcnlSb290UGF0aCA9IGRldGVybWluZVZDU1Jvb3QoKTsKIAogbXkg JWNoZWNrZWREaXJlY3RvcmllczsKLW15ICVjb3BpZWRGaWxlczsKIAogIyBOZWVkIHRvIHVzZSBh IHR5cGVnbG9iIHRvIHBhc3MgdGhlIGZpbGUgaGFuZGxlIGFzIGEgcGFyYW1ldGVyLAogIyBvdGhl cndpc2UgZ2V0IGEgYmFyZXdvcmQgZXJyb3IuCkBAIC0xNTQsOCArMTUzLDYgQEAgZm9yIG15ICRj b3B5RGlmZkhhc2hSZWYgKEBjb3B5RGlmZkhhc2hSZWZzKSB7CiAKICAgICBhZGREaXJlY3Rvcmll c0lmTmVlZGVkKGRpcm5hbWUoJGluZGV4UGF0aCkpOwogICAgIHNjbUNvcHkoJGNvcGllZEZyb21Q YXRoLCAkaW5kZXhQYXRoKTsKLQotICAgICRjb3BpZWRGaWxlc3skaW5kZXhQYXRofSA9ICRjb3Bp ZWRGcm9tUGF0aDsKIH0KIAogZm9yIG15ICRkaWZmSGFzaFJlZiAoQG5vbkNvcHlEaWZmSGFzaFJl ZnMpIHsKQEAgLTMxNywxMSArMzE0LDcgQEAgc3ViIHBhdGNoKCQpCiAgICAgbXkgJGRlbGV0aW9u ID0gMDsKICAgICBteSAkYWRkaXRpb24gPSAwOwogCi0gICAgIyBGSVhNRTogRG8gd2UgbmVlZCB0 aGUgZXhpc3RzKCRjb3BpZWRGaWxlcy4uLikgY2hlY2sgaGVyZT8gIEl0IGxvb2tzIGxpa2UKLSAg ICAjICAgICAgICBpdCBjYW4gYmUgcmVtb3ZlZCBzaW5jZSBtb2RpZmljYXRpb25zIHRvIGNvcGll cyBkbyBub3QgcmVnaXN0ZXIKLSAgICAjICAgICAgICBhcyBuZXcgZmlsZXMuICBJZiB0aGlzIGlz IHRoZSBjYXNlLCB3ZSBzaG91bGQgYmUgYWJsZSB0byByZW1vdmUKLSAgICAjICAgICAgICB0aGUg dmFyaWFibGUgZnJvbSB0aGlzIGZpbGUgZW50aXJlbHkuCi0gICAgJGFkZGl0aW9uID0gMSBpZiAo JGRpZmZIYXNoUmVmLT57aXNOZXd9IHx8ICRwYXRjaCA9fiAvXG5AQCAtMCwwIC4qIEBALykgJiYg IWV4aXN0cygkY29waWVkRmlsZXN7JGZ1bGxQYXRofSk7CisgICAgJGFkZGl0aW9uID0gMSBpZiAo JGRpZmZIYXNoUmVmLT57aXNOZXd9IHx8ICRwYXRjaCA9fiAvXG5AQCAtMCwwIC4qIEBALyk7CiAg ICAgJGRlbGV0aW9uID0gMSBpZiAoJGRpZmZIYXNoUmVmLT57aXNEZWxldGlvbn0gfHwgJHBhdGNo ID1+IC9cbkBAIC4qIFwrMCwwIEBALyk7CiAKICAgICBpZiAoISRhZGRpdGlvbiAmJiAhJGRlbGV0 aW9uICYmICEkaXNCaW5hcnkpIHsKQEAgLTQxNyw5ICs0MTAsOCBAQCBzdWIgZ2l0S25vd3NPZkZp bGUoJCkKICAgICBteSAkcGF0aCA9IHNoaWZ0OwogCiAgICAgYGdpdCBscy1maWxlcyAtLWVycm9y LXVubWF0Y2ggLS0gJHBhdGhgOwotICAgICMgRklYTUU6IENoYW5nZSB0aGlzIHRvIHVzZSBWQ1NV dGlscy5wbSdzIGV4aXRTdGF0dXMoKS4KLSAgICBteSAkZXhpdENvZGUgPSAkPyA+PiA4OwotICAg IHJldHVybiAkZXhpdENvZGUgPT0gMDsKKyAgICBteSAkZXhpdFN0YXR1cyA9IGV4aXRTdGF0dXMo JD8pOworICAgIHJldHVybiAkZXhpdFN0YXR1cyA9PSAwOwogfQogCiBzdWIgc2NtS25vd3NPZkZp bGUoJCkK