Last step of publish is selecting the wrong merge target

[chadwhitacre]

After a release we merge the release branch back to the base branch:

craft/src/commands/publish.ts

Lines 331 to 363 in 78d2ca9

	/**
	* Determines the closest branch we can merge to from the current checkout
	* Adapted from https://stackoverflow.com/a/55238339/90297
	* @param git our local Git client
	* @param remoteName Name of the remote to query for the default branch
	* @returns
	*/
	async function getMergeTarget(
	git: SimpleGit,
	remoteName: string
	): Promise<string> {
	const logOutput = await git.raw(
	'log',
	'--decorate',
	'--simplify-by-decoration',
	'--oneline'
	);
	logger.debug('Trying to find merge target:');
	logger.trace(logOutput);
	const branchName =
	stripRemoteName(
	logOutput
	.match(/^[\da-f]+ \((?!HEAD |tag: )([^)]+)\)/m)?.[1]
	?.split(',', 1)?.[0],
	remoteName
	) || (await getDefaultBranch(git, remoteName));
	if (!branchName) {
	throw new Error('Cannot determine where to merge to!');
	}
	return branchName;
	}

Unfortunately it looks like our algorithm for determining which branch to merge into if not explicitly provided is wrong. The past two sentry releases have merged back into the wrong branch:

https://github.com/getsentry/publish/runs/4218472116?check_suite_focus=true#step:8:123 → getsentry/sentry@9ae170c

https://github.com/getsentry/publish/runs/3909141316?check_suite_focus=true#step:8:666 →
getsentry/sentry@b15920f

The result is that (at least) the CHANGES file in the sentry repo has not been updated since 21.9.0 ... we're missing all of the new changelog-y goodness!

https://github.com/getsentry/sentry/blob/master/CHANGES

[chadwhitacre]

I'm pretty sure this is because master moves so quickly.

[chadwhitacre]

There is a CLI option. Can we use that for sentry? 🧐

craft/src/commands/publish.ts

Lines 75 to 80 in 95677da

	.option('merge-target', {
	alias: 'm',
	description:
	'Target branch to merge into. Uses the default branch from GitHub as a fallback',
	type: 'string',
	})

[chadwhitacre]

Hrm, not easily. 🤔

[BYK]

@chadwhitacre you can probably make "Uses the default branch from GitHub as a fallback" case the default and move the current logic into -m auto?

[chadwhitacre]

@BYK That sounds more reasonable to me. What was the use-case for the auto strategy in the first place? Are there other craft users/projects that depend on that? 🤔

[BYK]

@BYK That sounds more reasonable to me. What was the use-case for the auto strategy in the first place? Are there other craft users/projects that depend on that? 🤔

There are some projects that release from custom branches for RC, beta, and hotfix releases and this was to address those use cases.

An alternative would be to add this as an option to the config file instead of a CLI argument. This way projects can easily control the behavior?

[chadwhitacre]

An alternative would be to add this as an option to the config file instead of a CLI argument. This way projects can easily control the behavior?

That's more along the lines of what I was thinking. Basically any option that craft accepts on the command line should be available through the config file.

[BYK]

Basically any option that craft accepts on the command line should be available through the config file.

I dare you to make that happen 🤣 Remember how #235 went?

[BYK]

Thinking of this again, since the issue is master moving too quickly, I think another pull right before trying to determine the target should fix the issue mostly?

[jan-auer]

We just hit this same bug with sentry-native, see getsentry/publish#694 (comment)

The culprit here is the following:

• There's a branch ref pointing directly to an old commit on the master branch
• The release branch has been branched off after that commit
• Craft uses git log --decorate --simplify-by-decoration --oneline, which doesn't see the branch point
• It finds that old ref and uses it as a base branch.

I haven't had the chance to look into this much further. We would basically need to find the branching point rather than searching for refs in the log. Same when the base branch moves on in the meanwhile, which is what happened for the Sentry release.

[jan-auer]

I might have a solution to this. Depending on whether I can get to it, I will try to push up a PR and test this locally.

For background: The current approach searches the current branches history for branch annotations. This assumes that the base branch has not moved in the meanwhile, e.g. master still points to a direct ancestor.

