Reviewing a Patch/ru

Многим кажется, что у них маловато квалификации для полной проверки патчей PostgreSQL. Однако проверка состоит из множества шагов. При этом вы можете помочь проекту на ранних стадиях проверки. Если вы умеете накатывать патчи и использовать новые фичи, значит, вы достаточно подготовлены для их ревью.

Коммитеры также проверяют патчи перед вливанием в базовый код. Перед проверяющими стоит задача максимально снять нагрузку с коммитеров, обнаруживая и устраняя простые проблемы. Проверяющий не несет обязательств по качеству, а лишь отчитывается о найденных проблемах. Этот шаг считается выполненным, как только проверяющий сочтет, что патч готов для более глубокой проверки коммитерами. В качестве примера обратитесь к этому ревью. Проверки разных патчей, конечно, могут отличаться друг от друга, все зависит от конкретной задачи.

Обратите внимание на насмешливую презентацию Джошуа Толле (Josh Tolley), и более серьезный Обзор процесса проверки патчей.

Важные моменты:

• commitfest, взятые в работу, доступны здесь.
• У каждого commitfest есть менеджер, которого можно попросить о помощи или задать вопросы организационного характера.
• Согласование на проверку не требуется.
• Пожалуйста, подписывайтесь на проверку, как запланируете ревью. Некоторые участники снимают заявку до фактического завершения проверки. Так делать не стоит, это усложняет планирование.
• С другой стороны, мы просим, чтобы первоначальная проверка укладывалась в 5 дней, чтобы избегать блокировок на этой стадии. При этом возможно провести частичную проверку, либо запросить больше времени. Достаточно оставаться на связи.
• Основное общение по вопросам ревью и разработки должно производиться через рассылку pgsql-hackers.
• Результаты проверки необходимо отправлять ответом на письмо, содержащее исходный патч. Соблюдайте целостность переписки, иначе автор патча может не увидеть результатов проверки.

Типичные стадии проверки:

Подача заявки на проверку (требуемые навыки: патч, понимание английского языка)

• Патч предоставлен в 'context'-формате с контекстом? (см. context-формат diff)
  • Патчи diff в normal и plain-форматах, в которых отражены только строки без контекста не приемлемы.
  • По хорошему, в подаваемой заявке патч должен быть в context-формате (diff -c) или unified-формате (diff -u), что значительно облегчает чтение кода.
• Ложится ли патч на master-ветку git без проблем?
• Содержит ли патч необходимые тесты, исправления документации и тд.?

Проверка удобства использования (требуемые навыки: статическое тестирование, умение находить и читать спецификации)

Прочтите описание назначения патча и обдумайте:

• Действительно ли патч реализует заявленное?
• Нужно ли это вообще?
• Есть ли это на текущий момент?
• Соблюдены ли SQL-спецификации, или поведение, принятое сообществом?
• Включает ли поддержку pg_dump (если применимо)?
• Несет ли угрозы?
• Все ли обосновано?

Функциональное тестирование (требуемые навыки: использование команд patch, configure и make, вывод ошибок в лог)

Накатите патч, скомпилируйте и протестируйте:

• Работает ли функциональность, как заявлено?
• Существуют ли частности, неучтенные автором?
• Выводятся ли предупреждения или падает код?
  • Ревью необходимо проводить со включенными --enable-cassert и --enable-debug флагами configure, см. Работа с git. Это позволит обнаружить возможные пропущенные проблемы. Стоит помнить, что собранный с этими флагами PostgreSQL может работать значительно медленнее, чем без этих флагов. Если проверяется патч, связанный с производительностью, то непосредственно перед тестом на скорость, необходимо пересобрать уже без этих флагов. Некоторые издержки флага --enable-cassert можно снизить за счет опции debug_assertions = false конфигурационного файла postgresql.conf. Более подробно см. Опции для разработки. По умолчанию эта опция включена, если сервер собран с флагом --enable-cassert. Также использование флага --enable-debug однозначно приведет к издержкам с применением большинства компиляторов, кроме некоторых, таких как gcc, например.

Проверка производительности (требуемые навыки: умение замерять производительность)

• Приводит ли патч к снижению производительности на простых тестах?
• Заявлено ли улучшение производительности в описании?
• Не затрагивается ли производительность других компонентов?

Проверка кода (требуемые навыки: оценка согласно принятому стилю, опыт с переносимым кодом, базовое умение чтения кодов на C)

Прочтите изменения кода и задумайтесь:

• Соблюдается ли принятый стиль?
• Есть ли проблемы переносимости?
• Заработает ли на Windows/BSD и тд.?
• Есть ли комментарии и в достаточном ли объеме?
• Выполняется ли заявленная логика и корректно ли?
• Генерируются ли предупреждения при компиляции?
• Можете ли вы свалить патч?

Проверка архитектуры (требуемые навыки: опыт в архитектуре PostgreSQL)

Задумайтесь о вносимых изменениях в контексте всего проекта:

• Все ли согласуется в рамках проекта, функциональности/модули?
• Вносятся ли взаимозависимости, которые могут привнести проблемы?

Оценка проведенной проверки

• Проведена ли полная проверка всех предполагаемых моментов со стороны проверяющего?