Skip to content

Conversation

ykaire-qti
Copy link

Fixing crashes due to empty filenames
Adding reset delay

@andersson
Copy link
Collaborator

Please restructure your changes such that each commit does one thing only and give this a good commit message describing the problem that it is solving. Also add signed-off-by (git commit -s) to your commits.

@ykaire-qti
Copy link
Author

@andersson, i think i restructured the commits. please let me know if there's any other changes i should make

@andersson
Copy link
Collaborator

Please use "git rebase -i" rather than "git merge" to clean up the commits; in particular you have one commit that comments out parts of the Makefile and then another commit that reverts that patch. Once we merge this into the project, the git history wouldn't make sense.

For the "prevent crash when filename is NULL", I would prefer if you write a short commit message describing why filename could be NULL. (see how I do for my changes in the git log)

For the reset timeout, I think this would better be a separate PR, that we can merge independently. I would like the commit message to cover why it's needed (if you know, otherwise it should say that you don't know, but testing shows we need a value). And I would like the commit message to cover why 10 is a good value. Isn't 10 a long time? Why can't it be 1?
But, please use "git rebase -i" to drop that change from this branch and move it to another branch and open a new PR for that part.

I think the two changes as such looks good, I just want to understand why we're making them and for future readers of the code and git log to have this recorded.

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.

2 participants