Breaking change incorrectly reported for a stable package with a multi-user concurrent workflow

[dylan-bourque]

GitHub repository with your minimal reproducible example (do not leave this field blank or fill out this field with "github.com/bufbuild/buf" or we will automatically close your issue, see the instructions above!)

https://github.com/dylan-bourque/buf-breaking-repro

What's up?

The buf breaking command can incorrectly report a breaking change when different users separately make independent, non-breaking changes to a stable proto package. The linked repo demonstrates the problems.

The scenario is:
Given a stable proto package, 2 developers each create a new branch (feature1 and feature2). Each makes a non-breaking change in their branch. In the linked example the change is adding a new message.

After one new feature is merged, the developer working on the other will start to see an incorrect failure from buf breaking reporting a deleted message when comparing against the main branch in the Git remote.

> buf breaking --against "https://github.com/dylan-bourque/buf-breaking-repro.git#branch=main,subdir=proto"
proto/foo/bar/v1/foobar.proto:1:1:Previously present message "Baz" was deleted from file.

The Baz message was actually added by the other feature but a direct comparison between the main and the working branch will treat that diff as a delete. While the issue can easily be corrected by doing a forward merge (or rebase) to update the open feature branch with the new changes from main, it would be nice if this didn't require manual intervention.

Some potential solutions:

1. synthesize a temporary workspace that contains the working branch with any changes from main merged in and compare that to main
2. find the commit that the working branch was created from and translate the reference in --against to use that instead of main
3. update the comparison algorithm to do a 3-way comparison between the working branch, main and the common ancestor to eliminate changes made outside of the working branch

Option 1 was suggested by someone in the Buf Slack when I asked about this when I first noticed it a few months ago. Option 3 would be the most flexible, but I suspect also the most complicated to implement.

[emcfarlane]

Hi @dylan-bourque if this is about GitHub actions in CI you can use the pull_request GitHub event event to avoid this issue. Using this event type results in the merged state of the target branch. So this would avoid the error of this missing field.

If this is not about the CI: I don't think it's safe to alter the input . to be a merged state based on git as that would need to be applied to all commands, which it can't be. As an example, if the user was to run buf breaking --against=<git_input> which passes and then did buf push this would be a replace (not a merge) and make the breaking change in the module even though buf breaking passes. So excluding altering the current behaviour there may be something we could do based on a new input type or option to a .git input that would do this merge.

[nicksnyder]

To clarify, the context of this issue is about using the Buf CLI in CI, but without assuming any particular code host like GitHub, and also not assuming usage of the BSR.

For reasons that Ed mentioned (e.g. interactions with the BSR), I don't think this is a bug in the CLI, what it is doing is a reasonable/straightforward (and conservative) interpretation of what is being requested. That said, I think we want to support your use case too.

The current CLI already allows you to explicitly provide a ref to compare against, so you could do something like this to achieve the behavior of solution #2:

buf breaking --against "https://github.com/dylan-bourque/buf-breaking-repro.git#branch=main,subdir=proto,ref=$(git merge-base main HEAD)"

If we were to support a different behavior in the CLI, I think we would need to have a new option or flag to avoid breaking compatibility.

[dylan-bourque]

@nicksnyder I think using ref=... is a reasonable workaround but IMO it would be a better UX to provide an option to do that directly within the CLI command.

Either way, I think it would be a nice touch to at least document this scenario and call out either the option or the workaround.