Skip to content

Fenced code blocks 835#25

Merged
jemerick merged 8 commits intolivio:masterfrom
elkirby:fenced-code-blocks-835
Apr 20, 2021
Merged

Fenced code blocks 835#25
jemerick merged 8 commits intolivio:masterfrom
elkirby:fenced-code-blocks-835

Conversation

@elkirby
Copy link
Copy Markdown
Contributor

@elkirby elkirby commented Apr 19, 2021

No description provided.

@elkirby elkirby force-pushed the fenced-code-blocks-835 branch 3 times, most recently from a1d6b90 to 43a61f4 Compare April 20, 2021 13:18
Copy link
Copy Markdown
Contributor

@jmichalicek jmichalicek left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's give it a full test and get it merged if all seems to work as expected.

@elkirby elkirby force-pushed the fenced-code-blocks-835 branch from 43a61f4 to 82e405c Compare April 20, 2021 13:40
@joeygrover
Copy link
Copy Markdown
Member

Hey @elkirby, you should avoid force pushing unless absolutely necessary. You seem to be force pushing the same commits so I don't know why you'd need to do that.

@elkirby
Copy link
Copy Markdown
Contributor Author

elkirby commented Apr 20, 2021

Hey @elkirby, you should avoid force pushing unless absolutely necessary. You seem to be force pushing the same commits so I don't know why you'd need to do that.

Thank you for the guidance, @joeygrover. The reason I was doing so prior was because it was a feature branch and I was using interactive rebases to keep the commit history clean, and since the commit SHAs change afterwards, a force push is necessary in order to do so. I would never force push to a main or otherwise shared branch. This documentation from Gitlab explains why force pushes are necessary when squashing or fixing up commit histories.

That being said, I am more than happy to adhere to your team's suggested git flow strategy. I only explain myself with the intention of reassuring the Livio team that I would not be intentionally reckless or otherwise cavalier with my git practices, and would not perform any commands that I did not fully understand.

@joeygrover
Copy link
Copy Markdown
Member

Ah yea wasn't looking at the hashes, just the messages. So we avoid using rebase, we always add commits or merge for a number of reasons. During a code review if you need to make changes via an additional commit from that review it is difficult to find exactly what changed if every time you push it is a force push and especially if it is rebased. Commit history "clean up" can be done via squash merge from the PR if it is out of control, but most of the time it is fine. IMO true commit history is better than visually clean. So, if possible, for Livio owned repos we would prefer that you add commits and use merge instead of rebase, and therefore avoid the necessary force push scenario you are referencing.

@jemerick jemerick merged commit eebdc53 into livio:master Apr 20, 2021
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.

4 participants