Effective Git code Review

Make their job easier, and you look smarter.

Bri Hatch	Personal	Work
Bri Hatch	bri@ifokr.org	Dropzone AI daethnir@dropzone.ai

Git log basics

$ git log
commit 89c9ca9a70a47ba02978411e671f55121a1f7b63
Author: Bri Hatch <bri@ifokr.org>
Date:   Sat Nov 12 10:40:48 2022 -0800

    Add 2022 SeaGL to the presentations list

commit b062d1517322470448f3b3559c9a6b21b73b019b
Author: Bri Hatch <bri@ifokr.org>
Date:   Sat Nov 5 14:38:47 2022 -0700

    Fix frobnicate.sh url

commit f00baz196e48dad5561b07932189651ab205ae43
Author: Bri Hatch <bri@ifokr.org>
Date:   Sat Nov 5 14:19:39 2022 -0700

    Add seagl-2022-bash-completions

Git log basics (cont)

$ git log --name-only
commit 89c9ca9a70a47ba02978411e671f55121a1f7b63
Author: Bri Hatch <bri@ifokr.org>
Date:   Sat Nov 12 10:40:48 2022 -0800

    Add 2022 SeaGL to the presentations list

htdocs/bri/presentations/index.html
htdocs/bri/presentations/updates.yml

commit b062d1517322470448f3b3559c9a6b21b73b019b
Author: Bri Hatch <bri@ifokr.org>
Date:   Sat Nov 5 14:38:47 2022 -0700

    Fix frobnicate.sh url

htdocs/bri/presentations/seagl-2022-bash-completions/index.html

Git basics (cont)

$ git blame index.html
f00baz19 (Bri Hatch 2022-11-05 14:19:39 -0700 381) <h1>Robust Example (complete)</h1>
f00baz19 (Bri Hatch 2022-11-05 14:19:39 -0700 382) 
f00baz19 (Bri Hatch 2022-11-05 14:19:39 -0700 383) Available at 
b062d151 (Bri Hatch 2022-11-12 10:23:14 -0800 384) <a href="frobnicate.sh.txt"
b062d151 (Bri Hatch 2022-11-12 10:23:14 -0800 385)    > https://.../seagl-2022-bash-completions/frobnicate.sh.txt<</a>
f00baz19 (Bri Hatch 2022-11-05 14:19:39 -0700 386) </pre>
f00baz19 (Bri Hatch 2022-11-05 14:19:39 -0700 387) </div>

Git basics (cont)

$ git show b062d151
commit b062d1517322470448f3b3559c9a6b21b73b019b
Author: Bri Hatch <bri@ifokr.org>
Date:   Sat Nov 12 10:23:14 2022 -0800

    Post-delivery updates

@@ -366,8 +381,8 @@ The inner block continued
 
 Available at 

-  <a href="https://.../seagl-2022-bash-completion/frobnicate.sh"
-    > https://..../seagl-2022-bash-completion/frobnicate.sh</a>
+  <a href="frobnicate.sh.txt"
+    > https://.../seagl-2022-bash-completions/frobnicate.sh.txt</a>

Forced spelunking is bad

Just because people can dig in, doesn't mean you should require they do so.

Suggestions:

Meaningful commit messages
Concise changes
Consistent Style
Linear code evolution

Good commit messages

Messages say why the change exists, and requires no external reference to be understood. May also explain why implementation was done this way, and any 'gotchas' you might need to know.

First line declarative, present tense
Blank line
Description when necessary
50/72 rule
WIP / NFC

Git Commit Clarity

$ git log --one-line
75452dd broke prod again
89c9ca9 pushing on a friday
f00baz1 adds another layer of indirection to the frobnicator widg
et factory flow chart generation tool and refactors the token rin
g endofunctor to consume fewer tokens in the token bucket pool
a1a46fd lots of updates
7756828 finally fixed
187e066 f*ckity f*ck f*ck
2543232 sh*t, fixed?
9d8c605 fix BUG-8675309
745611f make code faster
735f375 from doug review

Git Commit Clarity (cont)

