Часть вторая. Как проходить code review по версии Google

[romich]

И сколько дней ваш PR смотрели?

[Lissov]

PR = Pull Request, другое название для MR.
Интересно, как 409 коммитов сделать на 18 файлах? За 2 месяца — то есть 10 коммитов в день в среднем.
И всё таки присоединюсь к вопросу — сколько дней ревьювер смотрел это всё?
Русскими буквами написано сколько времени Вы это писали, а не сколько времени проходило ревью.

[khim]

У меня есть ощущение, что тут как раз и «выстрелила» сделанное автором статьи «упрощение» и переименование CL в MR.

Потому что ну не верю я в то, что на эти 409 коммитов кто-то по отдельности смотрел.

А в Гугле CL — это как раз одно атомарное изменение. И, соответственно, один commit. Оно может не делать ничего полезного (например может просто переименовывать поля в базе данных, ничего больше не делая), но оно атомарно и решает какую одну задачу. И оно либо механическое (переименовали childs в children и изменили все места где эти childs использовались), либо, наоборот, осмысленное и небольшое.

При этом мы вполне можете реализовать любой рефакторинг как набор таких вот изменений и когда вы их рассматриваете, то вы можете на соседние изменения ссылаться… в сущности ничего нового: именно так всегда и вносились изменения в ядро Linux.

[Lissov]

Давняя шутка про ревью (лень искать источник):
10 lines of code = 10 issues
500 lines of code = looks fine

[khim]

Но когда сталкиваешься с кучей legacy, которая просто не может жить в нынешних условиях, буквально: тут чуть ковырнул — посыпалось здесь, однострочными изменениями никак :(

Ну вот как-то у разработчиков Android и Chrome это же получается. Вы думаете там мало легаси?

И речь не идёт об однострочных изменениях. Речь идёт об атомарных коммитах. Некоторые коммиты в Chrome могут и несколько тысяч строк включать — но только если это механические изменения…

[khim]

Одну строчку, букву поправил, сохранил. У себя.

Кого вообще волнует что у вас там «у себя» в приватном репозитории хранится? Речь идёт о публичных коммитах. Вот примерно таких (не надо искать конкретно в этих глубокого смысла, я просто взял первый попавшийся Merge Request в LKML… как известно именно для разработки ядра и был создан Git, потому я его использую в качестве модели).

Обратите внимание, что каждый коммит имеет смысл, что в первом из них — указано, сколько их всего (хотя это частная фишка LKML, в Android и Chrome этого не требуют) и так далее.

То есть публичный коммит — это специально подготовленная вещь. С описанием и прочими атрибутами.

А что вы там делаете у себя в приватном репозитории — это ваше личное дело, в общем-то. Это никого не волнует. Собственно именно потому что это ваше внутреннее дело Git (в отличие от Mercurial) и не хранит информацию о ветках и прочем. Это — ваша личная история. Считается что перед показом её кому-то — вы её причешете. У Git'а есть для этого инструменты…

[locutus]

++ термин CL скорее всего идет из Perforce, который использовала Google раньше, до перехода на собственную систему контроля версий.

[ehots]

Проблемы с внимательность по ходу. Сколько ревьювили твоё творение, а не сколько оплачиваемого времени ты на него потратил, так понятнее?

[Xobotun]

Да уж, автор так составил предложение, что мне показалось, что это он не вносил изменения два месяца и сделал мерж-реквест, а ему дали мерж-реквест и он два месяца его его облагораживал.

[varanio]

Да, в документе было именно «по традиции».

[Lissov]

Правильно, это стандартный выбор между А и Б, когда нет объективного преимущества одного над другим, но смесь из обоих это плохо. Тогда можно вначале хоть монетку бросить, а потом уже «придерживаться традиции».

[depadep]

По традиции? А есть чуть более логическое обоснование? Потому что я не вижу здесь никакой разницы.

Определенное обоснование есть (см. 5. Use the imperative mood in the subject line).

В целом большинство соглашений в программировании написаны конечно не кровью, как правила техники безопасности, — но некоторым образом выстраданы.

[varanio]

лично я считаю, что такие правила вредны. Мало ли какие традиции, и что там создает git при мерже. Разрабу нужно помнить еще одно бесполезное правило, которое не решает никаких проблем

[depadep]

“… какая разница – писать ли «моршрут» или «маршрут», «велосипед» или «виласипед»? От этого ведь велосипед мотоциклом не становится. Важно только, чтобы всё было понятно. А какая там буква в середине стоит — «а» или «о», — это, помоему, совершенно безразлично. И зачем только люди сами себе жизнь портят? Когда-нибудь они одумаются и отменят сразу все орфографические правила.”

(с) А.Алексин

Правила вроде орфографических, coding conventions, commit messages conventions и т.д. вроде бы практического смысла не несут, но позволяют читать код/текст чуть-чуть быстрее. Итоговая экономия времени может быть ощутимой.
Собственно смысл всех этих правил — сделать жизнь тех кто будет разбираться в коде/ревьювить реквест чуть проще и приятней. Чтобы могли разобраться быстро и без крови из глаз.
При работе в одиночку всё это не нужно, конечно.

[Modis]

Вариант, когда я лично не согласен с некоторыми аспектами:
Работаю над проектом. Прилетает задача: сделать окно с конфигурированием чего-то. Но вот архитектора нет, аналитиков нет, прочих руководителей нет — есть только я, абстрактная задача от менеджера (который не является программистом) и задача.
Я создаю новую ветку и начинается творческий процесс: прикидываю UI, пишу логику, создаю модели, проверяю/пробую… и вот эта череда действий много повторяется и при этом часто что-то стирается, переделывается и т.д. Т.е. идёт творческий процесс разработки с нуля. Ну вот сложно тут создавать какие-то умные пояснения к коммитам! Лично я в этом случае каждый день делаю коммит «daily commit».
А вот когда эта абстрактно-творческая фича запилена — вот тогда внесение правок, исправление багов и прочее уже можно делать полностью соответствуя рекомендациям в статье

[khim]

Ну вот сложно тут создавать какие-то умные пояснения к коммитам! Лично я в этом случае каждый день делаю коммит «daily commit».

А какая вообще разница что вы там пишите у себя в репозитории? Хоть на смеси матерного и суахили выражайтесь. Главное — что ревьюер их не видит и в публичном репозитории их нету.

А вот когда эта абстрактно-творческая фича запилена

А вот когда фича запилена — вы берёте её и распиливаете на удобные для обсуждения коммиты.

Ну блин, вас никто никогда не учил писать сочинения? Черновики и переписывание набело — тысячи лет назад изобретены, почему программисты считают, что к ним это всё не относится и норовят все свои «творческие порывы» влить в репу?

[Modis]

Я вас не понял.
Создаю ветку — делаю в ней работу с такими вот «творческими» коммитами, создаю MR на слияние этой ветки с основной (master). Ревьюер видит все мои коммиты.
Каким образом я могу сделать по-другому?

[v2kxyz]

Наверное как-то так. Т.е. rebase/pick/squash.

[khim]

У git'а есть много средств для управления историей. Хотя иногда после недели «творческих изысков» проще просто сесть и с нуля всю историю руками написать.

Но change'й без описания — в истории быть не должно (за исключением merge'ей — они могут не описываться если там не происходило никаких ручных правок, просто изменения из двух веток в одну слили).

