| Summary: | let check-webkit-style look at changes staged in the git index | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joe Mason <joenotcharles> | ||||||
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED WONTFIX | ||||||||
| Severity: | Enhancement | CC: | hamaji, levin, manyoso, morrita | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
|
Description
Joe Mason
2009-08-03 14:50:37 PDT
Created attachment 34006 [details]
patch adding --git-index to check-webkit-style
Does --git-commit=HEAD work for you? The problem with adding --git-index is that it won't work well if you've done changes locally that aren't in the index. For example, suppose you change lines 10-14 in the index and then delete lines 5-9 in your local file system. The checker uses the local files so it gives you style messages for lines 10-14 in the local file which is wrong. Note that you need an up to date version of check-webkit-style to use --git-commit=HEAD as I suggested because I just changed this option Friday evening (due to the problems that I previously mentioned). Comment on attachment 34006 [details]
patch adding --git-index to check-webkit-style
See my comments in the bug. If you still want to add this, then we can discuss how this can work without it being very confusing.
Oops, sorry. Should have updated before working on this. I think this approach is still needed with the latest version, though. "Does --git-commit=HEAD work for you?" No, that doesn't do quite what I want. "The problem with adding --git-index is that it won't work well if you've done changes locally that aren't in the index. For example, suppose you change lines 10-14 in the index and then delete lines 5-9 in your local file system. The checker uses the local files so it gives you style messages for lines 10-14 in the local file which is wrong." Actually, I just realized that this isn't a bug, it's exactly what I want it to do! The reason is that when I go to fix the file, I'll be editing the local version. I care about which lines the errors are on in the local version, not what's staged to the index. (I just don't want to get warnings about changes that aren't staged yet.) Consider the following example: I'm working on a feature that involves adding a 5 line function to the beginning of file A, and making various changes to file B. It's not finished yet, when I get an urgent request to rewrite an unrelated function in file A. I make changes to lines 100-105 in file A (which, because of my 5 lines added earlier, correspond to lines 95-100 in HEAD, which is the upstream version of the file). I use "git add -i" to stage only lines 100-105 of file A to the index, and run check-webkit-style. Ideally I want it to only give errors for the changes acually in the index, because that's all that I'm going to commit. --git-commit=HEAD will check all changes between HEAD and the working copy, including all the ones I haven't added to the index. I think I could use a list of files to check only file A, but it would still look at both changes in that file. --git-index would look only at the changes in the index. So far so good. Now, if it found a problem on the second line of my patch (that's line 96 in the index, line 101 in my working copy), what line should it report that on? Well, I'm going to fix it by editing my working copy, and then adding the new change to the index with "git add -i" again. So line 101 is more useful. Which is immaterial as I can't actually get it to work with the new version: git diff --cached reports lines numbers relative to the index, so an error message on line 101 is just filtered out because it doesn't appear in the diff. (This probably happened in the old version too and I just didn't test enough.) So --git-commit=HEAD and manually filter out the changes I don't care about will have to do for now. Created attachment 34069 [details]
patch to latest check-webkit-style - does not deal well with added/removed lines not in index
Attaching my latest (nonworking) patch for reference.
I think you got it. The key problem is that error messages will be filtered according to the lines in the patch, but those lines may have moved in the current file. A patch that says only line 10 changed will only get error messages for that line even though that line is now at line 16 in your local file system. |