I don't think this is really an improvement because now we've introduced the risk of someone forgetting COMMON_TOOL_FLAGS. I looked at the cirrus documentation and it doesn't look like you could make a function like $RUN_PLUGIN_TOOL(podspecs).

If you made the command being run not have to be positional with the presence of a new flag you could extractRUN_PLUGIN_TOOL=dart $PLUGIN_TOOL $COMMON_TOOL_FLAGS --command

I don't think this is really an improvement because now we've introduced the risk of someone forgetting COMMON_TOOL_FLAGS.

Yes, I went back and forth on this for the same reason, and didn't see a good solution. The main benefit is that it eliminates the strange setup wheretool_runner.sh is reading a magic env variable, and.cirrus.yml sets the magic variable, but it's totally non-obvious in each one in isolation what this variable is.

But I'll just revert this part for now and revisit it later.

If you made the command being run not have to be positional with the presence of a new flag you could extractRUN_PLUGIN_TOOL=dart $PLUGIN_TOOL $COMMON_TOOL_FLAGS --command

I looked into this a bit, and AFAICT allowing flags to be before the command withCommandRunner requires declaring them at different level that has totally different (and much more awkward) affordances for reading the flags. I can probably move just this one flag as a hack, but it seems ugly. Maybe I'll find a better solution when revisiting.

nit fyi: This would be more efficient as aStream<String> instead ofFuture<List<String>>.

Sure, I can change it in a follow-up. Neither the API nor the way it's being used are new in this PR, I'm just rearranging the flow a bit.

Actually, having looked at the details I'm not clear how it would be more efficient.getChangedFiles is taking the full stdout fromgit (the process API for the tools aren't stream-based) and splitting it by lines, giving a list. So returning aStream instead of aList doesn't change anything there.

At this call site I need this list in two places:_changesRequireFullTest and_getChangedPackages. I can't consume the stream twice, so I would need to.toList() it here.

So the difference would be that instead of taking aList and returning it to a caller that needs aList, I would be taking aList, converting it to aStream, and giving the stream to the caller to immediately turn back to aList. Am I missing a different way of structuring this?

Yea, if you are going to cache the result it doesn't make a difference, it's more of an academic change.

Returning a Stream is better because it allows people to process the output while it is being parsed. This is especially helpful when you are working on input from files/pipes. This also means the operation isn't memory bound and if a consumer decides to stop consuming mid-stream resources won't be wasted processing the output that happens after it stops.

It doesn't mean a lick of difference if you cache the results though so you can process it twice.

Returning a Stream is better because it allows people to process the output while it is being parsed. This is especially helpful when you are working on input from files/pipes.

The supply-side issue here is that thegit package doesn't steamgit output, it usesrun and returns the full output as a String.

not introduced in this change, but is it a problem we aren't using pathSeparator here?

In practice it's not because this is only ever called withgit output, which always uses POSIX paths. I changed this top.posix.split and added a comment to the method saying that they must be POSIX paths as a quick improvement. (The better, more involved solution would be to useFiles rather thanStrings.)