`git cl owners` is tripping up with owners comments |
|||
Issue descriptionI saw this issue with https://codereview.chromium.org/2820343002 where I own most of the files/directory being changed except core/html/BUILD.gn and core/layout/ (two files). This is a few copy-paste from `git cl owners`: ``` thakis is commented as: For *.gn* changes only. (at third_party/WebKit/PRESUBMIT*.py) Owner approval for 3rd party is only required for adding new libraries. Be sure to follow the instructions for adding new third party libraries. https://chromium.googlesource.com/chromium/src.git/+/master/docs/adding_to_third_party.md For changes to existing code that has no OWNER file, simply TBR= one of people below in addition to getting an appropriate code review. Generally third_party modules should have owners, so if you find yourself doing this more than once or twice for a directory, consider adding an OWNER file. If you're making changes and there isn't a more obvious person, the owner is probably you! (at third_party) For *.gn* changes only. (at third_party/WebKit) rune reviews core/css and parts of core/rendering (at third_party/WebKit/Source/core) ``` => it mentions rune, it also mentions the comment for PRESUBMIT that I do not touch ``` yutak is commented as: vollick reviews compositor integration, accelerated transitions, animations and scrolling, scrolling coordinator, CSS transforms (at third_party/WebKit/Source/core) ``` => happens for a lot of other cases where the wrong person is mentioned ``` jochen is commented as: EMEA based reviewer. (global status) Owner approval for 3rd party is only required for adding new libraries. Be sure to follow the instructions for adding new third party libraries. https://chromium.googlesource.com/chromium/src.git/+/master/docs/adding_to_third_party.md For changes to existing code that has no OWNER file, simply TBR= one of people below in addition to getting an appropriate code review. Generally third_party modules should have owners, so if you find yourself doing this more than once or twice for a directory, consider adding an OWNER file. If you're making changes and there isn't a more obvious person, the owner is probably you! (at third_party) foolip reviews <video>, <track>, WebVTT and Fullscreen. (at third_party/WebKit/Source/core) ``` => a bit similar to the first example but here I do touch third_party/, though, a sub directory so it shouldn't show me the top level comment.
,
Apr 18 2017
What would this have done before the OWNERS_STATUS change?
,
Apr 18 2017
This is unrelated to the owners status change. The problem just surfaced now since the presubmit check now also displays the comments we loaded (and not just an explicit git cl owners)
,
Apr 18 2017
ok, that was what I was guessing, just wasn't sure.
I considered something like the following:
1) If you have {blank_line}+{comment}+{!comment name}+, have the comments apply to all of the names
2) If you have {!comment name}+{comment}+{name}{name}+ I would have the comment apply to the single name.
I think rule 1) is your "latter" case. I think rule 2) could be useful for compatibility w/ existing files. However, I'm inclined to think it is making life more complicated than necessary, so I'd probably actually prefer just 1).
You said you were leaning against it, though. Why?
,
Apr 18 2017
uh, sorry, I meant I'm leaning towards the latter. btw, with your 1), the large comment at the beginning of src/base/OWNERS would not be used for any reviewer. I guess that's intentional? I think implementing 1) and 2) is do-able anyways...
,
Apr 18 2017
Okay, if we don't think we absolutely have to have 2), let's not do 2) for now. I hadn't considered the "big comment at beginning of file" case properly. Maybe ideally we'd do something like "if any files under X are changed, and there's a comment at the top of X, display it specially"? But, I'm not sure exactly how we'd implement that. I wouldn't want to repeat it for every owner or file, for example.
,
Apr 18 2017
,
Apr 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/tools/depot_tools/+/b624bfe0d0a5f759e7f1316918c37f0522426122 commit b624bfe0d0a5f759e7f1316918c37f0522426122 Author: Jochen Eisinger <jochen@chromium.org> Date: Thu Apr 20 08:17:01 2017 Only attribute comments to owners that are close to the comment A comment that is preceded with an empty line (or starts at the beginning of the file) will be attributed to owners listed directly below the comment. Otherwise, if the comment is in the middle of a list of owners, it will only be attributed to the next owner. BUG= 712589 R=dpranke@chromium.org Change-Id: I19bd7809836b6ee65ef56e2ec399e5cd09eaa132 Reviewed-on: https://chromium-review.googlesource.com/481303 Commit-Queue: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> [modify] https://crrev.com/b624bfe0d0a5f759e7f1316918c37f0522426122/tests/owners_finder_test.py [modify] https://crrev.com/b624bfe0d0a5f759e7f1316918c37f0522426122/owners.py [modify] https://crrev.com/b624bfe0d0a5f759e7f1316918c37f0522426122/tests/owners_unittest.py
,
Apr 20 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by jochen@chromium.org
, Apr 18 2017