Add the ability to pass long lists of inputs to 'gn refs'.

Add the ability to pass long lists of inputs to 'gn refs'.

This patch adds the ability to pass a "response file"-style argument
to gn refs, so that we can pass a long list of inputs in without needing
to worry about command line lengths. This will help ensure that
we can implement 'analyze' properly for even very large patches without
needing to call 'gn refs' multiple times.

R=brettw@chromium.org
BUG=454413
Committed: https://crrev.com/032c11b0975c897002c35e8b43fce895947c3833
Cr-Commit-Position: refs/heads/master@{#325734}

[Dirk Pranke, 2015-04-17 01:27:42]

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc
File tools/gn/command_refs.cc (right):

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc#ne...
tools/gn/command_refs.cc:413: base::SplitStringAlongWhitespace(contents,
&inputs);
I'm not sure if this is the best base routine to use here ... suggestions?

[brettw, 2015-04-17 02:44:27]

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc
File tools/gn/command_refs.cc (right):

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc#ne...
tools/gn/command_refs.cc:271: "gn refs <out_dir>
(<label_pattern>|<label>|<file>|<response_file>)* "
Can you make this ...|@...

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc#ne...
tools/gn/command_refs.cc:413: base::SplitStringAlongWhitespace(contents,
&inputs);
On 2015/04/17 01:27:41, Dirk Pranke wrote:
> I'm not sure if this is the best base routine to use here ... suggestions?

Yeah, this will fall over for command-lines that include spaces.

On Windows there is a nice function to call to do this parsing properly, but
there's not on Linux, and we would need to implement a bunch of annoying
unescaping.

I wonder if we could just have our own response file format be file-per-line,
since the input is always our own tools? Then you can split it on newlines, and
it's also easier to write from Python without worrying about weird issues.

[Dirk Pranke, 2015-04-17 02:58:12]

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc
File tools/gn/command_refs.cc (right):

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc#ne...
tools/gn/command_refs.cc:271: "gn refs <out_dir>
(<label_pattern>|<label>|<file>|<response_file>)* "
On 2015/04/17 02:44:27, brettw wrote:
> Can you make this ...|@...
> </response_file>

Will do.

https://codereview.chromium.org/1098603002/diff/1/tools/gn/command_refs.cc#ne...
tools/gn/command_refs.cc:413: base::SplitStringAlongWhitespace(contents,
&inputs);
On 2015/04/17 02:44:27, brettw wrote:
> On 2015/04/17 01:27:41, Dirk Pranke wrote:
> > I'm not sure if this is the best base routine to use here ... suggestions?
> 
> Yeah, this will fall over for command-lines that include spaces.
> 
> On Windows there is a nice function to call to do this parsing properly, but
> there's not on Linux, and we would need to implement a bunch of annoying
> unescaping.
> 
> I wonder if we could just have our own response file format be file-per-line,
> since the input is always our own tools? Then you can split it on newlines,
and
> it's also easier to write from Python without worrying about weird issues.

That works for me (the layout tests actually support such a format for long
lists of files). 

I didn't think of embedded spaces in the filenames :(.

Is there a base routine that will correctly handle CR, LF, and CRLF (i.e., split
on lines portably)? Or should I not worry about it and just split on LF (\n)?

[Dirk Pranke, 2015-04-17 21:15:45]

please take another look?

[brettw, 2015-04-17 21:23:25]

lgtm

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.cc
File tools/gn/command_refs.cc (right):

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.c...
tools/gn/command_refs.cc:400: for (auto it = args.begin() + 1; it != args.end();
it++) {
I think this case is much clearer as an integer iterator, personally:
  for (size_t i = 1; i < args.size(); i++)
and it's shorter.

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.c...
tools/gn/command_refs.cc:401: auto arg = *it;
This is secretly hiding a string copy. You could make it a reference, but I'd
just delete it (you actually didn't even use it consistently below anyway)

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.c...
tools/gn/command_refs.cc:407: bool ret =
base::ReadFileToString(base::FilePath(arg.substr(1)),
This will fail on Windows. You'll want UTF8ToFilePath (in filesystem utils)

[Dirk Pranke, 2015-04-17 21:35:16]

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.cc
File tools/gn/command_refs.cc (right):

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.c...
tools/gn/command_refs.cc:400: for (auto it = args.begin() + 1; it != args.end();
it++) {
On 2015/04/17 21:23:25, brettw wrote:
> I think this case is much clearer as an integer iterator, personally:
>   for (size_t i = 1; i < args.size(); i++)
> and it's shorter.

Done.

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.c...
tools/gn/command_refs.cc:401: auto arg = *it;
On 2015/04/17 21:23:25, brettw wrote:
> This is secretly hiding a string copy. You could make it a reference, but I'd
> just delete it (you actually didn't even use it consistently below anyway)

True. Done.

https://codereview.chromium.org/1098603002/diff/20001/tools/gn/command_refs.c...
tools/gn/command_refs.cc:407: bool ret =
base::ReadFileToString(base::FilePath(arg.substr(1)),
On 2015/04/17 21:23:25, brettw wrote:
> This will fail on Windows. You'll want UTF8ToFilePath (in filesystem utils)

Done.

[Dirk Pranke, 2015-04-17 21:35:23]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-04-17 21:35:24]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1098603002/#ps40001
(title: "update w/ more review feedback")

[commit-bot: I haz the power, 2015-04-17 21:36:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1098603002/40001

[commit-bot: I haz the power, 2015-04-17 23:21:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-17 23:22:51]

Patchset 3 (id:??) landed as
https://crrev.com/032c11b0975c897002c35e8b43fce895947c3833
Cr-Commit-Position: refs/heads/master@{#325734}