Release automation commits and pushes local changes

[fhd]

Environment

This happened during the Adblock Plus for Chrome/Opera/Safari 1.12.2 release, base revision was 44641c853190.

How to reproduce

1. Clone the adblockpluschrome and downloads repositories. Important: Make sure to configure different remotes than the central upstream repositories (i.e. local copies of the repositories), the release automation automatically pushes changes.
2. Make an arbitrary local change to the adblockpluschrome working directory, do not commit this.
3. Still in the adblockpluschrome working directory, run build.py -t chrome release -k adblockpluschrome.pem -k adblockplussafari.pem 1.12.2 to perform the 1.12.2 release. (The version number you use doesn't matter.)

Observed behaviour

The local changes you made in the adblockpluschrome repository will be committed along with the release commit (the message starts with Releasing Adblock Plus).

Expected behaviour

The release automation shouldn't even start when the repository is dirty. Instead it happily commits the changes and pushes them. That could lead to nasty issues.

Any untracked or uncommitted local changes should cause the release automation to print an error-message and automatically abort the process.

The user should be prompted with detected outgoing changesets, along with a question whether to continue or not:

If you proceed with the release, they will be included in the release and pushed.
Are you sure about continuing the release-process?
Please choose (yes / no):

What to test

• automatic abortion on
  • any uncommitted local change
  • any untracked local change

• list of outgoing changesets (local-only changesets)
• Abortion or or continuing of the release-automation, according to the users choice

[trev]

Changing title back - our release automation generally behaves that way, for Firefox as well.

Note that the proposed change won't solve the scenario where you already have local commits - these will be pushed as well. I guess that release automation could check for that as well.

[trev]

I'd suggest a hard abort for uncommitted changes. While in the other scenarios I can imagine answering "yes, continue with the release" occasionally, uncommitted changes which will end up being added to the "Releasing" commit just cannot be right.

[tlucas]

Replying to trev:

I'd suggest a hard abort for uncommitted changes. While in the other scenarios I can imagine answering "yes, continue with the release" occasionally, uncommitted changes which will end up being added to the "Releasing" commit just cannot be right.

I agree, changed the description accordingly

[kvas]

I've been looking at the review for this ticket and I have a proposal about making the three line "scary message" a bit more friendly and helpful:

I know that you took this from the ticket, but I'm wondering if this line and the following one actually add value. Imagine we have some outgoing commits. What the user will see is:

Detected outgoing changesets:

changeset 1
changeset 2

The above error/s has/have been detected within the repositories.
You might want to check whether this is ok or not.
Are you sure about continuing the release-process?

This is a lot of text, but it's not very informative. How about replacing the first two lines of the scary message with something like "If you proceed with the release, they will be included in the release and pushed.":

Detected outgoing changesets:

changeset 1
changeset 2

If you proceed with the release, they will be included in the release and pushed.
Are you sure about continuing the release-process?

What do you guys think?

[sebastian]

Shouldn't any local changes be committed separately (or reverted), anyway? So perhaps it would be simpler to just abort with an error message when detecting local changes, rather than prompting whether to include them into the version bump commit?

[tlucas]

Replying to sebastian:

Shouldn't any local changes be committed separately (or reverted), anyway? So perhaps it would be simpler to just abort with an error message when detecting local changes, rather than prompting whether to include them into the version bump commit?

Do you mean mean uncommitted changes or any changes (including changesets)?
In case of the latter, this would make the overall issue a little bit easier to fix.

What do you think about Vasily's proposal, regarding the error-message?

[sebastian]

Oh, I was talking about uncommitted changes, mistakenly assuming this is what this issue is about. As for outgoing commits, the warning/prompt Vasily suggested sounds good to me.

However, what would happen with uncommitted changes?

[tlucas]

Replying to sebastian:

Oh, I was talking about uncommitted changes, mistakenly assuming this is what this issue is about. As for outgoing commits, the warning/prompt Vasily suggested sounds good to me.

Okay, i will implement the message/prompt accordingly.

However, what would happen with uncommitted changes?

Taken from the description:

Any untracked or uncommitted local changes should cause the
release automation to print an error-message and automatically abort the process.

[sebastian]

I missed that. Sounds good to me.

[abpbot]

A commit referencing this issue has landed:
​Issue 4354, 4355 - handle dirty/outdated repos on release

[tlucas]

Please note: The patch introduced a regression, which is handled in #5736