Revisioni del codice in sicurezza

Determinare lo scopo del cambiamento proposto#section1

La nostra revisione del codice dovrebbe sempre cominciare in un sistema per i ticket, come Jira o GitHub. Non importa se il cambiamento proposto è una nuova feature, un bug fix, una security fix o un errore di battitura: tutti i cambiamenti dovrebbero cominciare con una descrizione del motivo per cui quella modifica è necessaria e quale dovrebbe essere il risultato una volta che sarà stata applicata la modifica. Questo ci permetterà di valutare accuratamente quando il cambiamento proposto sarà completo.

Il sistema di ticketing è dove traccerete la discussione riguardante i cambiamenti che devono essere fatti dopo aver revisionato il lavoro proposto. Dal ticketing system determinerete quale branch contiene il codice proposto. Supponiamo che il ticket che stiamo rivedendo oggi è 61524: è stato creato per sistemare un broken link nel nostro sito web. Potrebbe allo stesso modo trattarsi di un refactoring o di una nuova feature, ma io ho scelto un bug fix come esempio. Non importa quale sia la natura del cambiamento proposto: siccome ad ogni ticket corrisponde un solo branch nel repository, sarà più semplice rivedere, e chiudere, i ticket.

Configurate il vostro ambiente locale ed assicuratevi di poter riprodurre quello che attualmente è il sito live, completo del broken link che deve essere sistemato. Quando applicate il nuovo codice localmente, dovrete catturare qualsiasi regression o problema che possa introdurre. Potete farlo solo se conoscete sicuramente la differenza tra quello che è vecchio e quello che è nuovo.

Revisione dei cambiamenti proposti#section2

A questo punto siete pronti per buttarvi nel codice. Ipotizzerò che stiate lavorando con i repository Git, in un setup branch-per-issue, e che il cambiamento proposto sia parte di un repository remoto del team. Lavorare direttamente dalla riga di comando è un buon approccio universale e mi permette di creare delle istruzioni copia-incolla per i team indipendentemente dalla piattaforma.

Per cominciare, aggiornate la vostra lista locale dei branch.

git fetch

Poi elencate tutti i branch disponibili.

git branch -a

Verrà visualizzata una lista dei branch disponibili nella vostra finestra del terminale. Potrebbe somigliare a questa:

* master
remotes/origin/master
remotes/origin/HEAD -> origin/master
remotes/origin/61524-broken-link

Il simbolo * denota il nome del branch che state attualmente visualizzando (o da cui avete fatto “check out”). Le righe che cominciano con remotes/origin sono riferimento ai branch che abbiamo scaricato. Lavoreremo con una nuova copia locale del branch 61524-broken-link.

Quando clonate il vostro progetto, avrete una connessione al repository remoto nel suo insieme, ma non avrete una relazione read-write con ciascuno dei singoli branch nel remote repository. Farete una connessione esplicita quando cambierete il branch. Questo significa che se avete bisogno di far girare il comando git push per caricare i vostri cambiamenti, Git saprà in quale repository remoto volete pubblicare i vostri cambiamenti.

git checkout --track origin/61524-broken-link

Ta-da! Adesso avete la vostra copia del branch per il ticket 61524, che è connesso (“tracked”) alla copia originale nel repository remoto. Adesso potete cominciare la vostra revisione!

Per prima cosa, diamo un’occhiata allo storico dei commit di questo branch con il comando log.

git log master..

Output di esempio:

Author: emmajane 
Date: Mon Jun 30 17:23:09 2014 -0400

Link to resources page was incorrectly spelled. Fixed.

Resolves #61524.

Vi fornisce l’intero messaggio di log di tutti i commit che si trovano nel branch 61524-broken-link, ma che non si trovano anche nel branch master. Scorrete velocemente i messaggi per farvi un’idea di quel che sta accadendo.

Dopodiché, date una rapida occhiata al commit stesso usando il comando diff. Questo comando mostra la differenza tra due snapshot nel vostro repository. Dovete confrontare il codice nel vostro branch checked-out con il branch con cui farete il merge, che è convenzionalmente il branch master.

git diff master

Come leggere i patch files#section3

Quando date il comando che dà come risultato la differenza, l’informazione verrà presentata sotto forma di un patch file. Questi ultimi sono terribili da leggere. Dovete cercare le righe che cominciano con + o -. Si tratta rispettivamente di righe che sono state aggiunte o rimosse. Scorrete tra i cambiamenti usando le frecce su e giù e premete q per uscire quando avete finito la revisione. Se avete bisogno di un confronto ancora più conciso di quello che è successo nella patch, considerate la modifica del comando diff per elencare i file cambiati e poi osservate uno per uno i file modificati:

git diff master --name-only
git diff master <filename>

