Compare with last tag on the same branch as the target branch

[greg0ire]

Given `doctrine/migrations` has tags 3.0.0, 3.0.1 and 2.3.0
 When I open a PR against 2.3.x (for instance)
 Then the tool should detect that the last minor version is 2.3.0

Right now it detects that the last version is… 3.0.0, which is confusing.

I believe this could be done by detected what branch the commit being tested is based on. On most CI systems, the commit being tested is the result of a merge between the target branch and the last commit on PR. This means that the detection can be performed easily by looking at the parent commits, provided they are fetched, which implies doing stuff like this (example with Github Actions):

      - name: "Checkout"
        uses: "actions/checkout@v2"
        with:
          fetch-depth: 2

[Ocramius]

Hmm, yeah, implementing this sounds like a good addition, but here's another scenario that IMO should be considered:

1. I fork off 2.0.x (and I keep the branch name)
2. I send a PR against upstream 3.0.x
3. ???

It's a bit more blurry in this case, since the tool doesn't know about the branching mechanism at all.

We can most certainly try to make a convention, but it needs to be quite verbose in the output.

[greg0ire]

I'm not sure I see the issue, that must be because I don't know how the tool works well enough.
At step 3. , the CI is effectively run on a commit that is a merge of 3.0.x , and the other one is some commit that has 2.0.x in its ancestry. we follow 3.0.x ancestry chain until we find a tag, say 3.0.2, that we detect as the last minor. We then proceed as usual.

[Ocramius]

At step 3. , the CI is effectively run on a commit that is a merge of 3.0.x

That's only if your CI simulates merges

[greg0ire]

That's only if your CI simulates merges

Doesn't it always do that? By the way, we say it's the CI, but I think it is in fact Github that simulates the merge before passing it on to the actual CI system (Travis, Github Actions, Appveyor, etc.).

the docs about pull request events seems to say it always does that since GITHUB_SHA is the last merge commit on the GITHUB_REF branch.

[Ocramius]

In pull requests, the commit is verified AFAIK, not the merge.

Anyway, it is still an assumption, not the actual reality: the CLI tool should generally stay agnostic from this assumption.

Getting the closest tagged ancestor from a specific patch seems valid, but is a problem if the merge target has different ancestors.

[greg0ire]

In pull requests, the commit is verified AFAIK, not the merge.

Please check, I have always observed the opposite, the commit under test I have observed is always refs/pull/:prNumber/merge. For instance for doctrine/doctrine-website#373, it is https://github.com/doctrine/doctrine-website/commits/97c2c24a733855ce754ebfe8aca6b108b87b12bb The only situation I can think of where that happens is when there are conflicts and you cannot perform the merge.

Anyway, it is still an assumption, not the actual reality: the CLI tool should generally stay agnostic from this assumption.

True. What about this: if the verified commit is not a merge commit, behave as usual, otherwise use the behavior described in this PR. The contributor may still push a merge commit to their branch, but in that case I think the behavior would still be sensible: the tool would probably pick a parent and follow the ancestry until a tag.

UPD: an extra check might be to compare the commit under test with the last commit on the branch. If it's a merge commit and it is not the same commit, then the assumption that it is a test merge is reasonable IMO.

Another way to this properly could be to make the behavior configurable somehow.

[Ocramius]

As it currently stands, the list of tags is sorted and iterated upon:

BackwardCompatibilityCheck/src/Git/GetVersionCollectionFromGitRepository.php

Lines 30 to 32 in 2eae3f7

	$output = (new Process(['git', 'tag', '-l'], $checkedOutRepository->__toString()))
	->mustRun()
	->getOutput();

What we could do is do a --from-closest-released-ancestor=$SHA1 (usually HEAD), which would then proceed to search for the newest tagged ancestor.

The default behavior doesn't really consider the current checked out REF at all, unless provided.

[greg0ire]

git describe --tags --abbrev=0 $SHA1 should do the trick I think: https://stackoverflow.com/a/7261049/353612

[Ocramius]

Yes, but needs to be controlled by new parameter, which is the more complicated bit

[lcobucci]

Isn't it simpler to have each base branch using --from explicitly? It's not all pretty and automatic but reduces the potential edge-cases...

[Ocramius]

Looking back at this, I must say that --from="$(your-program-here)" is probably a healthier approach to this, given that the convention used by laminas/automatic-releases is extremely young and widely unknown

[lcobucci]

Here's an example of how to solve this using GH actions: https://github.com/lcobucci/jwt/pull/534/files#diff-828df298ab61cb562c901dd8bd15d49b8e7023ba87e3e60a6b1a06eefe252fd8

[greg0ire]

Works for me, I'm ok for closing this if you are.

[Ocramius]

Gonna close here then: example is succinct.

If you want, send a patch with docs, if relevant.