Creating a Samba patch series: Difference between revisions

From SambaWiki
(Created page with "= Introduction = When contributing to Samba, there are quality expectations that need to be met in order for your patches to be accepted. Please review these guidelines ''bef...")
 
m (italisise rationalle)
 
(28 intermediate revisions by 2 users not shown)
Line 1: Line 1:
= Introduction =
= Introduction =


When contributing to Samba, there are quality expectations that need to be met in order for your patches to be accepted. Please review these guidelines ''before'' writing your first commits, otherwise you may need to rebase your patches.
When [[Contribute|contributing to Samba]], there are quality expectations that need to be met in order for your patches to be accepted. Please review these guidelines ''before'' writing your first commits, otherwise you may need to rebase your patches.


This page is not meant to be a technical overview about using git, there are already [https://git-scm.com/docs plenty of resources] for learning to use git. It's expected when you reach this page that you already know these concepts.
== What this page is not ==


= What a patch series should include =
This page is not meant to be a technical overview about using git, there are already plenty of resources for learning to use git. It's expected when you reach this page that you already know these concepts.


Samba development is focussed around the ''patch series'', which then becomes a ''merge request''. In that series, each git commit should be succinct and only do one thing. Each new feature or bug fix should include a second commit containing a test to validate the change.
== What a commit should include ==


== A good commit message ==
Each git commit should be succinct and only do one thing.

{{:A_Good_Git_Commit_Message}}

== Bisectability ==

The problem with merge requests is that the '''Changes''' view shows a complete product, without intermediate stages. In Samba, we also care about the history when getting to the final product. Small succinct commits make it possible to run [https://git-scm.com/docs/git-bisect git bisect] on the history, allowing us to find commits that introduce bugs.

'''This means that each and every commit must compile on its own'''.

Each commit in the branch should also be expected to '''pass a full CI'''. There is ''not'' automatic verification of this, but running a build and the relevant tests locally after each commit is advised on larger merge requests. Use [https://git-scm.com/docs/git-rebase git rebase] to do this.

Sometimes developers will argue that reviewers should take the end result as a whole, and evaluate a patch series on it's functionality. We recognize the effort put into preparing patches for Samba, but requests '''cannot be accepted''' that are not '''bisectable'''.

== Write a story (for review) ==

A merge request is a story, told to the reviewer. It is important that '''both''' the MR description and the patches tell the story, as eventually the MR description will be lost but the git commits will remain in the history.

Each commit should tell one step at a time what is being changed, and why. Include some further details of the over-arching design or bug being fixed in a significant commit.

The idea is to bring the reviewer to a shared understanding that each commit is safe, sensible and reasonable alone (even if only fully useful in combination with the later work).

== Polished pearls ==

The end result of a MR is like [https://upload.wikimedia.org/wikipedia/commons/thumb/c/cd/Spine_Of_The_Centipede_using_Swarvoski_Pearls.jpg/698px-Spine_Of_The_Centipede_using_Swarvoski_Pearls.jpg a string of pearls]. Each pearl (commit) individually polished, then assembled as a string. The quality of the end result is almost entirely due to the quality of each element.

''We say this in particular to those submitting a larger change to Samba. Don't let this intimidate you on your first patch, but think about this allegory as you start to prepare a larger merge request.''

== Good patches and better patches: Samba is git patches as performance art ==

It might seem over the top, but one can think of Samba patches not just as a change to code, but an artistic expression, a [https://lists.samba.org/archive/samba-technical/2018-July/129353.html gift to future developers].

''We say this not to discourage, but to more clearly set our expectations as our experience is that a well-crafted patch series will breeze though [[CodeReview|code review]] in a way a patch with issues will not.''

= Tests =

Tests for the changes being made should be included in your merge request '''in their own commit'''.

== Tests before changes ==
Ideally, when changing tests in parts of Samba using the main [[The Samba Selftest system|make test]] the test should be introduced '''first''' with an entry in a new file in <code>selftest/knownfail.d/</code> to cover the failure, which is then removed when the issue is resolved in a later commit. This helps show that the test actually covers the change being made.

''Not all parts of Samba use this system however, most notably [[Running_CTDB_tests|CTDB]] and [[LDB]].''

== How to write tests==

{{:Writing_Tests_summary}}

= Samba git style: Centralised / Forking=

Samba is best described as using the [https://www.atlassian.com/git/tutorials/comparing-workflows#centralized-workflow centralised git workflow], with branches only for major releases.

That said, development happens in branches of either private (for new developers) or a the [https://gitlab.com/samba-team/devel/samba shared development repo]. In this sense Samba is using the [https://www.atlassian.com/git/tutorials/comparing-workflows/forking-workflow forking git workflow].

== '''git rebase origin/master''' often ==

Therefore a developer is expected to start work from '''master''' and <code>git rebase origin/master</code> often, because to be accepted the changes must do so eventually, and it is a pain if the changes can't be merged after all the work on code review. ''GitLab will also indicate that the code cannot be merged in the UI''.

= Examples =

=== What not to do ===

Here is an actual example from an open source project (identifying information has been removed), of a poorly created merge request:

[[File:Github_mr_1.png]]

[[File:Github_mr_2.png]]

This request contains '''47''' commits, the merge request title does not explain what the code change is doing, and the final result is a single line code change!

Assuming the cumulative code change listed here is what was intended (which isn't at all obvious from the merge request title), these 47 commits should have been rebased into a single commit, and likely a test should have been written to test the change.

=== A good example ===

Here is an example of a Samba merge request which was created correctly:

[[File:Gitlab-mr-1.png]]

[[File:Gitlab-mr-2.png]]

This request only has '''2''' commits, the first commit being the bug fix, and the second commit containing tests for the issue. The merge request title is very specific, mentioning the component being patched, the function name being patched, and explaining very briefly what was fixed. The patch contents only modify the single thing being fixed (excluding the tests).

Latest revision as of 06:58, 19 June 2020

Introduction

When contributing to Samba, there are quality expectations that need to be met in order for your patches to be accepted. Please review these guidelines before writing your first commits, otherwise you may need to rebase your patches.

This page is not meant to be a technical overview about using git, there are already plenty of resources for learning to use git. It's expected when you reach this page that you already know these concepts.

What a patch series should include

Samba development is focussed around the patch series, which then becomes a merge request. In that series, each git commit should be succinct and only do one thing. Each new feature or bug fix should include a second commit containing a test to validate the change.

A good commit message

Good commit messages are very important. Commits should always be separated in meaningful pieces. Please make sure that your commit messages include at least the following information:

  • A short summary of the fix/feature including the Samba version and component followed by a blank line.
  • A more verbose description.
  • A bug number if available (required for backports to a released branch, so please file a bug first)
  • The description for the release notes starting with "RN: ". Please make sure that administrators understand the issue.
  • Your Signed-off-by tag per the Samba copyright policy

Example:

s3:smbd: Add new funky shiny and groovy feature

The funky shiny and groovy feature has been added to
be able to do whatever you like. A typical usecase is the
following.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=1
RN: Fix problem xyz.

Signed-off-by: Author <author@example.org>

Bisectability

The problem with merge requests is that the Changes view shows a complete product, without intermediate stages. In Samba, we also care about the history when getting to the final product. Small succinct commits make it possible to run git bisect on the history, allowing us to find commits that introduce bugs.

This means that each and every commit must compile on its own.

Each commit in the branch should also be expected to pass a full CI. There is not automatic verification of this, but running a build and the relevant tests locally after each commit is advised on larger merge requests. Use git rebase to do this.

Sometimes developers will argue that reviewers should take the end result as a whole, and evaluate a patch series on it's functionality. We recognize the effort put into preparing patches for Samba, but requests cannot be accepted that are not bisectable.

Write a story (for review)

A merge request is a story, told to the reviewer. It is important that both the MR description and the patches tell the story, as eventually the MR description will be lost but the git commits will remain in the history.

Each commit should tell one step at a time what is being changed, and why. Include some further details of the over-arching design or bug being fixed in a significant commit.

The idea is to bring the reviewer to a shared understanding that each commit is safe, sensible and reasonable alone (even if only fully useful in combination with the later work).

Polished pearls

The end result of a MR is like a string of pearls. Each pearl (commit) individually polished, then assembled as a string. The quality of the end result is almost entirely due to the quality of each element.

We say this in particular to those submitting a larger change to Samba. Don't let this intimidate you on your first patch, but think about this allegory as you start to prepare a larger merge request.

Good patches and better patches: Samba is git patches as performance art

It might seem over the top, but one can think of Samba patches not just as a change to code, but an artistic expression, a gift to future developers.

We say this not to discourage, but to more clearly set our expectations as our experience is that a well-crafted patch series will breeze though code review in a way a patch with issues will not.

Tests

Tests for the changes being made should be included in your merge request in their own commit.

Tests before changes

Ideally, when changing tests in parts of Samba using the main make test the test should be introduced first with an entry in a new file in selftest/knownfail.d/ to cover the failure, which is then removed when the issue is resolved in a later commit. This helps show that the test actually covers the change being made.

Not all parts of Samba use this system however, most notably CTDB and LDB.

How to write tests

Most changes to Samba should have a test to demonstrate the bug being fixed, or test the feature being added. Most tests are run using 'make test' from a Samba source tree.

See writing and running Samba tests but in particular:

  • Writing Torture Tests: smbtorture in the source4/torture directory and provides direct C protocol tests.
  • Writing Python Tests: If the protocol under test is DCERPC, then PIDL will have already auto-generated Python bindings. Likewise LDAP is easily accessed via LDB.
  • Writing cmocka Tests: Idea for unit tests of C functions.
  • LDB: Tests for LDB are in lib/ldb/tests and are run from make test within lib/ldb
  • CTDB: Tests for CTDB are written as shell scripts under ctdb/tests and are run from make test within ctdb

Samba git style: Centralised / Forking

Samba is best described as using the centralised git workflow, with branches only for major releases.

That said, development happens in branches of either private (for new developers) or a the shared development repo. In this sense Samba is using the forking git workflow.

git rebase origin/master often

Therefore a developer is expected to start work from master and git rebase origin/master often, because to be accepted the changes must do so eventually, and it is a pain if the changes can't be merged after all the work on code review. GitLab will also indicate that the code cannot be merged in the UI.

Examples

What not to do

Here is an actual example from an open source project (identifying information has been removed), of a poorly created merge request:

Github mr 1.png

Github mr 2.png

This request contains 47 commits, the merge request title does not explain what the code change is doing, and the final result is a single line code change!

Assuming the cumulative code change listed here is what was intended (which isn't at all obvious from the merge request title), these 47 commits should have been rebased into a single commit, and likely a test should have been written to test the change.

A good example

Here is an example of a Samba merge request which was created correctly:

Gitlab-mr-1.png

Gitlab-mr-2.png

This request only has 2 commits, the first commit being the bug fix, and the second commit containing tests for the issue. The merge request title is very specific, mentioning the component being patched, the function name being patched, and explaining very briefly what was fixed. The patch contents only modify the single thing being fixed (excluding the tests).