$ git log --one-line
89c9ca9 Add 2022 SeaGL to the presentations list
b062d15 Fix frobnicate.sh url link
f00baz1 Add seagl-2022-bash-completions
a1a46fd Add SeaGL 2021 creative assets and video links
d94fc18 Improve good shell patterns colour scheme
9d8c605 Add SeaGL 2021 presentations
745611f Add SeaGL 2019/2020 presentation links to index
735f375 NFC: convert to 4 spaces indent in ancient perl code
a83cc3d Fix lint issues in preso generator

Git Commit Clarity (cont)

$ git show a83cc3d 
commit a83cc3d888badf00da7badcafefacedeadbabe
Author: Wendell Bagg <wendell@example.org>
Date:   Wed Oct 25 09:29:18 2023 -0700

    Fix lint issues in preso generator

    Disabled too-few-public-methods globally because
    we have so many shell classes.

    Removed broad-except as global exception and only
    put where it was important.

    All lintable errors now gone, we can enable lint
    enforcement in CI/CD.

    ref: TIC-1234

Code Clarity

Make your code/changes clear

When updating code, match style
When starting code, pick a style
When reformatting, separate commit from functional changes
Consider code formatters
- black (python)
- fmt (golang)
Consider type hinting (python)
Add to your CI/CD to enforce!

Code Clarity (cont)

$  black -S -l 72 --diff ugly.py 
def convert(input_fh, output_fh):
-    csv_writer = csv.DictWriter(output_fh, fieldnames=FIELD_MAPPING
.values())

+    csv_writer = csv.DictWriter(
+        output_fh, fieldnames=FIELD_MAPPING.values()
+    )

$  black -S -l 72 ugly.py 
reformatted /tmp/ugly.py
All done! ✨  🍰 ✨ 
1 file reformatted.

Making Pretty Commits

Entering the realm of Dark magic

Git is just a bunch of changes, and you can re-order them, re-edit them, squash, delete, etc.

Always make a new branch to fiddle with! git checkout -b rebase-blah
git add -p
git commit --amend
git rebase -i / git cherry-pick
if you need git reflog you've gone too far

The mess...

$ git log --oneline
ba592e7 MERGEME - remove debugging output
5453825 emblacken script
3a1bf76 Fixup please
21cb74c make read_csv a generator
9a99fd9 Make paths relative to source code
4c6e8ea Add code from robot
71581f8 Initial commit - README

git add -p

$ git diff
 import csv
 import os
+import sys
 
 def read_csv(csv_file):
-    """Reads a CSV file and returns a list of rows."""
+    """Reads a CSV file and returns rows as a generator."""
     with open(csv_file, "r") as f:
         reader = csv.reader(f)
         for row in reader:
             yield row
 
-def merge_csv_files(fname_lname_csv, name_username_csv, output_csv):
-    """Merges two CSV files and writes the merged CSV file to the output file."""
+def merge_csv_files(fname_lname_csv, name_username_csv, writer):
+    """Merge CSV files and write the merged CSV file to the output writer."""
(many more pages of changes...)

git add -p (cont)

$ git add -p
(entire file practically)
(1/1) Stage this hunk [y,n,q,a,d,s,e,?]?  s
Split into 8 hunks.
 import csv
 import os
+import sys
 
 def read_csv(csv_file):
(1/8) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? n
 
 def read_csv(csv_file):
-    """Reads a CSV file and returns a list of rows."""
+    """Reads a CSV file and returns rows as a generator."""
     with open(csv_file, "r") as f:
         reader = csv.reader(f)
(2/8) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? y

git add -p (cont)

$ git add -p
y - stage this hunk
n - do not stage this hunk
q - quit; do not stage this hunk or any of the remaining ones
a - stage this hunk and all later hunks in the file
d - do not stage this hunk or any of the later hunks in the file
g - select a hunk to go to
/ - search for a hunk matching the given regex
j - leave this hunk undecided, see next undecided hunk
J - leave this hunk undecided, see next hunk
K - leave this hunk undecided, see previous hunk
e - manually edit the current hunk
? - print help
(3/8) Stage this hunk [y,n,q,a,d,K,j,J,g,/,e,?]? q

git add -p (cont)

$ git status
Changes to be committed:
  (use "git restore --staged ..." to unstage)
	modified:   doit

Changes not staged for commit:
  (use "git add ..." to update what will be committed)
  (use "git restore ..." to discard changes in working directory)
	modified:   doit

