#3004 Issue `open`: Merge / Rebase master before merging PRs?¶

Labels: discuss / RFC, ReaR Project

schlomo opened issue at 2023-06-05 12:43:¶

@pcahyna raised the question why I like to merge / rebase master into a PR branch before merging that branch into master.

Here is my reasoning:

GH let's us merge a branch if there are not git merge conflicts, but that doesn't tell us if the code will continue to work with current master
Merging / rebasing onto master before merging the PR branch allows GH to run all the checks and actions on the codebase that would be the result of merging the PR into master. That means that we get a higher confidence that ReaR will continue to work after merging the PR.

https://github.com/rear/rear/pull/3003 is a nice example for this, the change in the Makefile has an impact on https://github.com/rear/rear/blob/62b9afac5c2aba542075c3692cf93a0fe280c8a2/.github/workflows/build-packages.yml#L26-L30 which can be observed only after I merged current master because at the same time I fixed a but in this workflow that prevented it from working.

Therefore I'd like to propose to always merge/rebase master before merging a PR branch.

Let's talk about this here, what would be reasons against doing this?

pcahyna commented at 2023-06-06 15:14:¶

I am not too concerned by this problem, in my experience it does not happen often in practice.

If you want to be on the safe side, I prefer to rebase the PR branch on top of the current master rather than merging master to the PR branch. The additional merge clutters the history a bit.
Rebasing the PR branch before merging is often advisable anyway, because one may need to improve commit messages and/or squash commits that correct mistakes in previous changes into the commits that they fix (to avoid cluttering the final history with commits like "do A" and then "correct the mistakes made while doing A").

jsmeix commented at 2023-06-07 05:39:¶

I wonder if it could be rather confusing when
(at a possibly longer time after a PR was merged)
others who like to understand what a PR was about
(e.g. to analyze if a PR is the reason for a regression)
see additional commits in a PR that do not belong
to the actually intended code changes of a PR:

When I look at the recent
https://github.com/rear/rear/pull/3003
it appears in current master code history as

# git log --format="%ae %H %ad%n%s :%n%b%n" --graph | less

*   schlomo+github@schapiro.org 29f51855f36d6a0152a691fcab1d6224642a6ffd Tue Jun 6 16:22:32 2023 +0200
|\  Merge pull request #3003 from pcahyna/make-srpm-improvements :
| | Improvements of the Makefile distribution targets
| | 
| * pcahyna@redhat.com a7a51df4512cf348a38487e3f3fe178c0c4b4461 Tue Jun 6 12:10:18 2023 +0200
| | Do not use the rpmspec tool for parsing RPM spec :
| | It is more elegant than rpm, as it can print the source package
| | information, which is what we are interested in. Unfortunately, it does
| | not exist on EL 6 yet.
| | 
| | 
| * pcahyna@redhat.com 1a842d2b73e985ac8c76480c006647f5b6078247 Mon Jun 5 22:24:20 2023 +0200
| | Do not assume that the temporary spec file exists :
| | There is no guarantee that "make srpm" will trigger rebuild of
| | the dist tarball dist/$(name)-$(distversion).tar.gz - the file may
| | already exist.  In this case the temporary file does not get saved.
| | 
| | Extract the spec file from the dist tarball instead.
| | 
| | 
| *   schlomo+github@schapiro.org 7122a911be8eea685a421be0461bf9d398c4b2cc Mon Jun 5 13:15:12 2023 +0200
| |\  Merge branch 'master' into make-srpm-improvements :
| | | 
| | | 
| * | pcahyna@redhat.com 149653c5f8bc047ff26f702d16dc715bd4f4b0f8 Fri Jun 2 20:40:40 2023 +0200
| | | Do not rebuild the dist tarball always :
| | | Make it depend on all its files instead.
| | | 
| | | 
| * | pcahyna@redhat.com f8c77f6831de09802038f60acd2ba2a0ba6fb864 Thu Jun 1 18:24:54 2023 +0200
| | | Eliminate a %define from RPM spec :
| | | Define it on the command line in Makefile instead. Allows us to
| | | eliminate one sed transform of the spec file.
| | | 
| | | Keep `%define debug_package %{nil}`, it needs to be set when building the RPM
| | | without the Makefile.
| | | 
| | | 
| * | pcahyna@redhat.com 67e71528092b07f8c91558d7f21b90baa055d908 Thu Jun 1 17:54:52 2023 +0200
| | | Avoid using wildcard in "make rpm" :
| | | The source RPM has been specified as a glob pattern:
| | | dist/$(name)-$(version)-*.src.rpm
| | | This breaks when there are multiple source RPMs under dist/ - leftovers
| | | from previous builds. It forces us to use clean or package-clean always
| | | before using "make rpm", which is not ideal.
| | | 
| | | Fix by determining the actual full package name (with the dist tag) by
| | | parsing the spec file and using that. Use the full name also for
| | | an informational message on terminal.
| | | 
| | | 
| * | pcahyna@redhat.com f63785da9e667af02f577fc827e4fa86b3e5d3a6 Thu Jun 1 17:52:12 2023 +0200
| | | Extract common RPM defines in Makefile to a var :
| | | 
| | | 
| * | pcahyna@redhat.com e5505c4d186274a4b9682217928252887e6dc360 Wed May 31 19:56:57 2023 +0200
| | | Always remake the dist tarball :
| | | The dist tarball (dist/$(name)-$(distversion).tar.gz) has been
| | | considered always up to date by make, and thus not remade when any of
| | | the files changed. This has caused "make srpm" (and other targets
| | | depending on the dist tarball - "make rpm", "make deb" etc.) to build
| | | packages without newer changes to the sources.
| | | 
| | | Fix by marking dist/$(name)-$(distversion).tar.gz as a phony target
| | | that will be always rebuilt.
| | | 
| | | 
* | | jsmeix@suse.com 62b9afac5c2aba542075c3692cf93a0fe280c8a2 Mon Jun 5 13:44:07 2023 +0200
| |/  Fixed 'r' and 'n' oprtions in getopt in usr/sbin/rear :
|/|   The 'r' option has a required argument so it must be 'r:' and
| |   the 'n' option has no argument so it must be 'n' in getopt,
| |   cf. https://github.com/rear/rear/commit/7f0b2fd88a84058977c6306d89ab351e9bb9002e#r116533758
| |   
* | schlomo+git@schapiro.org eea9751fc92caff4be82e7f25a55d7405a15a5df Mon Jun 5 13:14:42 2023 +0200
|/  Disable rawhide Docker image as it is broken :
|

