When contributing new code to open source projects, communicating
the change should be made is almost as
important as the code itself. The bigger the PR, the truer this becomes.
In this post, I’ll explain what git commit farming is, and how it will help you get your PRs merged into Tari faster,
Tari is hosted on
Github
, which started out as a nice web-based
. Things changed over time and today Github does a lot more than
proxy git pulls and pushes. But deep down at the core, git is still there, doing the grunt work behind Github.
Git is a really complex tool. I doubt I know 25% of its full capability. So this post will focus on one particular
aspect of git that is really valuable in large open-source project management: farming git logs.
Hello logs
If you know what git logs are and how to read them, skip ahead to the next section. There are more goodies for you
down there, I promise.
Git logs are the messages that get written every time a commit is made in a repository. You make a commit when running
– your editor will open and you’ll be asked to write a message
– git will write the commit message itself. And for this reason, rebases are often preferred over
Github PR descriptions – when PRs are merged, Github will use the PR description as the commit message (possibly
throwing away intermediate commit messages that the author has provided – more on that later).
To read the commit log, simply run this command from a terminal:
Here’s a snippet from the tari repo’s logs:
commit 2636cb56ddf1b67ed7bf4c4aea8c05f9369b11d0 (HEAD -> development, origin/development, origin/HEAD)
Author: mrnaveira <47919901+mrnaveira@users.noreply.github.com>
Date: Mon May 30 14:54:40 2022 +0100
feat(core): add contract acceptance utxo features (#4145)
Description
– Adds `ContractAcceptance` struct to the side-chain output features
– Implements consensus encoding/decoding for `ContractAcceptance`
– Updates related gRPC types and conversions
Motivation and Context
Create structs for contract acceptance in the side-chain features.
How Has This Been Tested?
– New unit test for consensus encoding/decoding of the `ContractAcceptance`
– Existing unit/integration test pass
commit 2b1a69a9219f29472d6fa26b1b2350be7880b11a
Author: stringhandler
Date: Mon May 30 15:41:10 2022 +0200
fix: move peer dbs into sub folders (#4147)
Description
Add sub folders for each application’s peer DB
Motivation and Context
If you run the base node and validator node in the same folder, you will get a file lock error. I added the same folder to the console wallet to keep it the same
How Has This Been Tested?
commit f79d383533c2e9c4db95d4a13973992c9a8739ef
Author: Denis Kolodin
Date: Mon May 30 15:35:28 2022 +0300
feat: add encrypted_value to the UnblindedOutput (#4142)
Description
The PR adds extra encrypted field to `UnblindedOutput` structs.
– Added `EncryptedValue` struct
– Implemented `ConsensusEncode`, `ConsensusDecode` and `ByteArray` for it
– Added the `encrypted_value` field of that type to the `UnblindedOutput` struct
– Added an extra parameter to metadata signature calculation methods
– Added an extra field to the `GRPC` protocol (`UnblindedOutput` type)
– Added an extra column and migrations for the `outputs` table
– The types `OutputSql` and `NewOutputSql` updated to store and restore the `EncryptedValue`
1. The value of `encrypted_value` passed as a parameter to the metadata signature evaluation but has not taken into account yet, because it will break the verification process of the `TransactionOutput` and we can complete that when the same `encrypted_value` field will be added to the `TransactionOutput` struct in the further PRs.
2. Real encryption process is not implemented yet. Instead of that I’ve added the method `todo_encrypt_from` that I used to mark the places where we have to put an encryption service and call it.
Motivation and Context
Add an encrypted amount to the `TransactionOutput`.
How Has This Been Tested?
commit 5cc9a406a807cfaeaba4e71e2af13fa3b4bde2ff
Merge: 3656c3247 987972cff
Author: Mike the Tike
Date: Mon May 30 12:51:59 2022 +0200
Merge branch ‘testnet-dibbler’ into development
# base_layer/wallet_ffi/src/lib.rs
# base_layer/wallet_ffi/wallet.h
You will notice a couple of things:
The author, commit hash and timestamp are all contained in the messages.
You can get a clear sense of what the commit contributed. In this snippet, the descriptions all come from Github
PR descriptions (because they use the
template the project has set up).
The last commit is a merge-commit, where changes from the
branch were pulled into
Log summaries
Sometimes, you don’t want such detailed log data. It turns out that there’s an enormous amount of flexibility in how
logs are displayed. The
command has, per git’s
, a gazillion tweaks and options.
A commonly-used one is to just print a one-line summary of each commit:
2636cb56d (HEAD -> development, origin/development, origin/HEAD) feat(core): add contract acceptance utxo features (#4145)
2b1a69a92 fix: move peer dbs into sub folders (#4147)
f79d38353 feat: add encrypted_value to the UnblindedOutput (#4142)
5cc9a406a Merge branch ‘testnet-dibbler’ into development
3656c3247 ci: docker image build for x86-64 & arm64 from x86-64 (#4135)
310a2d202 feat: scan base node for constitutions (#4144)
b4991a471 (leet/development) feat(wallet): new command to publish a contract definition transaction (#4133)
52ecb4940 chore(deps): bump async from 2.6.3 to 2.6.4 in /applications/tari_collectibles/web-app (#4143)
71c3a8a8f test: coverage for rolling average time struct (#4116)
e5d20f7ad test: coverage mocks for files common configuration files (PR #4116) (#4130)
d1827ea41 chore: update croaring and randomx (#4136)
ada31432e feat(core)!: add side-chain features and constitution to UTXOs (#4134)
446406491 fix(integration_test): fix wallet-cli integration tests (#4132)
682aa5d0e feat(core): impl consensus encoding for bool (#4120)
Print commits between branches
You can log the set of commits that would be included in a
of two commits are branches by running
$ git log commitA…commitB
Let’s consider a dummy repo with the following commit history:
(main): A -> B -> C -> D
(branch) \-> B’ -> C’
$ git log –oneline main
4888619 (HEAD -> main) D
$ git –oneline branch
Since we branched off at
, the commits that would be involved in a branch are
git log –oneline main…branch
$ git log –oneline main…branch
4888619 (HEAD -> main) D
The order doesn’t matter.
gives the same result as
There are lots of other ways to use
. Read the docs and try some of them out!
PR descriptions vs. Git logs
Depending on how PRs are merged in Github, the git messages get thrown away in favour of the PR description. Since
we treat the git commit messages as very important, we retain them and add them to the end of the PR description
when merging PRs from Github.
Producing great git commit messages
The git log is a really important resource in understanding not only how a code base works, but understanding its
history and gaining an appreciation for the reasoning behind why changes were made.
So how do we produce great git message logs when it’s impossible to anticipate how coding is going to
progress when writing a new feature? You always end up with dozens of little commits that are just tiny
bugfixes, formatting issues and any number of things that deviate from the platonic description of the code base’s
The good news is that this is fine. Really.
Starting with a plan
Let’s assume we’re writing a new feature in our codebase. As usual, we start from a new branch:
(main)$ git checkout -b my_feature
Switched to a new branch ‘my_feature’
We begin by writing a stub API. After writing this, I make a commit:
commit dfa1ca73867ed36ad3f7638b71e95ed23329499f (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 13:10:59 2022 +0100
Add sketelal API for Feature
No tests or code yest, just the
* `add_data` function
* `process_data` function
Off to the races! Ok, now to add some tests…
commit c05ad1a1a8f456fa398ee73bfc03ae1e580900b8 (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 13:13:15 2022 +0100
Add import tests
* add_data returns Ok(n) returning the number of records when adding
* `process_data` returns Ok() on succesful processing
Both these tests currently fail
Cool. Now to implement the body of the API so that the tests pass.
Hmm. This is taking longer than anticipated. There’s some issue on the backend that I can’t fix right now.
So let’s commit what we’ve done and continue tomorrow..
commit 563ee327276ba199b66053bb668cf60b1c49b0a5 (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 13:16:33 2022 +0100
WIP Implement add_data
add_data ingests data into the store and returns the total number of
WIP Backend times out sometimes. Investigating
Fresh day, fresh issues. We’ve identified the problem and fixed it on the backend.
commit 7323efc6db54d63efc2ff0f5b67f2ff1677c3b73 (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 13:30:36 2022 +0100
Fix timeout issues on backend
The backend expected inserts to be done in negative time.
This fix sets a reasonable timeout of 500ms
Cherry-picking to submit an independent PR
This bugfix should
, so let’s create a PR now quickly by cherry-picking this commit:
Switched to branch ‘main’
(main)$ git checkout -b timeout-fix
Switched to a new branch ‘timeout-fix’
(timeout-fix) $ git cherry-pick 7323efc6db54d63efc2ff0f5b67f2ff1677c3b73
[timeout-fix 6e9e937] Fix timeout issues on backend
Date: Wed Jun 1 13:30:36 2022 +0100
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 backend_fix.rs
(timeout-fix) $ git log
commit 6e9e93784c55f056f813978b0673ecc8edd3aedc (HEAD -> timeout-fix)
Author: CjS77
Date: Wed Jun 1 13:30:36 2022 +0100
Fix timeout issues on backend
The backend expected inserts to be done in negative time.
This fix sets a reasonable timeout of 500ms
commit 4888619a51c7ce128dbe5f6a60b55b87cd152363 (main)
Author: CjS77
Date: Tue May 31 15:18:45 2022 +0100
Awesome! Our fix can now be pushed as a standalone PR!
It gets reviewed and merged pretty quickly. So I can pull from the remote server and see that the main branch now
(main)$ git log –oneline
6e9e937 (HEAD -> main, timeout-fix) Fix timeout issues on backend
Tidy up the obsolete branch
Great! Time to nuke that
branch (it’s no longer needed) and jump back into
(main) $ git branch -d timeout-fix
Deleted branch timeout-fix (was 6e9e937).
(main) $ git checkout my_feature
Switched to branch ‘my_feature’
Continue with the feature development
After a few hours, I’ve fixed the implementation and committed the changes:
commit e668f3f5b2ec11a137fc84c8936e50360d476b21 (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 13:47:36 2022 +0100
Complete data ingestion
Data ingestion works. Increasing the timeout allows the backend to
respond, and the add_data test now passes.
Things start getting messy
Almost ready for a PR.. but wait! Let’s run
first, since it’s a CI requirement that clippy gives the
green light, so best to cut off any issues here.
Shoot! There are some clippy lints. No worries. That’s a quick fix…
commit 623e3573e1160c1791798c969e1bfe00acc37b67 (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 13:50:24 2022 +0100
Clippy fixes
Functional programming FtW
Crumbs. While I was waiting for tests to run I realised that I should also add a
API, along with some tests of course.
Ok, let’s handle that with the next set of commits:
4bfe58e (HEAD -> my_feature) Implement `delete_record`
64408c3 Add test for `delete_record`
3a21ec7 Add `delete_record_ method to API
623e357 Clippy fixes
Ok, tests pass. Time to create a PR.
But as I’m about to submit the PR, something occurs to me: An edge case on
. This leads me a bit down a
rabbit hole and my git log is becoming increasingly messy:
commit f73547597e41b3b3ea0b7c52b3a883f390e13b88 (HEAD -> my_feature)
Author: CjS77
Date: Wed Jun 1 14:48:31 2022 +0100
Tests pass
Clippy passes
Linting done
commit 0c54146fbe190106d0687c0d30ed39101142b204
Author: CjS77
Date: Wed Jun 1 14:47:57 2022 +0100
fixup! Stupid off by one error
We’ll get it this time. It’s 1am after all
commit b2985fc36f6534fc468c8d9c53cb9799c638eb20
Author: CjS77
Date: Wed Jun 1 14:47:19 2022 +0100
Fix error in process_data
The null test case broke `process_data`. Adding a fix.
commit 6f947fb3031bb15ba624a16b764cc626dcaa68cb
Author: CjS77
Date: Wed Jun 1 14:46:22 2022 +0100
Add guard for empty record in `add_data`
Catch the case where empty records are provided in `add_data` and return
commit d328a81c1e08b86934abd240a32a42be3e13d306
Author: CjS77
Date: Wed Jun 1 14:45:08 2022 +0100
Add edge_case test for `add_data`
If I add an empty record, the backend will crash! We must guard against
Test currently fails.
commit 4bfe58e198dc45c448035e5035bc43bfab06c1dc
Author: CjS77
Date: Wed Jun 1 14:41:47 2022 +0100
Implement `delete_record`
Taking stock, and figuring out what we want
We’re ready to submit a new PR, but my git log is a disaster! No-one reviewing this will have any idea what the
thinking behind the feature is beyond the fact that it’s a lot of caffeine-driven and sleep-deprived drivel.
Here’s what the current log summary looks like. I’ve gone and characterised each commit and noticed that there are
really 4 main categories of commit message –
and formatting/debugging:
f735475 (HEAD -> my_feature) Feature complete. <------ minor
0c54146 fixup! Stupid off by one error <------ minor
b2985fc Fix error in process_data <------ minor
6f947fb Add guard for empty record in `add_data` <------ API impl
d328a81 Add edge_case test for `add_data` <------ Tests
4bfe58e Implement `delete_record` <------ API impl
64408c3 Add test for `delete_record` <------ Tests
3a21ec7 Add `delete_record_ method to API <------ API def
623e357 Clippy fixes <------ minor
e668f3f Complete data ingestion <------ API impl
7323efc Fix timeout issues on backend <------ Bug fix
563ee32 WIP Implement add_data <------ API impl
c05ad1a Add import tests <------ Tests
dfa1ca7 Add sketelal API for Feature <------ API def
4888619 D <------ branched off main
What we need to do is clean up the git history and tell a unified story. In particular, we want the following:
We really only want 3 commits here: API definition, then tests, then implementation. That will give reviewers a
clear idea of what the feature should do, and how it's done.
The formatting / bugfix commits should get bundled in with of the 3 main commits that's most relevant to the changes.
The timeout bugfix commit should disappear because it's already in main.
In the meantime, other PRs have been merged, so
is quite a few commits down the line from us now. We'd also
like our PR to look like we've branched our PR from the latest HEAD of main (
rather than the one from a few days ago (
(main) $ git log --oneline
480454e (HEAD -> main) F <--- We want to have branched from here
6e9e937 Fix timeout issues on backend
4888619 D <--- We branched from here
git rebase (interactive) to the rescue
It turns out we can do all of this with one git command:
git rebase --interactive main
What this command says is "take all the commits I have made since deviating from
(D) and move (or re-base, if
you will) the entire branch onto the head of
branch. Oh, and let me edit all the commit messages first!"
Ok, let's see what happens when we run this magical command. It actually opens up an editor (vim, in my case) and
gives me this meta-commit file to edit:
pick dfa1ca7 Add sketelal API for Feature
pick c05ad1a Add import tests
pick 563ee32 WIP Implement add_data
pick e668f3f Complete data ingestion
pick 623e357 Clippy fixes
pick 3a21ec7 Add `delete_record_ method to API
pick 64408c3 Add test for `delete_record`
pick 4bfe58e Implement `delete_record`
pick d328a81 Add edge_case test for `add_data`
pick 6f947fb Add guard for empty record in `add_data`
pick b2985fc Fix error in process_data
pick 0c54146 fixup! Stupid off by one error
pick f735475 Feature complete.
# Rebase 480454e..f735475 onto 480454e (13 commands)
# p, pick
# r, reword
# e, edit
# s, squash
# f, fixup
# x, exec
# b, break = stop here (continue rebase later with ‘git rebase –continue’)
# d, drop
# l, label
Functional programming FtW
Functional programming FtW