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

Update firmware and kernel #356

Merged
merged 2 commits into from
Dec 15, 2018
Merged

Update firmware and kernel #356

merged 2 commits into from
Dec 15, 2018

Conversation

pepijndevos
Copy link

@jenkins-ag
Copy link

Can one of the admins verify this patch?

Copy link
Owner

@agherzan agherzan left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. Everything looks ok functional-wise but we will need to amend a little the commit logs. Can you please do that following http://www.openembedded.org/wiki/Commit_Patch_Message_Guidelines instructions? You can find tons of examples in git log.

@agherzan
Copy link
Owner

@pepijndevos And one more thing: cleanup the merge commit as well (rebase the branch). Thanks once again.

@pepijndevos
Copy link
Author

Sorry, I'm working towards a tight deadline, so I don't have the bandwidth to go back and learn a bunch of git fu to clean up the mess at this time. I hope you understand. I can do it properly in a few weeks, but it hardly seems worth it for this trivial change.

@agherzan
Copy link
Owner

We do consider them worth it @pepijndevos hench why we still adhere to those guidelines. I'll keep this PR open unless we include it in another one.

@pepijndevos
Copy link
Author

I woke up with git commands in my head, so there you go.

@agherzan
Copy link
Owner

@pepijndevos thanks.

@agherzan
Copy link
Owner

@jenkins-ag ok to test

Copy link
Owner

@agherzan agherzan left a comment

Choose a reason for hiding this comment

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

Just after approving it I realized you missed the signed of by line.

@pepijndevos
Copy link
Author

I give up. It's harder to get the commits right than it's for either of us to increment a version number.

@agherzan
Copy link
Owner

It's not really as complicated as you think. A simple git log shows you the format in all the past commits and as well I provided detailed information in the documented linked above.

@hhromic
Copy link
Contributor

hhromic commented Dec 15, 2018

@agherzan despite not liking the attitude of the author of this PR, maybe we can start suggesting to use git commit -s [...] so git itself adds the signed-off tag automatically in the commit logs. If you agree I can update the guidelines PR accordingly before merging.

Source: https://git-scm.com/docs/git-commit#git-commit--s

@kraj
Copy link
Collaborator

kraj commented Dec 15, 2018

@agherzan despite not liking the attitude of the author of this PR, maybe we can start suggesting to use git commit -s [...] so git itself adds the signed-off tag automatically in the commit logs. If you agree I can update the guidelines PR accordingly before merging.

Please do, although this has a downside where we will force people to sign off on
changes which originated elsewhere so make it clear also what SOB means

Source: https://git-scm.com/docs/git-commit#git-commit--s

Pepijn de Vos added 2 commits December 15, 2018 09:12
This fixes raspberrypi/firmware#1051

Signed-off-by: Pepijn de Vos <pepijndevos@gmail.com>
To match 20181211 firmware

Signed-off-by: Pepijn de Vos <pepijndevos@gmail.com>
@pepijndevos
Copy link
Author

Ah, so I'm not completely out of my mind. I was under the impression the signed-off tag was a git feature, and that it would happen semi-automatically.

For the record, I do feel kinda bad about not doing the commit properly, and if it were a more substantial change, I'd be a lot more willing to spend time to do it correctly. This PR could have been an issue "hey, firmware is out of date", and it'd take anyone familiar with the project guidelines under a minute to change it. Instead I pushed the code I already had anyway. I wasn't prepared for all this extra bookkeeping.

@agherzan
Copy link
Owner

@pepijndevos I'm tempted to get into "commit guidelines are not commit size dependent" but I'm sure you know this already. We appreciate and welcome any contribution but they need to adhere, informed or not, to some guidelines to make out life, the maintainers', easier.

@hhromic @kraj It is actually mentioned in the git command examples for creating email patches. And if that is not enough again, the second paragraph in the oe contribution guidelines mention sob and link this requirement to kernel's 5.4 section where this is explained in detail. In anyway, I do get that this looks like annoying metadata and usually when I join an opensource project I have two options:

  1. I blindly copy the format of a past commit (and tweak it for my needs) because I don't have the time to dig into all the details of commit guidelines or I don't really care
  2. I am truly interested in the project and I dig into details of these guidelines to understand them and compose a well informed commit log

Both are equally OK in my opinion and I don't think that any information in our own documentation would have helped. That being said though, I'm OK in adding as much as possible to try to avoid these kind of discussions.

@agherzan agherzan merged commit 1aa973c into agherzan:master Dec 15, 2018
@pepijndevos
Copy link
Author

pepijndevos commented Dec 15, 2018 via email

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.

5 participants