[flutter_tools] Make flutter upgrade only work with standard remotes

[royarg02]

This PR makes flutter upgrade to only work with "standard remotes", i.e., either https://github.com/flutter/flutter.git(or the SSH remote git@github.com:flutter/flutter.git) or the one set as the FLUTTER_GIT_URL environment variable.

Otherwise running flutter upgrade makes the tool throw an error and exit(The .git suffix of either of the standard URLs can be used without).

Error messages

The tool will display different error messages on different non-ideal user setups:

• The user is tracking a different remote other than https://github.com/flutter/flutter.git or git@github.com:flutter/flutter.git and having FLUTTER_GIT_URL unset:

Unable to upgrade Flutter: The Flutter SDK is tracking a non-standard remote
"git@github.com:RoyARG02/flutter.git".
Set the environment variable "FLUTTER_GIT_URL" to "git@github.com:RoyARG02/flutter.git", and retry. Alternatively,
re-install Flutter by going to https://flutter.dev/docs/get-started/install.
If this is intentional, it is recommended to use "git" directly to keep Flutter SDK up-to date.

• FLUTTER_GIT_URL is set to different url than the tracking remote:

Unable to upgrade Flutter: The Flutter SDK is tracking "git@github.com:RoyARG02/flutter.git" but "FLUTTER_GIT_URL"
is set to "https://github.com/flutter/flutter.git".
Either remove "FLUTTER_GIT_URL" from the environment or set "FLUTTER_GIT_URL" to
"git@github.com:RoyARG02/flutter.git", and retry. Alternatively, re-install Flutter by going to
https://flutter.dev/docs/get-started/install.
If this is intentional, it is recommended to use "git" directly to keep Flutter SDK up-to date.

