Update to Angular v9 automatically checks in the result!

[DeborahK]

🐞 Bug report

Command (mark with an x)

- [ ] new
- [ ] build
- [ ] serve
- [ ] test
- [ ] e2e
- [ ] generate
- [ ] add
- [x ] update
- [ ] lint
- [ ] xi18n
- [ ] run
- [ ] config
- [ ] help
- [ ] version
- [ ] doc

Is this a regression?

Yes, the previous version in which this bug was not present was: ....

Description

When running: `ng update @angular/cli @angular/core --next --allow-dirty`

It automatically checked in the result to my git repo. This is definitely NOT desired. In many use cases, this may be a "Trial" without a desire to check in. In other use cases, this may be a key dev environment that the developers would want to try out before committing.

In any case, this should NOT automatically check in without at least asking first.

UPDATE: In looking into this further, it does THREE commits during the update process.

🔬 Minimal Reproduction

1. Run: ng update @angular/cli @angular/core --next --allow-dirty

2. Check the Git change list, all changes are check in.

🔥 Exception or Error

No error

🌍 Your Environment

Was 8.1.3 prior to the update.

Now: 
Angular CLI: 9.0.0-rc.0
Node: 10.16.0
OS: win32 x64
Angular: 9.0.0-rc.0
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-rc.0
@angular-devkit/build-angular     0.900.0-rc.0
@angular-devkit/build-optimizer   0.900.0-rc.0
@angular-devkit/build-webpack     0.900.0-rc.0
@angular-devkit/core              9.0.0-rc.0
@angular-devkit/schematics        9.0.0-rc.0
@ngtools/webpack                  9.0.0-rc.0
@schematics/angular               9.0.0-rc.0
@schematics/update                0.900.0-rc.0
rxjs                              6.5.3
typescript                        3.6.4
webpack                           4.41.2

Anything else relevant?

@vikram @StephenFluin

[filipesilva]

This is especially tough because users that are not very proficient with git will have a hard time removing these commits.

[clydin]

This behavior can be disabled via the --skip-commits/-C option.

I believe the concept here was that an update is similar to the development of a PR for a feature or bug fix. A user would create a branch and then make a set of changes. After completion, adjustments would be made to ensure everything is working and then the results would be squashed/rebased and then pushed upstream for review and eventual merge.

The commit per migration also has the advantage of allowing the changes from each migration to be viewed in isolation and altered with a higher degree of confidence, if needed. Explanations for the migrations are also contained within the corresponding commit messages. Some migrations can result in hundreds or more small changes across the entire codebase.

cc @IgorMinar

[filipesilva]

We don't tell users to make a branch in the update guide though, I think.

I also wonder how these commits interact with projects that have git hooks for the commit messages. One of the projects I'm testing updates on uses husky for that.

[DeborahK]

This may work well for some users and use cases ... but not all developers using Angular are well experienced with git or have other standard processes and procedures.

Plus with the automatic check-in, it is much more difficult to see which files that the update modified.

Also, since it does at least 3 check ins, it makes it even harder for a developer to undo this.

We should NOT be doing this without explicit permission. Make the flag --skip commits true by default and/or ask a question. Then those devs with experience and who work the way you are expecting can CHOSE to allow automatic check ins.

Thanks!

[clydin]

That was just an example use case; although one that probably should be considered a recommended practice.

As to git hooks, they intentionally don't interact: git commit --no-verify -m "${message}"

[filipesilva]

Related to #14600