fix(changelog): changelog generator now handles merge commits better#1319
Merged
jsternberg merged 1 commit intomasterfrom Jun 3, 2019
Merged
fix(changelog): changelog generator now handles merge commits better#1319jsternberg merged 1 commit intomasterfrom
jsternberg merged 1 commit intomasterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1319 +/- ##
=======================================
Coverage 53.93% 53.93%
=======================================
Files 178 178
Lines 27198 27198
=======================================
Hits 14668 14668
Misses 11104 11104
Partials 1426 1426Continue to review full report at Codecov.
|
a591bd3 to
b12df87
Compare
wolffcm
reviewed
Jun 3, 2019
| commit, log = &prevCommit, prevLog | ||
| } else { | ||
| log.Printf("invalid commit message %v: %v", commit.Hash, err) | ||
| if currCommit.Committer.When.Before(prevCommit.Committer.When) { |
There was a problem hiding this comment.
Is this When field that actual time that the commit was added relative to other commits?
I have seen dates in the git log that are definitely from way before the commit was added, which must be due to git commit --amend or force push.
Contributor
Author
There was a problem hiding this comment.
It is. Git is... not very smart about this. It's what the actual merge-base command does. Here's the code that sets the comparison:
https://github.com/git/git/blob/master/commit-reach.c#L38
The generation that's referenced comes from a commit graph that isn't written to disk unless you explicitly write it to disk so it's almost always empty and isn't exposed by the go-git library.
The generation number would be much better and exactly what would be needed, but it doesn't exist for practical purposes. Git code just seems to use commit dates as a way of trying to reduce the amount of time for practical purposes, but it's obviously not very reliable.
When the git log contained merges, the changelog generator didn't
necessarily work correctly. First, it would attempt to parse the message
in merge commits which was always wrong and always printed an error
message. Now, the changelog generator is told to just skip those commits
and move on since merge commits won't contain the message we're looking
for anyway.
The second happened with patch releases and feature branches that were
under work for more than a release. If a release was performed on the
maint branch and another commit was done on master before merging
maint back to master, the tree could be constructed in a way where the
previous version commit could not be clearly found such as in this
example from the `v0.31.0` release.
* 3204dd2 ci: add codecov.io support to flux (#1273)
* f0b6850 Merge branch 'maint'
|\
| * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (#1284)
* | 2cce6fe feat(universe): support for dynamic queries (#1284)
|/
* 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (#1216)
If the changelog generator encountered this pattern, it was undefined
exactly how it would behave. In this specific scenario, it parsed every
commit on the left branch going back to the root commit and then found
the previous commit on the second side of the branch. The opposite could
happen too and then it would have missed adding the commit from the
other branch.
The changelog generator has now been modified to find the merge base
before it attempts to read the log to prevent searching back through the
entire history. In the above situations, `v0.29.0` would be found and
act as a final constraint for parent commits.
The changelog parser has also been modified to use a breadth-first
search instead of depth-first search and it does not use the `Log()`
method from the go-git library. This is because the go-git library does
not have a way to stop at a specific commit and stop looking at parent
commits. We now limit the breadth-first search of commits to either the
merge base or the previous version commit which should be the same
commit if the release process was followed correctly. A breadth-first
search is used to ensure we get all commit messages from any merges and
don't skip any commits because we reached the terminating commit first.
b12df87 to
1520e0d
Compare
Contributor
Author
|
I updated the code with comments to explain what I was doing. |
wolffcm
approved these changes
Jun 3, 2019
davidgs
pushed a commit
to davidgs/flux
that referenced
this pull request
Aug 20, 2019
…nfluxdata#1319) When the git log contained merges, the changelog generator didn't necessarily work correctly. First, it would attempt to parse the message in merge commits which was always wrong and always printed an error message. Now, the changelog generator is told to just skip those commits and move on since merge commits won't contain the message we're looking for anyway. The second happened with patch releases and feature branches that were under work for more than a release. If a release was performed on the maint branch and another commit was done on master before merging maint back to master, the tree could be constructed in a way where the previous version commit could not be clearly found such as in this example from the `v0.31.0` release. * 3204dd2 ci: add codecov.io support to flux (influxdata#1273) * f0b6850 Merge branch 'maint' |\ | * ec37117 (tag: v0.30.0) feat(universe): support for dynamic queries (influxdata#1284) * | 2cce6fe feat(universe): support for dynamic queries (influxdata#1284) |/ * 32fb2fb (tag: v0.29.0) fix(execute): properly use RefCount to reference count tables (influxdata#1216) If the changelog generator encountered this pattern, it was undefined exactly how it would behave. In this specific scenario, it parsed every commit on the left branch going back to the root commit and then found the previous commit on the second side of the branch. The opposite could happen too and then it would have missed adding the commit from the other branch. The changelog generator has now been modified to find the merge base before it attempts to read the log to prevent searching back through the entire history. In the above situations, `v0.29.0` would be found and act as a final constraint for parent commits. The changelog parser has also been modified to use a breadth-first search instead of depth-first search and it does not use the `Log()` method from the go-git library. This is because the go-git library does not have a way to stop at a specific commit and stop looking at parent commits. We now limit the breadth-first search of commits to either the merge base or the previous version commit which should be the same commit if the release process was followed correctly. A breadth-first search is used to ensure we get all commit messages from any merges and don't skip any commits because we reached the terminating commit first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When the git log contained merges, the changelog generator didn't
necessarily work correctly. First, it would attempt to parse the message
in merge commits which was always wrong and always printed an error
message. Now, the changelog generator is told to just skip those commits
and move on since merge commits won't contain the message we're looking
for anyway.
The second happened with patch releases and feature branches that were
under work for more than a release. If a release was performed on the
maint branch and another commit was done on master before merging
maint back to master, the tree could be constructed in a way where the
previous version commit could not be clearly found such as in this
example from the
v0.31.0release.If the changelog generator encountered this pattern, it was undefined
exactly how it would behave. In this specific scenario, it parsed every
commit on the left branch going back to the root commit and then found
the previous commit on the second side of the branch. The opposite could
happen too and then it would have missed adding the commit from the
other branch.
The changelog generator has now been modified to find the merge base
before it attempts to read the log to prevent searching back through the
entire history. In the above situations,
v0.29.0would be found andact as a final constraint for parent commits.
The changelog parser has also been modified to use a breadth-first
search instead of depth-first search and it does not use the
Log()method from the go-git library. This is because the go-git library does
not have a way to stop at a specific commit and stop looking at parent
commits. We now limit the breadth-first search of commits to either the
merge base or the previous version commit which should be the same
commit if the release process was followed correctly. A breadth-first
search is used to ensure we get all commit messages from any merges and
don't skip any commits because we reached the terminating commit first.
Fixes #1304.