3 puan yazan GN⁺ 2023-10-25 | 1 yorum | WhatsApp'ta paylaş
  • Küçük değişiklikler hem unified diff hem de split diff ile yeterince incelenebilir; ancak büyük ve karmaşık değişikliklerde yalnızca diff okuyarak yapılan inceleme sınıra dayanır
  • Büyük değişiklikler, değişiklik geçmişinden ziyade belirli bir andaki kod tabanının genel durumu temel alınarak, gerçek kodu düzenler gibi gezilip doğrulanmalıdır
  • İdeal inceleme ekranı, diskteki mevcut kod ile ilgili alanın unified diff görünümünü yan yana göstererek kod bağlamını ve değişiklikleri aynı anda takip etmeyi sağlamalıdır
  • Mevcut araç desteği yetersiz olduğundan, PR'ı lokale checkout edip commit'leri silerek yalnızca değişiklikleri bırakan düşük teknolojili bir iş akışı kullanılıyor
  • Bu yöntemle değişen dosyalar arasında gezinmek ve reviewed hunk işaretlemek mümkün olsa da, editördeki mevcut konum ile diff otomatik olarak eşitlenmediği için hâlâ manuel işlem gerekiyor

Diff'ten ziyade mevcut kod durumunu incelemek

  • split diff ve unified diff, küçük ve basit değişikliklerde ikisi de iyi çalışır
  • Değişiklik büyüyüp karmaşıklaştığında yalnızca diff okumak yeterli değildir
  • Büyük değişikliklerde, kod tabanının belirli bir andaki durumuna düzgünce bakmak gerekir
    • Yakın zamanda değişen alanlara dikkat etmekle birlikte, temelde normal kod incelemesi gibi bakılmalıdır
    • Testler çalıştırılmalıdır
    • goto definition gibi editör gezinti özellikleri kullanılmalıdır
    • Yerel değişiklikler uygulanarak başka bir yazım biçiminin mümkün olup olmadığı kontrol edilmelidir
    • Daha geniş bağlamda değişmiş olması gereken yerler aranmalıdır
    • Değişiklik geçmişinden bağımsız olarak mevcut kod tabanında tuhaf görünen noktalar fark edilebilmelidir

İstenen inceleme ekranı ve yerel geçici çözüm

  • İdeal diff görünümü, solda mevcut kod durumunu, sağda ise şu anda görünen kod alanının unified diff'ini birlikte gösteren bir yapıdadır
    • Soldaki kod, diskteki mevcut durumla aynı olmalıdır
    • Değişiklikler kenar boşluğunda hafifçe gösterilir
    • Sağdaki diff, solda o anda görünen kod alanıyla eşleşmelidir
  • Bu tür bir inceleme biçimi için mevcut araç desteği yetersizdir
  • Gerçek iş akışı, gpr script ile PR'ı lokale checkout etme şeklindedir
    • Son aşamada PR'ın commit'leri silinir ve yalnızca değişiklikler bırakılır
    • Bu durumda staging ve commit iş akışı kod incelemesi için kullanılır
  • edamagit ile değişen dosyaların listesi görülebilir ve editörde sonraki/önceki değişikliğe gidilebilir
    • staging area kullanılarak daha önce incelenmiş hunk'lar da işaretlenebilir
  • Geriye kalan rahatsızlık, magit status buffer ile editörde o anda açık olan dosya arasında otomatik eşitleme olmamasıdır
    • Mevcut dosyayı ve diff'i yan yana görmek için diff'i manuel olarak açmak ve o anda bakılan konuma kadar elle kaydırmak gerekir
  • Koda bu kadar yakın duran bir inceleme yapmak için şu anda geçici araçları bizzat oluşturmak gerekiyor
  • Kod incelemesinin temel amacı her zaman yalnızca kod denetimi olmak zorunda değildir; ilgili yazı olarak Two Kinds of Code Review da okunabilir

