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

Merge dpJudas's renderslices branch #1356

Merged
merged 7 commits into from Mar 26, 2021
Merged

Conversation

XaserAcheron
Copy link
Contributor

dpJudas wrote:

This should generally be ready for merging now.

Graf Zahl wrote:

There is no PR so there's nothing to merge as far as I am concerned.

Here's a PR, then. Figured I may as well open one instead of waiting for y'all to finish waiting on each other. :P

@madame-rachelle madame-rachelle merged commit a5cba1a into ZDoom:master Mar 26, 2021
@coelckers
Copy link
Member

@madame-rachelle Was it really necessary to squash such a large change into a single commit? This forfeits any chance to analyze potential problems that may come with it.

@drfrag666
Copy link
Contributor

It's pretty much the same i think, most changes are in the first commit and the others are bugfixes. Another issue is that XaserAcheron appears as the author too when he just created the PR.

@madame-rachelle
Copy link
Collaborator

I can revert the massive commit if need be and then drop in the changeset individually but I'd really prefer to do it without altering histories.

madame-rachelle added a commit that referenced this pull request Mar 26, 2021
@XaserAcheron
Copy link
Contributor Author

Frankly, I was a bit surprised that GitHub let me make the PR anyway (and then signed my name on the dang merge :P ). Guess I accidentally hit my mischief quota for the day. :P

@drfrag666
Copy link
Contributor

The PR itself was fine, i think it's a side effect of the merge when you choose one of the three options.

@madame-rachelle
Copy link
Collaborator

Yeah that is why Graf didn't like the way I did it. I normally use squash merges most of the time because a bunch of PR's come in like "- did this" "- reverted previous commit" "- oops" and we try and get rid of those. It's a pretty common request that the PR's get squashed. I prefer having a commit history to be quite honest, but at the same time, it's too easy to have a lot of useless fluff commits - and those are bad enough from the internal developers, having them come from externally too is far from ideal.

@drfrag666
Copy link
Contributor

I normally use the third option, rebase and merge.

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.

None yet

5 participants