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

Relax commit check #2807

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Relax commit check #2807

merged 1 commit into from
Jun 27, 2019

Conversation

MrAnno
Copy link
Collaborator

@MrAnno MrAnno commented Jun 26, 2019

In my opinion, the following subjects should be accepted:

  Revert "cfg-parser: add aliases for yesno"
  geoip2: extend msg with ip
  Light: Remove unused Light dependencies
  autotools,cmake: add -Wundef
  $(grep): fixed possible segfault on parse errors
  elastic-v2: fixed debian packaging
  network(): added grammar skeleton and plugin registration
  version.sh: unset CDPATH
  dbld/images/centos{6,7}: add Python/pip support

@kira-syslogng
Copy link
Contributor

Build SUCCESS

alltilla
alltilla previously approved these changes Jun 26, 2019

subject=$(git show -s --format='%s' $commit)

if echo "$subject" | egrep -q '^Revert '; then
revert_commit=1
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want the "revert" commits to be excluded ?
The revert commit does not really have any details that distinguish it from other commits (message can be altered any point).
By including the Signed-off in the commit message, does not really mean anything at all. (git at least does not specify, and I could not find any statement in the syslog-ng repository either, except that you must include in every commit)

Despite the above I like Signed-Off as you can easily give credit with it to other people.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 22 revert commits in this repository, only 3 of them contain signoff lines. We can start using signoffs there too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks.

@@ -40,7 +40,7 @@ git log --no-merges --pretty="%H" $commit_range -- | (

subject=$(git show -s --format='%s' $commit)

if ! echo "$subject" | egrep -q '^[a-z/_-]+:'; then
if ! echo "$subject" | egrep -q '^[a-zA-Z0-9/(){}$,._-]+:'; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was kinda happy with the strict version, especially not to include upper case letters :) I see the value in the rest modification. As this is a very opinionated topic, it is not a change request.

The following subjects are accepted now:

  Revert "cfg-parser: add aliases for yesno"
  geoip2: extend msg with ip
  Light: Remove unused Light dependencies
  autotools,cmake: add -Wundef
  $(grep): fixed possible segfault on parse errors
  elastic-v2: fixed debian packaging
  network(): added grammar skeleton and plugin registration
  version.sh: unset CDPATH
  tests/copyright: set copyright policy for new dbld related files
  dbld/images/centos{6,7}: add Python/pip support

Signed-off-by: László Várady <laszlo.varady@balabit.com>
@MrAnno MrAnno force-pushed the relax-commit-check branch from 8afd601 to cc2960d Compare June 26, 2019 16:13
@kira-syslogng
Copy link
Contributor

Build SUCCESS

@furiel
Copy link
Collaborator

furiel commented Jun 27, 2019

I have a commit like autotools, cmake: add -Wundef that fails now. What is the guidance here? Should I split to two commits and have autotools: add -Wundef and cmake: add -Wundef? Or should I make up a module name, like build: add -Wundef?

@Kokan
Copy link
Collaborator

Kokan commented Jun 27, 2019

@furiel according to this PR (see the examples): autotools,cmake: add -Wundef (removing the space)
Personally I would prefer the space removal in this case, but sure if we find some case that fails it could be additionally included. (let this be an incremental process to improve the check script)

@mitzkia
Copy link
Contributor

mitzkia commented Jun 27, 2019

Update:

  • Sorry, correct me if I am wrong. I assumed that there is some problem with the subject:
    "Light: Remove unused Light dependencies"

Sorry, but the "Light" starts with capital "L", I would not like change it to: "l"
What is the main goal with this rule?
I understand that it would be easier to read if the subjects are consequent.

@MrAnno
Copy link
Collaborator Author

MrAnno commented Jun 27, 2019

I personally prefer lowercase subjects, but I've added uppercase letters to the commit-checker because I don't think we should be that strict.

@mitzkia
Copy link
Contributor

mitzkia commented Jun 27, 2019

@MrAnno Thanks for the clarification. I am approving this.

@mitzkia mitzkia merged commit 1742b11 into syslog-ng:master Jun 27, 2019
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.

6 participants