🏗 Count all cherry picks when setting the version

[estherkim]

Use case: When cherry-picking, sometimes a non-master commit needs to be cherry-picked to resolve merge conflicts.

What happens: A new AMP version is created instead of appending the existing AMP version with the # of cherry-picked commits.

This is because it grabs the wrong timestamp with HEAD~{# of master cherry-picked commits}

Example: amp-release-2012301722002, which has 2 master commits and 2 non-master commits, was created as release 2101152047000 instead of the expected 2012301722004 (or 2012301722002 depending on how we want to count).

Fix: Count both master and non-master commits when setting the AMP version.

[amp-owners-bot]

Hey @danielrozenberg! These files were changed:

build-system/compile/internal-version.js

[danielrozenberg]

Some musings: we had these lines originally to forcefully indicate non-cherry-pick commits. The real issue we have here is that it's (afaiu) programmatically impossible to tell whether a modified commit is a indeed cherry pick or not

This resolved the issue at hand, but returns the original issue that this was supposed to fix, which is that we wanted to have an indicator for when somebody is trying to release a potentially unauthorized version. Not sure how to resolve both of these issues together

[danielrozenberg]

lol yes I'm talking above about exactly this TODO 😅

[jridgewell]

This effectively reverts #29051, which we introduced because some cherry-picks were not being properly picked up as cherry-picks. I can't remember the scenarios where this happens, but I think this is going to actually cause more incorrectly-versioned issues.

[estherkim]

An example would really help here. I haven't seen any instances where git cherry doesn't get the right # of commits.

However I have seen git cherry annotate a + when it should be a -. Example:

% git cherry upstream/master upstream/amp-release-2101300534005
- 0baf344463931f5b0ed5ca8518375bb71d4960a4
- 6a07a7ac04adf2b2d514455312e1c835499bf2e4
+ d0406efd81b2ffbc1adef4d1c208d1e74d92477e
- e3d046e0e17ba71e4fe748130037cc699deebe2a
- 84c4ecb7d4828fc55d9d301c5dee7defd723252

Is that the rare mis-categorization you're talking about?

[jridgewell]

(For reference, upstream/master is currently 4ebe517)

Yah, that's a perfect example. I have no idea why d0406ef isn't being marked as a proper cherry-pick here.

[estherkim]

Hmm does the + matter though? We just need the number of commits the branch is ahead.

[jridgewell]

Hmm does the + matter though? We just need the number of commits the branch is ahead.

Yes. For instance, if I check out one of my PR branches and run git cherry, I see a + SHA:

$ git cherry upstream/master blurry-placeholder-no-js
+ de03f31cb5fca2ed5365beb3e151d5e88953906c

This commit doesn't exist on master (PR isn't merged yet), so it shouldn't count as a cherry-pick.

[estherkim]

That's okay IMO because

1. this adds support for non-master commits to fix the example I added in the PR description, and
2. a dev branch isn't really a use case; this is for release automation to run on release branches

[jridgewell]

I think we need to immediately follow up with a PR that verifies the commits exist on master, but this is good enough to unblock us.