Difference between revisions of "Creating a Samba patch series"

m (Dmulder moved page Creating Git Commits to Creating Gitlab Merge Requests: Better description of page purpose)
(3 intermediate revisions by the same user not shown)
Line 2: Line 2:
  
 
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 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.
 
== What this page is not ==
 
  
 
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.
 
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.
Line 9: Line 7:
 
= What a commit should include =
 
= What a commit should include =
  
Each git commit should be succinct and only do one thing.
+
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.
 +
 
 +
See the [[Contribute#A_good_commit_message|Contribute page]] for details on how to write a good commit message.
  
 
== Bisectability ==
 
== Bisectability ==
Line 16: Line 16:
  
 
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.
 
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.
 +
 +
= Effective Review =
 +
 +
It is the submitter's responsibility to make the most of code review. The reviewer will likely only point out a single instance of any given problem. The submitter is expected to find every instance of this mistake and fix it throughout the code.
  
 
= Examples =
 
= Examples =
Line 28: Line 32:
  
 
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!
 
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!
 +
 +
=== 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).

Revision as of 19:38, 18 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 commit should include

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.

See the Contribute page for details on how to write a good 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 git bisect on the history, allowing us to find commits that introduce bugs.

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.

Effective Review

It is the submitter's responsibility to make the most of code review. The reviewer will likely only point out a single instance of any given problem. The submitter is expected to find every instance of this mistake and fix it throughout the code.

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!

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).