ros2 / tinydir_vendor

Vendor package for providing tinydir within a cmake package
Apache License 2.0
0 stars 7 forks source link

[Dashing] Update tinydir_cmakelists.txt #19

Closed nuclearsandwich closed 4 years ago

nuclearsandwich commented 4 years ago

Backport of #10 for Dashing

nuclearsandwich commented 4 years ago
nuclearsandwich commented 4 years ago

I somehow managed to merge this PR onto master instead of Dashing. The CI results are valid as this branch was checked out and tested against a Dashing repos file so I am going to cherry-pick this commit directly onto the Dashing branch. @emersonknapp I think it's also reasonable to snip this commit off of master with the following git operations:

git checkout master
git reset --hard 54b3fd1
git push origin master --force

Are you opposed and do you have an alternative suggestion?

emersonknapp commented 4 years ago

Hmm... I don't like force pushing to shared development branches (even ones that others are unlikely to be touching, like this package). I realize that the commit was unnecessary, but it also isn't harmful, so my instinct would be to just leave it.

Edit: I think I misread the situation. We like this change, but the commit message is incorrect, right? I still don't feel good about a force push - maybe a followup empty commit on master saying "Amend message of previous commit: etc"?

nuclearsandwich commented 4 years ago

I realize that the commit was unnecessary, but it also isn't harmful, so my instinct would be to just leave it.

I think the only fallout will be that any future rebases or other history altering operations will freak out about the empty commit f1ee71b on master and require either removing it or explicitly allowing the empty commit to persist.

Edit: I think I misread the situation. We like this change, but the commit message is incorrect, right? I still don't feel good about a force push - maybe a followup empty commit on master saying "Amend message of previous commit: etc"?

emersonknapp commented 4 years ago

Yeah, let's just leave it. Do you have any ideas for mechanisms to help minimize the possibility for that particular human error in the future? It definitely happens sometimes, just good to brainstorm if we can make it harder to make mistakes.

nuclearsandwich commented 4 years ago

It definitely happens sometimes, just good to brainstorm if we can make it harder to make mistakes.

Nothing that doesn't smack of over-automation to me. Automating the backport process using a bot or running a GitHub action that checks every PR title for a rosdistro tag to match a branch. It's something that reviewers of backport PRs should try to spot check during review. (Which is not an attempt to shift or distribute blame, I also need to check that when I review backports.)

emersonknapp commented 4 years ago

Ok, I agree that would be overkill for now. We'll have to be vigilant! I'll be looking more carefully at the destination branch next time I review a backport.