95177 2012-08-28 00:55:34 -0700 check-webkit-style fails when WebKit is in a Git submodule 2012-08-29 15:47:24 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 kevin.funk webkit-unassigned abarth dpranke hausmann ojan webkit.review.bot oldest_to_newest 706155 0 kevin.funk 2012-08-28 00:55:34 -0700 The problem is in /Tools/Scripts/webkitpy/common/checkout/scm/git.py, function find_checkout_root(). If WebKit is checked out using git submodules, git.py identifies "$WEBKIT_SUBMODULE\..\.git\modules" as the checkout root, which is wrong. === Example (here: Qt5 repository layout): qt5 - qtbase (submodule) - ... - qtwebkit (submodule) Issue: find_checkout_root() then returns "qt5/.git/modules" as the checkout root (wrong!). === Patch following. 706163 1 160925 kevin.funk 2012-08-28 01:03:17 -0700 Created attachment 160925 Patch 706363 2 160925 abarth 2012-08-28 08:16:33 -0700 Comment on attachment 160925 Patch We need to use self._filesystem, not os.path so that this code will work properly in unit tests. 706387 3 kevin.funk 2012-08-28 08:43:28 -0700 Okay. Would you accept the patch if I am re-adding the two lines with the self._filesystem* calls? 706399 4 abarth 2012-08-28 08:55:12 -0700 I would either need to ask a git expert or read the documentation about the difference between those two flags. We would like to fix this bug, and I'm willing to believe you're doing it the right way. I just need to check. :) Also, please be sure to run ./Tools/Scripts/test-webkitpy. Ideally, you'd also add a test to make sure we don't regress this bug. 706464 5 hausmann 2012-08-28 10:06:07 -0700 I think using --show-toplevel is indeed better than using --git-dir and then trying to parse away the .git portion of the path. 706469 6 abarth 2012-08-28 10:11:18 -0700 Thanks. Ok, so it sounds like we just need to go through self._filesystem. 707432 7 161224 kevin.funk 2012-08-29 07:35:33 -0700 Created attachment 161224 Patch v2 707535 8 dpranke 2012-08-29 09:43:35 -0700 Interesting. It does look like --show-toplevel is a better thing to use, but from the documentation, it sounds like --git-dir is supposed to be returning path-to/.git and we're getting path-to/.git/modules instead. Is this a git bug? 707692 9 kevin.funk 2012-08-29 11:38:04 -0700 Just checked again: In Qt5's qtwebkit submodule there is a *file* named ".git" which breaks the assumed behaviour. Reading out the file yields: $ cat qtwebkit/.git gitdir: ../.git/modules/qtwebkit So git rev-parse --git-dir within qtwebkit will refer to "../.git/modules/qtwebkit" (hard-coded). I am not entirely sure, but I think the reason for doing this is to share the Git history between several submodules (there are at least "qtwebkit" and "qtwebkit1" registered as submodules). So, Dirk, you are probably right. In normal cases --git-dir works fine, but not with that "separate-git-dir" feature used within qtwebkit. Reference: http://www.kernel.org/pub/software/scm/git/docs/git-init.html - '--separate-git-dir' 707696 10 dpranke 2012-08-29 11:43:30 -0700 interesting. I could believe separate-git-dir might confuse things. I'll have to play around with it a bit more. 707957 11 161224 webkit.review.bot 2012-08-29 15:47:21 -0700 Comment on attachment 161224 Patch v2 Clearing flags on attachment: 161224 Committed r127059: <http://trac.webkit.org/changeset/127059> 707958 12 webkit.review.bot 2012-08-29 15:47:24 -0700 All reviewed patches have been landed. Closing bug. 160925 2012-08-28 01:03:17 -0700 2012-08-29 07:35:33 -0700 Patch 0001-Fix-check-webkit-style-when-WebKit-is-in-a-git-submo.patch text/plain 2116 kevin.funk RnJvbSBiMGU1OTc2Njk4MmEyNWY0MTkwZWNiMTgzZTJmOWU1NzNjNDJmZmE4IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLZXZpbiBGdW5rIDxrZXZpbi5mdW5rQGtkYWIuY29tPgpEYXRl OiBUdWUsIDI4IEF1ZyAyMDEyIDEwOjAyOjE4ICswMjAwClN1YmplY3Q6IFtQQVRDSF0gRml4IGNo ZWNrLXdlYmtpdC1zdHlsZSB3aGVuIFdlYktpdCBpcyBpbiBhIGdpdCBzdWJtb2R1bGUKCi0tLQog VG9vbHMvQ2hhbmdlTG9nICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8IDExICsr KysrKysrKysrCiBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0 LnB5IHwgIDYgKystLS0tCiAyIGZpbGVzIGNoYW5nZWQsIDEzIGluc2VydGlvbnMoKyksIDQgZGVs ZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hhbmdlTG9nIGIvVG9vbHMvQ2hhbmdlTG9n CmluZGV4IDAyZGViNWQuLjQzYTc2MWEgMTAwNjQ0Ci0tLSBhL1Rvb2xzL0NoYW5nZUxvZworKysg Yi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAxMi0wOC0yOCAgS2V2aW4gRnVu ayAgPGtldmluLmZ1bmtAa2RhYi5jb20+CisKKyAgICAgICAgRml4IGNoZWNrLXdlYmtpdC1zdHls ZSAoYW5kIHByb2JhYmx5IG90aGVycykgd2hlbiBXZWJLaXQgaXMgaW4gYSBnaXQgc3VibW9kdWxl CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05NTE3Nwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpbmQgdGhl IHJlYWwgY2hlY2tvdXQgcm9vdCBieSB1c2luZyAnZ2l0IHJldi1wYXJzZSAtLXNob3ctdG9wbGV2 ZWwnIGluc3RlYWQgb2YgJy0tZ2l0LWRpcicKKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkv Y29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHk6CisKIDIwMTItMDgtMjMgIERpcmsgUHJhbmtlICA8 ZHByYW5rZUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgbGVhayBkZXRlY3Rpb24gYnJva2VuIGFm dGVyIHIxMjYyNDcgb24gbWFjCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9jaGVja291dC9zY20vZ2l0LnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24v Y2hlY2tvdXQvc2NtL2dpdC5weQppbmRleCA0MWUxZTFlLi5hYzE0MGZlIDEwMDY0NAotLS0gYS9U b29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9zY20vZ2l0LnB5CisrKyBiL1Rv b2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHkKQEAgLTEwOSwx MCArMTA5LDggQEAgY2xhc3MgR2l0KFNDTSwgU1ZOUmVwb3NpdG9yeSk6CiAKICAgICBkZWYgZmlu ZF9jaGVja291dF9yb290KHNlbGYsIHBhdGgpOgogICAgICAgICAjICJnaXQgcmV2LXBhcnNlIC0t c2hvdy1jZHVwIiB3b3VsZCBiZSBhbm90aGVyIHdheSB0byBnZXQgdG8gdGhlIHJvb3QKLSAgICAg ICAgZ2l0X291dHB1dCA9IHNlbGYuX2V4ZWN1dGl2ZS5ydW5fY29tbWFuZChbc2VsZi5leGVjdXRh YmxlX25hbWUsICdyZXYtcGFyc2UnLCAnLS1naXQtZGlyJ10sIGN3ZD0ocGF0aCBvciAiLi8iKSkK LSAgICAgICAgKGNoZWNrb3V0X3Jvb3QsIGRvdF9naXQpID0gc2VsZi5fZmlsZXN5c3RlbS5zcGxp dChnaXRfb3V0cHV0KQotICAgICAgICBpZiBub3Qgc2VsZi5fZmlsZXN5c3RlbS5pc2FicyhjaGVj a291dF9yb290KTogICMgU29tZXRpbWVzIGdpdCByZXR1cm5zIHJlbGF0aXZlIHBhdGhzCi0gICAg ICAgICAgICBjaGVja291dF9yb290ID0gc2VsZi5fZmlsZXN5c3RlbS5qb2luKHBhdGgsIGNoZWNr b3V0X3Jvb3QpCisgICAgICAgIGdpdF9vdXRwdXQgPSBzZWxmLl9leGVjdXRpdmUucnVuX2NvbW1h bmQoW3NlbGYuZXhlY3V0YWJsZV9uYW1lLCAncmV2LXBhcnNlJywgJy0tc2hvdy10b3BsZXZlbCdd LCBjd2Q9KHBhdGggb3IgIi4vIikpLnN0cmlwKCkKKyAgICAgICAgY2hlY2tvdXRfcm9vdCA9IG9z LnBhdGguYWJzcGF0aChnaXRfb3V0cHV0KQogICAgICAgICByZXR1cm4gY2hlY2tvdXRfcm9vdAog CiAgICAgZGVmIHRvX29iamVjdF9uYW1lKHNlbGYsIGZpbGVwYXRoKToKLS0gCjEuNy4xMS5tc3lz Z2l0LjEKCg== 161224 2012-08-29 07:35:33 -0700 2012-08-29 15:47:21 -0700 Patch v2 0002-Fix-check-webkit-style-when-WebKit-is-in-a-git-submo.patch text/plain 2018 kevin.funk RnJvbSBkMTFjNDY0ZjY3ZmRhYmRlMGYwMDdkNDcwZmY0M2EzODU4NzFhM2I5IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBLZXZpbiBGdW5rIDxrZXZpbi5mdW5rQGtkYWIuY29tPgpEYXRl OiBXZWQsIDI5IEF1ZyAyMDEyIDE2OjMyOjM1ICswMjAwClN1YmplY3Q6IFtQQVRDSCAyLzJdIEZp eCBjaGVjay13ZWJraXQtc3R5bGUgd2hlbiBXZWJLaXQgaXMgaW4gYSBnaXQgc3VibW9kdWxlCgot LS0KIFRvb2xzL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgfCAg IDExICsrKysrKysrKysrCiBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2NvbW1vbi9jaGVja291dC9z Y20vZ2l0LnB5IHwgICAgMyArLS0KIDIgZmlsZXMgY2hhbmdlZCwgMTIgaW5zZXJ0aW9ucygrKSwg MiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cgYi9Ub29scy9DaGFu Z2VMb2cKaW5kZXggMDJkZWI1ZC4uNDNhNzYxYSAxMDA2NDQKLS0tIGEvVG9vbHMvQ2hhbmdlTG9n CisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDEyLTA4LTI4ICBLZXZp biBGdW5rICA8a2V2aW4uZnVua0BrZGFiLmNvbT4KKworICAgICAgICBGaXggY2hlY2std2Via2l0 LXN0eWxlIChhbmQgcHJvYmFibHkgb3RoZXJzKSB3aGVuIFdlYktpdCBpcyBpbiBhIGdpdCBzdWJt b2R1bGUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk1 MTc3CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRmlu ZCB0aGUgcmVhbCBjaGVja291dCByb290IGJ5IHVzaW5nICdnaXQgcmV2LXBhcnNlIC0tc2hvdy10 b3BsZXZlbCcgaW5zdGVhZCBvZiAnLS1naXQtZGlyJworCisgICAgICAgICogU2NyaXB0cy93ZWJr aXRweS9jb21tb24vY2hlY2tvdXQvc2NtL2dpdC5weToKKwogMjAxMi0wOC0yMyAgRGlyayBQcmFu a2UgIDxkcHJhbmtlQGNocm9taXVtLm9yZz4KIAogICAgICAgICBsZWFrIGRldGVjdGlvbiBicm9r ZW4gYWZ0ZXIgcjEyNjI0NyBvbiBtYWMKZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0 cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2Nv bW1vbi9jaGVja291dC9zY20vZ2l0LnB5CmluZGV4IDQxZTFlMWUuLmYyOWJkOTAgMTAwNjQ0Ci0t LSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvY29tbW9uL2NoZWNrb3V0L3NjbS9naXQucHkKKysr IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9jb21tb24vY2hlY2tvdXQvc2NtL2dpdC5weQpAQCAt MTA5LDggKzEwOSw3IEBAIGNsYXNzIEdpdChTQ00sIFNWTlJlcG9zaXRvcnkpOgogCiAgICAgZGVm IGZpbmRfY2hlY2tvdXRfcm9vdChzZWxmLCBwYXRoKToKICAgICAgICAgIyAiZ2l0IHJldi1wYXJz ZSAtLXNob3ctY2R1cCIgd291bGQgYmUgYW5vdGhlciB3YXkgdG8gZ2V0IHRvIHRoZSByb290Ci0g ICAgICAgIGdpdF9vdXRwdXQgPSBzZWxmLl9leGVjdXRpdmUucnVuX2NvbW1hbmQoW3NlbGYuZXhl Y3V0YWJsZV9uYW1lLCAncmV2LXBhcnNlJywgJy0tZ2l0LWRpciddLCBjd2Q9KHBhdGggb3IgIi4v IikpCi0gICAgICAgIChjaGVja291dF9yb290LCBkb3RfZ2l0KSA9IHNlbGYuX2ZpbGVzeXN0ZW0u c3BsaXQoZ2l0X291dHB1dCkKKyAgICAgICAgY2hlY2tvdXRfcm9vdCA9IHNlbGYuX2V4ZWN1dGl2 ZS5ydW5fY29tbWFuZChbc2VsZi5leGVjdXRhYmxlX25hbWUsICdyZXYtcGFyc2UnLCAnLS1zaG93 LXRvcGxldmVsJ10sIGN3ZD0ocGF0aCBvciAiLi8iKSkuc3RyaXAoKQogICAgICAgICBpZiBub3Qg c2VsZi5fZmlsZXN5c3RlbS5pc2FicyhjaGVja291dF9yb290KTogICMgU29tZXRpbWVzIGdpdCBy ZXR1cm5zIHJlbGF0aXZlIHBhdGhzCiAgICAgICAgICAgICBjaGVja291dF9yb290ID0gc2VsZi5f ZmlsZXN5c3RlbS5qb2luKHBhdGgsIGNoZWNrb3V0X3Jvb3QpCiAgICAgICAgIHJldHVybiBjaGVj a291dF9yb290Ci0tIAoxLjcuOS41Cgo=