You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@netbeans.apache.org by Matthias Bläsing <mb...@doppel-helix.eu.INVALID> on 2025/01/08 16:34:10 UTC
Please squash/cleanup PRs before merge
Hello,
I'd like to ask you squash PRs before merge sensibly.
While a PR is in review, adding commits to a PR makes sense, so that
reviewers can see what changed and why. This also allows for
incremental improvements while the PR is open and thus supports
development. It can also make sense to merge master into a PR when the
two states diverged too much.
On the other hand there are drawbacks:
- A PR consisting of many commits makes it hard to revert it,
if necessary. There is always the chance to miss a commit, causing
unexpected results.
- The same is true for merges from master. Reverting a merge commit is
ugly and makes it even harder to revert.
- While in review the incremental changes are the most important part,
in the history, possibly year later, the big picture of the changes
becomes more important. If that is spread over 10+ commits it is hard
to see what was done and if a merge commit is in that range it gets
nearly impossible.
There are projects, that squash every PR into a single commit. The
prime example is the OpenJDK project.
From my perspective there is a sensible middle ground: Squashing
commits inside a PR into logical units. We have had multiple PRs, where
cleanups of the affected source files and the actual changes where
done. In that case it makes sense to split the cleanup, which should
not change behavior, from the actual behavioral change.
Cleaningup with git is not that hard:
- If there are merges from master, rebase the changeset onto master
- Then use the interactive rebase feature of git to cleanup the commits
as necessary. Commits can be reordered and squashed together.
- Then force-push into the original PR branch
A diff between the original HEAD and the squashed HEAD can verify, that
there were no code changes missed.
If anybody needs help with that, it can be provided.
Thank you for taking time to read.
Greetings
Matthias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Handling authorship on squashed PRs (was: Re: Please squash/cleanup PRs before merge)
Posted by Matthias Bläsing <mb...@doppel-helix.eu.INVALID>.
Hi,
as there as a inquiry how to handle the situation here is one option:
Use the "Co-authored-by" trailer entry, as was done for example here:
https://github.com/apache/netbeans/pull/5866/commits/697d0320eaffbd054813a5d0c369bf5ebae47703
------------------------------------------------------
matthias@enterprise:~/src/netbeans$ git log -n 1 --pretty=fuller 697d0320eaffbd054813a5d0c369bf5ebae47703
commit 697d0320eaffbd054813a5d0c369bf5ebae47703 (origin/pr-5866, github/pr/5866)
Author: Josh Juneau <ju...@gmail.com>
AuthorDate: Fri Apr 21 07:50:41 2023 -0500
Commit: Matthias Bläsing <mb...@doppel-helix.eu>
CommitDate: Tue May 16 22:42:34 2023 +0200
NETBEANS-4035: CDI Bean as Named is not listed in xhtml for selection
Closes: #4035
Co-authored-by: José Contreras <pe...@apache.org>
Co-authored-by: Matthias Bläsing <mb...@doppel-helix.eu>
------------------------------------------------------
The author was Josh, José and I contributed further changes, I did the
squashing. All contributions are recorded here and are also correctly
picked up by github UI.
Here is the comment about/info about/motivation for the squasing:
https://github.com/apache/netbeans/pull/5866#issuecomment-1550347489
Greetings
Matthias
Am Mittwoch, dem 08.01.2025 um 17:34 +0100 schrieb Matthias Bläsing:
> Hello,
>
> I'd like to ask you squash PRs before merge sensibly.
>
> While a PR is in review, adding commits to a PR makes sense, so that
> reviewers can see what changed and why. This also allows for
> incremental improvements while the PR is open and thus supports
> development. It can also make sense to merge master into a PR when the
> two states diverged too much.
>
> On the other hand there are drawbacks:
>
> - A PR consisting of many commits makes it hard to revert it,
> if necessary. There is always the chance to miss a commit, causing
> unexpected results.
> - The same is true for merges from master. Reverting a merge commit is
> ugly and makes it even harder to revert.
> - While in review the incremental changes are the most important part,
> in the history, possibly year later, the big picture of the changes
> becomes more important. If that is spread over 10+ commits it is hard
> to see what was done and if a merge commit is in that range it gets
> nearly impossible.
>
> There are projects, that squash every PR into a single commit. The
> prime example is the OpenJDK project.
>
> From my perspective there is a sensible middle ground: Squashing
> commits inside a PR into logical units. We have had multiple PRs, where
> cleanups of the affected source files and the actual changes where
> done. In that case it makes sense to split the cleanup, which should
> not change behavior, from the actual behavioral change.
>
> Cleaningup with git is not that hard:
>
> - If there are merges from master, rebase the changeset onto master
> - Then use the interactive rebase feature of git to cleanup the commits
> as necessary. Commits can be reordered and squashed together.
> - Then force-push into the original PR branch
>
> A diff between the original HEAD and the squashed HEAD can verify, that
> there were no code changes missed.
>
> If anybody needs help with that, it can be provided.
>
> Thank you for taking time to read.
>
> Greetings
>
> Matthias
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Michael Bien <mb...@gmail.com>.
On 12.01.25 14:45, Michael Bien wrote:
>>> Not sure if we can make it more strickt without it getting in the way
>>> though.
>> Such a check would be a constant reminder for oldschoolers and
>> wouldn't stay
>> in a way, as it could be disabled by adding a special label.
>
> If this is mainly about finding ways to remind committers about things
> to watch out for, what if we inject the pre-merge checklist into the
> PR text template?
https://github.com/apache/netbeans/pull/8216
little approval/merge check list via PR template
-mbien
>
> best regards,
>
> michael
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Michael Bien <mb...@gmail.com>.
Hi Jaroslav,
On 12.01.25 06:52, Jaroslav Tulach wrote:
> I've noticed there is some commit validation checker as I got one failure
> during development.
>
> Why doesn't the checker:
> - fail a PR check if there are two commits by the same person?
> - e.g. two commits by jaroslav.tulach would fail the PR check.
> - unless there is a label like "Merge allowed" attached to the PR
i thought about this before but I fear that this might not help. The
basic idea would be to make multi-commit PRs opt-in via label.
This would have the following effects I believe:
- PRs would essentially always fail the paperwork job while they are
still in the development/review phase
- committers which anticipate a multi-commit merge, will set the flag
-> the checks are now green again
- the same accidents can still happen, since the system does not know
what the /intended/ number of commits is
The inverted case is to make a needs-squashing label fail the checks.
However, who would set that? And where is the difference between setting
a label and leaving a short note while approving?
>> Not sure if we can make it more strickt without it getting in the way
>> though.
> Such a check would be a constant reminder for oldschoolers and wouldn't stay
> in a way, as it could be disabled by adding a special label.
Yes a flag has indeed the potential to work as a reminder, but this will
come with extra annoyances. E.g having to force a rebuild just to make
the checks happy if the label was not set etc.
I think the best UI fix for this would be a confirm dialog on the github
side in the sense of "are you sure you want to merge 15 commits?". But
there are no hooks for things like this which I am aware off.
If this is mainly about finding ways to remind committers about things
to watch out for, what if we inject the pre-merge checklist into the PR
text template?
best regards,
michael
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Jaroslav Tulach <ja...@gmail.com>.
Please accept my apology for not cleaning up history of my latest PR enough.
Not being constantly reminded about the current NetBeans commit policy one
forgets and continues to live in the old times...
> Worth noting that our commit header checker script which we added after
> https://lists.apache.org/thread/rb09p94qwl4z0tw040f12053qbobnp72
> should pick up many author/mail issues now and fail the paperwork job.
I've noticed there is some commit validation checker as I got one failure
during development.
Why doesn't the checker:
- fail a PR check if there are two commits by the same person?
- e.g. two commits by jaroslav.tulach would fail the PR check.
- unless there is a label like "Merge allowed" attached to the PR
> Not sure if we can make it more strickt without it getting in the way
> though.
Such a check would be a constant reminder for oldschoolers and wouldn't stay
in a way, as it could be disabled by adding a special label.
-jt
Dne neděle 12. ledna 2025 0:49:27 CET, Michael Bien napsal(a):
> On 08.01.25 21:28, Matthias Bläsing wrote:
> > Hi Chris,
> >
> > Am Mittwoch, dem 08.01.2025 um 21:11 +0100 schrieb Chris:
> >> Thx for the headsup. I know that pain but I thought that this is just
> >> the button "squash and merge" at github, isn't it?
> >>
> >>> On 08.01.25 17:34, Matthias Bläsing wrote:
> >>>> Cleaningup with git is not that hard:
> >>>>
> >>>> - If there are merges from master, rebase the changeset onto master
> >>>> - Then use the interactive rebase feature of git to cleanup the commits
> >>>>
> >>>> as necessary. Commits can be reordered and squashed together.
> >>>>
> >>>> - Then force-push into the original PR branch
> >
> > that button is disable for the Apache NetBeans repository. The reason
> > is, that github managed to break that feature.
> >
> > The behavior of the git CLI client is, that it keeps the author
> > information intact/takes it from one of the squashed commits and makes
> > the rebasing user the committer (that are two different fields in git
> > commits).
> >
> > The github GUI in the past randomly removed user names and email
> > information. If I understand it correctly it depends on the settings of
> > the affected user whether he makes its realname and/or emails visible.
> >
> > Have a look at the output of
> >
> > git log -n 1 --pretty=fuller b080e1ec9bfaaa552c78146496b798fac2770b66
> >
> > That is a recent merge commit created by github. The authors name is
> > listed, the email part is replaced by an github pseudonym.
> >
> > We also had situations where users hid their realname.
>
> Worth noting that our commit header checker script which we added after
> https://lists.apache.org/thread/rb09p94qwl4z0tw040f12053qbobnp72
> should pick up many author/mail issues now and fail the paperwork job.
> (and also some other things like empty commit messages). #6630 has some
> screenshots.
>
> Not sure if we can make it more strickt without it getting in the way
> though.
>
> So essentially i am out of ideas at this point - nothing will replace
> reviewers who point out issues with a PR before hitting approve and/or
> committers who go through the checklist before merge.
>
> -mbien
>
> > Greetings
> >
> > Matthias
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> > For additional commands, e-mail: dev-help@netbeans.apache.org
> >
> > For further information about the NetBeans mailing lists, visit:
> > https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Michael Bien <mb...@gmail.com>.
On 08.01.25 21:28, Matthias Bläsing wrote:
> Hi Chris,
>
> Am Mittwoch, dem 08.01.2025 um 21:11 +0100 schrieb Chris:
>> Thx for the headsup. I know that pain but I thought that this is just
>> the button "squash and merge" at github, isn't it?
>>
>>> On 08.01.25 17:34, Matthias Bläsing wrote:
>>>> Cleaningup with git is not that hard:
>>>>
>>>> - If there are merges from master, rebase the changeset onto master
>>>> - Then use the interactive rebase feature of git to cleanup the commits
>>>> as necessary. Commits can be reordered and squashed together.
>>>> - Then force-push into the original PR branch
> that button is disable for the Apache NetBeans repository. The reason
> is, that github managed to break that feature.
>
> The behavior of the git CLI client is, that it keeps the author
> information intact/takes it from one of the squashed commits and makes
> the rebasing user the committer (that are two different fields in git
> commits).
>
> The github GUI in the past randomly removed user names and email
> information. If I understand it correctly it depends on the settings of
> the affected user whether he makes its realname and/or emails visible.
>
> Have a look at the output of
>
> git log -n 1 --pretty=fuller b080e1ec9bfaaa552c78146496b798fac2770b66
>
> That is a recent merge commit created by github. The authors name is
> listed, the email part is replaced by an github pseudonym.
>
> We also had situations where users hid their realname.
Worth noting that our commit header checker script which we added after
https://lists.apache.org/thread/rb09p94qwl4z0tw040f12053qbobnp72
should pick up many author/mail issues now and fail the paperwork job.
(and also some other things like empty commit messages). #6630 has some
screenshots.
Not sure if we can make it more strickt without it getting in the way
though.
So essentially i am out of ideas at this point - nothing will replace
reviewers who point out issues with a PR before hitting approve and/or
committers who go through the checklist before merge.
-mbien
>
> Greetings
>
> Matthias
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Chris <ch...@gmx.net.INVALID>.
Hey Matthias,
understood and thx for the explamanation :).
Cheers
Chris
Am 08.01.2025 um 21:28 schrieb Matthias Bläsing:
> Hi Chris,
>
> Am Mittwoch, dem 08.01.2025 um 21:11 +0100 schrieb Chris:
>> Thx for the headsup. I know that pain but I thought that this is just
>> the button "squash and merge" at github, isn't it?
>>
>>> On 08.01.25 17:34, Matthias Bläsing wrote:
>>>> Cleaningup with git is not that hard:
>>>>
>>>> - If there are merges from master, rebase the changeset onto master
>>>> - Then use the interactive rebase feature of git to cleanup the commits
>>>> as necessary. Commits can be reordered and squashed together.
>>>> - Then force-push into the original PR branch
> that button is disable for the Apache NetBeans repository. The reason
> is, that github managed to break that feature.
>
> The behavior of the git CLI client is, that it keeps the author
> information intact/takes it from one of the squashed commits and makes
> the rebasing user the committer (that are two different fields in git
> commits).
>
> The github GUI in the past randomly removed user names and email
> information. If I understand it correctly it depends on the settings of
> the affected user whether he makes its realname and/or emails visible.
>
> Have a look at the output of
>
> git log -n 1 --pretty=fuller b080e1ec9bfaaa552c78146496b798fac2770b66
>
> That is a recent merge commit created by github. The authors name is
> listed, the email part is replaced by an github pseudonym.
>
> We also had situations where users hid their realname.
>
> Greetings
>
> Matthias
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Matthias Bläsing <mb...@doppel-helix.eu.INVALID>.
Hi Chris,
Am Mittwoch, dem 08.01.2025 um 21:11 +0100 schrieb Chris:
> Thx for the headsup. I know that pain but I thought that this is just
> the button "squash and merge" at github, isn't it?
>
> > On 08.01.25 17:34, Matthias Bläsing wrote:
> > >
> > > Cleaningup with git is not that hard:
> > >
> > > - If there are merges from master, rebase the changeset onto master
> > > - Then use the interactive rebase feature of git to cleanup the commits
> > > as necessary. Commits can be reordered and squashed together.
> > > - Then force-push into the original PR branch
that button is disable for the Apache NetBeans repository. The reason
is, that github managed to break that feature.
The behavior of the git CLI client is, that it keeps the author
information intact/takes it from one of the squashed commits and makes
the rebasing user the committer (that are two different fields in git
commits).
The github GUI in the past randomly removed user names and email
information. If I understand it correctly it depends on the settings of
the affected user whether he makes its realname and/or emails visible.
Have a look at the output of
git log -n 1 --pretty=fuller b080e1ec9bfaaa552c78146496b798fac2770b66
That is a recent merge commit created by github. The authors name is
listed, the email part is replaced by an github pseudonym.
We also had situations where users hid their realname.
Greetings
Matthias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Laszlo Kishalmi <la...@gmail.com>.
On 1/8/25 12:11, Chris wrote:
> Thx for the headsup. I know that pain but I thought that this is just
> the button "squash and merge" at github, isn't it?
>
>
Well, not really. That method has it's own issues and we disabled that.
> Cheers
>
> Chris
>
> Am 08.01.2025 um 18:33 schrieb Michael Bien:
>> +1
>>
>> checking the PR commits is also part of our PR approval and merge
>> checklists:
>> https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide
>>
>>
>>
>> if the PR isn't squashed properly it either shouldn't get approved, or
>> approved with a request to squash (there is also a label for it).
>>
>> -mbien
>>
>>
>> On 08.01.25 17:34, Matthias Bläsing wrote:
>>> Hello,
>>>
>>> I'd like to ask you squash PRs before merge sensibly.
>>>
>>> While a PR is in review, adding commits to a PR makes sense, so that
>>> reviewers can see what changed and why. This also allows for
>>> incremental improvements while the PR is open and thus supports
>>> development. It can also make sense to merge master into a PR when the
>>> two states diverged too much.
>>>
>>> On the other hand there are drawbacks:
>>>
>>> - A PR consisting of many commits makes it hard to revert it,
>>> if necessary. There is always the chance to miss a commit, causing
>>> unexpected results.
>>> - The same is true for merges from master. Reverting a merge commit is
>>> ugly and makes it even harder to revert.
>>> - While in review the incremental changes are the most important part,
>>> in the history, possibly year later, the big picture of the changes
>>> becomes more important. If that is spread over 10+ commits it is
>>> hard
>>> to see what was done and if a merge commit is in that range it gets
>>> nearly impossible.
>>>
>>> There are projects, that squash every PR into a single commit. The
>>> prime example is the OpenJDK project.
>>>
>>> From my perspective there is a sensible middle ground: Squashing
>>> commits inside a PR into logical units. We have had multiple PRs, where
>>> cleanups of the affected source files and the actual changes where
>>> done. In that case it makes sense to split the cleanup, which should
>>> not change behavior, from the actual behavioral change.
>>>
>>> Cleaningup with git is not that hard:
>>>
>>> - If there are merges from master, rebase the changeset onto master
>>> - Then use the interactive rebase feature of git to cleanup the commits
>>> as necessary. Commits can be reordered and squashed together.
>>> - Then force-push into the original PR branch
>>>
>>> A diff between the original HEAD and the squashed HEAD can verify, that
>>> there were no code changes missed.
>>>
>>> If anybody needs help with that, it can be provided.
>>>
>>> Thank you for taking time to read.
>>>
>>> Greetings
>>>
>>> Matthias
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
>>> For additional commands, e-mail: dev-help@netbeans.apache.org
>>>
>>> For further information about the NetBeans mailing lists, visit:
>>> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>>>
>>>
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
>> For additional commands, e-mail: dev-help@netbeans.apache.org
>>
>> For further information about the NetBeans mailing lists, visit:
>> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>>
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Chris <ch...@gmx.net.INVALID>.
Thx for the headsup. I know that pain but I thought that this is just
the button "squash and merge" at github, isn't it?
Cheers
Chris
Am 08.01.2025 um 18:33 schrieb Michael Bien:
> +1
>
> checking the PR commits is also part of our PR approval and merge
> checklists:
> https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide
>
>
> if the PR isn't squashed properly it either shouldn't get approved, or
> approved with a request to squash (there is also a label for it).
>
> -mbien
>
>
> On 08.01.25 17:34, Matthias Bläsing wrote:
>> Hello,
>>
>> I'd like to ask you squash PRs before merge sensibly.
>>
>> While a PR is in review, adding commits to a PR makes sense, so that
>> reviewers can see what changed and why. This also allows for
>> incremental improvements while the PR is open and thus supports
>> development. It can also make sense to merge master into a PR when the
>> two states diverged too much.
>>
>> On the other hand there are drawbacks:
>>
>> - A PR consisting of many commits makes it hard to revert it,
>> if necessary. There is always the chance to miss a commit, causing
>> unexpected results.
>> - The same is true for merges from master. Reverting a merge commit is
>> ugly and makes it even harder to revert.
>> - While in review the incremental changes are the most important part,
>> in the history, possibly year later, the big picture of the changes
>> becomes more important. If that is spread over 10+ commits it is hard
>> to see what was done and if a merge commit is in that range it gets
>> nearly impossible.
>>
>> There are projects, that squash every PR into a single commit. The
>> prime example is the OpenJDK project.
>>
>> From my perspective there is a sensible middle ground: Squashing
>> commits inside a PR into logical units. We have had multiple PRs, where
>> cleanups of the affected source files and the actual changes where
>> done. In that case it makes sense to split the cleanup, which should
>> not change behavior, from the actual behavioral change.
>>
>> Cleaningup with git is not that hard:
>>
>> - If there are merges from master, rebase the changeset onto master
>> - Then use the interactive rebase feature of git to cleanup the commits
>> as necessary. Commits can be reordered and squashed together.
>> - Then force-push into the original PR branch
>>
>> A diff between the original HEAD and the squashed HEAD can verify, that
>> there were no code changes missed.
>>
>> If anybody needs help with that, it can be provided.
>>
>> Thank you for taking time to read.
>>
>> Greetings
>>
>> Matthias
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
>> For additional commands, e-mail: dev-help@netbeans.apache.org
>>
>> For further information about the NetBeans mailing lists, visit:
>> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>>
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: Please squash/cleanup PRs before merge
Posted by Michael Bien <mb...@gmail.com>.
+1
checking the PR commits is also part of our PR approval and merge
checklists:
https://cwiki.apache.org/confluence/display/NETBEANS/PRs+and+You+-+A+reviewer+Guide
if the PR isn't squashed properly it either shouldn't get approved, or
approved with a request to squash (there is also a label for it).
-mbien
On 08.01.25 17:34, Matthias Bläsing wrote:
> Hello,
>
> I'd like to ask you squash PRs before merge sensibly.
>
> While a PR is in review, adding commits to a PR makes sense, so that
> reviewers can see what changed and why. This also allows for
> incremental improvements while the PR is open and thus supports
> development. It can also make sense to merge master into a PR when the
> two states diverged too much.
>
> On the other hand there are drawbacks:
>
> - A PR consisting of many commits makes it hard to revert it,
> if necessary. There is always the chance to miss a commit, causing
> unexpected results.
> - The same is true for merges from master. Reverting a merge commit is
> ugly and makes it even harder to revert.
> - While in review the incremental changes are the most important part,
> in the history, possibly year later, the big picture of the changes
> becomes more important. If that is spread over 10+ commits it is hard
> to see what was done and if a merge commit is in that range it gets
> nearly impossible.
>
> There are projects, that squash every PR into a single commit. The
> prime example is the OpenJDK project.
>
> From my perspective there is a sensible middle ground: Squashing
> commits inside a PR into logical units. We have had multiple PRs, where
> cleanups of the affected source files and the actual changes where
> done. In that case it makes sense to split the cleanup, which should
> not change behavior, from the actual behavioral change.
>
> Cleaningup with git is not that hard:
>
> - If there are merges from master, rebase the changeset onto master
> - Then use the interactive rebase feature of git to cleanup the commits
> as necessary. Commits can be reordered and squashed together.
> - Then force-push into the original PR branch
>
> A diff between the original HEAD and the squashed HEAD can verify, that
> there were no code changes missed.
>
> If anybody needs help with that, it can be provided.
>
> Thank you for taking time to read.
>
> Greetings
>
> Matthias
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
> For additional commands, e-mail: dev-help@netbeans.apache.org
>
> For further information about the NetBeans mailing lists, visit:
> https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@netbeans.apache.org
For additional commands, e-mail: dev-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists