Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2061683002:
New misc scripts, header_usage.sh and author_line_count.sh. (Closed)
|
Created:
4 years, 6 months ago by nisse-webrtc Modified:
4 years, 6 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew misc scripts, header_usage.sh and author_line_count.sh.
NOTRY=True
BUG=
Committed: https://crrev.com/786f4815770c00540c9543683190f6c8f089f970
Cr-Commit-Position: refs/heads/master@{#13148}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments. #Patch Set 3 : Produce a list of net line count by author. #
Total comments: 4
Patch Set 4 : Address nits. #Messages
Total messages: 16 (6 generated)
nisse@webrtc.org changed reviewers: + kjellander@webrtc.org
These two scripts query a git repository in different ways. I guess I should also put some copyright header at the top? And the convention is to use underscore rather than dash in script file names?
On 2016/06/13 12:33:05, nisse-webrtc wrote: > These two scripts query a git repository in different ways. > > I guess I should also put some copyright header at the top? Yes, Use the normal one (you'll find it commented with # in most .py scripts). > And the convention is to use underscore rather than dash in script file names? Yes. One more thing: can you add .sh extension to the scripts?
publishing comments. https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_count File webrtc/tools/author_line_count (right): https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:1: #! /bin/sh Change to #!/bin/bash https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:1: #! /bin/sh Provide a brief description. https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:4: if [ -z "$AUTHOR" ] ; then Use [[ ]] style test. https://engdoc.corp.google.com/eng/doc/shell.xml?cl=head#Test,_%5B_and_%5B%5B https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:4: if [ -z "$AUTHOR" ] ; then Prefer brace-quoting of multiple-character variables: https://engdoc.corp.google.com/eng/doc/shell.xml?showone=Variable_expansion&c... https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/header_usage File webrtc/tools/header_usage (right): https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/header_usage#new... webrtc/tools/header_usage:11: git ls-files '*.h' | while read header ; do We do have two .hpp files. Might want to rename them :) $ git ls-files '*.hpp' webrtc/system_wrappers/source/spreadsortlib/constants.hpp webrtc/system_wrappers/source/spreadsortlib/spreadsort.hpp https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/header_usage#new... webrtc/tools/header_usage:12: name="$(basename "$header")" Prefer curly brace quoting of variables: https://engdoc.corp.google.com/eng/doc/shell.xml?showone=Variable_expansion&c...
Addressed comments. Generalized author_line_count.sh to produce a sorted list. https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_count File webrtc/tools/author_line_count (right): https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:1: #! /bin/sh On 2016/06/13 12:49:52, kjellander_webrtc wrote: > Change to #!/bin/bash Done. https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:4: if [ -z "$AUTHOR" ] ; then On 2016/06/13 12:49:52, kjellander_webrtc wrote: > Use [[ ]] style test. > https://engdoc.corp.google.com/eng/doc/shell.xml?cl=head#Test,_%5B_and_%5B%5B Done. https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/header_usage File webrtc/tools/header_usage (right): https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/header_usage#new... webrtc/tools/header_usage:11: git ls-files '*.h' | while read header ; do On 2016/06/13 12:49:52, kjellander_webrtc wrote: > We do have two .hpp files. Might want to rename them :) > $ git ls-files '*.hpp' > webrtc/system_wrappers/source/spreadsortlib/constants.hpp > webrtc/system_wrappers/source/spreadsortlib/spreadsort.hpp Added '*.hpp' to the ls-files argument list. https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/header_usage#new... webrtc/tools/header_usage:12: name="$(basename "$header")" On 2016/06/13 12:49:52, kjellander_webrtc wrote: > Prefer curly brace quoting of variables: > https://engdoc.corp.google.com/eng/doc/shell.xml?showone=Variable_expansion&c... Done.
Nice to get the long list of authors. It is possible to get removed and added lines as separate entries (assuming the numbers shown is the net sum) ? lgtm with comments addressed https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_count File webrtc/tools/author_line_count (right): https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:1: #! /bin/sh On 2016/06/14 09:38:47, nisse-webrtc wrote: > On 2016/06/13 12:49:52, kjellander_webrtc wrote: > > Change to #!/bin/bash > > Done. Not done :) Still needs to remove the space. https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/author_line_... File webrtc/tools/author_line_count.sh (right): https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/author_line_... webrtc/tools/author_line_count.sh:14: git log "$@" --pretty=format:%ae --shortstat | sed '/^ /s/,/\n/g' | gawk ' nit: Pipelines should be split one per line if they don't all fit on one line. https://engdoc.corp.google.com/eng/doc/shell.xml?cl=head#Pipelines https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/header_usage.sh File webrtc/tools/header_usage.sh (right): https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/header_usage... webrtc/tools/header_usage.sh:1: #! /bin/bash #!/bin/bash
https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_count File webrtc/tools/author_line_count (right): https://codereview.webrtc.org/2061683002/diff/1/webrtc/tools/author_line_coun... webrtc/tools/author_line_count:1: #! /bin/sh On 2016/06/14 14:11:12, kjellander_webrtc wrote: > On 2016/06/14 09:38:47, nisse-webrtc wrote: > > On 2016/06/13 12:49:52, kjellander_webrtc wrote: > > > Change to #!/bin/bash > > > > Done. > > Not done :) Still needs to remove the space. Ok. That's unorthodox, script files are traditionally assumed to use the 32-bit magic number "#! /". https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/author_line_... File webrtc/tools/author_line_count.sh (right): https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/author_line_... webrtc/tools/author_line_count.sh:14: git log "$@" --pretty=format:%ae --shortstat | sed '/^ /s/,/\n/g' | gawk ' On 2016/06/14 14:11:12, kjellander_webrtc wrote: > nit: Pipelines should be split one per line if they don't all fit on one line. > https://engdoc.corp.google.com/eng/doc/shell.xml?cl=head#Pipelines Done. https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/header_usage.sh File webrtc/tools/header_usage.sh (right): https://codereview.webrtc.org/2061683002/diff/40001/webrtc/tools/header_usage... webrtc/tools/header_usage.sh:1: #! /bin/bash On 2016/06/14 14:11:12, kjellander_webrtc wrote: > #!/bin/bash Done.
On 2016/06/14 14:11:13, kjellander_webrtc wrote: > Nice to get the long list of authors. > It is possible to get removed and added lines as separate entries (assuming the > numbers shown is the net sum) ? Adding up the insert and delete numbers from git --shortstat separately would be easy. But not very useful since a cl changing a lot of lines makes both numbers grow (but not the net sum). It would make more sense to compute the net line count per commit, and accumulate negative and positive commits separately. One could also try to get a "lines changed" number. But that no longer a quick change. BTW, I don't think commits with lots of renaming contribute to line count.
Description was changed from ========== New misc scripts, header_usage and author_line_count. BUG= ========== to ========== New misc scripts, header_usage.sh and author_line_count.sh. NOTRY=True BUG= ==========
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2061683002/#ps60001 (title: "Address nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2061683002/60001
Message was sent while issue was closed.
Description was changed from ========== New misc scripts, header_usage.sh and author_line_count.sh. NOTRY=True BUG= ========== to ========== New misc scripts, header_usage.sh and author_line_count.sh. NOTRY=True BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== New misc scripts, header_usage.sh and author_line_count.sh. NOTRY=True BUG= ========== to ========== New misc scripts, header_usage.sh and author_line_count.sh. NOTRY=True BUG= Committed: https://crrev.com/786f4815770c00540c9543683190f6c8f089f970 Cr-Commit-Position: refs/heads/master@{#13148} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/786f4815770c00540c9543683190f6c8f089f970 Cr-Commit-Position: refs/heads/master@{#13148} |