Diamo un’occhiata al formato di un patch file.

diff --git a/about.html b/about.html
index a3aa100..a660181 100644
	--- a/about.html
	+++ b/about.html
@@ -48,5 +48,5 @@
	(2004-05)

- A full list of <a href="emmajane.net/events">public 
+ A full list of <a href="http://emmajane.net/events">public 
presentations and workshops</a> Emma has given is available

Tendo ad evitare i metadati quando leggo le patch e mi concentro solo sulle righe che cominciano con - o +. Ciò significa che comincio a leggere alla riga immediatamente dopo @@. Vengono fornite alcune righe di contesto prima delle modifiche. Queste righe sono indentate ciascuna di uno spazio. Le righe di codice cambiate vengono poi mostrate con un - (riga rimossa) o con un + (riga aggiunta) che le precede.

Andare oltre la riga di comando#section4

Usando un browser di Git repository, come gitk, vi permette di ottenere un riassunto leggermente migliore a livello visuale dell’informazione che abbiamo visto finora. La versione di Git fornita da Apple non include gitk, io ho usato Homebrew per re-installare Git e per avere questa utility. Qualunque repository browser va bene, comunque, e ci sono molti client GUI disponibili sul sito di Git.

gitk

Quando date il comando gitk, si aprirà un tool grafico dalla riga di comando. Un esempio dell’output viene dato nel seguente screenshot. Cliccate su ciascuno dei commit per averne maggiori informazioni. Molti sistemi di ticke vi permetteranno anche di vedere le modifiche in una proposta di merge “side-by-side”, quindi se lo trovate scomodo, cliccate un po’ in giro nel vostro sistema di ticketing per trovare i tool di confronto che potrebbero già avere. So per certo che GitHub offre questa feature.

Ora che avete osservato bene il codice, buttate giù le risposte alle seguenti domande:

1. Il codice è conforme agli standard di programmazione identificati per il vostro progetto?
2. Il codice si limita all’ambito identificato nel ticket?
3. Il codice segue le best practice del settore nel modo più efficiente possibile?
4. Il codice è stato implementato nella maniera migliore possibile secondo le vostre specifiche interne? È importante separare le vostre preferenze e le differenze stilistiche dai problemi reali del codice.

Applicate i cambiamenti proposti#section5

Adesso è il momento di avviare il vostro ambiente di test e di osservare nel contesto i cambiamenti proposti. Come sembra? La vostra soluzione è in linea con quello che il programmatore pensa di aver realizzato? Se non sembra giusto, dovete svuotare la cache o magari ricreare l’output Sass per aggiornare il CSS del progetto?

Adesso è anche il momento di testare il codice con qualunque test suite usiate.

1. Il codice introduce qualche regression?
2. Il nuovo codice ha performance buone come quello vecchio? Rientra ancora nel budget di performance del vostro progetto per quel che riguarda il download time e il page rendering time?
3. Le parole sono tutte scritte correttamente? Seguono delle specifiche linee guida per il brand di cui siete in possesso?

A seconda del contesto per questo particolare cambiamento nel codice, ci potrebbero essere altre ovvie questioni da gestire come parte della propria revisione del codice.

Fate del vostro meglio per creare la lista più esauriente possibile di qualunque cosa trovate sbagliata (e giusta) nel codice. È fastidioso avere feedback da qualcuno come parte del processo di revisione, quindi cercheremo di evitare “un’ultima cosa” ovunque sia possibile.

Preparare il feedback#section6

Supponiamo che ora abbiate una lunga e ricca lista di feedback. Magari non ne avrete, ma ne dubito. Se siete arrivati fin qua nell’articolo, è perché vi piace esaminare attentamente il codice tanto quanto piace a me. Issate la vostra bandiera di persona strana e strutturate la vostra revisione in maniera che sia utilizzabile dai vostri colleghi.

Suddividete tutti gli appunti che avete accumulato finora nelle seguenti categorie:

1. Il codice non funziona. Non compila, introduce una regression, non passa la testing suite, o in qualche modo fallisce in maniera dimostrabile. Questi sono problemi che devono assolutamente essere risolti.
2. Il codice non segue le best practice. Avete delle convenzioni, l’industria del web ha alcune linee guida. Questi fix sono piuttosto importanti da fare, ma possono avere delle sfumature delle quali lo sviluppatore può non essere a conoscenza.
3. Il codice non è come l’avreste scritto voi. Siete uno sviluppatore con delle opinioni testate in battaglia e sapete di aver ragione, semplicemente non avete ancora avuto l’occasione di aggiornare la pagina di Wikipedia per provarlo.

Inviate la vostra valutazione#section7

Basandovi su questa nuova classificazione, siete pronti per cominciare a scrivere codice in maniera passivo-aggressiva. Se il problema è chiaramente un errore di battitura e ricade in una delle prime due categorie, procedete con la sistemazione. Gli errori di battitura ovvi non devono per forza tornare indietro all’autore originale, giusto? Certo, il vostro collega sarà un pochino imbarazzato, ma apprezzerà che gli avrete fatto risparmiare un po’ di tempo e aumenterete l’efficienza del team riducendo il numero di giri che il codice deve fare tra lo sviluppatore e il revisore.

Se la modifica che non vedete l’ora di fare cade nella terza categoria: stop. Non toccate il codice. Al contrario, andate dal collega e fatevi descrivere il suo approccio. Chiedere “perché” potrebbe portarvi a delle conversazioni davvero interessanti riguardanti i merii dell’approccio intrapreso. Potrebbe anche rivelare i limite dell’approccio allo sviluppatore originale. Iniziando la conversazione, vi aprite alla possibilità che magari il vostro modo di fare le cose non è l’unica soluzione possibile.

Se dovete fare un qualunque cambiamento al codice, deve essere assolutamente piccolo e minore. Non dovreste fare degli edit sostanziali in un processo di peer review. Fate degli edit minori e poi aggingete i cambiamenti al vostro repository locale nel modo seguente:

git add .
git commit -m "[#61524] Correcting <list problem> identified in peer review."

Potete scrivere un breve messaggio dal momento che i vostri cambiamenti saranno piccoli. A questo punto dovreste fare il push del codice revisionato sul server così che lo sviluppatore originale possa controllarlo e revisionarlo. Supponendo che abbiate impostato il branch come tracking, si dovrebbe ridurre il tutto al comando seguente:

git push

Aggiornate la issue nel vostro sistema di ticketing in maniera appropriata per la vostra revisione. Magari il codice necessita di più lavoro o andava bene così come era scritto ed è ora il momento di chiudere la issue queue.

Ripetete i passi in questa sezione fino a che non sarà completo il cambiamento proposto e pronto per essere fuso con il branch principale.

Far il merge nel trunk della modifica approvata#section8

Finora, avete confrontato il ticket branch con il master branch nel repository. Si fa riferimento a questo branch principale come al “trunk” del vostro progetto. Lo step finale del processo di revisione consisterà nel fare il merge del ticket branch nel trunk e nel liberare i correspondenti ticket branch.

Cominciate con l’aggiornare il vostro master branch per assicurarvi di poter pubblicare i vostri cambiamenti dopo il merge.

git checkout master
git pull origin master

Fate un respiro profondo e fate il merge del vostro ticket branch nel repository principale. Per come è scritto, il seguente comando non creerà un nuovo commit nella vostra cronologia del repositori. I commit si riposizioneranno semplicemente nel master branch, facendo git log −−graph sembrerà come se un branch separato non sia mai esistito. Se volete mantenere l’illusione di un branch passato, aggiungete semplicemente il parametro −−no-ff al comando di merge, che renderà chiaro, attraverso la storia del grafo e un nuovo messaggio di commit, che avete fatto il merge di un branch a questo punto. Controllate con il vostro team quale sia l’opzione da preferite.

git merge 61524-broken-link

Il merge può o fallire o avere successo. Se non ci sono errori dal merge, siete pronti per condividere il master branch rivisto caricandolo nel repository centrale.

git push

Se ci sono messaggi di errore dal merge, gli sviluppatori originali spesso avranno maggiori possibilità di capire in che modo sistemarli, quindi potreste dover chiedere a loro di risolvere i conflitti al vostro posto.

Una volta che i nuovi commit sono stati integrati con successo nel branch master, potete cancellare le vecchie copie dei ticket branch sia dal vostro repository locale sia dal repository centrale. A questo punto sono semplici faccende domestiche.

git branch -d 61524-broken-link
git push origin --delete 61524-broken-link

Conclusioni#section9

Questo è il processo che ha funzionato per i team di cui ho fatto parte. Senza un processo di peer review, può essere difficile gestire i problemi in una codebase senza critiche. Con questo, il codice diventa molto più collaborativo: quando si introduce un errore è perché tutti e due l’abbiamo mancato. E quando si trova un errore prima che venga fatto il commit, tiriamo entrambe un sospirto di sollievo per averlo trovato quando l’abbiamo trovato.

Indipendentemente se stiate usando Git o un altro source control system, il processo di peer review può aiutare il vostro team. Può servire più tempo per sviluppare codice rivisto dai colleghi, ma contiene meno errori e ha un team forte e più diversificato che lo supporta. E, sì, sono nota per aver imparato le abitudini dei miei reviewer per per aver scelto lo stile di revisione più appropriato per il mio lavoro, proprio come facevo da bambina.

Illustrazioni: {carlok}