search

rustic-rs / dev-docs

rustic dev documentation
http://rustic.cli.rs/dev-docs/
Mozilla Public License 2.0
1 stars 1 forks source link

Seemingly contradictory guidance about force pushing #35

Open intgr opened 1 month ago

intgr commented 1 month ago

The development documentation appears to contradict itself about force pushing.

On the one hand it states: https://rustic.cli.rs/dev-docs/contributing-to-rustic.html#release-early-and-often-also-applies-to-pull-requests

Please don’t force push commits in your branch, in order to keep commit history and make it easier for us to see changes between reviews.

Then just a little bit later it instructs not to make sequential commits, but prefer to squash/rebase. That requires force pushing:

If you want to fix an older commit of yours, or merge several commits into a single one (squash them), rebase interactively. We don’t want to have a commit history like this:

  • add stuff
  • fix typo in stuff
  • fix compilation
  • change stuff a bit
  • and so on...

... So which is it? I can't "fix earlier commits" without force pushing.

And even disregarding the contradiciton, IMHO this is just instruction creep. Too many rules for contributors.

Since Rustic seems to use "squash and merge" in most cases, the commit history won't be retained after anyway merging. And GitHub allows diffing across force pushes. Does it really matter what workflow the contributor uses in their PR?

Also, for people who contribute to lots of open source projects, it's a PITA to keep track of the idiosyncrasies of each individual project's conventions/workflow, or to re-read the documentation every time.

simonsan commented 1 month ago

Since Rustic seems to use "squash and merge" in most cases, the commit history won't be retained after anyway merging.

That's true, though this is not about merging, but about reviewing PRs. I don't think commit history is important after a PR has been merged, but until then it makes sense for easier reviews of code changes.

And GitHub allows diffing across force pushes. Does it really matter what workflow the contributor uses in their PR?

I'm not aware of that, it's possible to check manually what was affected from a force-push, but in not in the review window, afaics. I can also not find anything regarding that online, if that has changed recently, please share.

On topic, I don't see much contradiction to be fair. It might be not as clear when you read it together, but on the one hand it is stated to not force-push changes for us as reviewers (so after a review was done/requested). The other cited paragraph doesn't instruct anyone to do anything, it explains a workflow that can be used in someone own branch until making a PR to keep the branch tidy.

EDIT: If you would propose any change to the wording, so it's more clear that one is our expectation and the other is a description of a workflow I would happily merge it.

simonsan commented 1 month ago

Also, for people who contribute to lots of open source projects, it's a PITA to keep track of the idiosyncrasies of each individual project's conventions/workflow, or to re-read the documentation every time.

Regarding that: I think that is part of contributing to OSS projects, to read yourself into their guidelines and act accordingly to make it easier for the maintainers of said projects to merge proposed changes. Keep in mind, that people are doing this in their spare time. So I think it's a managable to put some effort in so changes can be merged more easily.

intgr commented 1 month ago

And GitHub allows diffing across force pushes. [...]

I'm not aware of that, it's possible to check manually what was affected from a force-push, but in not in the review window, afaics.

This 'Compare' button is what I'm referring to:

image

I don't see much contradiction to be fair. It might be not as clear when you read it together

I was genuinely confused before reading your clarification here.

The other cited paragraph doesn't instruct anyone to do anything, it explains a workflow that can be used in someone own branch until making a PR to keep the branch tidy.

OK, so the force push guideline is trying to say the following?