Почему важно хорошо оформлять Pull Request'ы и коммиты

На работе я постоянно агитирую за хорошее оформление Pull Request’ов и git-коммитов. При этом, я часто встречаюсь с непониманием со стороны коллег. Один из наиболее популярных вопросов:

Зачем заморачиваться с оформлением PRов и коммитов, если можно просто указать номер задачи в баг-трэкере? Ведь в задаче уже описана проблема.

Далее будет развёрнутый ответ на этот вопрос.

Примечание:
Всё описанное ниже касается и PRов, и коммитов. Я вообще долгое время работал с Gerrit (opens new window), где PR == коммит. Таким образом, если вы хорошо оформили PR, стоит позаботиться, чтобы после его мержа он превратился в хорошо оформленный коммит. Эта проблема особенно актуальна, если вы работаете с GitLab или BitBucket, которые любят сквошить изменения в коммит с описанием “Merge PR #123”.

Разберем задачу

Часто фэйлится запрос ‘/some/request’ - вместо ответа возвращает ошибку с текстом “ERROR”.

Разработчик изучил код и понял, что проблема в слишком маленьком таймауте - он убедился:

• что запрос в среднем отрабатывает не 1 сек, а 3 сек
• что для данного запроса это нормально

и внес изменения:

conf := Config {
  someRequestTimeout: 1 * time.Second,
  // ...
}

Заменил на:

conf := Config {
  someRequestTimeout: 3 * time.Second,
  // ...
}

Автор решил, что изменение слишком маленькое, чтобы его комментировать - ведь изменился всего один символ и суть изменения очевидна. Он оставил такое описание к своему PR:

[TEAM-1234] fix failed request

Далее PR уходит на code review.

Code Review

На ревью пришел другой разработчик, который не знает контекст модуля / проекта. Должен ли он поставить апрув?

Для этого ему необходимо разобраться:

• почему раньше таймаут был 1 сек?
• почему теперь 3 сек? Почему не 5?
• можно ли было оптимизировать код и не повышать таймаут? И т.п.

И все эти вопросы отпали бы сами собой, если бы описание было хорошим. Например, таким:

Примечание:
тут для удобства текст написан на русском, но в историю гита лучше писать на английском - это касается коммитов (в PRах обычно можно и на русском). Также здесь опущено правильное форматирование commit-message.

Что это даёт

Во время ревью:

• Помогает убедиться в корректности и оптимальности решения
• Экономит время ревьюера

После ревью: история гита позволит быстро понять, почему тут было решено указать именно 3 секунды.

Далее подробнее про эти пункты.

Проверка корректности и оптимальности решения

Глядя на такое описание, ревьюер сразу понимает корни проблемы, понимает почему было принято именно такое решение.

Далее он либо ставит апрув, либо оспаривает решение - например:

• указывает автору на какую-то простую оптимизацию, которая сократит время выполнения запроса
• подсказывает, что автор чего-то не учел, и лучше поставить таймаут чуть больше / чуть меньше.
• а может проблема вообще не в таймауте, и автор ошибся?

Самое главное: в этом и только в этом случае ревьюер может оспорить решение без изучения кода и не зная контекста.

Если же описания нет, то всё что видит ревьюер:

1. “проблема - запрос возвращает ошибку”
2. “решение - увеличиваем таймаут”

Почему автор увеличил таймаут? Он внимательно всё проанализировал? Или же просто увидел, что проблема в таймауте и решил его увеличить? Как он понял, что 3-х секунд будет достаточно? А может 1 секунда - это вообще был баг? Пытался ли автор оптимизировать код? И т.п.

А что если автор вообще ошибся и перепутал задачку? Он думал, что решает задачку TEAM-1235, но опечатался и указал TEAM-1234. И фиксил он падающие тесты в соседнем файле. Ревьюер же сам додумал, что это фикс падающего запроса и тыкнул апрув.

Экономия времени ревьюера

Далее простая математика.

Сколько времени понадобится на составление такого описания? Допустим, минут 5-10.

Сколько ревьюеру понадобится на изучение сути правок без описания, если он не знаком с контекстом?
На мой взгляд - примерно столько же, сколько ушло у автора PRа после того как он понял, что проблема в таймауте. Чтобы убедиться в корректности решения, ревьюер должен проделать те же умозаключения, что и автор: достаточно ли 3х секунд? стоит ли оптимизировать? и т.п.

Если у автора ушел на это час, то и ревьюер потратит примерно час.

Да, ревьюер может сократить это время, выяснив всё у автора лично. Но тогда автор потратит на ответ примерно столько же времени, сколько потратил бы на составление нормального описания. Плюс он отвлекается от важной задачи, затягивает время ревью, отнимает время ревьюера.

Теперь умножаем проблемы одного ревьюера на количество ревьюеров - ведь в у PRов часто несколько ревьюеров.

Также учитываем ценность времени ревьюера - код мог писать джун / мидл, а ревьюить - тимлид / архитектор.

Бонусы после ревью

Допустим, через пару месяцев после мержа PRа в конфиг заглядывает другой разработчик и пытается понять, почему тут именно 3 секунды, а не 1, как раньше?

Он смотрит git blame и далее сталкивается ровно с теми же проблемами, что и ревьюер - они либо видит “фикс такой-то задачи”, либо подробный текст с объяснением решения. В случае последнего ему не требуется искать в баг-трекере эту задачу, писать автору PRа и т.п.

Дополнительные риски:

• Задачка в баг-трекере может потеряться (удалили, отредактировали, переехали на другой багтрэкер и др.)
• Задачка в баг-трекере может быть оформлена примерно также, как сам PR - без подробностей. Либо подробности в каких-то других задачах (выше по цепочке подзадач), либо вообще в отдельном документе в Confluence. Всё это я встречаю регулярно.
• Автор PRа мог уволиться или просто забыть контекст