Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detection of reverted commits #47

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Detection of reverted commits #47

wants to merge 4 commits into from

Conversation

shful
Copy link

@shful shful commented Apr 23, 2016

The changelog may contain commits that have been reverted.
A warning line is added on each of these commits.

Why ? Before we publish, one team member will revise and rework the changelog.
The warning aids him to not accidentially advertise a reverted feature.

A warning is added in the changelog on any commit reverted by another one.
Whether a commit is a "revert commit" is determined by a regular expression for the commit message body.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 91.647% when pulling fcb3240 on shful:master into e9759c2 on vaab:master.

@vaab
Copy link
Owner

vaab commented Apr 23, 2016

Thanks, I've read the code, and understood what this does. A test would have been welcome, as a doc. But before working on that, I suggest you to wait, because I need to think about whether it is a good idea to add this as you did.
What could bother me is that we are adding adhoc code for specifics and this has a limit: we already have too much variables to my taste. Your implementation is fine, don't get me wrong, it follows the current scheme. I'm just wondering if this is the right way to go to keep the maintenance head-ache free.

I was thinking that this type of modification should be possible via the filters (it is not currently feasible because we don't have access to the full commit object in filters, and we probably could). This would allow your whole PR code to be a single filter function, with the addition of being very easily configurable, or removed...

BTW, forcing the unrolling of the log generator is not a good idea. The generator is meant to manage huge list of commits that wouldn't fit in memory. And I don't really understand why you need to make this regex lookup at once before the big loop. Sorry if I missed anything obvious.

@shful
Copy link
Author

shful commented Apr 23, 2016

You are right; I see that GitRepos.log() guarantees reverse chronological order. I wasn't aware of that before, and I will eliminate the log unrolling.
This ordering fits well with any filters that use information from younger commits to manipulate older ones, as I did here.

I also think that a generic solution like filters is preferrable. My gut feeling is that more specific extensions are about to come.
As soon as the concept seems right, I'll add test coverage.

This avoids storing all commits in a list, as commit 2f8dd4 did.
It relies on log() returning commits in reverse chronological order.
@coveralls
Copy link

coveralls commented May 1, 2016

Coverage Status

Coverage decreased (-0.6%) to 91.566% when pulling 2dcfb8f on shful:master into e9759c2 on vaab:master.

The repository fork reason is added.
@coveralls
Copy link

coveralls commented May 27, 2016

Coverage Status

Coverage decreased (-0.6%) to 91.566% when pulling d6da1db on shful:master into e9759c2 on vaab:master.

@vaab vaab force-pushed the master branch 3 times, most recently from ce9843a to 7c6b7f1 Compare February 22, 2017 16:36
@vaab vaab force-pushed the master branch 3 times, most recently from fedee4e to b1cb854 Compare March 10, 2017 03:36
@vaab vaab force-pushed the master branch 3 times, most recently from b8bd6c6 to f53d1cc Compare March 17, 2017 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants