Skip to content

[tool] Provide a --base-branch flag #3322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion script/tool/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,17 +117,24 @@ code changes across many packages:
cd <repository root>
dart run script/tool/bin/flutter_plugin_tools.dart update-release-info \
--version=minimal \
--base-branch=upstream/main \
--changelog="Fixes violations of new analysis option some_new_option."
```

The `minimal` option for `--version` will skip unchanged packages, and treat
each changed package as either `bugfix` or `next` depending on the files that
have changed in that package, so it is often the best choice for a bulk change.

For cases where you know the change time, `minor` or `bugfix` will make the
For cases where you know the change type, `minor` or `bugfix` will make the
corresponding version bump, or `next` will update only `CHANGELOG.md` without
changing the version.

If you have a standard repository setup, `--base-branch=upstream/main` will
usually give the behavior you want, finding all packages changed relative to
the branch point from `upstream/main`. For more complex use cases where you want
a different diff point, you can pass a different `--base-branch`, or use
`--base-sha` to pick the exact diff point.

### Publish a Release

**Releases are automated for `flutter/packages`.**
Expand Down
13 changes: 10 additions & 3 deletions script/tool/lib/src/common/git_version_finder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import 'package:yaml/yaml.dart';
/// Finding diffs based on `baseGitDir` and `baseSha`.
class GitVersionFinder {
/// Constructor
GitVersionFinder(this.baseGitDir, String? baseSha) : _baseSha = baseSha;
GitVersionFinder(this.baseGitDir, {String? baseSha, String? baseBranch})
: assert(baseSha == null || baseBranch == null,
'At most one of baseSha and baseBranch can be provided'),
_baseSha = baseSha,
_baseBranch = baseBranch ?? 'FETCH_HEAD';

/// The top level directory of the git repo.
///
Expand All @@ -21,6 +25,9 @@ class GitVersionFinder {
/// The base sha used to get diff.
String? _baseSha;

/// The base branche used to find a merge point if baseSha is not provided.
final String _baseBranch;

static bool _isPubspec(String file) {
return file.trim().endsWith('pubspec.yaml');
}
Expand Down Expand Up @@ -101,13 +108,13 @@ class GitVersionFinder {
}

io.ProcessResult baseShaFromMergeBase = await baseGitDir.runCommand(
<String>['merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'],
<String>['merge-base', '--fork-point', _baseBranch, 'HEAD'],
throwOnError: false);
final String stdout = (baseShaFromMergeBase.stdout as String? ?? '').trim();
final String stderr = (baseShaFromMergeBase.stderr as String? ?? '').trim();
if (stderr.isNotEmpty || stdout.isEmpty) {
baseShaFromMergeBase = await baseGitDir
.runCommand(<String>['merge-base', 'FETCH_HEAD', 'HEAD']);
.runCommand(<String>['merge-base', _baseBranch, 'HEAD']);
}
baseSha = (baseShaFromMergeBase.stdout as String).trim();
_baseSha = baseSha;
Expand Down
22 changes: 17 additions & 5 deletions script/tool/lib/src/common/package_command.dart
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,17 @@ abstract class PackageCommand extends Command<void> {
help: 'The base sha used to determine git diff. \n'
'This is useful when $_runOnChangedPackagesArg is specified.\n'
'If not specified, merge-base is used as base sha.');
argParser.addOption(_baseBranchArg,
help: 'The base branch whose merge base is used as the base SHA if '
'--$_baseShaArg is not provided. \n'
'If not specified, FETCH_HEAD is used as the base branch.');
argParser.addFlag(_logTimingArg,
help: 'Logs timing information.\n\n'
'Currently only logs per-package timing for multi-package commands, '
'but more information may be added in the future.');
}

static const String _baseBranchArg = 'base-branch';
static const String _baseShaArg = 'base-sha';
static const String _excludeArg = 'exclude';
static const String _logTimingArg = 'log-timing';
Expand Down Expand Up @@ -188,6 +193,11 @@ abstract class PackageCommand extends Command<void> {
return (argResults![key] as String?) ?? '';
}

/// Convenience accessor for String arguments.
String? getNullableStringArg(String key) {
return argResults![key] as String?;
}

/// Convenience accessor for List<String> arguments.
List<String> getStringListArg(String key) {
// Clone the list so that if a caller modifies the result it won't change
Expand Down Expand Up @@ -338,7 +348,7 @@ abstract class PackageCommand extends Command<void> {
if (lastCommitOnly) {
print(
'--$_packagesForBranchArg: using parent commit as the diff base.');
changedFileFinder = GitVersionFinder(await gitDir, 'HEAD~');
changedFileFinder = GitVersionFinder(await gitDir, baseSha: 'HEAD~');
} else {
changedFileFinder = await retrieveVersionFinder();
}
Expand All @@ -361,7 +371,7 @@ abstract class PackageCommand extends Command<void> {
}
} else if (getBoolArg(_runOnDirtyPackagesArg)) {
final GitVersionFinder gitVersionFinder =
GitVersionFinder(await gitDir, 'HEAD');
GitVersionFinder(await gitDir, baseSha: 'HEAD');
print('Running for all packages that have uncommitted changes\n');
// _changesRequireFullTest is deliberately not used here, as this flag is
// intended for use in CI to re-test packages changed by
Expand Down Expand Up @@ -473,10 +483,12 @@ abstract class PackageCommand extends Command<void> {
///
/// Throws tool exit if [gitDir] nor root directory is a git directory.
Future<GitVersionFinder> retrieveVersionFinder() async {
final String baseSha = getStringArg(_baseShaArg);
final String? baseSha = getNullableStringArg(_baseShaArg);
final String? baseBranch =
baseSha == null ? getNullableStringArg(_baseBranchArg) : null;

final GitVersionFinder gitVersionFinder =
GitVersionFinder(await gitDir, baseSha);
final GitVersionFinder gitVersionFinder = GitVersionFinder(await gitDir,
baseSha: baseSha, baseBranch: baseBranch);
return gitVersionFinder;
}

Expand Down
37 changes: 31 additions & 6 deletions script/tool/test/common/git_version_finder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ void main() {
});

test('No git diff should result no files changed', () async {
final GitVersionFinder finder = GitVersionFinder(gitDir, 'some base sha');
final GitVersionFinder finder =
GitVersionFinder(gitDir, baseSha: 'some base sha');
final List<String> changedFiles = await finder.getChangedFiles();

expect(changedFiles, isEmpty);
Expand All @@ -49,7 +50,8 @@ void main() {
file1/file1.cc
file2/file2.cc
''';
final GitVersionFinder finder = GitVersionFinder(gitDir, 'some base sha');
final GitVersionFinder finder =
GitVersionFinder(gitDir, baseSha: 'some base sha');
final List<String> changedFiles = await finder.getChangedFiles();

expect(changedFiles, equals(<String>['file1/file1.cc', 'file2/file2.cc']));
Expand All @@ -60,7 +62,8 @@ file2/file2.cc
file1/pubspec.yaml
file2/file2.cc
''';
final GitVersionFinder finder = GitVersionFinder(gitDir, 'some base sha');
final GitVersionFinder finder =
GitVersionFinder(gitDir, baseSha: 'some base sha');
final List<String> changedFiles = await finder.getChangedPubSpecs();

expect(changedFiles, equals(<String>['file1/pubspec.yaml']));
Expand All @@ -73,8 +76,28 @@ file1/pubspec.yaml
file2/file2.cc
''';

final GitVersionFinder finder = GitVersionFinder(gitDir, null);
final GitVersionFinder finder = GitVersionFinder(gitDir);
await finder.getChangedFiles();
verify(gitDir.runCommand(
<String>['merge-base', '--fork-point', 'FETCH_HEAD', 'HEAD'],
throwOnError: false));
verify(gitDir.runCommand(
<String>['diff', '--name-only', mergeBaseResponse, 'HEAD']));
});

test('uses correct base branch to find base sha if specified', () async {
mergeBaseResponse = 'shaqwiueroaaidf12312jnadf123nd';
gitDiffResponse = '''
file1/pubspec.yaml
file2/file2.cc
''';

final GitVersionFinder finder =
GitVersionFinder(gitDir, baseBranch: 'upstream/main');
await finder.getChangedFiles();
verify(gitDir.runCommand(
<String>['merge-base', '--fork-point', 'upstream/main', 'HEAD'],
throwOnError: false));
verify(gitDir.runCommand(
<String>['diff', '--name-only', mergeBaseResponse, 'HEAD']));
});
Expand All @@ -85,7 +108,8 @@ file2/file2.cc
file1/pubspec.yaml
file2/file2.cc
''';
final GitVersionFinder finder = GitVersionFinder(gitDir, customBaseSha);
final GitVersionFinder finder =
GitVersionFinder(gitDir, baseSha: customBaseSha);
await finder.getChangedFiles();
verify(gitDir
.runCommand(<String>['diff', '--name-only', customBaseSha, 'HEAD']));
Expand All @@ -97,7 +121,8 @@ file2/file2.cc
file1/pubspec.yaml
file2/file2.cc
''';
final GitVersionFinder finder = GitVersionFinder(gitDir, customBaseSha);
final GitVersionFinder finder =
GitVersionFinder(gitDir, baseSha: customBaseSha);
await finder.getChangedFiles(includeUncommitted: true);
// The call should not have HEAD as a final argument like the default diff.
verify(gitDir.runCommand(<String>['diff', '--name-only', customBaseSha]));
Expand Down