#1071 carnage fixes [1088]

[squidgetx]

resolves #1071

[orenyk]

This should probably be 'CAS Username', but otherwise this looks good!

[squidgetx]

The prompt_field method prints username for us aleady so printf'ing "CAS " ends up prompting "CAS Username" :)

[orenyk]

That's awesome, great work!

[orenyk]

One inline comment, squash / merge when ready!

[orenyk]

ha you merged before me :-P

[squidgetx]

On another note it looks like there were actually two places 'NetID' needed to be replaced with 'CAS', so it makes more sense wrt git that the following happened.

• @orenyk fixed rubocop locally
• I fixed one of the two prompts
• I squashed and force pushed, negating the rubocop fixes

[orenyk]

aaaaaaaaaaand this is why I don't like force pushing :-P

[mnquintana]

I don't see how the squash / force push would negate the rubocop fixes. Like you force pushed before @orenyk pushed the rubocop fixes?

[squidgetx]

I imagine I wouldve force pushed after oren pushed the rubocop changes:

• we both have local copies of the branch
• oren pushes rubocop changes to remote
• I rebase off master without pulling from remote first
• I force push and the remote branch gets overwritten

[orenyk]

Yea, I was going to say he probably just forgot to pull in my changes before rebasing.

[squidgetx]

The rebase/squash flow does make it a lot easier to follow commit history. I think we can stick with it, we just have to (really severely) discourage force pushing once a branch is open to review and changes by other people, or at least encourage pulling before pushing

[orenyk]

The problem is that there are two times when you'd need to rebase / force push. The first would be before review (when I would say you shouldn't squash commits since that history has value during the review process) and then the second after review is complete to squash / deal with any changes that have occurred on master. We could use the workflow labels ('Needs Review' and 'Reviewed') for that; every time you are rebasing new changes on top of master, you have to put it through review, and once it's been labeled 'Reviewed' you should pull and squash rebase. Does that make sense?

[squidgetx]

Yeah that's good I think. I think you also only need to rebase before review if significant changes have gone into master that you want in your feature branch anyway, correct?

[orenyk]

Yea, you'd only need to rebase if there would be a merge conflict (e.g. if you'd need to git merge master before GitHub would automatically merge).

[mnquintana]

This rebase issue would only ever really be a problem for us though, right? (ie. reservations maintainers)

The only reason this cropped up is because @orenyk pushed changes directly to @squidgetx's branch, which we can do because we all have push access. Maybe the more bulletproof solution is to never push changes directly to someone else's branch, but rather, make a PR to get your changes merged into it. This way there's no chance of there being any changes at origin that the branch author is unaware of. What do you think?

[orenyk]

Oooh that's a good point. It's a bit more convoluted but you're right, if we're trying to be rigorous about keeping track of changes we should only allow one person to commit directly to a given branch. We just need to make sure that branch 'owners' know to check for PR's on their branches.

[mnquintana]

Of course now that I've written that I thought of an exception – it would be nice to collaborate on #1056 on the same branch. 😕

[orenyk]

Hmmm... we could, just using PR's instead of worrying about merge conflicts and force pushes. Also, during development you can use git merges since they'll be squashed anyway during the final rebase. So how's this: git merge during development and review as much as you want, and then you should perform a single git rebase -i and squash all commits followed by a git push --force after review but before merging the PR. I think that should solve all of our issues.