Lets you run sort-headers.py on all files changed since some git branch.

Adds a tool that lets you run a command for each modified file on your git branch or in your workspace.

Also, add a flag to sort-headers.py to make it more useful when used in conjunction with that tool, that suppresses the y/N prompt after each diff (as vetting a bulk change may be easier once it's in your working directory, using your favorite diff tool).  Refactor the script slightly while I'm in there.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=108097

[Nico, 2011-10-26 17:53:38]

The -f/-q change is fine. Can't the rest be done much simpler with

  tools/sort-headers.py $(git diff  --name-only trunk | egrep '\.(cc|mm|cpp|c)')

?

http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py
File tools/sort-headers.py (right):

http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode118
tools/sort-headers.py:118: def FilenamesFromGit(branch_name):
Needs docstring

http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode138
tools/sort-headers.py:138: parser.add_option('-q', '--quiet',
action='store_false', default=True,
Confirm prompt suppression flags are usually called -f / --force

[Jói, 2011-10-26 19:27:43]

That's clever, and you could probably make something like that work.
(I guess --name-only would still include deleted files, on which the
script would barf).

Using Python makes it cross-platform.  Keeping the code in the same
file makes it more discoverable but avoids adding a separate file
(sort-all-headers.sh ?).  Reusing for SVN should also be easier.

Regardless, I already wrote the code, so using the code that's been
written and tested is probably simplest :D

Will change the no-prompt flag to -f / --force.

Cheers,
Jói


On Wed, Oct 26, 2011 at 5:53 PM,  <thakis@chromium.org> wrote:
> The -f/-q change is fine. Can't the rest be done much simpler with
>
>  tools/sort-headers.py $(git diff  --name-only trunk | egrep
> '\.(cc|mm|cpp|c)')
>
> ?
>
>
>
> http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py
> File tools/sort-headers.py (right):
>
> http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode118
> tools/sort-headers.py:118: def FilenamesFromGit(branch_name):
> Needs docstring
>
> http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode138
> tools/sort-headers.py:138: parser.add_option('-q', '--quiet',
> action='store_false', default=True,
> Confirm prompt suppression flags are usually called -f / --force
>
> http://codereview.chromium.org/8390055/
>

[Jói, 2011-10-26 20:05:48]

New version updated, PTAL.

2011/10/26 Jói Sigurðsson <joi@chromium.org>:
> That's clever, and you could probably make something like that work.
> (I guess --name-only would still include deleted files, on which the
> script would barf).
>
> Using Python makes it cross-platform.  Keeping the code in the same
> file makes it more discoverable but avoids adding a separate file
> (sort-all-headers.sh ?).  Reusing for SVN should also be easier.
>
> Regardless, I already wrote the code, so using the code that's been
> written and tested is probably simplest :D
>
> Will change the no-prompt flag to -f / --force.
>
> Cheers,
> Jói
>
>
> On Wed, Oct 26, 2011 at 5:53 PM,  <thakis@chromium.org> wrote:
>> The -f/-q change is fine. Can't the rest be done much simpler with
>>
>>  tools/sort-headers.py $(git diff  --name-only trunk | egrep
>> '\.(cc|mm|cpp|c)')
>>
>> ?
>>
>>
>>
>> http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py
>> File tools/sort-headers.py (right):
>>
>>
http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode118
>> tools/sort-headers.py:118: def FilenamesFromGit(branch_name):
>> Needs docstring
>>
>>
http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode138
>> tools/sort-headers.py:138: parser.add_option('-q', '--quiet',
>> action='store_false', default=True,
>> Confirm prompt suppression flags are usually called -f / --force
>>
>> http://codereview.chromium.org/8390055/
>>
>

[Jói, 2011-10-27 17:20:09]

ping

2011/10/26 Jói Sigurðsson <joi@chromium.org>:
> New version updated, PTAL.
>
> 2011/10/26 Jói Sigurðsson <joi@chromium.org>:
>> That's clever, and you could probably make something like that work.
>> (I guess --name-only would still include deleted files, on which the
>> script would barf).
>>
>> Using Python makes it cross-platform.  Keeping the code in the same
>> file makes it more discoverable but avoids adding a separate file
>> (sort-all-headers.sh ?).  Reusing for SVN should also be easier.
>>
>> Regardless, I already wrote the code, so using the code that's been
>> written and tested is probably simplest :D
>>
>> Will change the no-prompt flag to -f / --force.
>>
>> Cheers,
>> Jói
>>
>>
>> On Wed, Oct 26, 2011 at 5:53 PM,  <thakis@chromium.org> wrote:
>>> The -f/-q change is fine. Can't the rest be done much simpler with
>>>
>>>  tools/sort-headers.py $(git diff  --name-only trunk | egrep
>>> '\.(cc|mm|cpp|c)')
>>>
>>> ?
>>>
>>>
>>>
>>> http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py
>>> File tools/sort-headers.py (right):
>>>
>>>
http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode118
>>> tools/sort-headers.py:118: def FilenamesFromGit(branch_name):
>>> Needs docstring
>>>
>>>
http://codereview.chromium.org/8390055/diff/1/tools/sort-headers.py#newcode138
>>> tools/sort-headers.py:138: parser.add_option('-q', '--quiet',
>>> action='store_false', default=True,
>>> Confirm prompt suppression flags are usually called -f / --force
>>>
>>> http://codereview.chromium.org/8390055/
>>>
>>
>

[Nico, 2011-10-27 17:55:07]

On 2011/10/26 19:27:43, Jói wrote:
> That's clever, and you could probably make something like that work.
> (I guess --name-only would still include deleted files, on which the
> script would barf).
> 
> Using Python makes it cross-platform.  Keeping the code in the same
> file makes it more discoverable but avoids adding a separate file
> (sort-all-headers.sh ?).  Reusing for SVN should also be easier.

It increases the line count of this script by 50% and brings it from the "do one
thing and one thing only (sorting headers)" business into the "do one thing, and
also this other thing where it interacts with a subset of our version control
systems in some way" business.

One way we've dealt with discoverability before is to put a comment at the top
that says "to run this on all your new files, use $magic_invocation".

You're right that --name-only doesn't work well with removed files here, that's
a bummer.

> Regardless, I already wrote the code, so using the code that's been
> written and tested is probably simplest :D

How about putting the git-walking code into tools/git/for-all-touched-files.py?
It would take a command to run on all touched files (e.g. sort-headers.py). It
seems this might be useful with other scripts, and that way both scripts do a
single well-defined thing.

[Jói, 2011-10-28 10:35:08]

> How about putting the git-walking code into
> tools/git/for-all-touched-files.py?

I like that idea, will switch to that and update the review thread
when it's ready.

Cheers,
Jói


On Thu, Oct 27, 2011 at 5:55 PM,  <thakis@chromium.org> wrote:
> On 2011/10/26 19:27:43, Jói wrote:
>>
>> That's clever, and you could probably make something like that work.
>> (I guess --name-only would still include deleted files, on which the
>> script would barf).
>
>> Using Python makes it cross-platform.  Keeping the code in the same
>> file makes it more discoverable but avoids adding a separate file
>> (sort-all-headers.sh ?).  Reusing for SVN should also be easier.
>
> It increases the line count of this script by 50% and brings it from the "do
> one
> thing and one thing only (sorting headers)" business into the "do one thing,
> and
> also this other thing where it interacts with a subset of our version
> control
> systems in some way" business.
>
> One way we've dealt with discoverability before is to put a comment at the
> top
> that says "to run this on all your new files, use $magic_invocation".
>
> You're right that --name-only doesn't work well with removed files here,
> that's
> a bummer.
>
>> Regardless, I already wrote the code, so using the code that's been
>> written and tested is probably simplest :D
>
> How about putting the git-walking code into
> tools/git/for-all-touched-files.py?
> It would take a command to run on all touched files (e.g. sort-headers.py).
> It
> seems this might be useful with other scripts, and that way both scripts do
> a
> single well-defined thing.
>
> http://codereview.chromium.org/8390055/
>

[Jói, 2011-10-31 18:13:03]

Please take another look, I've split things up as discussed.

Cheers,
Jói


On 2011/10/28 10:35:08, Jói wrote:
> > How about putting the git-walking code into
> > tools/git/for-all-touched-files.py?
> 
> I like that idea, will switch to that and update the review thread
> when it's ready.
> 
> Cheers,
> Jói
> 
> 
> On Thu, Oct 27, 2011 at 5:55 PM,  <mailto:thakis@chromium.org> wrote:
> > On 2011/10/26 19:27:43, Jói wrote:
> >>
> >> That's clever, and you could probably make something like that work.
> >> (I guess --name-only would still include deleted files, on which the
> >> script would barf).
> >
> >> Using Python makes it cross-platform. &nbsp;Keeping the code in the same
> >> file makes it more discoverable but avoids adding a separate file
> >> (sort-all-headers.sh ?). &nbsp;Reusing for SVN should also be easier.
> >
> > It increases the line count of this script by 50% and brings it from the "do
> > one
> > thing and one thing only (sorting headers)" business into the "do one thing,
> > and
> > also this other thing where it interacts with a subset of our version
> > control
> > systems in some way" business.
> >
> > One way we've dealt with discoverability before is to put a comment at the
> > top
> > that says "to run this on all your new files, use $magic_invocation".
> >
> > You're right that --name-only doesn't work well with removed files here,
> > that's
> > a bummer.
> >
> >> Regardless, I already wrote the code, so using the code that's been
> >> written and tested is probably simplest :D
> >
> > How about putting the git-walking code into
> > tools/git/for-all-touched-files.py?
> > It would take a command to run on all touched files (e.g. sort-headers.py).
> > It
> > seems this might be useful with other scripts, and that way both scripts do
> > a
> > single well-defined thing.
> >
> > http://codereview.chromium.org/8390055/
> >

[(unused - use chromium), 2011-10-31 18:19:56]

lgtm with comments addressed

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
File tools/git/for-all-touched-files.py (right):

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:50: def FilenamesFromGit(branch_name,
extensions):
needs docstring

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:78: parser =
optparse.OptionParser(usage=__doc__)
usage=__doc__ is a nice trick, didn't know about this one :-)

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:79: parser.add_option(
Format this like the other add_option() calls below?

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:91: help=('Sets what to diff to (default
origin/master). Set '
nit: Since this is already in a function call, you don't need the parens around
the help string

http://codereview.chromium.org/8390055/diff/13001/tools/sort-headers.py
File tools/sort-headers.py (right):

http://codereview.chromium.org/8390055/diff/13001/tools/sort-headers.py#newcode8
tools/sort-headers.py:8: Shows a diff and prompts for confirmation before doing
the deed.
Maybe add "Works great with tools/git/for-all-touched-files.py" here?

http://codereview.chromium.org/8390055/diff/13001/tools/sort-headers.py#newco...
tools/sort-headers.py:74: def DiffAndConfirm(filename, should_confirm):
needs docstring

[Jói, 2011-10-31 18:32:08]

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
File tools/git/for-all-touched-files.py (right):

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:50: def FilenamesFromGit(branch_name,
extensions):
On 2011/10/31 18:19:56, thakis wrote:
> needs docstring

Done.

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:79: parser.add_option(
On 2011/10/31 18:19:56, thakis wrote:
> Format this like the other add_option() calls below?

Done.

http://codereview.chromium.org/8390055/diff/13001/tools/git/for-all-touched-f...
tools/git/for-all-touched-files.py:91: help=('Sets what to diff to (default
origin/master). Set '
On 2011/10/31 18:19:56, thakis wrote:
> nit: Since this is already in a function call, you don't need the parens
around
> the help string

Done.

http://codereview.chromium.org/8390055/diff/13001/tools/sort-headers.py
File tools/sort-headers.py (right):

http://codereview.chromium.org/8390055/diff/13001/tools/sort-headers.py#newcode8
tools/sort-headers.py:8: Shows a diff and prompts for confirmation before doing
the deed.
On 2011/10/31 18:19:56, thakis wrote:
> Maybe add "Works great with tools/git/for-all-touched-files.py" here?

Done.

http://codereview.chromium.org/8390055/diff/13001/tools/sort-headers.py#newco...
tools/sort-headers.py:74: def DiffAndConfirm(filename, should_confirm):
On 2011/10/31 18:19:56, thakis wrote:
> needs docstring

Done.