1 yorum

 
GN⁺ 2023-10-25
Hacker News yorumları
  • Büyük değişikliklerde diff incelemesi değil de, belirli bir andaki kod tabanını düzgünce inceleyip son değişen alanlara özellikle dikkat etmek istemek bana şahsen genel kod inceleme anlayışıyla çok uyumlu gelmiyor
    Takıma ve ticket'a göre değişir ama, inceleyen kişi tüm koddan esasen sorumlu olan principal engineer benzeri bir rolde değilse, kod incelemesi genelde ikinci bir göz olarak yapılan basit bir tutarlılık kontrolüne daha yakındır diye düşünüyorum
    PR içindeki commit'ler iyi düzenlenmiş ya da squash edilmişse artımlı değişiklikleri ayrıca incelemek de nispeten kolay olur
    Sonuçta şikayet daha çok inceleme türleri arasındaki farka benziyor. Yaygın olan tutarlılık kontrolü incelemesi ile derin mimari/işlevsel pre-merge incelemesi var; web araçlarındaki kod incelemesi çoğunlukla ilkine daha yakın görünüyor

    • Kod incelemesini sadece basit bir tutarlılık kontrolü olarak görmek fazla zayıf bir bakış açısı. Kod incelemesi, bireysel kod sahipliğini azaltan, teamülleri yayan ve teknoloji aktarımı sağlayan temel bir mekanizmadır
      İyi bir kod incelemesi iyi bir PR ile başlar. PR, amacını ve bu amaca nasıl ulaştığını açıklamalıdır
      İyi bir incelemenin ilk maddesi kodun açıklanan hedefe ulaşıp ulaşmadığını doğrulamak, ikincisi iddia edilen işlevi sınayan testlerin olup olmadığını görmek, üçüncüsü mevcut sistemin mimari teamüllerine uyup uymadığını kontrol etmek, dördüncüsü stilin beklentilere uygun olup olmadığına bakmak ve son olarak API kullanımını daha iyi hale getirecek öneriler olup olmadığını değerlendirmektir
      Sadece tutarlılık kontrolü yapan kod incelemesi, CI'ın da yapabileceği bir iş olduğu için pek faydalı değildir. Kod incelemesi insani bir süreçtir; bu yüzden insanların yapabileceği şeylerden mümkün olduğunca yararlanmalı, geri kalanını makinelere bırakmalıyız
      Bu maddelerin içinde örtük olarak şu soru da vardır: “Bu değişiklik gelecekteki katkılar için iyi bir örnek olur mu?”
    • Yüzeysel kod incelemelerinin yalnızca yüzeysel sonuçlar doğurduğunu düşünüyorum. Kod incelemesi yaygın olmasına rağmen çok sayıda hatanın production'a çıkmasının nedeni de bu
      Kod incelemesini hep tester bakış açısından yaptım. Kodu derleyip çalıştırarak amaçlandığı gibi davranıp davranmadığına bakıyor, zayıf noktaları arıyor ve bozmaya çalışıyorum
      Diff son değil, başlangıçtır. Sadece kodun neresini kurcalamak gerektiğini gösteren bir ipucudur; asıl kanıt gerçek davranıştan gelir
      “Bu tester'ın işidir, mühendisin değil” denebilir ama, kodu anlayan ve nereye bakması gerektiğini bilen birinden daha iyi kim test edebilir ki?
      Bu yaklaşımın azınlıkta olduğunun farkındayım. Birçok mühendis, kodu bizzat çalıştırmaya, hatta debugger kullanmaya bile garip biçimde direnç gösteriyor gibi görünüyor. Her şeyi otomatikleştirmek için devasa altyapılar kuruyorlar ama otomasyon da sonuçta hata içerebilen bir koddur. Peki testleri kim test ediyor, Quis custodiet ipsos custodes?
    • Kod incelemesinden beklenenler konusunda katılıyorum ama bunun düzgün işlemesi için önce belli bir çalışma biçiminin yerleşmiş olması gerekiyor
      Kod incelemesi, CI hariç son savunma hattıdır ama çalıştığım ekiplerde PR'ın getirdiği büyük yönelim o noktadan önce zaten birkaç kişiyle tartışılmış olurdu. Eşli çalışma, derin açıklamalar, kahve sohbetleri; karmaşıklığa uygun hangi yöntemse o yeterli
      Böyle olunca PR, yeni modül ayırma, büyük bir bağımlılık ekleme ya da API yüzeyini yeniden işleme gibi stratejinin kendisini değil, o stratejiyi hayata geçiren taktikleri inceleme yeri olur
      Önceden iletişim olmadan kodun sadece bir kısmında tutarlılık kontrolü yapmak, kod sahipliğinin parçalanması riskini doğurabilir. “Şu modül ne yapıyor?” “Bilmiyorum, Bob'a sor” gibi bir duruma dönüşür
      Bundan bağımsız olarak, inceleme modu ne olursa olsun yazarın önerdiği diff görünümü fikri hoşuma gitti
    • Söylenenlerde doğruluk payı var ama diff incelemesi, tekrar tekrar yaşanınca çoğu zaman düpedüz anlaşılmaz hale geliyor; bu yüzden biraz anlamsızlaşıyor
      Kırmızı ve yeşil satırların iç içe geçtiği bloklarla herhangi bir inceleme yapmak zor. Bu, kodu görmenin faydalı bir yolu değil; en azından varsayılan görünüm olarak uygun değil
      Yine de iş olduğu için bir şekilde yapıyoruz ama yazarın söylediği gibi bunun pek kullanışlı bir görünüm olmadığına %100 katılıyorum
    • Modern teknoloji şirketi kültürü, geliştirme yönetimi için daha resmî süreçlerden fazla uzaklaşmış durumda
      Hata, özellik vb. konuları ele alan temel bir süreç olmayınca, her mühendis bir özelliğin işbirliği içinde nasıl yayınlanacağını her seferinde yeniden icat etmek zorunda kalıyor
      Çoğu kişi neredeyse hiçbir şey yapmıyor ve kod incelemesi, bu konuda anlamlı ayrıntı düzeyinde yürütülen tek işbirliği haline geliyor. Böyle olunca da mümkün olan her müdahale tek bir ana sıkıştırılmaya çalışılıyor
  • Üçüncü ya da dördüncü bir seçenek olarak difftastic de anılabilir. Satır bazlı diff yerine “yapısal” diff kullanarak değişiklikleri daha ayrıntılı vurguluyor
    https://github.com/Wilfred/difftastic

    • Dördüncü ya da beşinci bir seçenek olarak patdiff de var: https://opensource.janestreet.com/patdiff/
      Hatırladığım kadarıyla bazen diff'i yaklaşık %35 daha okunur hale getiriyordu, genelde %60 oranında fark yaratmıyordu ve nadiren %5 oranında daha zor okunur hale getiriyordu
      Bunu birkaç yıl önce kullandım, bu yüzden somut sorunları pek hatırlamıyorum. Kullanmayı bırakmamın tek nedeni Git diff için magit kullanmaya başlamış olmamdı
    • Gerçekten çok iyi bir yaklaşım. Yalnız yaygın biçimde entegre edilmemiş olması üzücü. Diğer araçların kullanabileceği düzgün bir yapılandırılmış JSON çıktısının da ancak yakın zamanda geldiğini biliyorum
    • Emacs'teki ediff de bunu yapıyor. Farklı kelimeleri vurgulayan özellik Refine
    • git diff --word-diff içinde de buna benzer bir özellik var ve oldukça iyi
  • Vim ile incelerken de benzer hissediyorum
    PR açmak için ufak bir betik var; değişen dosyalar için kabaca vimdiff <(git show baseref:file) file gibi çalışıyor. Vim sekmeleri aslında birer görünüm olduğu için, tek tek buffer'ları farklı durumlarda aynı anda açık tutabilmek güzel
    Varsayılan görünümde kaydırma kilidi beklendiği gibi çalışıyor, gerekirse ayrı sekmelerde bundan kaçınılabiliyor
    [c ve ]c, çalışma dizinine göre değil PR'daki değişiklik kümesine göre hunk'lar arasında geçiş yapıyor
    dp ve dg ile hunk'ları salt okunur diff buffer'ına push/pull edip “tamamlandı” diye işaretleyebiliyorsunuz; sonra da canlı buffer'daki vurgulanmış değişikliklerden gizleniyor
    Canlı buffer'da bir değişiklik doğrudan commit edilebiliyor ya da yorum olarak push edilebiliyor
    Go to definition, build entegrasyonu, açılır belge pencereleri gibi normal editör özellikleri de mevcut ağaç durumunda aynen çalışıyor
    Bu yaklaşım, PR'ın temel branch'e göre değişikliklerini görürken çalışma dizinini temiz tutuyor. Çalışma dizinini kirletilmiş durumda değişikliklere bakan matklad'ın çözümünden çok daha iyi görünüyor
    Çalıştığım ortamda değişiklikler sık sık --fixup commit'leri olarak ekleniyor ve araç merge anında git-interpret-trailers çağrısıyla fixup commit yazarını asıl commit'e yeniden bağlayacak şekilde bunları tekrar birleştiriyor; bu yüzden özellikle iyi. Bazen PR'daki metin yorumlarını çıkarıp uygunsa Reviewed-by trailer'ı ekliyor, ya da +1 anlamına geliyorsa Acked-by trailer'ı ekliyor

    • “yorum olarak push etmek” ve “metin yorumlarını çıkarmak” ile tam olarak ne kastedildiğini merak ediyorum. Bu şirkete özel bir özel mantık mı?
  • Üzerinde çok sayıda mühendisin çalıştığı çok büyük ve karmaşık kod tabanlarında bu büyük bir sorun
    Kod incelemesinin zor olmasının nedeni, diff'in her zaman makul görünmesi, testlerin her zaman geçmesi ve temel şeylerin hep kontrol edilmiş olması
    Ama değişiklik makul görünse bile gerçekte yanlış olduğu durumlar sık yaşanıyor
    Makul görünen tek bir kötü değişiklik bile tüm mimarinin sürüklenmesine yol açabilir
    Her zamanki gibi bu da teknik olarak bir araç sorunu değil; daha çok kültür ve bilgi paylaşımı sorunu. Daha iyi araçlar tek başına bunu çözmez

    • “testler her zaman geçer” ile “değişiklik makul görünse bile sık sık yanlış olur” cümlelerini birlikte kabul etmek zor
      Kod incelemesinde bizzat bug bulmak nadirdir. Bunun yerine, böyle bug'ları yakalayacak testlerin olup olmadığına bakarım
  • Bir şeyi kaçırıyor gibiyim. Yazar split diff'in kendisine uymadığını söylüyor ama nedenini açıklamıyor
    İdeal diff, yinelenen bağlamı çıkarılmış split diff'e neredeyse benziyor. Bağlamın iki tarafta da tekrar etmesi yerine yalnızca solda bulunuyor
    Sağ panelde yinelenen bağlamı kaldırmanın ne fayda sağladığını merak ediyorum

    • Yazar; test çalıştırmaya, go to definition ve diğer editör gezinme özelliklerini kullanmaya, başka şekilde yazılıp yazılamayacağını görmek için yerel değişiklikler uygulamaya, daha geniş bağlama bakıp birlikte değişmesi gereken yerleri bulmaya ve mevcut kod tabanında tarihsel değişim yolundan bağımsız olarak tuhaf şeyleri fark etmeye ihtiyaç duyduğunu yazıyor
      Kullandığım editör/IDE'lerde diff içindeki bu tür destekler genelde çok temel düzeyde oluyor. Go to definition bazen yalnızca aynı dosya içinde çalışıyor, otomatik tamamlama yeni eklenen öğelerde iyi çalışmıyor ve çoğu zaman refactoring özellikleri bulunmuyor
    • Sol taraf, değiştirilmemiş mevcut kodu gösteriyor. Bir tarafta kod gezinme özellikleriyle tam bağlamı, diğer tarafta ise değişiklikleri gösterince, belirli bir bağlamda hangi değişikliğin uygulandığını görebiliyorsunuz
      Benim anladığım kadarıyla yazar diff'i tersine çevirmek istiyor. Değişikliğin kendisine bakmak yerine koda bakıp orada ilgili bir değişiklik olup olmadığını görmek istiyor
      Yazı ayrıntı açısından zayıf ama görünüşe göre implementasyonlara ya da çağrı noktalarına gidip uygun değişikliklerin yapılıp yapılmadığını kontrol etmek istiyor
    • Yazardan değil ama buna katılıyorum
      Diff vurguları sanılandan daha dikkat dağıtıcı olabiliyor; kapatıldığında okunabilirlik belirgin biçimde artabiliyor. Ben de bazen diff görüntüleyicide bunu kapatıp koda taze gözle bakıyorum, ama birleşik diff'i split pane içinde birlikte gösterme seçeneği yok
  • GitHub'da . tuşuna basınca tarayıcı içindeki tam IDE'ye giriliyor
    İnceleme yaparken çok faydalı oldu. Parçalara bakmak yerine değişiklikleri dosyanın tüm bağlamı içinde görebildiğim için ince tasarım sorunlarını çok daha iyi yakalıyorum

    • Bu gerçekten harika. GitHub'da dosyaları doğrudan düzenlerken bu IDE'yi görmüştüm ama commit diff'inden doğrudan giden bir kısayol olduğunu bilmiyordum
      Bu arada bu kısayol GitLab'da da çalışıyor
  • p4merge'ün bazı özelliklerini özlüyorum. Örneğin split diff'ler arasındaki boşlukta, dosyanın hangi bölümünün başka dosyanın hangi bölümüyle eşleştiğini gösterirken iki tarafta da diff etkisini birlikte gösterebiliyor: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    3-way merge özelliğini de seviyorum: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    Bu özellikler yaygın kullanılan web tabanlı diff araçlarına gelmedi. Zor merge sonuçlarını göstermek için 3-way diff'in iyi bir yöntem olduğunu düşünüyorum

  • GitHub kullanıyorsanız github.com'daki pull request sayfasında . tuşuna basabilir ya da alan adını github.dev olarak değiştirebilirsiniz
    Böylece tarayıcıda VSCode açılır ve pull request diff görünümü ile gösterilir
    Avantajı, dosyanın tamamını görebilmeniz ve diff algoritmasının github.com'dakinden farklı olması nedeniyle karmaşık diff'lerde bazen daha okunaklı olması

    • Dezavantajı yavaş ve hatalı olması
      PR yorumlarını satır dışı bir liste olarak gösteren side-by-side diff görünümü çok yararlı olabilir ama bunun için tarayıcıda tam bir VSCode örneği açmak zorunda olmak can sıkıcı
      Aslında o kadar yararlı ki, GitHub'da eskiden buna benzer bir split view vardı; insanları AI/ML/crypto/blockchain/cloud-enabled geliştirme ortamları gibi moda sözcük yığınlarına itmeye gerek yoktu
      Fikir iyi ama uygulama berbat. Hatta bugünlerde GitHub'ın genelini özetleyen bir ifade gibi geliyor
  • Meld bu tür kullanım için oldukça uygun
    https://meldmerge.org/

    • https://kdiff3.sourceforge.net/ da birçok iyi görünüm sunduğu için birlikte anılmaya değer
    • Apple geliştirici araçlarına dahil olan FileMerge ile oldukça benzer görünüyor
  • En sevdiğim araç diff2html-cli: https://diff2html.xyz/
    https://www.npmjs.com/package/diff2html-cli
    diff'i HTML olarak görüntüleyebiliyor ve yan yana ya da unified biçimini destekliyor