Therein the

| | 
| *   schlomo+github@schapiro.org 7122a911be8eea685a421be0461bf9d398c4b2cc Mon Jun 5 13:15:12 2023 +0200
| |\  Merge branch 'master' into make-srpm-improvements :
| | |

is not confusing.

When I look at the merge commit
https://github.com/rear/rear/commit/29f51855f36d6a0152a691fcab1d6224642a6ffd
it only shows the actually intended code changes of
https://github.com/rear/rear/pull/3003
i.e.
https://github.com/rear/rear/commit/29f51855f36d6a0152a691fcab1d6224642a6ffd
shows the same as
https://github.com/rear/rear/pull/3003/files

In particular the additional changes in the PR
because of the "Merge branch 'master' ..." commit
https://github.com/rear/rear/commit/7122a911be8eea685a421be0461bf9d398c4b2cc
do not appear in the merge commit
https://github.com/rear/rear/commit/29f51855f36d6a0152a691fcab1d6224642a6ffd

So I think additional changes in a PR
because of a "Merge branch 'master' ..." commit
are not confusing when others like to understand
what a PR was about (i.e. after a PR was merged).

jsmeix commented at 2023-06-07 06:10:¶

Now I am wondering if it could be rather confusing
while a PR is "work in progress" (i.e. before it is merged)
for the one who is currently working on "his" PR and also
for others who like to understand what the PR is about
(e.g. to review the code changes in a PR)
when additional commits appear in a PR that do not belong
to the actually intended code changes of the PR?

@pcahyna
your
https://github.com/rear/rear/pull/3003#issuecomment-1576685594
looks as if you got confused while you were working on your
https://github.com/rear/rear/pull/3003
by the unexpected additional changes in your PR
because of a "Merge branch 'master' ..." commit?

pcahyna commented at 2023-06-07 07:53:¶

So I think additional changes in a PR
because of a "Merge branch 'master' ..." commit
are not confusing when others like to understand
what a PR was about (i.e. after a PR was merged).

Well, maybe it is not confusing when thinking about what the PR was about, but it still needs additional brain power if you need to decipher the history in detail because of more parallel lines in your "git log --graph" output that one will need to keep track of when reading it. It becomes much more difficult to determine which line of development (with all its commits) belonged to the PR and which belonged to the master branch or another branches. I.e. I now have to think whether the commit jsmeix@suse.com 62b9afac5c2aba542075c3692cf93a0fe280c8a2 Mon Jun 5 13:44:07 2023 +0200 belongs to the PR or not. In this case, it is fairly easy, because there are not many commits and I recognize my own name in the PR branch. But think about the possibility where there are long-running branches with many other commits happening in the meantime, and you don't recognize the names of their authors.

schlomo commented at 2023-06-07 08:23:¶

100% agree that rebasing is much better than merging, need to see if the "Update" button on GitHub can be configured to rebase instead of merge.

pcahyna commented at 2023-06-07 08:28:¶

Now I am wondering if it could be rather confusing while a PR is "work in progress" (i.e. before it is merged) for the one who is currently working on "his" PR and also for others who like to understand what the PR is about (e.g. to review the code changes in a PR) when additional commits appear in a PR that do not belong to the actually intended code changes of the PR?

@pcahyna your #3003 (comment) looks as if you got confused while you were working on your #3003 by the unexpected additional changes in your PR because of a "Merge branch 'master' ..." commit?

I was not really "confused", but having an unrelated (merge) commit appearing in my branch is certainly a bit of complication.

pcahyna commented at 2023-06-07 08:30:¶

For me asking the PR author to rebase would be the best option. Doing it for them (using WebUI or otherwise) would be confusing for the PR author who might be working on the branch in the meantime and now is forced to update and rebase their unpushed changes etc.

jsmeix commented at 2023-06-07 08:38:¶

On my current https://github.com/rear/rear/pull/3006
I see near the bottom

This branch has no conflicts with the base branch     [ Update branch | v ]
Merging can be performed automatically.

Clicking the 'v' of the "[ Update branch | v ]" button shows

[V] Update with merge commit
    ...
[ ] Update with rebase
    ...

where "Update with merge commit" is preselected.

I do not want to play around with that on my current active
https://github.com/rear/rear/pull/3006
because I do not want to get things somehow
changed in my PR in possibly unexpected ways.

[Export of Github issue for rear/rear.]

#3004 Issue open: Merge / Rebase master before merging PRs?¶