Pull to refresh

Comments 29

Добавлю. Если у вас большая долгоживущая ветка (вроде упомянутого рефакторинга), в которой вы в ходе работы внесли кучу изменений и фиксов уже написанного кода, перед ревью сделайте интерактивный rebase и скомпонуйте коммиты так, как будто вы с первого раза писали идеальный код. Это тот случай, когда force push абсолютно оправдан (да и все равно долгоживущую ветку вы ребейзили на мастер, не так ли?). Ревьюер отсмотрит код по коммитам (а как ещё такой огромный пул реквест отсматривать?), и ему совершенно неинтересно читать историю ваших косяков.

… и после ревью, очевидно, так же засквошить «правки по ревью» и «исправление замечаний», никакой ценности для истории они не несут.

Ну это уже не про ревью, а про историю. А так да, не стал писать ввиду нерелевантности теме.

Допустим, такая ситуация - у нас есть 10 тематических коммитов, потом по результатам review принято решение переименовать метод который используется во всех них. Вы аккуратно разносите переименования по 10 комитам, чтобы было как будто написано сразу так?

Хотелось бы советов/инструментов по наименее геморройному ребейз интерактиву.

Лично я бы не стал в каждом из 10 коммитов править, это немножко геморройно (что если их 200, такое тоже бывает). К тому же история она на то и история - тут вот так написал, тут решили переименовать - как раз всё просто и понятно. Тут скорее зависит от соглашений, принятых в команде. Если все перфекционисты, которым нужна идеальная история, и у вас есть время этим заниматься - то почему нет.

А я бы стал. Если выработать систему, геморроя никакого. 200 не встречал, но даже с 40 коммитами условно проблем нет.

Если вам на ревью по 200 коммитам сразу замечания делают, у вас что-то либо с оформлением истории, либо с системой ревью не так.
Вот мы сейчас пилим долгую жирную фичу. в отдельной ветке. на каждый коммит-два заводится отдельное небольшое ревью. Не надо это все копить.
«ой все может переделаться на 10 раз позже» — ну и что, и переделки тоже отревьюятся отдельно.
200 коммитов это скорее исключение, чем правило — такой пр невозможно адекватно отсмотреть. Мне вот другое интересно — действительно настолько часто приходится возвращаться к истории, чтобы тратить время на приведение ее в идеальный вид? У меня такое случается раз в несколько месяцев, и в подавляющем большинстве случаев это какой-то один отдельный коммит — т е в принципе неважно, был он приведен в идеальный вид или нет. Вдруг упускаю что-то важное.
действительно настолько часто приходится возвращаться к истории, чтобы тратить время на приведение ее в идеальный вид

Не знаю к в вашей в работе а у меня это называется мердж реквесты и они есть на КАЖДУЮ фичу, в 75% реквестов есть минимум 1 замечание, значит в 3/4 случаев приходится переоформлять историю.
Т.е. ну не каждый день, но несколько раз в неделю (представьте сами).

т е в принципе неважно, был он приведен в идеальный вид или нет.

Ну это ваш подход в вашей компании, я ж не говорю что у всех одинаково. Лично для меня это раздолбайство, а во многих проектах все эти сотни комитов «fix build»/«fix review» даже не сквошат. (вот пример подобного треша из опенсорс проекта — сравните текст и правки github.com/kleinschrader/kleinsHTTP/commit/ccec65a131a6f6126fc7ce2ef0d4719e093a0b53 (если что mapron даже этого не просил))
Пулл-реквест или мерж-реквест, по-моему, это просто разные названия для одного и того же. У нас тоже на каждое изменение отдельный ПР, который не будет влит без нескольких аппрувов и успешных сборок/прогонов тестов на CI. Я имею ввиду, вот вы привели историю в идеальный вид, влили, часто ли после этого возвращаетесь к ней?
Чтобы прояснить — я лично каждый раз делаю интерактивный ребейз перед ревью, но не до такой степени, как вы описываете :)
Простите, я не пытался сделать акцент на термине «мердж-реквест», я тоже оба термина использую взаимозаменяемо. Возможно это была неудачная тонкая шутка на ваше «у меня такое бывает раз в несколько месяцев», не обижайтесь.

часто ли после этого возвращаетесь к ней

