importing dart:io is banned from the tool. Instead, use https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/base/io.dart

For testing, don't use dart:io's Process.run, but instead the ProcessManager from the context: https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/context_runner.dart#L306. In addition, direct inject the process manager via the constructor, so we can unit test it with a FakeProcessManager.

The IOSDeploy class is an example that does this.

Also, for direct injection, these static methods should probably be moved to instance methods.

Ok, thanks for the feedback. I figured I would have to change the static utils class setup, this sounds good, thanks for the example to reference.

Why do we need to clone a new copy of the repo?

Is it because we will be calling flutter create from a previous commit?

Yes, we require the full flutter tool from previous versions to be able to accurately reproduce reference apps to migrate off of. Unfortunately, the only way to do this is to clone flutter. The migrate tool isn't run frequently, so the overhead here is more palatable.

Why --shallow-exclude=v1.0.0?

And why --single-branch?

These parameters greatly reduce the time it takes to clone the repo by using a shallow clone of the master branch. We don't claim to support migration apps created before 1.0, so this hard limit significantly improves performance of the command.

I'm actually still experimenting with the --single-branch option, so I may remove that for now.

I'm not convinced that shallow cloning is faster (I benchmarked it locally and they were exactly the same, which seems surprising), and the Flutter CLI tool is not guaranteed to work in a shallow clone environment (certain operations depend on access to the git history, I'm not sure I can guarantee flutter create won't crash in some situations).

Following up, I don't think this is the correct way to shallow clone; I actually did get a clone speedup from doing git clone $REMOTE --depth 1, however, again, I wouldn't recommend it as that voids your warranty on the Flutter CLI tool.

Ok, i'll remove this for now then until we can come up with a better way for performance improvements.

nit. can you construct this as a single list literal using collection for and collection if? something like:

final List<String> cmdArgs = <String>[
  'create',
  if (!legacyNameParameter) '--project-name=$name',
  '--android-language=$androidLanguage',
  '--ios-language=$iosLanguage',
  if (platforms.isNotEmpty) '--platforms=${platforms.join(',')}',
  '--no-pub',
  if (legacyNameParameter) name,
  else outputDirectory,
];

Rather than recursively calling this function, could you first check the Flutter version, then configure the parameters accordingly the first time?

I also worry that we could recurse infinitely here.

The flutter version tag is not always reliable in representing what parameters are available as it is not fine enough, I also tried using the hash to see if the boundaries between commits that add parameters can reliably obtained, but depending on the branch, this method was also not reliable due to various branches and releases.

This technique is the most robust way to approach it. Since we are effectively limiting the backwards compatibility to version v1.0.0, this should be able to self-pare its way down to the minimal runnable command. I can add a counter also to restrict this from infinite looping if you would like.

The flutter version tag is not always reliable in representing what parameters are available as it is not fine enough

Can't we just determine the first tagged version that had a specific change? GitHub will even do this for you if you look at a specific commit in the web UI:
.

It is not uncommon for projects to be created off of a custom branch with a commit hash that is not on the master branch. This is especially true for our first party sample projects which were largely created with squashed local commits.

In these cases, we can't really say what version of flutter it was created with or the version we are using in reference to the tags and if we get this wrong, the command will fail and we would have to fall back to this system anyways.

Why only limit to 1.0? Where does the --platforms flag first appear? If it helps to delete this logic to instead require creation after 2.0 for migration to work, I think that would be totally reasonable.

It feels like this whole class should just be an enum, and the base constructor case a different data type.

This class is meant to represent the output of a git diff-filescommand including stuff like exit code. I've changed the is into an enum though!

What about making MergeResult an abstract class, and implement it with either a StringMergeResult or BinaryMergeResult

packages/flutter_tools/test/general.shard is for unit tests, which should not use the LocalFileSystem. We probably do want to call git to verify these commands work, so I recommend moving this to packages/flutter_tools/test/integration.shard.

don't use dart:io

I probably shouldn't review this test file further until the other changes have been made. Let me know when I should take another look!

Is the user ever hand-editing this, or is it just the tool? If the former, maybe we should try to help them by telling them what exactly made this invalid, if the latter maybe we should just throw an exception so we get crash reporting because it's a bug in the tool.

This file is not meant to be hand-edited. If this file becomes invalid, it is from the user accidentally modifying it on their own without our blessing.

can you make these keys constants at the top level scope of the file?

DBC:

Please do not check in commented out code.

Is this called somewhere? If not, it should be held back and only checked in once it's used/tested somewhere.

Ditto comment

This is only called once, above in gitRepoExists. It could be a library private function.

This will soon be used in all of the subcommands

This is only used in this class. Can it be library-private?

This will soon be used in all of the subcommands

This should probably also fail if the markers have been partially removed. For example if the user removes the '>>>' and '===', but leaves in the '<<<'. Given that, I think this can be simplified as:

@visibleForTesting
bool conflictsResolved(String contents) {
  final bool hasMarker = contents.contains('>>>>>>>') ||
                         contents.contains('=======') ||
                         contents.contains('<<<<<<<');
  return !hasMarker;
}

This should also have a unit test.

Ahh, he actually had it that way, and I asked him to use &&. But this makes sense, my bad.

This is not called or tested anywhere. Please don't check in dead / untested code.

Why only limit to 1.0? Where does the --platforms flag first appear? If it helps to delete this logic to instead require creation after 2.0 for migration to work, I think that would be totally reasonable.

Flip the sense, and unindent.

I haven't studied how this code is organized deeply, but I'm sure that it's likely possible to avoid duplicating methods like this across libraries.