doc: Rebasing, when and when not to do it #17940

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:devnotes_rebasing changing 1 files +22 −3

luke-jr commented at 2:21 PM on January 16, 2020: member

No description provided.
doc: Rebasing, when and when not to do it eb293e5b1d
emilengler commented at 2:24 PM on January 16, 2020: contributor

Concept ACK. Maybe it's worth pointing it out that drahtbot will give a warning if a rebase is required.
fanquake added the label Docs on Jan 16, 2020
michaelfolkson commented at 2:35 PM on January 16, 2020: contributor

Concept ACK.

~~Do we need to make clear that this guidance doesn't apply to squashing commits (which also uses the git rebase command)?~~

The `git rebase` command may still be useful for fixups or reorganization *within* your branch; that's usually ok to do. covers this.

Perhaps should be stronger. Squashing commits isn't "usually ok to do", it is encouraged?

So replace "that's usually ok to do" with "see the contributing doc for guidance on this"?
Sjors commented at 2:40 PM on January 16, 2020: member

ACK eb293e5b1d2b87ec82eebea8cbf5dd783dfb54fd

in doc/developer-notes.md:856 in eb293e5b1d

 853 | +
 854 | +Note that when there are no conflicts at all, however, rebasing should be
 855 | +avoided, as it can be disruptive to reviewers. (The `git rebase` command may
 856 | +still be useful for fixups or reorganization *within* your branch; that's
 857 | +usually ok to do.)
 858 | +

fjahr commented at 4:01 PM on January 16, 2020:

nit: could mention git help rebase to learn more about rebase.

in doc/developer-notes.md:39 in eb293e5b1d

  34 | @@ -35,7 +35,9 @@ Developer Notes
  35 |          - [Shebang](#shebang)
  36 |      - [Source code organization](#source-code-organization)
  37 |      - [GUI](#gui)
  38 | -    - [Subtrees](#subtrees)
  39 | +    - [Git Usage](#git-usage)
  40 | +    - [Rebasing](#rebasing)

fjahr commented at 4:04 PM on January 16, 2020:

indentation

fjahr commented at 4:08 PM on January 16, 2020: member

Concept ACK

in doc/developer-notes.md:855 in eb293e5b1d

 852 | +conflicts, and you should rebase when fixing them.
 853 | +
 854 | +Note that when there are no conflicts at all, however, rebasing should be
 855 | +avoided, as it can be disruptive to reviewers. (The `git rebase` command may
 856 | +still be useful for fixups or reorganization *within* your branch; that's
 857 | +usually ok to do.)

promag commented at 4:27 PM on January 16, 2020:

nit "that's usually common or even necessary in order to have fully working commits" or similar.

in doc/developer-notes.md:853 in eb293e5b1d

 850 | +Sometimes conflicts are invisible to git: the branches will merge cleanly, but
 851 | +builds will fail, or functionality will break. These are still considered
 852 | +conflicts, and you should rebase when fixing them.
 853 | +
 854 | +Note that when there are no conflicts at all, however, rebasing should be
 855 | +avoided, as it can be disruptive to reviewers. (The `git rebase` command may

MarcoFalke commented at 8:25 PM on January 16, 2020:

It is no more disruptive than any other force push. git range-diff can handle rebases easily

promag commented at 1:23 AM on January 17, 2020: member

@emilengler BTW I don't agree with your suggestion because @DrahtBot is not part of the project and integrates with GitHub.
MarcoFalke commented at 6:35 PM on March 19, 2020: member

@luke-jr Are you still working on this?
MarcoFalke commented at 6:36 PM on March 19, 2020: member

Also, there has been a related pull in the meantime: #18283
MarcoFalke added the label Waiting for author on Mar 19, 2020
luke-jr commented at 6:51 PM on March 19, 2020: member

I am satisfied with the current PR as-is.
MarcoFalke removed the label Waiting for author on Mar 20, 2020
MarcoFalke commented at 2:13 PM on March 20, 2020: member

I am satisfied with the current PR as-is.

Ok, removed the "Waiting for author" label, but it still would be nice if you replied to the feedback to say why it doesn't apply.
RiccardoMasutti approved
RiccardoMasutti commented at 2:14 PM on August 13, 2020: contributor

ACK eb293e5
in doc/developer-notes.md:845 in eb293e5b1d
```
 842 | +---------
 843 | +
 844 | +### Rebasing
 845 | +
 846 | +When the master development branch develops conflicts with your pull request,
 847 | +you need to rebase it. This means rebuilding your branch on top of a new base.
```
jnewbery commented at 2:36 PM on August 13, 2020:

"rebuilding" here is ambiguous. I think you mean "apply the commits in your branch to the master branch", but it could also be interpreted as meaning "compiling and linking"
michaelfolkson commented at 2:40 PM on August 13, 2020: contributor

I don't think this is optimal as there are couple of review nits that could improve it. But this has been open a while and author doesn't want to address nits so ACKing. Willing to review a follow up PR that picks up the nits.

ACK eb293e5b1d2b87ec82eebea8cbf5dd783dfb54fd

[edit: also happy to open a PR myself addressing the nits if this is merged]
luke-jr closed this on Sep 12, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels