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

Add commit message lints #3545

Open
kchibisov opened this issue Mar 1, 2024 · 16 comments · May be fixed by #3587
Open

Add commit message lints #3545

kchibisov opened this issue Mar 1, 2024 · 16 comments · May be fixed by #3587
Labels
S - maintenance Repaying technical debt

Comments

@kchibisov
Copy link
Member

kchibisov commented Mar 1, 2024

Unfortunatelly, it's impossible for everyone to at least wrap their commit messages. After numerous warnings folks can't still do such things.

Thus, the squash merge will be disabled, and the only option will be rebase with the commit lint enabled for each commit in the series.

@kchibisov kchibisov added P - high Vital to have S - maintenance Repaying technical debt labels Mar 1, 2024
@madsmtm
Copy link
Member

madsmtm commented Mar 1, 2024

I'm probably one of the offenders here, but disagree that this is a priority, IMO just get your editor / terminal / git to wrap the lines for you if you want them wrapped.

That said, I'm fine with CI jobs or something to fix this, but I believe there is great value in not telling contributors to rebase, and squashing instead.

@kchibisov
Copy link
Member Author

That said, I'm fine with CI jobs or something to fix this, but I believe there is great value in not telling contributors to rebase, and squashing instead.

You can do that yourself if you want to, I can't be bothered to deal with these commit messages anymore.

I told about that numerous times during the year(I think), so no it's a priority, since mostly all commit messages expept for the ones from @notgull and me are formatted like garbage.

@daxpedda
Copy link
Member

daxpedda commented Mar 1, 2024

I agree that this is not a priority. It would also be good to agree on something and write it down somewhere. Personally I couldn't care less about wrapping commit messages (to a degree ofc), but if we agree on something and write it down somewhere I'm happy to oblige.

I don't like squashing either, fast-forward-merging anybody?

@daxpedda daxpedda added the C - nominated Nominated for discussion in the next meeting label Mar 1, 2024
@kchibisov
Copy link
Member Author

I mean there's nothing to agree, it's a standard to wrap body at 72 and title around 50 chars. I also asked to strip garbage github adds, because I can't be bothered to read any of that and it just leaves a feeling that no one cares to write anything useful in the end of the day.

image

Or like that

image

@madsmtm
Copy link
Member

madsmtm commented Mar 1, 2024

All I see is an issue with your terminal window being too wide / git not automatically wrapping the message to a readable width like any normal text-box in a modern system otherwise does ;).

Or do you have other complaints with the commit messages?

@kchibisov
Copy link
Member Author

All I see is an issue with your terminal window being too wide / git not automatically wrapping the message to a readable width like any normal text-box in a modern system otherwise does ;).

Ofc, just read yours and then e.g. John's or mine. Or read the alacritty's messages. It's feels like you throw garbage which no-one cares about and then dance around the tree because your thing got merged.

And no, it's not an issue with my system in particular, you should just follow what git uses by default and at least put a bit of effort into writing about the work you've done.

@daxpedda
Copy link
Member

daxpedda commented Mar 1, 2024

It's feels like you throw garbage which no-one cares about and then dance around the tree because your thing got merged.

I don't believe this language is appropriate. Please remain on topic.


what git uses by default

I'm not aware of git imposing any rules by default? What are you referring to here?

@kchibisov
Copy link
Member Author

kchibisov commented Mar 1, 2024

I don't believe this language is appropriate. Please remain on topic.

I was polite about this during the year. Mostly all my suggestions were ignored and they don't want to improve on purpose resulting in lower code quality in the project, because git commit messages are part of that. Given that I can't stand it anymore I take it personally and treat as a threat it as power abuse since 99% of the time I rewrite their commit messages for them,
which they still not appreciate and continue to merge arbitrary garbage. If they can't behave, I'll make the rules stricter for everyone, including good participants.

I'm not aware of git imposing any rules by default? What are you referring to here?

50 and 72 wrapping is de-facto wrapping in git. I don't force 50, you can still write a bit more in title, by like 10 chars, but general aim is 50.

@kchibisov
Copy link
Member Author

I've disabled squash and forced explicit review on each PR.

@madsmtm
Copy link
Member

madsmtm commented Mar 7, 2024

I've set up a Git commit-msg hook now.

@madsmtm madsmtm closed this as completed Mar 7, 2024
@madsmtm madsmtm reopened this Mar 7, 2024
@kchibisov
Copy link
Member Author

I've set up a Git commit-msg hook now.

For yourself or what do you mean by that?

@madsmtm
Copy link
Member

madsmtm commented Mar 8, 2024

Yeah, for myself

@madsmtm
Copy link
Member

madsmtm commented Mar 15, 2024

I'll try to set up a CI job that checks this, so that we can re-enable squash merging

@kchibisov
Copy link
Member Author

The issue is that CI won't be able to prevent squash merging, because you write commit messages on github, thus CI won't run for it.

The only way CI for commits can work is for rebase itself. The squash will be re-enabled eventually.

@daxpedda
Copy link
Member

I think we should agree on some rules in #3549 first.

@kchibisov
Copy link
Member Author

I think the point is to lint on width at least. The lints for commit messages are present in e.g. systemd, etc, iirc.

@madsmtm madsmtm linked a pull request Mar 15, 2024 that will close this issue
@kchibisov kchibisov removed P - high Vital to have C - nominated Nominated for discussion in the next meeting labels Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - maintenance Repaying technical debt
Development

Successfully merging a pull request may close this issue.

3 participants