Integration Candidate 2020-03-25#52
Conversation
|
Are you planning to fix the "release table address" commit message? I don't follow why this broke... was a rebase on master missed? |
|
Nevermind, I see why it broke. |
The original changeset must have been based on a previous master before the name changes went in. It's a clean merge text-wise but really a conflict. This is precisely why we do the "integration-candidate" branch - to catch and fix this type of more subtle merge issue, before it gets to master. |
skliper
left a comment
skliper
left a comment
There was a problem hiding this comment.
Could you add context to the commit message? I assume this is intended as a FASTTRACK associated with the 2020-02-25 integration branch?
|
Nope, this wasn't fast tracked. |
|
FWIW, I would have re-done the "integration-candidate" branch in the sample_app to clean this up. One can merge with "--no-commit" and fix the non-detected conflict, then commit. This will result in cleaner history. Want me to do that? |
|
I can squash once it works. I'll fumble my way through it. Honestly I don't mind a slightly messy history if it reflects reality. |
|
I'm question "FASTTRACK" in context of the changes you are making right now that I assumed we would just review quickly as a fix to IC and push to master... or do you plan to back this change out of the IC and put it through as a new pull request (not fasttrack?) |
|
Anyways, I'll stop commenting on a work in progress :) |
|
No need to stop commenting, that's what PRs are meant for. And yes we can fast track these changes. I'll request once it passes CI or when I get stuck. What would a more detailed commit message look like? |
|
My preference is to fumble around as much as we need to in an ephemeral branch, but then clean it up for the final merge so we don't have to sort through the mess in the future. (I hate seeing "fixup" commits in the permanent history). At any rate, now I see you were referring to the log message on the original commit, ceb2aa7. This is a spot I would consider sending it back to the original author to write a better commit message. The person doing the merges shouldn't be rewriting commit messages, they should meet the guidelines to begin with. |
|
May want to manually rebase and fix the commit message this time around, but we really should start enforcing/rejecting commits at the CCB if the logs aren't meeting the guidelines |
|
Agreed, I prefer getting the commit message to meet standard prior to IC merge. There's a few examples in most of the repos... https://github.com/nasa/sample_app/commits/master |
|
I actually don't think this is a fasttrack situation.... this is simply a merge conflict that wasn't detected by the auto-merge algorithm because it has to do with semantics, not just syntax. This is where I would simply re-do the merge but manually fix the conflict, as it should have been if the merge algorithm was semantic-aware |
|
But since the commit message needs to be fixed anyway, just rebase it and fix both. Then it will cleanly merge and that solves everything |
|
I have no problem either way as long as there isn't a dangling commit without context |
|
Yep - rebase commit ceb2aa7, write a better commit log in the process that adheres to guidelines, then re-do integration-candidate with this instead. Problem solved. |
Add CFE_TBL_ReleaseAddress after access of table data.
astrogeco
force-pushed
the
integration-candidate
branch
from
7c7012d to
34321ff
Compare
|
Its OK - would have preferred to include the fix in the rebase rather than a separate commit. But it looks like it should work. |
4 participants
Describe the contribution
Fixes #19
Testing performed
See PR #48
Bundle CI - Pass
Expected behavior changes
Add CFE_TBL_ReleaseAddress after access of table data
System(s) tested on
Bundle CI - Ubuntu: Bionic
Contributor Info - All information REQUIRED for consideration of pull request
Anh Van - NASA/GSFC
Gerardo E. Cruz-Ortiz - NASA/GSFC