[khim]

Каким образом я могу сделать по-другому?

Посмотреть на то, что вы натворили и «переписать историю набело»? Если изменение получилось не слишком большое — можно просто всю историю в один commit сжать, если большое — выделяете из вашего изменения атомарные куски, пишите историю до тех пор, пока она вам понравится.

Ну собственно как это всегда и делалось. Ну посмотрите же на LKML — оттуда же всё растёт! Что-то типа такого (не ищите глубокого смысла в этом изменение — просто «что под руку попалось»): первый changeset делает то, второй — сё, третий — ешё что-то, после 125го — у нас есть новая готовая фича.

Принципиальная идея Git'а — это управление историей. Отсюда всякие штуки типа cherry-pick interactive и прочее.

Ревьюер видит все мои коммиты.

А нафига они ему? Если вы им даже названия дать не может и сами себе объяснить не можете — что вы там делали, почему и зачем?

Кому и в чём они помогут лет через 5? Перед тем, как оправлять ваше изменение на review историю нужно бы подчистить — в это основная идея всех этих review.

[EgDude]

На мой взгляд лучше уж многочисленные коммиты типа этих, чем безликие daily commit

*** Create models for some feature
*** Add UI for some task
*** Change feature models structure
*** Delete feature.type field
*** Update UI for new structure

[Modis]

Да вариант. Но тоже палка о двух концах. В таком творческом процессе постоянная фиксация таких коммитов отвлекает. ИМХО

[EgDude]

Если творческий процесс слишком часто приводит к противоречивым коммитам, то стоит попробовать лучше прорабатывать задачи предварительно, хотя бы в своей голове.

[khim]

Зависит от того, насколько оно всё такое из себя творческое.

Я обычно стараюсь минимизировать «масштаб бедствий» и грубоко «в творчество» не уходить.

Поправили чуть интерфейс — давайте от'review'им, за'commit'им, авось не только мне легче будет.

Пока все согласны что код после моих правок не стал хуже, чем до… вообще проблем нет.

А вот когда у тебя цепочка из 20 commit'ов, которые связаны между собой и усложнение интерфейса в 1м упрощает жизнь в 20м… тут да, приходится долго общаться с review'ером и иногда и переделывать…

[elegorod]

Лично я в этом случае каждый день делаю коммит «daily commit».

Зачем вообще делать коммиты каждый день? Можно доделать задачу и уже по ней сделать один коммит.

[dmitryprogrammer]

Благодарю за интересный и полезный материал. Интересно всё же узнать расшифровку CL которая заменяется на MR в статье. Что это?

[varanio]

Change list