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

Code in rti.c made available as library #174

Merged
merged 13 commits into from
Mar 6, 2023
Merged

Code in rti.c made available as library #174

merged 13 commits into from
Mar 6, 2023

Conversation

Jakio815
Copy link
Collaborator

@Jakio815 Jakio815 commented Mar 6, 2023

No description provided.

@Jakio815
Copy link
Collaborator Author

Jakio815 commented Mar 6, 2023

I think it will work if the main branch reverts to commit 4f78b42a which was the commit just before I started pushing. I also merged #169 together what you merged right after my PR. Would you please have a look? @lhstrh.

Tell me if I shouldn't merge #169 together. I'll revert it.

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

This PR also seems to loose the history. Note that rti_lib.c is entirely authored by @Jakio815 even though it is mostly copied from rti.c.

@lhstrh
Copy link
Member

lhstrh commented Mar 6, 2023

The trick is to use git mv for the file that will carry most of the history and create new ones for the smaller ones... Whatever gets factored out, however, will not of course loose history, there is no way around that.

@lhstrh
Copy link
Member

lhstrh commented Mar 6, 2023

@Jakio815 did you see the link that @erlingrj shared about doing this?

@edwardalee
Copy link
Contributor

You can preserve the history even on the smaller file, although it may not be worth the trouble if it is small enough. See:

https://devblogs.microsoft.com/oldnewthing/20190916-00/?p=102892

@Jakio815
Copy link
Collaborator Author

Jakio815 commented Mar 6, 2023

I'm not sure what you are mentioning. Are you mentioning this?

And actually when I checkout to this branch, I see the file history. I followed the link above which professor @edwardalee mentioned. I see that the Git's Files Changed tap seems to change everything, but in my VSCode GitLens extension, it has the original history. I'm not sure if this is the right way. Would you please check? @lhstrh

@edwardalee
Copy link
Contributor

Oh, interesting. Yes, it could be just that the history doesn't show up in Files Changed. I hadn't realized that, but it makes sense.

@Jakio815
Copy link
Collaborator Author

Jakio815 commented Mar 6, 2023

I also see that the contributors for rti_lib.c is only me. I'm not sure how I can check this on the Github web.

@Jakio815
Copy link
Collaborator Author

Jakio815 commented Mar 6, 2023

I found out that if you tap 'Blame', you could see the history of every lines. Also, if you tap the 'History' you could check the history below saying that it was renamed.

Sorry for bothering everyone with this problem.

@lhstrh lhstrh changed the title Redo rti leaving history. Code in rti.c made available as library Mar 6, 2023
@lhstrh
Copy link
Member

lhstrh commented Mar 6, 2023

I reverted my merge commit of the previous PR, so once approved, you can just go ahead and hit the merge button. Please mark it "ready for review" so that @edwardalee can approve it. Thanks!

@Jakio815 Jakio815 marked this pull request as ready for review March 6, 2023 07:54
Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for fixing this.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@lhstrh lhstrh merged commit a267815 into main Mar 6, 2023
@lhstrh lhstrh deleted the redo-rti branch March 6, 2023 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants