171450 2017-04-28 14:28:10 -0700 Add a new function for getting the Git hash for a pure git directory. 2017-05-01 16:15:38 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jmarcell webkit-unassigned bshafiei buildbot dbates ddkilzer jmarcell kocsen_chung lforschler matthew_hanson oldest_to_newest 1302788 0 jmarcell 2017-04-28 14:28:10 -0700 Add a new function for getting the Git hash for a pure git directory. 1302791 1 308595 jmarcell 2017-04-28 14:29:40 -0700 Created attachment 308595 Patch 1302798 2 308595 matthew_hanson 2017-04-28 14:38:39 -0700 Comment on attachment 308595 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review Perhaps we should warn the user if the directory is not a Git directory, since that is likely indicative of a mistake on the part of the caller. This is a great addition; The semicolon needs to be addressed and I'd like to see a warning when this function is called on non-Git repos. > Tools/Scripts/VCSUtils.pm:451 > + my $revision; I know that the variable name $revision is consistent with the other functions of this ilk, but you might consider calling it $commit or $hash to match the name of your function. > Tools/Scripts/VCSUtils.pm:454 > + warn "$directory is a pure git repo."; Is this debugging code? > Tools/Scripts/VCSUtils.pm:458 > + chomp($revision) Missing semicolon. 1302806 3 308595 kocsen_chung 2017-04-28 14:47:27 -0700 Comment on attachment 308595 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >> Tools/Scripts/VCSUtils.pm:454 >> + warn "$directory is a pure git repo."; > > Is this debugging code? I assume we also don't mind that git-svn repos will execute this code path. > Tools/Scripts/VCSUtils.pm:456 > + $command = "LC_ALL=C $command" if !isWindows(); Curious what is this for? 1302816 4 308595 jmarcell 2017-04-28 15:19:21 -0700 Comment on attachment 308595 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >> Tools/Scripts/VCSUtils.pm:451 >> + my $revision; > > I know that the variable name $revision is consistent with the other functions of this ilk, but you might consider calling it $commit or $hash to match the name of your function. Yep. I guess I'll go with $commit unless anyone objects. >>> Tools/Scripts/VCSUtils.pm:454 >>> + warn "$directory is a pure git repo."; >> >> Is this debugging code? > > I assume we also don't mind that git-svn repos will execute this code path. This is debugging code (which I will remove before landing). This function will work fine for a git-svn repo, however, we wouldn't/shouldn't use it because there is an already-existing function called `svnRevisionForDirectory` (which, not coincidentally, this function was heavily based on). >> Tools/Scripts/VCSUtils.pm:456 >> + $command = "LC_ALL=C $command" if !isWindows(); > > Curious what is this for? I'm not sure. It's some copy pasta from `svnRevisionForDirectory`. You're right to question it. We should figure out if we even need it here before landing. >> Tools/Scripts/VCSUtils.pm:458 >> + chomp($revision) > > Missing semicolon. Will fix. 1303260 5 308595 jmarcell 2017-05-01 09:52:07 -0700 Comment on attachment 308595 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >>> Tools/Scripts/VCSUtils.pm:456 >>> + $command = "LC_ALL=C $command" if !isWindows(); >> >> Curious what is this for? > > I'm not sure. It's some copy pasta from `svnRevisionForDirectory`. You're right to question it. We should figure out if we even need it here before landing. I've done some research. Why do this? From here: https://unix.stackexchange.com/a/87763 We're setting the locale to something generic in this case because the returned data is not intended for human consumption and, thus, does not need locale specific interpretation. > Any time where you process input data or output data that is not intended from/for a human. If you're talking to a user, you may want to use their convention and language, but for instance, if you generate some numbers to feed some other application that expects English style decimal points, or English month names, you'll want to set LC_ALL=C Why are we excluding Windows? https://bugs.webkit.org/show_bug.cgi?id=93339 1303266 6 308595 jmarcell 2017-05-01 09:59:45 -0700 Comment on attachment 308595 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308595&action=review >>> Tools/Scripts/VCSUtils.pm:451 >>> + my $revision; >> >> I know that the variable name $revision is consistent with the other functions of this ilk, but you might consider calling it $commit or $hash to match the name of your function. > > Yep. I guess I'll go with $commit unless anyone objects. Actually, I decided to go with 'hash' because of the name I already chose for the function. 1303270 7 308728 jmarcell 2017-05-01 10:05:20 -0700 Created attachment 308728 Patch 1303320 8 308728 ddkilzer 2017-05-01 12:11:08 -0700 Comment on attachment 308728 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308728&action=review r=me > Tools/Scripts/VCSUtils.pm:96 > &svnRevisionForDirectory > + &gitHashForDirectory > &svnStatus Please alphabetize within the list. 308595 2017-04-28 14:29:40 -0700 2017-05-01 10:05:18 -0700 Patch bug-171450-20170428142940.patch text/plain 1748 jmarcell U3VidmVyc2lvbiBSZXZpc2lvbjogMjE1OTQ3CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggYzE4ZWNhNjI5ZjFhYWEwNWFjNjUwNzdlYzIyNDNiZmU3 MmYwNDg4OC4uMGJkZDM3NTJkNTkxZDAzNjA5ZWQ0MmViNmI0YjQ1MDg4MGU0Zjg2ZiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE3LTA0LTI4ICBKYXNvbiBNYXJjZWxsICA8am1hcmNlbGxAYXBwbGUuY29tPgorCisg ICAgICAgIEFkZCBhIG5ldyBmdW5jdGlvbiBmb3IgZ2V0dGluZyB0aGUgR2l0IGhhc2ggZm9yIGEg cHVyZSBnaXQgZGlyZWN0b3J5LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTcxNDUwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgKiBTY3JpcHRzL1ZDU1V0aWxzLnBtOgorICAgICAgICAoZ2l0SGFzaEZvckRp cmVjdG9yeSk6CisKIDIwMTctMDQtMjggIENocmlzIER1bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4K IAogICAgICAgICBVcGRhdGUgRE9NVG9rZW5MaXN0LnJlcGxhY2UoKSB0byBtYXRjaCB0aGUgbGF0 ZXN0IERPTSBzcGVjaWZpY2F0aW9uCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL1ZDU1V0aWxz LnBtIGIvVG9vbHMvU2NyaXB0cy9WQ1NVdGlscy5wbQppbmRleCA0ZDZjZDI5M2YyMjUwYTY4ZTM5 Y2Y0MGM1ZTc2M2ZiM2U1YjIwZWI5Li40OGViMGI2YmFiNTA4MjM4Y2MzMmEzNzgzNTcxNWI5NGEy YTlhYzkyIDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL1ZDU1V0aWxzLnBtCisrKyBiL1Rvb2xz L1NjcmlwdHMvVkNTVXRpbHMucG0KQEAgLTkyLDYgKzkyLDcgQEAgQkVHSU4gewogICAgICAgICAm c3ZuSW5mb0ZvclBhdGgKICAgICAgICAgJnN2blJlcG9zaXRvcnlSb290Rm9yUGF0aAogICAgICAg ICAmc3ZuUmV2aXNpb25Gb3JEaXJlY3RvcnkKKyAgICAgICAgJmdpdEhhc2hGb3JEaXJlY3RvcnkK ICAgICAgICAgJnN2blN0YXR1cwogICAgICAgICAmc3ZuVVJMRm9yUGF0aAogICAgICAgICAmdG9X aW5kb3dzTGluZUVuZGluZ3MKQEAgLTQ0NCw2ICs0NDUsMjUgQEAgc3ViIHN2blJldmlzaW9uRm9y RGlyZWN0b3J5KCQpCiAgICAgcmV0dXJuICRyZXZpc2lvbjsKIH0KIAorc3ViIGdpdEhhc2hGb3JE aXJlY3RvcnkoJCkKK3sKKyAgICBteSAoJGRpcmVjdG9yeSkgPSBAXzsKKyAgICBteSAkcmV2aXNp b247CisKKyAgICBpZiAoaXNHaXREaXJlY3RvcnkoJGRpcmVjdG9yeSkpIHsKKyAgICAgICAgd2Fy biAiJGRpcmVjdG9yeSBpcyBhIHB1cmUgZ2l0IHJlcG8uIjsKKyAgICAgICAgbXkgJGNvbW1hbmQg PSAiZ2l0IC1DIFwiJGRpcmVjdG9yeVwiIHJldi1wYXJzZSBIRUFEIjsKKyAgICAgICAgJGNvbW1h bmQgPSAiTENfQUxMPUMgJGNvbW1hbmQiIGlmICFpc1dpbmRvd3MoKTsKKyAgICAgICAgJHJldmlz aW9uID0gYCRjb21tYW5kYDsKKyAgICAgICAgY2hvbXAoJHJldmlzaW9uKQorICAgIH0KKyAgICBp ZiAoIWRlZmluZWQoJHJldmlzaW9uKSkgeworICAgICAgICAkcmV2aXNpb24gPSAidW5rbm93biI7 CisgICAgICAgIHdhcm4gIlVuYWJsZSB0byBkZXRlcm1pbmUgY3VycmVudCBHaXQgaGFzaCBpbiAk ZGlyZWN0b3J5IjsKKyAgICB9CisgICAgcmV0dXJuICRyZXZpc2lvbjsKK30KKwogc3ViIHN2bklu Zm9Gb3JQYXRoKCQpCiB7CiAgICAgbXkgKCRmaWxlKSA9IEBfOwo= 308728 2017-05-01 10:05:20 -0700 2017-05-01 12:11:08 -0700 Patch bug-171450-20170501100519.patch text/plain 1662 jmarcell U3VidmVyc2lvbiBSZXZpc2lvbjogMjE2MDE1CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggN2I0ZTlhNmNkNDczODk3YTNiMmNjYWVkZDE0Mjg0YTM1 MjkxMmYwNS4uNDcyYTk5ZGNlMzE5MmI5ZjBiMTE2NGM3YWQzNGQ1MTEwNjNkZDA4YyAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDEz IEBACisyMDE3LTA1LTAxICBKYXNvbiBNYXJjZWxsICA8am1hcmNlbGxAYXBwbGUuY29tPgorCisg ICAgICAgIEFkZCBhIG5ldyBmdW5jdGlvbiBmb3IgZ2V0dGluZyB0aGUgR2l0IGhhc2ggZm9yIGEg cHVyZSBnaXQgZGlyZWN0b3J5LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9MTcxNDUwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISku CisKKyAgICAgICAgKiBTY3JpcHRzL1ZDU1V0aWxzLnBtOgorICAgICAgICAoZ2l0SGFzaEZvckRp cmVjdG9yeSk6CisKIDIwMTctMDUtMDEgIEpvYW5tYXJpZSBEaWdncyAgPGpkaWdnc0BpZ2FsaWEu Y29tPgogCiAgICAgICAgIEFYOiBbR1RLXSBBZGQgc3VwcG9ydCB0byBxdWVyeSBmb3IgYXJpYS1j dXJyZW50CmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL1ZDU1V0aWxzLnBtIGIvVG9vbHMvU2Ny aXB0cy9WQ1NVdGlscy5wbQppbmRleCA0ZDZjZDI5M2YyMjUwYTY4ZTM5Y2Y0MGM1ZTc2M2ZiM2U1 YjIwZWI5Li43YzA4MTkyOGNjY2Q3ZTM5MTUzYWU5M2U3ZTI2N2NhYjk1OWY2NGQwIDEwMDY0NAot LS0gYS9Ub29scy9TY3JpcHRzL1ZDU1V0aWxzLnBtCisrKyBiL1Rvb2xzL1NjcmlwdHMvVkNTVXRp bHMucG0KQEAgLTkyLDYgKzkyLDcgQEAgQkVHSU4gewogICAgICAgICAmc3ZuSW5mb0ZvclBhdGgK ICAgICAgICAgJnN2blJlcG9zaXRvcnlSb290Rm9yUGF0aAogICAgICAgICAmc3ZuUmV2aXNpb25G b3JEaXJlY3RvcnkKKyAgICAgICAgJmdpdEhhc2hGb3JEaXJlY3RvcnkKICAgICAgICAgJnN2blN0 YXR1cwogICAgICAgICAmc3ZuVVJMRm9yUGF0aAogICAgICAgICAmdG9XaW5kb3dzTGluZUVuZGlu Z3MKQEAgLTQ0NCw2ICs0NDUsMjQgQEAgc3ViIHN2blJldmlzaW9uRm9yRGlyZWN0b3J5KCQpCiAg ICAgcmV0dXJuICRyZXZpc2lvbjsKIH0KIAorc3ViIGdpdEhhc2hGb3JEaXJlY3RvcnkoJCkKK3sK KyAgICBteSAoJGRpcmVjdG9yeSkgPSBAXzsKKyAgICBteSAkaGFzaDsKKworICAgIGlmIChpc0dp dERpcmVjdG9yeSgkZGlyZWN0b3J5KSkgeworICAgICAgICBteSAkY29tbWFuZCA9ICJnaXQgLUMg XCIkZGlyZWN0b3J5XCIgcmV2LXBhcnNlIEhFQUQiOworICAgICAgICAkY29tbWFuZCA9ICJMQ19B TEw9QyAkY29tbWFuZCIgaWYgIWlzV2luZG93cygpOworICAgICAgICAkaGFzaCA9IGAkY29tbWFu ZGA7CisgICAgICAgIGNob21wKCRoYXNoKTsKKyAgICB9CisgICAgaWYgKCFkZWZpbmVkKCRoYXNo KSkgeworICAgICAgICAkaGFzaCA9ICJ1bmtub3duIjsKKyAgICAgICAgd2FybiAiVW5hYmxlIHRv IGRldGVybWluZSBjdXJyZW50IEdpdCBoYXNoIGluICRkaXJlY3RvcnkiOworICAgIH0KKyAgICBy ZXR1cm4gJGhhc2g7Cit9CisKIHN1YiBzdm5JbmZvRm9yUGF0aCgkKQogewogICAgIG15ICgkZmls ZSkgPSBAXzsK