Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Clarify whether amending commits / force-push is discuraged #5

Open
simeonschaub opened this issue Jun 22, 2020 · 2 comments
Open

Clarify whether amending commits / force-push is discuraged #5

simeonschaub opened this issue Jun 22, 2020 · 2 comments

Comments

@simeonschaub
Copy link

Guidance on contributing PRs says:

You should not squash down commits while review is still on-going.

  • Squashing commits prevents the reviewer being able to see what commits are added since the last review.

It is not quite clear to me, whether this includes amending commits and then force-pushing. This is probably something that should be discouraged for non-trivial changes, but IMHO amending makes sense, if you realize right away that you made a dumb mistake and want to push a quick fix. Is that something that should be added? Do people have different opinions on this?

@oxinabox
Copy link
Contributor

there is probably a way to rephrase it.

Would it be clearer to say:

try to avoid rewriting history (rebase, squash, filter-branch) such that commits that have been reviewed are removed from the history prior to the PR being approved, as this makes it hard to see what changes have been made since the last review.

Basically its fine to go and ammend your last commit that you pushed 3 minutes ago if you spot a typo etc.
But its annoying if a commit with comments on it just vanishes. (I think github handles that better now than it used to)
And its especially annoying if all history becomes a single commit and the reviewer can't see what was added to fix a problem they pointed out

@simeonschaub
Copy link
Author

That sounds reasonable to me.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants