3 पॉइंट द्वारा GN⁺ 2023-10-25 | 1 टिप्पणियां | WhatsApp पर शेयर करें
  • छोटे बदलाव unified diff और split diff, दोनों में पर्याप्त रूप से देखे जा सकते हैं, लेकिन बड़े और जटिल बदलावों में केवल diff पढ़कर review करना अपनी सीमा पर पहुंच जाता है
  • बड़े बदलावों में बदलाव के इतिहास की बजाय किसी खास समय पर पूरे codebase की स्थिति के आधार पर, असली code को edit करने की तरह explore और verify करना चाहिए
  • आदर्श review screen में disk पर मौजूद वर्तमान code और उस क्षेत्र का unified diff साथ-साथ दिखना चाहिए, ताकि code context और बदलावों को एक साथ follow किया जा सके
  • मौजूदा tools का support पर्याप्त नहीं होने के कारण PR को local में checkout करने के बाद commits हटाकर सिर्फ बदलाव छोड़ने वाला low-tech workflow इस्तेमाल किया जाता है
  • इस तरीके से बदली हुई files में navigation और reviewed hunk का marking संभव है, लेकिन editor की वर्तमान position और diff अपने-आप sync नहीं होते, इसलिए manual操作 बाकी रहता है

diff की बजाय मौजूदा code state को review करना

  • split diff और unified diff, छोटे और सरल बदलावों में दोनों अच्छी तरह काम करते हैं
  • जब बदलाव बड़े और जटिल हो जाते हैं, तो सिर्फ diff पढ़ना पर्याप्त नहीं होता
  • बड़े बदलावों में किसी खास समय पर codebase को ठीक से देखना जरूरी है
    • हाल में बदले गए हिस्सों पर ध्यान देते हुए भी, मूल रूप से इसे सामान्य code review की तरह देखना चाहिए
    • tests चलाने चाहिए
    • goto definition जैसी editor navigation सुविधाओं का उपयोग करना चाहिए
    • local बदलाव apply करके देखना चाहिए कि क्या कोई और लिखने का तरीका संभव था
    • बड़े context में वे हिस्से खोजने चाहिए जिन्हें बदलना चाहिए था
    • बदलाव के इतिहास से अलग, मौजूदा codebase में अजीब चीजें पहचान सकनी चाहिए

इच्छित review screen और local workaround

  • आदर्श diff view ऐसा होना चाहिए जिसमें बाईं ओर मौजूदा code state और दाईं ओर अभी दिख रहे code region का unified diff साथ में दिखे
    • बाईं ओर का code disk पर मौजूद वर्तमान state जैसा होना चाहिए
    • बदलाव margin में हल्के ढंग से दिखने चाहिए
    • दाईं ओर का diff, बाईं ओर दिख रहे current code region से match करना चाहिए
  • इस तरह के review format के लिए फिलहाल tool support कम है
  • असली workflow में gpr script से PR को local में checkout किया जाता है
    • अंतिम step में PR के commits हटाकर सिर्फ बदलाव छोड़े जाते हैं
    • इस state में staging और commit वाले workflow को code review के लिए इस्तेमाल किया जाता है
  • edamagit से बदली हुई files की सूची देखी जा सकती है, और editor में अगले/पिछले बदलाव पर जाया जा सकता है
    • staging area का उपयोग करके पहले से review किए गए hunk को mark भी किया जा सकता है
  • बची हुई असुविधा यह है कि magit status buffer और editor में currently open file के बीच automatic sync नहीं है
    • current file और diff को साथ-साथ देखने के लिए diff को manually खोलना और अभी देखी जा रही position तक खुद scroll करना पड़ता है
  • code के इतने करीब जाकर review करना हो, तो फिलहाल temporary tools खुद बनाने की स्थिति है
  • code review का मुख्य लक्ष्य हमेशा सिर्फ code जांचना ही नहीं होता, और संबंधित लेख के रूप में Two Kinds of Code Review भी देखा जा सकता है