My idea is based on https://stackoverflow.com/a/17843908. Instead, use git show-branch to search for the nearest ancestor commit (marked with a *) that resides in a different branch. Example borrowed from the stack overflow answer with the relevant ancestor commit marked:

 A---B---D <-main (e.g. on version 2.0)
      \
       \
        C---E*---I <-maintenance/1.x
             \
              \
               F---G---H <-release/1.7.0

[jan-auer]

@BYK is that something you remember considering initially, or any gotchas on your end before i go down the rabbit hole?

[BYK]

@BYK is that something you remember considering initially, or any gotchas on your end before i go down the rabbit hole?

@jan-auer I chose the current method in https://stackoverflow.com/a/55238339/90297 due to the reasons they listed under "Why did not show-branch work for me". I did not encounter them but the last case kind of looked possible (develop vs master for hotfixes). I think we can try the other approach and see if it actually gives us any headaches.

[jan-auer]

Spent a little bit of time playing with this and I believe the best overall solution would be to remove this feature from craft and instead always merge into the default branch when no merge base is given via CLI. We can make this a "branch to merge into (leave empty for default branch)" argument on the release action, which should be bullet proof.

---

Next, let's address all the points from the stackoverflow answer explaining why show-branch wouldn't work:

• detached HEAD – including detached head case means replacing grep '\*' \ for grep '!' \ – and that is just the beginning of all the troubles

By definition, we always have a release branch that we need to track to its parent branch. Detached head does not make sense for our problem statement. That said, given we can write procedural code, it would be rather straight forward to use the extended ref (e.g. development~2) to track down the correct branch. This is only a problem if you need to do all of this in the shell.

• running the script on master and develop results in develop and "" respectively

That is precisely what we want. And here again, we would never run this on master.

• branches on master branch (hotfix/ branches) end up with the develop as a parent since their closest master branch parent was marked with ! instead of * for a reason.

I believe that this was a bug in parsing the output through grep / awk. ! is only used to list current heads in the output, but not for the commit list after ---. Likely a combination of them trying to get this working in detached head state, which we don't want or need.

---

If we want to keep this functionality around, then I think the show-branch based solution should be more reliable for us. This way, one doesn't have to deal with old heads (branches pointing to a master commit) and tags.

There's only a single case that I was able to find where show-branch outputs the wrong branch: If the latest master commit is also pointed to by a branch without own commits. For example:

 A---B---D <- master, feat/something
      \
       \
        C <- release/1.2.3

In that case, show-branch would choose feat/something rather than master.

[BYK]

There's only a single case that I was able to find where show-branch outputs the wrong branch: If the latest master commit is also pointed to by a branch without own commits. For example:

I think we can easily detect this case and prefer master or just throw an exception saying "ambitious branch to merge to".

I believe the best overall solution would be to remove this feature from craft and instead always merge into the default branch when no merge base is given via CLI

I think this is not very intuitive. We may at least want to require an explicit flag to be passed when/if we detect a "non-standard" branch.

[chadwhitacre]

when/if we detect a "non-standard" branch.

I thought @jan-auer's suggestion is that we no longer do any branch detection. We either merge to the default branch or to an explicitly specified branch. Basically #327 (comment) but s/-m auto/-m mybranch. No?

keep this [automatic branch detection] functionality around

👎

remove this feature from craft and instead always merge into the default branch when no merge base is given via CLI

👍

[chadwhitacre]

For the record #245 is where we introduced merge branch detection. I'm a little confused how this relates to #223.

[jan-auer]

This happened again, unfortunately, and I didn't even notice it until I accidentally stumbled across the changelog entry missing. Craft selects the wrong target and there's not even a big warning or anything. Given that we merge into the main branch in almost every case, this is another +1 for me to back this feature out and make it manual. At least then it's intentional.

[bruno-garcia]

We hit this on sentry-unity. 👍 here to help prioritize.

[chadwhitacre]

Bummer. I'll probably hit this today with self-hosted 22.2.0. :-/

This is an action item on INC-95. @jan-auer Once I get through getsentry/publish#666 I would be happy to look into this as well if need be.