$ git diff --cached  # would show just the cached changes
$ git commit -m 'NFC: Fix description of read_csv'
$ git commit -a -m  'Use dicts so we can handle CSVs with unsorted data'

git commit --amend

$ git status
Changes not staged for commit:
    (use "git add ..." to update what will be committed)
    (use "git restore ..." to discard changes in working directory)
        modified:   frob.py

$ git commit --amend -a
# Drops you into previous commit message including new
# changes to frob.py

Changing History

$ git rebase -i 71581f8  # Some hash, or e.g. HEAD^^^ or HEAD~3
pick 4c6e8ea Add code from smart robots
pick 9a99fd9 Make paths relative to source code
pick 21cb74c make read_csv a generator
pick 3a1bf76 Fixup please
pick 5453825 emblacken script
pick ba592e7 MERGEME - remove debugging output
pick 1b769e6 NFC: Fix description of read_csv
pick 8db2807 Use dicts so we can handle CSVs with unsorted data

# Rebase 71581f8..8db2807 onto 8db2807 (8 commands)
# p, pick  = use commit
# r, reword  = use commit, but edit the commit message
# e, edit  = use commit, but stop for amending
# s, squash  = use commit, but meld into previous commit
# f, fixup  = like "squash", but discard this commit's log message

Changing History - squash

$ git rebase -i 71581f8  # Some hash, or e.g. HEAD^^^ or HEAD~3

pick 4c6e8ea Add code from smart robots
pick 9a99fd9 Make paths relative to source code
pick 21cb74c make read_csv a generator
squash 1b769e6 NFC: Fix description of read_csv     # Moved here
pick 3a1bf76 Fixup please
pick 5453825 emblacken script
pick ba592e7 MERGEME - remove debugging output
pick 8db2807 Use dicts so we can handle CSVs with unsorted data

:wq

You're given the chance to edit your commit message

Changing History - edit

$ git rebase -i 71581f8  # Some hash, or e.g. HEAD^^^ or HEAD~3

pick 4c6e8ea Add code from smart robots
pick 9a99fd9 Make paths relative to source code
edit 9837423 make read_csv a generator
pick 8477456 Fixup please
pick 2434325 emblacken script
pick cd2d384 MERGEME - remove debugging output
pick b2b34b2 Use dicts so we can handle CSVs with unsorted data

:wq

Changing History - edit (cont)

$ status
interactive rebase in progress; onto 71581f8
Last commands done (3 commands done):
   pick 9a99fd9 Make paths relative to source code
   edit 9837423 make read_csv a generator
  (see more in file .git/rebase-merge/done)
Next commands to do (5 remaining commands):
   pick 8477456 Fixup please
   pick 2434325 emblacken script
  (use "git rebase --edit-todo" to view and edit)
You are currently editing a commit while rebasing branch 'rebase-workbranch' on '71581f8'.
  (use "git commit --amend" to amend the current commit)
  (use "git rebase --continue" once you are satisfied with your changes)

$ git branch
* (no branch, rebasing rebase-workbranch)
  main

Changing History - edit (cont)

$ git reset HEAD^
Unstaged changes after reset:
M	doit
$ git add -p
$ git commit -m 'Make read_csv a generator'
$ git diff
$ git commit -m 'Rename left/right cvs file names'
$ git status
$ git rebase --continue

Successfully rebased and updated refs/heads/rebase-workbranch.
$ git branch
* rebase-workbranch
  main

Changing History - edit (cont)

$ git log --oneline
2e01f17 Use dicts so we can handle CSVs with unsorted data
b52bb56 NFC: Fix description of read_csv
2815cc3 MERGEME - remove debugging output
a646c02 emblacken script
011b65f Fixup please
42eb3a9 Rename left/right csv file names  <--- This one is new
1ef3a62 make read_csv a generator         <--- This one is now clean
9a99fd9 Make paths relative to source code

Gitlab MR Code Review

Thanks!

Presentation: https://www.ifokr.org/bri/presentations/seagl-2023-effective-git-code-review/

Personal	Work
Bri Hatch bri@ifokr.org	Bri Hatch Dropzone AI daethnir@dropzone.ai

SeaGL, 2023-11-04