rfc 0001 process: say "do not force push" changes

[pnkfelix]

As people modify the text of their RFCs, they are sometimes force pushing changes up to github, which can cause the history of previous iterations of the rfc to be lost (which is not so bad), and it also can cause the comment history to be lost on the rfc's ticket (which is much more severe).

See e.g. discussion on PR 10

RFC 0001 should be amended to explicitly advise people to not force push changes to their forks of the rfcs repository when developing an rfc.

[adrientetar]

If people comment of the diff tab and not on the commit itself, this is a non-issue.

[bharrisau]

Is it handy to see the evolution of the RFC? Normally you would rebase and
fast-forward, maybe for this repo it is better to have one commit per
revision?

[pnkfelix]

@adrientetar said "If people comment of the diff tab and not on the commit itself, this is a non-issue."

From the conversation on PR #10, it seems like there are still problems (albeit less severe ones) when you comment on diff's, since outdated diffs collapse their presentation of comments.

[nikomatsakis]

I think it's handy to comment on the commit itself, since you can target a particular section. I also think it's handy to track the various revisions that the RFC went through. And I don't see any value to force-pushing for this particular repository.

[nikomatsakis]

Scratch my last comment; I misunderstood what "comment on the diff" meant.

[adrientetar]

Just not force-pushing should do it otherwise; we shouldn't care about tight history for this repo anyways.

[brson]

Fine by me.

[erickt]

I've filed a bug with GitHub support to ask them to preserve comments during a force push. I'll update this issue if they get back to me.

[steveklabnik]

Comments get preserved (but hidden) on diffs and discussions, it's only if you make a comment on a commit that 'dissapears', and that's because the commit is no longer in the tree.

I don't expect they'll change this behavior, as it's pretty good.

[steveklabnik]

^^^^ click that and you get old comments.

[asb]

@steveklabnik yes, but there is a belief that once those commits are GCed then the comments will be lost too. If a Github person can confirm for sure that doesn't happen, we'd know there's no chance of losing valuable history for RFC discussion.

[pnkfelix]

@steveklabnik also, if I want to search for occurrences of a word/phrase in a long discussion, it doesn't help if I have to go through and click a bunch of "Show outdated diffs" before I search. This was my main complaint in my response to adrien above (see also discussion on PR #10).

[thestinger]

If the line comments are ever garbage collected, then I agree with not force pushing. However, I would prefer for the issue to be fixed by GitHub if it's the case that they're lost when the commit is gone. Force pushing is a nice way of making myself look cleverer than I really am and I'll be sad if I can't keep doing it.

[adrientetar]

It is not a GitHub issue. If you comment on a commit and force push changes branch pointers, then this commit is not part of the branch anymore and incidentally, the commit comments neither.
The biggest problem with diff comments is that you have to comment on the whole diff at once, which is a non-issue for RFCs.

[thestinger]

I understand how rebasing and force pushing works. GitHub is able to preserve line comments after a force push and it will continue to list them as part of the pull request. The part that I'm unclear on is whether these will stick around after it garbage collects the old commits. If they do stick around, we just need to avoid commit comments until GitHub preserves those too - line comments are fine. A good review system like Gerrit understands rebases at a much deeper level than GitHub and never manages to lose the reviews.

[nocnokneo]

@thestinger Do you have link to the bug that you filed with GitHub about preserving comments during a force push?

[thestinger]

I'm not interested in trying to fix GitHub. Use Gerrit if you want good code review.

[nocnokneo]

Whoops, wrong person.

@erickt Do you have a link to the bug that you filed with GitHub about preserving comments during a force push?

[ticki]

I believe this can be closed. Seems dead.

[nagisa]

Still relevant, I believe. I’ve seen many interesting comment threads disappear after RFC was force-pushed without comments really being addressed.

EDIT:

What I mean by disappear, is that a comment left on a particular line, as opposed to a commit, completely disappears, without a trace. The only way to retrieve these comments later is to stitch the thread from notification emails.

Moreover, AFAIR, linking to a comment that has been collapsed will not automatically expand the relevant block of comments.

All in all, I would agree with thestinger that github for any kind of review sucks hard and it doesn’t seem to be close to being fixed anytime soon. The next best thing after moving to a different solution would be adapting our own processes to whatever Github expects people to do, and this is what this issue proposes.

[pnkfelix]

triage: I-nominated

cc @rust-lang/core

(at least, I think this is something that the core team simply needs to make an executive decision about, and if we're going to do this, we should do so ASAP)

[aturon]

@pnkfelix 👍, I see little downside.

[steveklabnik]

I am vaguely pro force pushing, mostly because that's the right workflow, git wise. However, the downside is a gross history, which matters less for this repo. So I guess I'm okay with this change, even if it makes me feel dirty.

[pnkfelix]

Okay it seems like there's enough vaguely positive sentiment to warrant my making an actual amendment PR

[ticki]

👍

[pnkfelix]

Is #1569 covering enough of this that I can close this issue? (Perhaps I should expand the additional paragraph to list "force push" among the things not to do, alongside squash and rebasing. .. though strictly speaking I think saying no force push alone covers those cases. ..)

[aturon]

@pnkfelix I think it's probably enough to close this issue, but if you wanted to explicitly add a prohibition against "force push" that'd be fine by me.

[Centril]

Closing since this hasn't gone anywhere for more than 2 years and I don't think there is a problem in practice.