• FlutterVersion.repositoryUrl is null (The Flutter tool couldn't determine the upstream repository):

Unable to upgrade Flutter: The tool could not determine the url of the remote upstream which is currently being
tracked by the SDK.
Re-install Flutter by going to https://flutter.dev/docs/get-started/install.

Additional fixes

Additionally, replaces the git commands with flutter channel upon detached HEAD state and clarifies the error message upon missing upstream for current branch.

Related Issues

Fixes #78591.
Fixes #49713.
Fixes #79366.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
• All existing and new tests are passing.

[jmagman]

Might be able to close #49713 as well. I mean I don't know why that happened, but it would have failed earlier...

[royarg02]

#49713 seems very similar to the use case of #79366.

[royarg02]

This PR could fix multiple open issues relating to flutter upgrade (#49713, #78591, #59697, #79366), but I've converted it to draft for now, as I'm having a hard time on how to accommodate all of them. Here's my issue(and some of my possible solutions):

As the error message is depending on flutterVersion.repositoryUrl and flutterVersion.channel, it will produce bogus messages:

Your local copy of Flutter is tracking an unsupported remote "null".
To use the unofficial remote, set the environment variable "FLUTTER_GIT_URL" to "null",
or to use the official remote, run "git remote add origin https://github.com/flutter/flutter" and "git branch
--set-upstream-to=origin/unknown" if remote "origin" exists ...

Having to throwToolExit at the beginning of the upgrade workflow makes these throwToolExits

flutter/packages/flutter_tools/lib/src/commands/upgrade.dart

Lines 247 to 252 in 8158b47

	throwToolExit(
	'You are not currently on a release branch. Use git to '
	'check out an official branch (\'stable\', \'beta\', \'dev\', or \'master\') '
	'and retry, for example:\n'
	' git checkout stable'
	);

and

flutter/packages/flutter_tools/lib/src/commands/upgrade.dart

Lines 254 to 257 in 8158b47

	throwToolExit(
	'Unable to upgrade Flutter: no origin repository configured. '
	'Run \'git remote add origin '
	'https://github.com/flutter/flutter\' in $workingDirectory');

completely useless; flutterVersion.repositoryUrl will always provide null values in either case.

The possible solution is to include this message in FetchLatestVersion after it has been determined that the HEAD points to a branch having an upstream.

flutter/packages/flutter_tools/lib/src/commands/upgrade.dart

Lines 228 to 263 in 8158b47

	Future<FlutterVersion> fetchLatestVersion() async {
	String revision;
	try {
	// Fetch upstream branch's commits and tags
	await globals.processUtils.run(
	<String>['git', 'fetch', '--tags'],
	throwOnError: true,
	workingDirectory: workingDirectory,
	);
	// '@{u}' means upstream HEAD
	final RunResult result = await globals.processUtils.run(
	<String>[ 'git', 'rev-parse', '--verify', '@{u}'],
	throwOnError: true,
	workingDirectory: workingDirectory,
	);
	revision = result.stdout.trim();
	} on Exception catch (e) {
	final String errorString = e.toString();
	if (errorString.contains('fatal: HEAD does not point to a branch')) {
	throwToolExit(
	'You are not currently on a release branch. Use git to '
	'check out an official branch (\'stable\', \'beta\', \'dev\', or \'master\') '
	'and retry, for example:\n'
	' git checkout stable'
	);
	} else if (errorString.contains('fatal: no upstream configured for branch')) {
	throwToolExit(
	'Unable to upgrade Flutter: no origin repository configured. '
	'Run \'git remote add origin '
	'https://github.com/flutter/flutter\' in $workingDirectory');
	} else {
	throwToolExit(errorString);
	}
	}
	return FlutterVersion(workingDirectory: workingDirectory, frameworkRevision: revision);
	}

Somewhere after line 261.

This can be summarized as:

fetchOriginTags();
if (headPointsToBranch) {
  if (upstreamIsConfigured) {
    if (! upstreamIsSupported) {
     throwToolExit(...);
    }
  } else {
  // similarly throwToolExit below for failed cases

But this has a caveat: it still fetches from the upstream, whether supported or not, which in my opinion is completely unnecessary.

We could make it to fetch tags only if we determine that the upstream is supported:

if (headPointsToBranch) {
  if (upstreamIsConfigured) {
    if (upstreamIsSupported) {
      fetchOriginTags();
    } else {
      throwToolExit(...);
    }
  } else {
  // similarly throwToolExit below for failed cases
continueUpgrade();

But this will fetch after the HEAD of the upstream is obtained, in the case the upstream is properly configured.

Additionally, making this error message dependent on FlutterVersion means fetchLatestVersion would somehow have to obtain an instance, possibly representing the local version. This would result in the change of tests.

[christopherfujino]

This looks like a good change! Two changes requested.

[christopherfujino]

Can we consolidate this getter and the one in //flutter_tools/lib/src/version.dart by moving them both into //flutter_tools/lib/src/globals.dart, where it would look something like this:

String get flutterGit => platform.environment['FLUTTER_GIT_URL'] ?? 'https://github.com/flutter/flutter.git';

[royarg02]

Will do.

[royarg02]

It would be better to make this change in a different PR after this one lands.
Opened #79568.

[christopherfujino]

As the error message is depending on flutterVersion.repositoryUrl and flutterVersion.channel, it will produce bogus messages:

Your local copy of Flutter is tracking an unsupported remote "null".
To use the unofficial remote, set the environment variable "FLUTTER_GIT_URL" to "null",
or to use the official remote, run "git remote add origin https://github.com/flutter/flutter" and "git branch
--set-upstream-to=origin/unknown" if remote "origin" exists ...

So we're getting null because there isn't a slash returned from git rev-parse --abbrev-ref --symbolic @{u} https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/version.dart#L107. I'm guessing this is because the repo is currently in a detached HEAD state (repo is not currently on a branch). What if we specifically handle this case (where repositoryUrl == null) in UpgradeCommand, with a message like, "Could not detect a tracking branch, are you in a detached HEAD state?"

[royarg02]

As the error message is depending on flutterVersion.repositoryUrl and flutterVersion.channel, it will produce bogus messages:

Your local copy of Flutter is tracking an unsupported remote "null".
To use the unofficial remote, set the environment variable "FLUTTER_GIT_URL" to "null",
or to use the official remote, run "git remote add origin https://github.com/flutter/flutter" and "git branch
--set-upstream-to=origin/unknown" if remote "origin" exists ...

So we're getting null because there isn't a slash returned from git rev-parse --abbrev-ref --symbolic @{u} https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/version.dart#L107. I'm guessing this is because the repo is currently in a detached HEAD state (repo is not currently on a branch). What if we specifically handle this case (where repositoryUrl == null) in UpgradeCommand, with a message like, "Could not detect a tracking branch, are you in a detached HEAD state?"

Isn't this covered by

flutter/packages/flutter_tools/lib/src/commands/upgrade.dart

Lines 247 to 252 in 8158b47

	throwToolExit(
	'You are not currently on a release branch. Use git to '
	'check out an official branch (\'stable\', \'beta\', \'dev\', or \'master\') '
	'and retry, for example:\n'
	' git checkout stable'
	);

we just need to add some lines addressing detached HEAD state.

[royarg02]

I've decided to go with the first approach of #79372 (comment)

The possible solution is to include this message in FetchLatestVersion after it has been determined that the HEAD points to a branch having an upstream.

Fetching the unsupported remote is something I'm OK to live with.

[christopherfujino]

As the error message is depending on flutterVersion.repositoryUrl and flutterVersion.channel, it will produce bogus messages:

Your local copy of Flutter is tracking an unsupported remote "null".
To use the unofficial remote, set the environment variable "FLUTTER_GIT_URL" to "null",
or to use the official remote, run "git remote add origin https://github.com/flutter/flutter" and "git branch
--set-upstream-to=origin/unknown" if remote "origin" exists ...

So we're getting null because there isn't a slash returned from git rev-parse --abbrev-ref --symbolic @{u} https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/version.dart#L107. I'm guessing this is because the repo is currently in a detached HEAD state (repo is not currently on a branch). What if we specifically handle this case (where repositoryUrl == null) in UpgradeCommand, with a message like, "Could not detect a tracking branch, are you in a detached HEAD state?"

Isn't this covered by

flutter/packages/flutter_tools/lib/src/commands/upgrade.dart

Lines 247 to 252 in 8158b47

	throwToolExit(
	'You are not currently on a release branch. Use git to '
	'check out an official branch (\'stable\', \'beta\', \'dev\', or \'master\') '
	'and retry, for example:\n'
	' git checkout stable'
	);

we just need to add some lines addressing detached HEAD state.

Doesn't that check happen after the new one you added in this PR?

[royarg02]

@christopherfujino I would suggest you to not review this PR for now (I've marked it as draft). I'm currently in the process of writing tests for a different version of this PR. Will ping you to review when I'm done with testing on my end and satisfied with its state.

[royarg02]

I'm not sure this error message was accurately being displayed if the user was not on a release branch (branches other than kOfficialChannels). I changed this to only warn about detached HEAD states.

[christopherfujino]

If the user is on a detached HEAD, they would also not be on a release branch right? Even if this message is more specific, I doubt most casual git users understand what a detached HEAD state is, when what is relevant to their use of Flutter is that they are not on an official channel. What do you think?

[christopherfujino]

Also, while we're at it, we can advise the user to use flutter channel stable, rather than doing a git checkout directly.

[royarg02]

Done.

[royarg02]

Hi @christopherfujino, PR is ready for review.
I replied to the changes you requested, LMK what you think about them.

[christopherfujino]

The code here looks good. I just have some requested changes for exact messaging we communicate to the user, and a few points I wasn't sure on.

Overall, I really appreciate you contributing to this part of the codebase, as you know it is confusing and rife with edge cases. Overall, we're trying to nudge users away from directly manipulating their git repositories via the CLI, because if they get into a weird state they will file a bug, and it will be difficult for us to diagnose. And if they're a power user, they'll disregard our recommendations and hopefully they will be able to get their repo back into a good state.

[christopherfujino]

rather than listing them out, can you refer the reader to look up the _flutterGit getter, in case we ever change that function but forget to update the comment.

[royarg02]

Done.

[christopherfujino]

Although the details here are true, they may change later. You can future-proof this comment by merely saying "Using flutter upgrade is not supported from a non-standard remote."

[royarg02]

Done

[christopherfujino]

Can you rename this verifyStandardRemote()? When I read check I think that it's going to return a value, rather than potentially exit.

[royarg02]

Done.

[christopherfujino]

From experience, I try to avoid suggesting git commands to users via the CLI tool. There are many ways for a user to accidentally misconfigure their repo if they're not familiar with git, and these become very difficult to decipher issues.

Instead, can you refer the user to this page on the website to re-install Flutter: https://flutter.dev/docs/get-started/install

[royarg02]

Should I include this link to the toolExit message for detached HEAD and missing upstream as well?

[christopherfujino]

Yes, that sounds good!

[christopherfujino]

Suggested change

	Future<FlutterVersion> fetchLatestVersion({@required FlutterVersion localVersion}) async {
	Future<FlutterVersion> fetchLatestVersion({
	@required FlutterVersion localVersion,
	}) async {

[royarg02]

Done.

[christopherfujino]

If the user is on a detached HEAD, they would also not be on a release branch right? Even if this message is more specific, I doubt most casual git users understand what a detached HEAD state is, when what is relevant to their use of Flutter is that they are not on an official channel. What do you think?

[christopherfujino]

Also, while we're at it, we can advise the user to use flutter channel stable, rather than doing a git checkout directly.

[christopherfujino]

note that this method could return '[user-branch]'

[christopherfujino]

this could also throw if they're in a detached head state, but tool exiting with the message "fatal: HEAD does not point to a branch" will probably be more confusing than the original tool exit message.

[royarg02]

This toolExit won't show for detached HEAD states. The toolExit above takes care of that. This is only if the local branch isn't tracking anything(Similar to the use case of #79366).

[christopherfujino]

ahh, yeah, you're right.

[royarg02]

If any user sets the tracking repository as https://github.com/flutter/flutter(no ".git" suffix) and having FLUTTER_GIT_URL unset, this will fail. How do I handle this case?

[christopherfujino]

Good catch, and that's annoying. You could write a utility function and pass both localVersion.repositoryUrl and _flutterGit through it before comparing. Something like:

String stripDotGit(String s) {
  RegExp pattern = RegExp(r'(.*)(\.git)$');
  final RegExpMatch match = pattern.firstMatch(s);
  if (match == null) {
    return s;
  }
  return match.group(1);
}

As with any regex, we'd want to unit test this :)

[royarg02]

I've split the error messages for specific use cases depending on whether FLUTTER_GIT_URL is set or not:

• FLUTTER_GIT_URL is set

Unable to upgrade Flutter: The Flutter SDK is tracking "https://github.com/RoyARG02/flutter.git" but
"FLUTTER_GIT_URL" is set to "https://github.com/flutter/flutter.git".
Either remove "FLUTTER_GIT_URL" from the environment or set "FLUTTER_GIT_URL" to
"https://github.com/RoyARG02/flutter.git", and retry.

As an alternative, if you are okay with losing local changes you made to the SDK, re-install Flutter by
going to https://flutter.dev/docs/get-started/install.

• FLUTTER_GIT_URL is unset

Unable to upgrade Flutter: The Flutter SDK is tracking a non-standard remote
"https://github.com/RoyARG02/flutter.git".
  - To use the current remote, set the environment variable "FLUTTER_GIT_URL" to
"https://github.com/RoyARG02/flutter.git", and retry.
  - To use the standard remote, change the url of the tracking remote to
"https://github.com/flutter/flutter.git", and retry, for example, to change the url of the remote "origin", run:

      git remote set-url origin https://github.com/flutter/flutter.git

As an alternative, if you are okay with losing local changes you made to the SDK, re-install Flutter by
going to https://flutter.dev/docs/get-started/install.

WDYT?

[royarg02]

@christopherfujino PTAL

[fluttergithubbot]

This pull request is not suitable for automatic merging in its current state.

• This pull request has changes requested by @christopherfujino. Please resolve those before re-applying the label.

[christopherfujino]

LGTM

[christopherfujino]

Reverting to unblock the plugins tree #83373