1 टिप्पणियां

 
GN⁺ 2023-10-25
Hacker News की राय
  • बड़े बदलावों में diff review की जगह किसी खास समय पर codebase को ठीक से review करना और हाल की बदलाव-क्षेत्रों पर ध्यान देना चाहिए — यह बात मुझे व्यक्तिगत रूप से सामान्य code review से मेल नहीं खाती लगती है।
    टीम और ticket के हिसाब से यह अलग हो सकता है, लेकिन अगर reviewer पूरे code के लिए मुख्य ज़िम्मेदार principal engineer जैसी भूमिका में नहीं है, तो आम तौर पर code review मुझे दूसरी नज़र से की जाने वाली एक साधारण consistency check के ज़्यादा करीब लगता है।
    अगर PR के commits अच्छी तरह व्यवस्थित हों या squash किए गए हों, तो incremental changes को अलग से review करना भी अपेक्षाकृत आसान होता है।
    आख़िरकार शिकायत review के प्रकार के अंतर से ज़्यादा जुड़ी लगती है। एक आम consistency-check review होता है और एक गहरा architecture/feature pre-merge review; और web tools में code review ज़्यादातर पहले वाले के क़रीब दिखता है।

    • code review को सिर्फ़ साधारण consistency check मानना बहुत सीमित नज़रिया है। code review व्यक्तिगत code ownership कम करने, conventions फैलाने, और knowledge transfer करने का एक मुख्य mechanism है।
      अच्छा code review एक अच्छे PR से शुरू होता है। PR में यह बताया जाना चाहिए कि लक्ष्य क्या है और उसे कैसे हासिल किया गया है।
      अच्छे review की पहली चीज़ यह देखना है कि code बताए गए लक्ष्य को हासिल करता है या नहीं; दूसरी, क्या दावे किए गए functionality को verify करने वाले tests हैं; तीसरी, क्या यह मौजूदा system के architectural conventions का पालन करता है; चौथी, क्या style अपेक्षाओं के अनुरूप है; और आख़िर में, क्या API usage को बेहतर बनाने के लिए कोई सुझाव हैं।
      जो code review सिर्फ़ consistency check करता है, वह ऐसा काम है जो CI भी कर सकता है, इसलिए उसकी उपयोगिता कम है। code review एक मानवीय प्रक्रिया है, इसलिए उसमें उन चीज़ों का अधिकतम उपयोग होना चाहिए जो सिर्फ़ इंसान कर सकते हैं, और बाकी मशीनों पर छोड़ देना चाहिए।
      इन सब बिंदुओं में यह सवाल अप्रत्यक्ष रूप से शामिल है: “क्या यह बदलाव भविष्य के contributors के लिए एक अच्छा उदाहरण बनेगा?”
    • मेरा मानना है कि सतही code review से सतही नतीजे ही निकलते हैं। code review आम होने के बावजूद इतने bugs production में जाने की यही एक वजह है।
      मैंने हमेशा tester के नज़रिए से code review किया है। code को build और run करके देखता हूँ कि वह इरादे के मुताबिक़ काम करता है या नहीं, उसकी कमज़ोरियाँ ढूँढता हूँ, और उसे तोड़कर देखने की कोशिश करता हूँ।
      diff अंत नहीं, शुरुआत है। वह सिर्फ़ यह बताने वाला सुराग है कि code में कहाँ टटोलना चाहिए; असली सबूत तो वास्तविक व्यवहार से आता है।
      कोई कह सकता है, “वह tester का काम है, engineer का नहीं,” लेकिन जो व्यक्ति code को समझता है और जानता है कि कहाँ देखना है, उससे बेहतर test और कौन कर सकता है?
      मुझे पता है कि यह रवैया अल्पमत में है। बहुत से engineers में code को खुद चलाने, यहाँ तक कि debugger इस्तेमाल करने को लेकर भी अजीब-सी हिचक दिखती है। वे हर चीज़ automate करने के लिए बहुत बड़ा infrastructure बना लेते हैं, लेकिन automation भी आख़िर bug वाला code ही हो सकता है। फिर tests को कौन test करेगा, Quis custodiet ipsos custodes?
    • code review से क्या अपेक्षा होनी चाहिए, इस पर मैं सहमत हूँ, लेकिन उसके ठीक से काम करने के लिए पहले एक खास working style का स्थापित होना ज़रूरी है।
      CI को छोड़ दें तो code review आख़िरी रक्षा पंक्ति है, लेकिन जिन टीमों में मैंने काम किया, वहाँ PR जिस बड़े दिशा-परिवर्तन को लाता था, उस पर उस चरण से पहले ही कई लोगों के साथ चर्चा हो चुकी होती थी। Pairing, गहरी explanation, coffee chat — जटिलता के हिसाब से कोई भी तरीका चलेगा।
      तब PR नई module separation, बड़ी dependency लाना, या API surface को फिर से गढ़ना जैसी strategy को तय करने की जगह नहीं होता, बल्कि उस strategy को लागू करने वाली tactics को review करने की जगह बन जाता है।
      अगर पहले से communication न हो और सिर्फ़ code के एक हिस्से की consistency check की जाए, तो code ownership के बिखरने का जोखिम रहता है। हालत ऐसी हो सकती है: “वह module क्या करता है?” “पता नहीं, Bob से पूछो।”
      इसके अलावा, review mode चाहे जो भी हो, लेखक का सुझाया हुआ diff view idea मुझे पसंद आया।
    • इसमें अच्छी बातें हैं, लेकिन diff review बार-बार अनुभव करने पर कई बार बस समझ से बाहर हो जाता है, इसलिए काफ़ी हद तक निरर्थक लगने लगता है।
      लाल और हरी लाइनों के उलझे हुए बड़े समूह में कोई भी review करना मुश्किल होता है। यह code का उपयोगी view नहीं है, और कम से कम default view के रूप में तो बिल्कुल उपयुक्त नहीं है।
      फिर भी काम है, इसलिए किसी तरह करते रहते हैं, लेकिन लेखक की इस बात से 100% सहमत हूँ कि यह view बहुत उपयोगी नहीं है।
    • आधुनिक tech company culture development management के लिए अधिक औपचारिक process से बहुत दूर चली गई है।
      defects, features आदि को संभालने के लिए कोई बुनियादी process नहीं है, इसलिए हर engineer को collaboration के ज़रिए feature कैसे ship करनी है, यह हर बार खुद ही फिर से गढ़ना पड़ता है।
      नतीजा यह होता है कि ज़्यादातर लोग लगभग कुछ भी नहीं करते, और code review ही इस विषय पर अर्थपूर्ण detail level पर होने वाला एकमात्र collaboration बन जाता है। इसलिए जो भी दख़ल देना है, वह सब एक ही क्षण में ठूँस देने जैसी स्थिति बन जाती है।
  • तीसरे, या चौथे विकल्प के रूप में difftastic का भी ज़िक्र किया जा सकता है। यह line-based diff की जगह “structural” diff इस्तेमाल करता है, इसलिए बदलावों को और बारीकी से highlight करता है।
    https://github.com/Wilfred/difftastic

    • चौथे, या पाँचवें विकल्प के रूप में patdiff भी है: https://opensource.janestreet.com/patdiff/
      याद के मुताबिक़, कभी-कभी यह diff को लगभग 35% ज़्यादा पढ़ने योग्य बना देता था, आम तौर पर 60% मामलों में कोई फ़र्क नहीं पड़ता था, और बहुत कम, लगभग 5% मामलों में इसे पढ़ना और मुश्किल हो जाता था।
      मैंने इसे कुछ साल पहले इस्तेमाल किया था, इसलिए ठोस समस्याएँ ठीक-ठीक याद नहीं हैं। इसे छोड़ने की एकमात्र वजह यह थी कि मैंने Git diff के लिए magit इस्तेमाल करना शुरू कर दिया था।
    • यह सचमुच बहुत अच्छा approach है। बस अफ़सोस यह है कि इसका व्यापक integration नहीं हुआ। मेरी जानकारी में, दूसरे tools के इस्तेमाल लायक़ सही structured JSON output भी हाल ही में आया है।
    • Emacs का ediff भी यह करता है। अलग शब्दों को highlight करने वाली feature का नाम Refine है।
    • git diff --word-diff में भी इससे मिलती-जुलती सुविधा है और यह काफ़ी अच्छी है।
  • Vim में review करते समय भी कुछ ऐसा ही महसूस होता है
    PR खोलने के लिए एक छोटा-सा script है, और बदली हुई files पर लगभग vimdiff <(git show baseref:file) file जैसा काम करता है। Vim के tabs असल में views होते हैं, इसलिए अलग-अलग buffers को अलग state में एक साथ खोलकर रखना अच्छा लगता है
    default view में scroll lock उम्मीद के मुताबिक काम करता है, और ज़रूरत हो तो अलग tab में उससे बचा भी जा सकता है
    [c और ]c working directory नहीं, बल्कि PR में मौजूद बदलावों के set के आधार पर hunks के बीच ले जाते हैं
    dp और dg से hunks को read-only diff buffer में push/pull करके “पूरा” के रूप में mark किया जा सकता है, और फिर वे live buffer में highlighted changes से छिप जाते हैं
    live buffer में किसी बदलाव को तुरंत commit किया जा सकता है या comment के रूप में push किया जा सकता है
    मौजूदा tree state में go to definition, build integration, popup docs जैसी सामान्य editor सुविधाएँ भी वैसे ही काम करती हैं
    यह तरीका PR की base branch के मुकाबले बदलाव देखते हुए भी working directory को साफ रखता है। working directory को गंदा करके बदलाव देखने वाले matklad के हल से यह काफ़ी बेहतर लगता है
    जहाँ मैं काम करता हूँ वहाँ बदलाव अक्सर --fixup commits के रूप में जोड़े जाते हैं, और merge के समय tool git-interpret-trailers कॉल करके fixup commit के author को original commit से फिर जोड़ देता है, इसलिए यह तरीका खास तौर पर अच्छा है। PR के text comments निकालकर, उपयुक्त होने पर Reviewed-by trailer जोड़ दिया जाता है, या अगर वह +1 के बराबर हो तो Acked-by trailer लगाया जाता है

    • “comment के रूप में push” और “text comments निकालना” से ठीक-ठीक क्या मतलब है, यह जानने की जिज्ञासा है। क्या यह कंपनी के अंदर इस्तेमाल होने वाला custom logic है?
  • बहुत बड़े और जटिल codebase, जिन पर बहुत सारे engineers काम करते हैं, वहाँ यह बड़ी समस्या है
    code review कठिन होने की वजह यह है कि diff हमेशा ठीक-ठाक लगता है, tests हमेशा pass होते हैं, और बुनियादी चीज़ें हमेशा check हो चुकी होती हैं
    लेकिन कई बार बदलाव तर्कसंगत दिखते हुए भी असल में ग़लत होते हैं
    ऐसा एक भी ख़राब बदलाव, जो ऊपर-ऊपर से ठीक लगे, पूरी architecture को drift करा सकता है
    हमेशा की तरह, सख्ती से देखें तो यह tool की नहीं बल्कि culture और knowledge sharing की समस्या के ज़्यादा क़रीब है। सिर्फ़ बेहतर tools से इसका हल नहीं होगा

    • “tests हमेशा pass होते हैं” और “बदलाव तर्कसंगत दिखते हुए भी अक्सर ग़लत होते हैं” — इन दोनों वाक्यों को साथ स्वीकार करना मुश्किल लगता है
      code review करते समय bug को सीधे पकड़ना कम ही होता है। उसकी बजाय यह देखा जाता है कि ऐसे bug को पकड़ने के लिए tests मौजूद हैं या नहीं
  • लगता है मैं कुछ मिस कर रहा हूँ। लेखक कहता है कि split diff उसके लिए काम नहीं करता, लेकिन वजह नहीं बताता
    आदर्श diff तो ऐसा लगता है जैसे duplicate context हटाए हुए split diff जैसा हो। यानी context दोनों तरफ़ दोहराने के बजाय सिर्फ़ बाईं तरफ़ हो
    दाएँ panel से duplicate context हटाने से क्या फ़ायदा मिलता है, यह जानना चाहता हूँ

    • लेखक के मुताबिक़ ज़रूरत यह है कि tests चलाए जा सकें, go to definition और दूसरी editor navigation सुविधाओं का इस्तेमाल हो सके, local changes लागू करके यह जाँचा जा सके कि कोई और तरीका काम कर सकता था या नहीं, wider context देखकर वे हिस्से ढूँढ़े जा सकें जिन्हें साथ में बदलना चाहिए था, और मौजूदा codebase में historical change path से अलग हटकर अजीब चीज़ें नोटिस की जा सकें
      मैंने जिन editors/IDEs का इस्तेमाल किया है, उनमें diff के अंदर इन सुविधाओं का support आमतौर पर बहुत बुनियादी होता है। go to definition भी अक्सर सिर्फ़ उसी file के अंदर काम करता है, autocomplete नई जोड़ी गई चीज़ों पर ठीक से नहीं चलता, और refactoring features तो कई बार होते ही नहीं
    • बाईं तरफ़ बिना बदलाव वाला मौजूदा code दिखता है। अगर एक तरफ़ code navigation वाली पूरी context हो और दूसरी तरफ़ बदलाव दिखें, तो यह समझा जा सकता है कि किसी खास context में कौन-सा बदलाव लागू हो रहा है
      मेरी समझ से लेखक diff को उलटना चाहता है। बदलाव को देखने के बजाय वह code को देखना चाहता है, और उसमें यह जाँचना चाहता है कि उससे जुड़े बदलाव हैं या नहीं
      लेख में details कम हैं, लेकिन लगता है कि वह implementation या call sites तक navigate करते हुए देखना चाहता है कि उपयुक्त बदलाव मौजूद हैं या नहीं
    • लेखक की बात नहीं, लेकिन मैं इससे सहमत हूँ
      diff highlighting उम्मीद से ज़्यादा distracting हो सकती है, और उसे बंद करने पर readability सचमुच बेहतर हो जाती है। मैं भी कभी-कभी diff viewer में इसे बंद करके code को नई नज़र से देखता हूँ, लेकिन unified diff को split pane में साथ दिखाने का विकल्प नहीं है
  • GitHub में . दबाने पर browser के अंदर पूरा IDE खुल जाता है
    review के समय यह बहुत उपयोगी रहा। सिर्फ़ टुकड़ों को देखने के बजाय पूरे file context में बदलाव दिखने से सूक्ष्म design समस्याएँ कहीं बेहतर पकड़ी जा सकती हैं

    • यह सचमुच बढ़िया है। GitHub में files सीधे edit करते समय ही मैंने इस IDE को खोला था; यह नहीं पता था कि commit diff से सीधे जाने का shortcut भी है
      जानकारी के लिए, यह shortcut GitLab में भी काम करता है
  • p4merge की कुछ सुविधाएँ याद आती हैं। उदाहरण के लिए, split diff के बीच की खाली जगह में यह दिखाता है कि file का कौन-सा हिस्सा दूसरी file के किस हिस्से से मेल खाता है, और दोनों तरफ़ diff का असर भी साथ दिखाता है: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    इसकी 3-way merge सुविधा भी पसंद है: https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
    ऐसी सुविधाएँ आम तौर पर इस्तेमाल होने वाले web-based diff tools में नहीं आई हैं। मुश्किल merges का नतीजा दिखाने के लिए 3-way diff एक अच्छा तरीका लगता है

  • अगर आप GitHub इस्तेमाल करते हैं, तो github.com के pull request page में . दबाएँ या domain को github.dev में बदल दें
    इससे browser में VSCode खुल जाएगा, और pull request diff view में दिखाई देगा
    फ़ायदा यह है कि पूरी file देखी जा सकती है, और github.com से अलग diff algorithm होने की वजह से कभी-कभी जटिल diffs ज़्यादा पढ़ने योग्य लगते हैं

    • कमी यह है कि यह धीमा है और इसमें bugs बहुत हैं
      PR comments को line के बाहर सूची के रूप में दिखाने वाला side-by-side diff view बहुत उपयोगी हो सकता है, लेकिन इसके लिए browser में पूरा VSCode instance चलाना पड़े, यह खलता है
      दरअसल यह इतना उपयोगी है कि GitHub में पहले कभी ऐसा split view था, और लोगों को AI/ML/crypto/blockchain/cloud-enabled development environment जैसे buzzword-भरे ढाँचे में धकेलने की ज़रूरत नहीं थी
      idea अच्छा है, लेकिन implementation बहुत ख़राब है। आजकल के GitHub को समेटने के लिए शायद यही सही वाक्य है
  • Meld इस तरह के उपयोग के लिए काफ़ी अच्छी तरह फिट बैठता है
    https://meldmerge.org/

    • https://kdiff3.sourceforge.net/ का भी साथ में ज़िक्र किया जा सकता है, क्योंकि उसमें भी कई अच्छे views हैं
    • यह Apple developer tools में शामिल FileMerge से काफ़ी मिलता-जुलता लगता है
  • मेरा सबसे पसंदीदा tool diff2html-cli है: https://diff2html.xyz/
    https://www.npmjs.com/package/diff2html-cli
    इसमें diff को HTML में देखा जा सकता है, और यह side-by-side या unified फ़ॉर्म को support करता है