You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Alexey Romanenko <ar...@gmail.com> on 2020/08/04 14:13:03 UTC
Git commit history: "fixup" commits
Hi all,
I’d like to attract your attention regarding our Git commit history and related issue. A while ago I noticed that it started getting not very clear and quite verbose comparing to how it was before. We have quite significant amount of recent commits like “fix”, “address comments”, “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and actually is just supplementary commits to “main” and initial commit of PR, added after several PRs review rounds.
AFAIR, we already had several discussion in the past about this topic and we agreed that we should avoid such commits in a final merge and have only one (in most cases) or several (if necessary) logical commits that should be atomic and properly explain what they do.
Why these “tiny" commits are bad practice? Just several main reasons:
- They pollute our git repository history and don’t give any additional and useful further information;
- They are not atomic and we can’t easily revert (rollback) this supplementary commit since the state of the build before was likely broken or had incorrect behaviour. So, in this case, the whole set of PRs commits should be reverted which is not convenient and error-prone. It’s also expected that all checks were green before merging a PR (take a part flaky tests).
- They are not informative in terms of commit message. So it makes more hard to identify Git annotated code and how the lines of code are related together.
Following this, I just want to briefly remind our Committers rules regarding PR merging [1].
Every commit:
- should do one thing and reflect it in commit message;
- should contain Jira Tag;
- all “fixup” and “address comments” type of commits should be squashed by author or committer before merging.
Please, pay attention on what is finally committed and merged into our repository and it should help to keep our commit history clear, which will be transferred to saving a time of other developers in the end.
[1] https://beam.apache.org/contribute/committer-guide/#finishing-touches <https://beam.apache.org/contribute/committer-guide/#finishing-touches>
Regards,
Alexey
Re: Git commit history: "fixup" commits
Posted by Ahmet Altay <al...@google.com>.
+1 and thank you for doing this.
Tangential point: github actions are getting more and more useful, they
have a public roadmap and now they support using hosted VMs as dedicated
workers for a project. If the current set of github actions prove useful
and reliable we can consider moving more of our automations/test workloads
to github actions.
On Wed, Aug 5, 2020 at 10:00 AM Tyson Hamilton <ty...@google.com> wrote:
> +1 to automating this!
>
> On Wed, Aug 5, 2020 at 8:38 AM Alexey Romanenko <ar...@gmail.com>
> wrote:
>
>> Thanks Robert and Udi for links. Perhaps we should integrate such check
>> if there are no objections from community. I’ll take a look on this more
>> closely.
>>
>> On 4 Aug 2020, at 19:31, Udi Meiri <eh...@google.com> wrote:
>>
>> https://github.com/marketplace/actions/gs-commit-message-checker
>>
>> On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <ro...@google.com>
>> wrote:
>>
>>> +1, thanks for the reminder.
>>>
>>> This should be really easy to automate, using
>>> https://developer.github.com/webhooks/event-payloads/#pull_request to
>>> give a warning when the change history is not sufficiently "clean."
>>> I'm not sure where to host this though (or if it could be integrated
>>> into jenkins--basically I'd just want to run a Python script with the
>>> PR number (or better, just point to the local git repo and have the
>>> master's commit handy) as another precommit).
>>>
>>> On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <ru...@google.com> wrote:
>>> >
>>> > +1 thanks Alexey.
>>> >
>>> > My apologies that I merged such a case recently (but not
>>> intentionally). I tried to use the "squash and merge" button with a
>>> consolidated commit message. After clicking the button, github showed
>>> "failed to merge" and gave a retry button, and after clicking that retry
>>> button, github magically switched to "create merge commit" approach thus
>>> merged some fixup commits to the main branch.
>>> >
>>> > This is a rare case (I only encountered once). But I will pay more
>>> attention next time. I could ask PR authors to squash their commits before
>>> merging when it is possible.
>>> >
>>> >
>>> > -Rui
>>> >
>>> > On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <
>>> aromanenko.dev@gmail.com> wrote:
>>> >>
>>> >> Yes, good point, thanks Valentyn.
>>> >>
>>> >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <va...@google.com>
>>> wrote:
>>> >>
>>> >> +1, thanks, Alexey.
>>> >>
>>> >> Also a reminder from the contributor guide: do not use the default
>>> GitHub commit message for merge commits, which looks like:
>>> >>
>>> >> Merge pull request #1234 from some_user/transient_branch_name
>>> >>
>>> >> Instead, add the commit message into the subject line, for example:
>>> "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
>>> >>
>>> >> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <
>>> aromanenko.dev@gmail.com> wrote:
>>> >>>
>>> >>> Hi all,
>>> >>>
>>> >>> I’d like to attract your attention regarding our Git commit history
>>> and related issue. A while ago I noticed that it started getting not very
>>> clear and quite verbose comparing to how it was before. We have quite
>>> significant amount of recent commits like “fix”, “address comments”,
>>> “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a
>>> prefix and actually is just supplementary commits to “main” and initial
>>> commit of PR, added after several PRs review rounds.
>>> >>>
>>> >>> AFAIR, we already had several discussion in the past about this
>>> topic and we agreed that we should avoid such commits in a final merge and
>>> have only one (in most cases) or several (if necessary) logical commits
>>> that should be atomic and properly explain what they do.
>>> >>>
>>> >>> Why these “tiny" commits are bad practice? Just several main reasons:
>>> >>> - They pollute our git repository history and don’t give any
>>> additional and useful further information;
>>> >>> - They are not atomic and we can’t easily revert (rollback) this
>>> supplementary commit since the state of the build before was likely broken
>>> or had incorrect behaviour. So, in this case, the whole set of PRs commits
>>> should be reverted which is not convenient and error-prone. It’s also
>>> expected that all checks were green before merging a PR (take a part flaky
>>> tests).
>>> >>> - They are not informative in terms of commit message. So it makes
>>> more hard to identify Git annotated code and how the lines of code are
>>> related together.
>>> >>>
>>> >>> Following this, I just want to briefly remind our Committers rules
>>> regarding PR merging [1].
>>> >>> Every commit:
>>> >>> - should do one thing and reflect it in commit message;
>>> >>> - should contain Jira Tag;
>>> >>> - all “fixup” and “address comments” type of commits should be
>>> squashed by author or committer before merging.
>>> >>>
>>> >>> Please, pay attention on what is finally committed and merged into
>>> our repository and it should help to keep our commit history clear, which
>>> will be transferred to saving a time of other developers in the end.
>>> >>>
>>> >>> [1]
>>> https://beam.apache.org/contribute/committer-guide/#finishing-touches
>>> >>>
>>> >>> Regards,
>>> >>> Alexey
>>> >>
>>> >>
>>>
>>
>>
Re: Git commit history: "fixup" commits
Posted by Tyson Hamilton <ty...@google.com>.
+1 to automating this!
On Wed, Aug 5, 2020 at 8:38 AM Alexey Romanenko <ar...@gmail.com>
wrote:
> Thanks Robert and Udi for links. Perhaps we should integrate such check if
> there are no objections from community. I’ll take a look on this more
> closely.
>
> On 4 Aug 2020, at 19:31, Udi Meiri <eh...@google.com> wrote:
>
> https://github.com/marketplace/actions/gs-commit-message-checker
>
> On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <ro...@google.com>
> wrote:
>
>> +1, thanks for the reminder.
>>
>> This should be really easy to automate, using
>> https://developer.github.com/webhooks/event-payloads/#pull_request to
>> give a warning when the change history is not sufficiently "clean."
>> I'm not sure where to host this though (or if it could be integrated
>> into jenkins--basically I'd just want to run a Python script with the
>> PR number (or better, just point to the local git repo and have the
>> master's commit handy) as another precommit).
>>
>> On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <ru...@google.com> wrote:
>> >
>> > +1 thanks Alexey.
>> >
>> > My apologies that I merged such a case recently (but not
>> intentionally). I tried to use the "squash and merge" button with a
>> consolidated commit message. After clicking the button, github showed
>> "failed to merge" and gave a retry button, and after clicking that retry
>> button, github magically switched to "create merge commit" approach thus
>> merged some fixup commits to the main branch.
>> >
>> > This is a rare case (I only encountered once). But I will pay more
>> attention next time. I could ask PR authors to squash their commits before
>> merging when it is possible.
>> >
>> >
>> > -Rui
>> >
>> > On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <
>> aromanenko.dev@gmail.com> wrote:
>> >>
>> >> Yes, good point, thanks Valentyn.
>> >>
>> >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <va...@google.com>
>> wrote:
>> >>
>> >> +1, thanks, Alexey.
>> >>
>> >> Also a reminder from the contributor guide: do not use the default
>> GitHub commit message for merge commits, which looks like:
>> >>
>> >> Merge pull request #1234 from some_user/transient_branch_name
>> >>
>> >> Instead, add the commit message into the subject line, for example:
>> "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
>> >>
>> >> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <
>> aromanenko.dev@gmail.com> wrote:
>> >>>
>> >>> Hi all,
>> >>>
>> >>> I’d like to attract your attention regarding our Git commit history
>> and related issue. A while ago I noticed that it started getting not very
>> clear and quite verbose comparing to how it was before. We have quite
>> significant amount of recent commits like “fix”, “address comments”,
>> “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a
>> prefix and actually is just supplementary commits to “main” and initial
>> commit of PR, added after several PRs review rounds.
>> >>>
>> >>> AFAIR, we already had several discussion in the past about this topic
>> and we agreed that we should avoid such commits in a final merge and have
>> only one (in most cases) or several (if necessary) logical commits that
>> should be atomic and properly explain what they do.
>> >>>
>> >>> Why these “tiny" commits are bad practice? Just several main reasons:
>> >>> - They pollute our git repository history and don’t give any
>> additional and useful further information;
>> >>> - They are not atomic and we can’t easily revert (rollback) this
>> supplementary commit since the state of the build before was likely broken
>> or had incorrect behaviour. So, in this case, the whole set of PRs commits
>> should be reverted which is not convenient and error-prone. It’s also
>> expected that all checks were green before merging a PR (take a part flaky
>> tests).
>> >>> - They are not informative in terms of commit message. So it makes
>> more hard to identify Git annotated code and how the lines of code are
>> related together.
>> >>>
>> >>> Following this, I just want to briefly remind our Committers rules
>> regarding PR merging [1].
>> >>> Every commit:
>> >>> - should do one thing and reflect it in commit message;
>> >>> - should contain Jira Tag;
>> >>> - all “fixup” and “address comments” type of commits should be
>> squashed by author or committer before merging.
>> >>>
>> >>> Please, pay attention on what is finally committed and merged into
>> our repository and it should help to keep our commit history clear, which
>> will be transferred to saving a time of other developers in the end.
>> >>>
>> >>> [1]
>> https://beam.apache.org/contribute/committer-guide/#finishing-touches
>> >>>
>> >>> Regards,
>> >>> Alexey
>> >>
>> >>
>>
>
>
Re: Git commit history: "fixup" commits
Posted by Alexey Romanenko <ar...@gmail.com>.
Thanks Robert and Udi for links. Perhaps we should integrate such check if there are no objections from community. I’ll take a look on this more closely.
> On 4 Aug 2020, at 19:31, Udi Meiri <eh...@google.com> wrote:
>
> https://github.com/marketplace/actions/gs-commit-message-checker <https://github.com/marketplace/actions/gs-commit-message-checker>
>
> On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <robertwb@google.com <ma...@google.com>> wrote:
> +1, thanks for the reminder.
>
> This should be really easy to automate, using
> https://developer.github.com/webhooks/event-payloads/#pull_request <https://developer.github.com/webhooks/event-payloads/#pull_request> to
> give a warning when the change history is not sufficiently "clean."
> I'm not sure where to host this though (or if it could be integrated
> into jenkins--basically I'd just want to run a Python script with the
> PR number (or better, just point to the local git repo and have the
> master's commit handy) as another precommit).
>
> On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <ruwang@google.com <ma...@google.com>> wrote:
> >
> > +1 thanks Alexey.
> >
> > My apologies that I merged such a case recently (but not intentionally). I tried to use the "squash and merge" button with a consolidated commit message. After clicking the button, github showed "failed to merge" and gave a retry button, and after clicking that retry button, github magically switched to "create merge commit" approach thus merged some fixup commits to the main branch.
> >
> > This is a rare case (I only encountered once). But I will pay more attention next time. I could ask PR authors to squash their commits before merging when it is possible.
> >
> >
> > -Rui
> >
> > On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <aromanenko.dev@gmail.com <ma...@gmail.com>> wrote:
> >>
> >> Yes, good point, thanks Valentyn.
> >>
> >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <valentyn@google.com <ma...@google.com>> wrote:
> >>
> >> +1, thanks, Alexey.
> >>
> >> Also a reminder from the contributor guide: do not use the default GitHub commit message for merge commits, which looks like:
> >>
> >> Merge pull request #1234 from some_user/transient_branch_name
> >>
> >> Instead, add the commit message into the subject line, for example: "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
> >>
> >> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <aromanenko.dev@gmail.com <ma...@gmail.com>> wrote:
> >>>
> >>> Hi all,
> >>>
> >>> I’d like to attract your attention regarding our Git commit history and related issue. A while ago I noticed that it started getting not very clear and quite verbose comparing to how it was before. We have quite significant amount of recent commits like “fix”, “address comments”, “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and actually is just supplementary commits to “main” and initial commit of PR, added after several PRs review rounds.
> >>>
> >>> AFAIR, we already had several discussion in the past about this topic and we agreed that we should avoid such commits in a final merge and have only one (in most cases) or several (if necessary) logical commits that should be atomic and properly explain what they do.
> >>>
> >>> Why these “tiny" commits are bad practice? Just several main reasons:
> >>> - They pollute our git repository history and don’t give any additional and useful further information;
> >>> - They are not atomic and we can’t easily revert (rollback) this supplementary commit since the state of the build before was likely broken or had incorrect behaviour. So, in this case, the whole set of PRs commits should be reverted which is not convenient and error-prone. It’s also expected that all checks were green before merging a PR (take a part flaky tests).
> >>> - They are not informative in terms of commit message. So it makes more hard to identify Git annotated code and how the lines of code are related together.
> >>>
> >>> Following this, I just want to briefly remind our Committers rules regarding PR merging [1].
> >>> Every commit:
> >>> - should do one thing and reflect it in commit message;
> >>> - should contain Jira Tag;
> >>> - all “fixup” and “address comments” type of commits should be squashed by author or committer before merging.
> >>>
> >>> Please, pay attention on what is finally committed and merged into our repository and it should help to keep our commit history clear, which will be transferred to saving a time of other developers in the end.
> >>>
> >>> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches <https://beam.apache.org/contribute/committer-guide/#finishing-touches>
> >>>
> >>> Regards,
> >>> Alexey
> >>
> >>
Re: Git commit history: "fixup" commits
Posted by Udi Meiri <eh...@google.com>.
https://github.com/marketplace/actions/gs-commit-message-checker
On Tue, Aug 4, 2020 at 10:25 AM Robert Bradshaw <ro...@google.com> wrote:
> +1, thanks for the reminder.
>
> This should be really easy to automate, using
> https://developer.github.com/webhooks/event-payloads/#pull_request to
> give a warning when the change history is not sufficiently "clean."
> I'm not sure where to host this though (or if it could be integrated
> into jenkins--basically I'd just want to run a Python script with the
> PR number (or better, just point to the local git repo and have the
> master's commit handy) as another precommit).
>
> On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <ru...@google.com> wrote:
> >
> > +1 thanks Alexey.
> >
> > My apologies that I merged such a case recently (but not intentionally).
> I tried to use the "squash and merge" button with a consolidated commit
> message. After clicking the button, github showed "failed to merge" and
> gave a retry button, and after clicking that retry button, github magically
> switched to "create merge commit" approach thus merged some fixup commits
> to the main branch.
> >
> > This is a rare case (I only encountered once). But I will pay more
> attention next time. I could ask PR authors to squash their commits before
> merging when it is possible.
> >
> >
> > -Rui
> >
> > On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <
> aromanenko.dev@gmail.com> wrote:
> >>
> >> Yes, good point, thanks Valentyn.
> >>
> >> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <va...@google.com>
> wrote:
> >>
> >> +1, thanks, Alexey.
> >>
> >> Also a reminder from the contributor guide: do not use the default
> GitHub commit message for merge commits, which looks like:
> >>
> >> Merge pull request #1234 from some_user/transient_branch_name
> >>
> >> Instead, add the commit message into the subject line, for example:
> "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
> >>
> >> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <
> aromanenko.dev@gmail.com> wrote:
> >>>
> >>> Hi all,
> >>>
> >>> I’d like to attract your attention regarding our Git commit history
> and related issue. A while ago I noticed that it started getting not very
> clear and quite verbose comparing to how it was before. We have quite
> significant amount of recent commits like “fix”, “address comments”,
> “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a
> prefix and actually is just supplementary commits to “main” and initial
> commit of PR, added after several PRs review rounds.
> >>>
> >>> AFAIR, we already had several discussion in the past about this topic
> and we agreed that we should avoid such commits in a final merge and have
> only one (in most cases) or several (if necessary) logical commits that
> should be atomic and properly explain what they do.
> >>>
> >>> Why these “tiny" commits are bad practice? Just several main reasons:
> >>> - They pollute our git repository history and don’t give any
> additional and useful further information;
> >>> - They are not atomic and we can’t easily revert (rollback) this
> supplementary commit since the state of the build before was likely broken
> or had incorrect behaviour. So, in this case, the whole set of PRs commits
> should be reverted which is not convenient and error-prone. It’s also
> expected that all checks were green before merging a PR (take a part flaky
> tests).
> >>> - They are not informative in terms of commit message. So it makes
> more hard to identify Git annotated code and how the lines of code are
> related together.
> >>>
> >>> Following this, I just want to briefly remind our Committers rules
> regarding PR merging [1].
> >>> Every commit:
> >>> - should do one thing and reflect it in commit message;
> >>> - should contain Jira Tag;
> >>> - all “fixup” and “address comments” type of commits should be
> squashed by author or committer before merging.
> >>>
> >>> Please, pay attention on what is finally committed and merged into our
> repository and it should help to keep our commit history clear, which will
> be transferred to saving a time of other developers in the end.
> >>>
> >>> [1]
> https://beam.apache.org/contribute/committer-guide/#finishing-touches
> >>>
> >>> Regards,
> >>> Alexey
> >>
> >>
>
Re: Git commit history: "fixup" commits
Posted by Robert Bradshaw <ro...@google.com>.
+1, thanks for the reminder.
This should be really easy to automate, using
https://developer.github.com/webhooks/event-payloads/#pull_request to
give a warning when the change history is not sufficiently "clean."
I'm not sure where to host this though (or if it could be integrated
into jenkins--basically I'd just want to run a Python script with the
PR number (or better, just point to the local git repo and have the
master's commit handy) as another precommit).
On Tue, Aug 4, 2020 at 10:10 AM Rui Wang <ru...@google.com> wrote:
>
> +1 thanks Alexey.
>
> My apologies that I merged such a case recently (but not intentionally). I tried to use the "squash and merge" button with a consolidated commit message. After clicking the button, github showed "failed to merge" and gave a retry button, and after clicking that retry button, github magically switched to "create merge commit" approach thus merged some fixup commits to the main branch.
>
> This is a rare case (I only encountered once). But I will pay more attention next time. I could ask PR authors to squash their commits before merging when it is possible.
>
>
> -Rui
>
> On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <ar...@gmail.com> wrote:
>>
>> Yes, good point, thanks Valentyn.
>>
>> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <va...@google.com> wrote:
>>
>> +1, thanks, Alexey.
>>
>> Also a reminder from the contributor guide: do not use the default GitHub commit message for merge commits, which looks like:
>>
>> Merge pull request #1234 from some_user/transient_branch_name
>>
>> Instead, add the commit message into the subject line, for example: "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
>>
>> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <ar...@gmail.com> wrote:
>>>
>>> Hi all,
>>>
>>> I’d like to attract your attention regarding our Git commit history and related issue. A while ago I noticed that it started getting not very clear and quite verbose comparing to how it was before. We have quite significant amount of recent commits like “fix”, “address comments”, “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and actually is just supplementary commits to “main” and initial commit of PR, added after several PRs review rounds.
>>>
>>> AFAIR, we already had several discussion in the past about this topic and we agreed that we should avoid such commits in a final merge and have only one (in most cases) or several (if necessary) logical commits that should be atomic and properly explain what they do.
>>>
>>> Why these “tiny" commits are bad practice? Just several main reasons:
>>> - They pollute our git repository history and don’t give any additional and useful further information;
>>> - They are not atomic and we can’t easily revert (rollback) this supplementary commit since the state of the build before was likely broken or had incorrect behaviour. So, in this case, the whole set of PRs commits should be reverted which is not convenient and error-prone. It’s also expected that all checks were green before merging a PR (take a part flaky tests).
>>> - They are not informative in terms of commit message. So it makes more hard to identify Git annotated code and how the lines of code are related together.
>>>
>>> Following this, I just want to briefly remind our Committers rules regarding PR merging [1].
>>> Every commit:
>>> - should do one thing and reflect it in commit message;
>>> - should contain Jira Tag;
>>> - all “fixup” and “address comments” type of commits should be squashed by author or committer before merging.
>>>
>>> Please, pay attention on what is finally committed and merged into our repository and it should help to keep our commit history clear, which will be transferred to saving a time of other developers in the end.
>>>
>>> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches
>>>
>>> Regards,
>>> Alexey
>>
>>
Re: Git commit history: "fixup" commits
Posted by Alexey Romanenko <ar...@gmail.com>.
Hmm, this is strange if it was “mergeable” before, never had this.
Thanks Rui!
> On 4 Aug 2020, at 19:09, Rui Wang <ru...@google.com> wrote:
>
> +1 thanks Alexey.
>
> My apologies that I merged such a case recently (but not intentionally). I tried to use the "squash and merge" button with a consolidated commit message. After clicking the button, github showed "failed to merge" and gave a retry button, and after clicking that retry button, github magically switched to "create merge commit" approach thus merged some fixup commits to the main branch.
>
> This is a rare case (I only encountered once). But I will pay more attention next time. I could ask PR authors to squash their commits before merging when it is possible.
>
>
> -Rui
>
> On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <aromanenko.dev@gmail.com <ma...@gmail.com>> wrote:
> Yes, good point, thanks Valentyn.
>
>> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <valentyn@google.com <ma...@google.com>> wrote:
>>
>> +1, thanks, Alexey.
>>
>> Also a reminder from the contributor guide: do not use the default GitHub commit message for merge commits, which looks like:
>>
>> Merge pull request #1234 from some_user/transient_branch_name
>>
>> Instead, add the commit message into the subject line, for example: "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
>>
>> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <aromanenko.dev@gmail.com <ma...@gmail.com>> wrote:
>> Hi all,
>>
>> I’d like to attract your attention regarding our Git commit history and related issue. A while ago I noticed that it started getting not very clear and quite verbose comparing to how it was before. We have quite significant amount of recent commits like “fix”, “address comments”, “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and actually is just supplementary commits to “main” and initial commit of PR, added after several PRs review rounds.
>>
>> AFAIR, we already had several discussion in the past about this topic and we agreed that we should avoid such commits in a final merge and have only one (in most cases) or several (if necessary) logical commits that should be atomic and properly explain what they do.
>>
>> Why these “tiny" commits are bad practice? Just several main reasons:
>> - They pollute our git repository history and don’t give any additional and useful further information;
>> - They are not atomic and we can’t easily revert (rollback) this supplementary commit since the state of the build before was likely broken or had incorrect behaviour. So, in this case, the whole set of PRs commits should be reverted which is not convenient and error-prone. It’s also expected that all checks were green before merging a PR (take a part flaky tests).
>> - They are not informative in terms of commit message. So it makes more hard to identify Git annotated code and how the lines of code are related together.
>>
>> Following this, I just want to briefly remind our Committers rules regarding PR merging [1].
>> Every commit:
>> - should do one thing and reflect it in commit message;
>> - should contain Jira Tag;
>> - all “fixup” and “address comments” type of commits should be squashed by author or committer before merging.
>>
>> Please, pay attention on what is finally committed and merged into our repository and it should help to keep our commit history clear, which will be transferred to saving a time of other developers in the end.
>>
>> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches <https://beam.apache.org/contribute/committer-guide/#finishing-touches>
>>
>> Regards,
>> Alexey
>
Re: Git commit history: "fixup" commits
Posted by Rui Wang <ru...@google.com>.
+1 thanks Alexey.
My apologies that I merged such a case recently (but not intentionally). I
tried to use the "squash and merge" button with a consolidated commit
message. After clicking the button, github showed "failed to merge" and
gave a retry button, and after clicking that retry button, github magically
switched to "create merge commit" approach thus merged some fixup commits
to the main branch.
This is a rare case (I only encountered once). But I will pay more
attention next time. I could ask PR authors to squash their commits before
merging when it is possible.
-Rui
On Tue, Aug 4, 2020 at 9:40 AM Alexey Romanenko <ar...@gmail.com>
wrote:
> Yes, good point, thanks Valentyn.
>
> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <va...@google.com> wrote:
>
> +1, thanks, Alexey.
>
> Also a reminder from the contributor guide: do not use the default GitHub
> commit message for merge commits, which looks like:
>
> Merge pull request #1234 from some_user/transient_branch_name
>
> Instead, add the commit message into the subject line, for example: "Merge
> pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
>
> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <ar...@gmail.com>
> wrote:
>
>> Hi all,
>>
>> I’d like to attract your attention regarding our Git commit history and
>> related issue. A while ago I noticed that it started getting not very clear
>> and quite verbose comparing to how it was before. We have quite significant
>> amount of recent commits like “fix”, “address comments”, “typo”,
>> “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and
>> actually is just supplementary commits to “main” and initial commit of PR,
>> added after several PRs review rounds.
>>
>> AFAIR, we already had several discussion in the past about this topic and
>> we agreed that we should avoid such commits in a final merge and have only
>> one (in most cases) or several (if necessary) logical commits that should
>> be atomic and properly explain what they do.
>>
>> Why these “tiny" commits are bad practice? Just several main reasons:
>> - They pollute our git repository history and don’t give any additional
>> and useful further information;
>> - They are not atomic and we can’t easily revert (rollback) this
>> supplementary commit since the state of the build before was likely broken
>> or had incorrect behaviour. So, in this case, the whole set of PRs commits
>> should be reverted which is not convenient and error-prone. It’s also
>> expected that all checks were green before merging a PR (take a part flaky
>> tests).
>> - They are not informative in terms of commit message. So it makes more
>> hard to identify Git annotated code and how the lines of code are related
>> together.
>>
>> Following this, I just want to briefly remind our Committers rules
>> regarding PR merging [1].
>> Every commit:
>> - should do one thing and reflect it in commit message;
>> - should contain Jira Tag;
>> - all “fixup” and “address comments” type of commits should be squashed
>> by author or committer before merging.
>>
>> Please, pay attention on what is finally committed and merged into our
>> repository and it should help to keep our commit history clear, which will
>> be transferred to saving a time of other developers in the end.
>>
>> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches
>>
>> Regards,
>> Alexey
>>
>
>
Re: Git commit history: "fixup" commits
Posted by Alexey Romanenko <ar...@gmail.com>.
Yes, good point, thanks Valentyn.
> On 4 Aug 2020, at 18:29, Valentyn Tymofieiev <va...@google.com> wrote:
>
> +1, thanks, Alexey.
>
> Also a reminder from the contributor guide: do not use the default GitHub commit message for merge commits, which looks like:
>
> Merge pull request #1234 from some_user/transient_branch_name
>
> Instead, add the commit message into the subject line, for example: "Merge pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
>
> On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <aromanenko.dev@gmail.com <ma...@gmail.com>> wrote:
> Hi all,
>
> I’d like to attract your attention regarding our Git commit history and related issue. A while ago I noticed that it started getting not very clear and quite verbose comparing to how it was before. We have quite significant amount of recent commits like “fix”, “address comments”, “typo”, “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and actually is just supplementary commits to “main” and initial commit of PR, added after several PRs review rounds.
>
> AFAIR, we already had several discussion in the past about this topic and we agreed that we should avoid such commits in a final merge and have only one (in most cases) or several (if necessary) logical commits that should be atomic and properly explain what they do.
>
> Why these “tiny" commits are bad practice? Just several main reasons:
> - They pollute our git repository history and don’t give any additional and useful further information;
> - They are not atomic and we can’t easily revert (rollback) this supplementary commit since the state of the build before was likely broken or had incorrect behaviour. So, in this case, the whole set of PRs commits should be reverted which is not convenient and error-prone. It’s also expected that all checks were green before merging a PR (take a part flaky tests).
> - They are not informative in terms of commit message. So it makes more hard to identify Git annotated code and how the lines of code are related together.
>
> Following this, I just want to briefly remind our Committers rules regarding PR merging [1].
> Every commit:
> - should do one thing and reflect it in commit message;
> - should contain Jira Tag;
> - all “fixup” and “address comments” type of commits should be squashed by author or committer before merging.
>
> Please, pay attention on what is finally committed and merged into our repository and it should help to keep our commit history clear, which will be transferred to saving a time of other developers in the end.
>
> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches <https://beam.apache.org/contribute/committer-guide/#finishing-touches>
>
> Regards,
> Alexey
Re: Git commit history: "fixup" commits
Posted by Valentyn Tymofieiev <va...@google.com>.
+1, thanks, Alexey.
Also a reminder from the contributor guide: do not use the default GitHub
commit message for merge commits, which looks like:
Merge pull request #1234 from some_user/transient_branch_name
Instead, add the commit message into the subject line, for example: "Merge
pull request #1234: [BEAM-7873] Fix the foo bizzle bazzle".
On Tue, Aug 4, 2020 at 7:13 AM Alexey Romanenko <ar...@gmail.com>
wrote:
> Hi all,
>
> I’d like to attract your attention regarding our Git commit history and
> related issue. A while ago I noticed that it started getting not very clear
> and quite verbose comparing to how it was before. We have quite significant
> amount of recent commits like “fix”, “address comments”, “typo”,
> “spotless”, etc. Most of them also doesn’t contain Jira Tag as a prefix and
> actually is just supplementary commits to “main” and initial commit of PR,
> added after several PRs review rounds.
>
> AFAIR, we already had several discussion in the past about this topic and
> we agreed that we should avoid such commits in a final merge and have only
> one (in most cases) or several (if necessary) logical commits that should
> be atomic and properly explain what they do.
>
> Why these “tiny" commits are bad practice? Just several main reasons:
> - They pollute our git repository history and don’t give any additional
> and useful further information;
> - They are not atomic and we can’t easily revert (rollback) this
> supplementary commit since the state of the build before was likely broken
> or had incorrect behaviour. So, in this case, the whole set of PRs commits
> should be reverted which is not convenient and error-prone. It’s also
> expected that all checks were green before merging a PR (take a part flaky
> tests).
> - They are not informative in terms of commit message. So it makes more
> hard to identify Git annotated code and how the lines of code are related
> together.
>
> Following this, I just want to briefly remind our Committers rules
> regarding PR merging [1].
> Every commit:
> - should do one thing and reflect it in commit message;
> - should contain Jira Tag;
> - all “fixup” and “address comments” type of commits should be squashed by
> author or committer before merging.
>
> Please, pay attention on what is finally committed and merged into our
> repository and it should help to keep our commit history clear, which will
> be transferred to saving a time of other developers in the end.
>
> [1] https://beam.apache.org/contribute/committer-guide/#finishing-touches
>
> Regards,
> Alexey
>