Разумеется. У нас продукт долгоиграющий, нет такого что влили фичу и больше её править не приходится. Соответственно когда ты возвращаешься к модулю, git blame это лучший инструмент чтобы быстро освежить в памяти «а ПОЧЕМУ» тут было сделано так, когда комментариев недостаточно. Если вы не пользуетесь git blame/git log в ежедневной работе — то конечно, нафиг не уперлось оформление истории :)
Да нет, я-то как раз пользуюсь, но видимо не так часто — как уже сказал, раз в несколько месяцев приходится это делать. Может везет с командой, но в большинстве случаев и так все понятно, без дополнительных инструментов.
А сколько лет кодовой базе над которой вы работаете? 10? 20?
Продукт разрабатывается с 2005 года. За это время он был несколько раз переписан (последний большой рывок — с C++ на C#), наверное поэтому к древней истории мы практически не возвращаемся, только если в справочных целях.
Ага! Все так!

Допустим история выглядит так:
— Подфича А
— Подфича Б
— Интеграция А и Б.

Затем по ревью мне написали 2 замечания по подфиче Б и 3 по интеграции. я сделаю такую историю:
— Подфича А
— Подфича Б
— Интеграция А и Б.
+1 Подфча Б — исправил «замечание 1»
+2 Подфча Б — исправил «замечание 2»
+1 Интеграция А и Б. — исправил «замечание 1»
+2 Интеграция А и Б. — исправил «замечание 2»
+3 Интеграция А и Б. — исправил «замечание 3»
(т.о. во время ревью не происходит перезаписывания истории).
После апрува, все это через интерактивный ребейз fixup-ится в соответствующие коммиты.
Никакого геморроя. (главное местами не переставлять).

В описанном случае не так важно, зависит от подхода команды к чистоте истории.


Я скорее про другого рода случаи.


Допустим, решили отрефакторить длинный метод с парой флагов и почти идентичной копипастой, сделать extract method.


На метод были тесты. Отрефакторили, тесты проходят, но при ручном тестировании видны проблемы: ой, а тест-то был не полный (что неудивительно при таком толстом методе). Увидели, что при рефакторинге внесли баг. Добавили тесткейс, поправили отрефакторенный код.


Сейчас у вас 3 коммита: выделение метода, добавление тесткейса, правка бага.


Прежде, чем отправить на review, я бы сделал rebase: первым коммитом бы шло добавление тесткейса, а вторым — рефакторинг (с засквошенной в него правкой бага).

рефакторинг (с засквошенной в него правкой бага).

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

Не-не. Если баг уже был, то это совершенно другой разговор.


Речь о кейсе


Увидели, что при рефакторинге внесли баг.

Тут речь о баге, который я внес в новом коде, который в рамках этой ветки и написал, после чего его обнаружил и поправил.


То же относится к любому новому коду, написанному в рамках фичеветки. Зачем эта история косяков разработчика, которые он исправил до мержа? Это в чистом виде мусор.


Да, на таком коротком примере можно просто засквошить все в один коммит прямо при мердже, но давайте представим себе, что в рамках одного рефакторинга таких случаев, ну, 10.

Понял, согласен, прошу прощения за свой косяк изначально. Вы полностью правы.

Всё зависит от того как принято проводить ревью в команде.
Компоновать красивые коммиты может быть затратно по времени и через историю коммитов мало кто проверяет пул реквест (по моему опыту).

Чаще всё же через просмотр изменений по всему реквесту.

Зависит от платформы. например в Bitbucket (ex stash) удобно реквест смотреть как и целиком, так и по коммитам, если их больше пары.

Я постоянно читаю (не только тут в комментах, во всех статьях по ревью), мол оформлять историю затратно, мутно долго и тп, вообще в упор не могу понять этого подхода. Ты колбасил код неделю, отдаешь на ревью и не можешь 10 минут потратить на оформление истории безо всяких эти "+ фикс правки после правок"?

Мне кажется, все разработчики которые спустя рукава и с ленцой относятся к оформлению истории «да кто ее читать будет вообще» в проекте, никогда в своей жизни не пользовались «git blame/git log» для того чтобы понять что было сделано и зачем по какому-то месту в файле. Только не надо вот «для этого должны быть комментарии в коде» — не всегда они есть, даже если все комментируется исправно, и не всегда их достаточно.

Историю безусловно стоит оформлять, но не в рамках PR. Зачем? Он же все равно будет сквошиться в 1 коммит, в котором будет и описание и номера задач в которых можно посмотреть контекст.

Мне кажется тезиса, что история коммитов не важна в принципе в комментариях не было. Обсуждаем исключительно в рамках пулл реквеста. А там уже каждый сам себе хозяин. Я например могу черновые разработки коммитить, чтобы появилась активность в задаче и ПМ видел что на задачу не забили. Потом вообще переписать этот код и декомпозировать. Поэтому эти коммиты сами по себе никакой ценности не несут. А целиком PR со всеми изменения - несет.

А зачем открывать PR если задача в статусе черновика и разработка не завершена? ИМХО, стоит оформлять историю в лучшем виде и до открытия реквеста, и после (когда сквошатся замечания).

Засквошить в один коммит изменения на несколько тысяч строк - это так себе идея.

А для видимости прогресса совершенно ни к чему делать PR, достаточно пушить фичеветку.

А для видимости прогресса совершенно ни к чему делать PR, достаточно пушить фичеветку.

Да, я про это и говорю, что она не всегда пушится валидным кодом, который доживет до PR.

Поинт про интерактивный ребейз я понял, это хорошая мысль. Главное чтобы в компании не было политики - все PR сквошим, тогда в этом отпадает смысл

Главное чтобы в компании не было политики — все PR сквошим, тогда в этом отпадает смысл

Из любого правила есть исключения, здравый смысл всегда должен быть на первом месте.


Если в описанном мной случае кто-то будет настаивать на том, что надо сквошить, потому что надо сквошить, я, скажем так, сильно удивлюсь.


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

Все зависит еще от объема пулл-реквеста.


У меня как-то был пулл-реквест рефакторинга ОЧЕНЬ старого кода на десятки тысяч строк. Отревьюить его целиком просто невозможно: читая весь changeset сверху вниз, просто непонятна логика — особенно учитывая то, что в команде не осталось ни одного из авторов того кода, и уже никто не помнил, как он работает. Поэтому я все разбил на коммиты (их там было порядка 30), и, просматривая коммиты последовательно, ревьюеру становится очевидна логика, которой я руководствовался, какие сущности добавились или модицифировались, какое API добавилось, и как этот API потом используются, какие тесты добавились. Конечно, естественная последовательность коммитов была совершенно другой, и я потратил где-то полчаса на rebase, но ревьюеру не то, что сэкономил часы — я думаю, что без этого вообще никто бы не взялся это полноценно ревьюить, и все бы закончилось чем-то типа "ну блин, офигеть, но если ты уверен, что все хорошо, давай на стейдж, пусть QA тестит".


До этого случая так принято не было, после того, как я это сделал, стало принято — ревьюил тимлид, и ему такой подход понравился :)

Согласен, подход интересный, попробую в следующий раз на огромных PR :)

UFO just landed and posted this here
UFO just landed and posted this here
Sign up to leave a comment.

Articles