docs(CONTRIBUTING): add warning about forced push

[dmitriz]

Add warning about possible consequences of a forced push

[gkalpak]

It's the same PR (you probably meant "previous commit").
I thought GitHub will retain the comments ? They just won't be shown in-line in your new commit.

I think this warning will be more confusing than helpful. We are advising people to force-push, but the warn them (as if it were a bad/dangrous thing).

I think we should either change the instructions to suggest creating a new commit with the changes (as I believe is suggested by @petebacondarwin) or not warn them against the inconveniences of force-pushing.

WDYT, @petebacondarwin ?

[dmitriz]

You are right, it should be "in your PR".

I would probably not advice force push at all.
I understand that it creates some duplication but losing people's comments is worse.

[dmitriz]

For instance, if I force push right now, this discussion will disappear!

[gkalpak]

I don't think it disappears, but it will be difficult to correlate with the new commit.

[dmitriz]

So I've tried to experiment and indeed it didn't disappear, it did happen with other repos but I can't reproduce it here, must be some Angular magic :)

So misregard the PR, sorry.

[dmitriz]

Could not reproduce the warning problem.

[petebacondarwin]

If you have some comments attached to specific changes that are removed by the forced push then I believe those comments could be lost. But general comments on the PR are not lost.

[dmitriz]

Thanks for confirming!

I had a use case when I've added a Gulpfile into a PR to Angular UI Bootstrap, and they asked me to remove it. When I removed it from the history via rebase -i and forced pushed, all remarks by the team on my important code were gone! Even though they were not related to the removed Gulpfile.

It was an unpleasant surprise that I'd rather not have.

[gkalpak]

I still believe that the comments are retained under a "some-user commented on an outdated diff some time ago" remark (and you can so the comment thread by clicking on "Show outdated diff", but maybe the comments are indeed lost under certain circumstances.
Maybe when the comments are on the commit itself, not the PR, they are lost ? I don't know.

In any case, @petebacondarwin, do you think we should change the instructions and stop suggesting a force-push ?

[dmitriz]

So I have tested it here and sadly have to confirm that comments CAN get lost!

Here is the scenario:

• You send your PR
• Someone leaves in-code comment in the commit A
• They also ask you to squash into fewer commits
• You rebase and squash commit A
• You force push
• The comments are gone!

The reason I did not reproduce it here, because there is only one commit that I could not squash.

So yes - the use case is common and real - and people's comments CAN get lost for good :(

Thank you, Github! :(

[gkalpak]

@dmitriz, were the comments on the PR or on the commits themselves ?
Do you happen to know the SHAs of the initial commits.

Just, ooc...

[dmitriz]

Yes, I have actually restored and updated it with more precise information, it is ready to be merged:

https://github.com/dmitriz/angular.js/tree/patch-3

Did it not update the PR?

[dmitriz]

As it does not let me reopen after force-push, I'm resubmitting as another PR.

[gkalpak]

Sorry, I didn't get the answers (if there were any 😛):

Were the comments on the PR or on the commits themselves ?
What are the SHAs of the initial (unsquashed) commits (if you still have them) ?

[dmitriz]

I can see your comments when clicking on the "Show outdated diff"

They lead to the updated test-commit I've made: https://github.com/dmitriz/angular.js/commit/84bba65cab954e9018467e4b4d4ec43621a2eff9

The original one must be findable but I'm not git-ninja enough to know how to find it. Do you need it for any reason?

[petebacondarwin]

Shall we just change the recommended practice to "just add new commits to your PR - no force push required - as we can squash commits when we merge". And then move on from this discussion.
I like this idea as actually it is quite informative to be able to go back to old PRs and see how they evolved.

[gkalpak]

@dmitriz: I meant from your test PR. Nevermind, I was just curiou if you can find the cooments using the SHAs. I don't actually need them for something useful (just ooc).

@petebacondarwin: If that's what we'd like people to do then that's what we should write 😄

[petebacondarwin]

I think so. I always end up rebasing and modifying PR commits when merging in any case - so it is not saving us much by them force pushing (in the majority of cases).

[dmitriz]

@gkalpak I've found how to do it, here is the output if of any help to you:

git reflog | grep "CONTRIBUT"

55e379d HEAD@{0}: commit: docs(CONTRIBUTING): add warning about forced push
0fc28f3 HEAD@{3}: pull --rebase upstream master: docs(CONTRIBUTING): add warning about forced push
4920b4d HEAD@{6}: pull --rebase upstream master: docs(CONTRIBUTING): add warning about forced push
c5c5ba9 HEAD@{9}: rebase -i (squash): docs(CONTRIBUTING): add warning about forced push
cd45794 HEAD@{17}: commit: docs(CONTRIBUTING): add warning about forced push
84bba65 HEAD@{28}: rebase -i (squash): docs(CONTRIBUTING): add warning about forced push
1ce5cae HEAD@{45}: rebase -i (squash): docs(CONTRIBUTING): add warning about forced push
27613ab HEAD@{49}: commit: docs(CONTRIBUTING): add warning about forced push
adcb18e HEAD@{52}: pull --rebase upstream master: docs(CONTRIBUTING): add warning about forced push