Qual è il modo migliore per rivedere un codice prima che venga eseguito il commit sul trunk? (SVN)

Ci sono due scuole però - ciò che proponi o "rivedi prima di commettere". La maggior parte delle differenze possono essere viste come negative e / o positive. - Ad es. Nessuna traccia delle modifiche causate da una recensione: vuoi vederle come commit discreti o sei interessato solo al lavoro finale?

Esamina prima del commit - non è richiesta alcuna ramificazione (sebbene possa essere eseguita se lo si desidera), deve consentire ai revisori di accedere alle cartelle di lavoro. Il codice può essere modificato durante e dopo la revisione senza tracciamento. Le correzioni causate dalla revisione non appaiono nel repository.

Revisione dopo il commit (su un ramo) - È necessario ruotare un ramo per ogni revisione (anche se questo potrebbe già essere nel flusso di lavoro). Il codice inviato per la revisione non può essere modificato senza tenere traccia delle modifiche. Qualcuno deve unire i rami esaminati e tenere traccia di ciò che è stato rivisto e cosa no.

Dipende in gran parte dalla cultura e dall'esperienza del team. Qual è il tuo modello di fiducia, qual è lo scopo principale per le recensioni? Personalmente preferisco la revisione dopo il commit, in quanto consente di tenere traccia delle modifiche a seguito della revisione. Ora usiamo Git e Gerrit in quanto forniscono un buon equilibrio tra le due opzioni.

1. Prima di eseguire il comando eseguire "svn diff" per generare un file di patch.
2. Invia il file patch al revisore che lo applica a una copia pulita del trunk.
3. Il revisore esamina le modifiche utilizzando il visualizzatore delle differenze di scelta.
4. Reviewer esegue build ed esegue test.
5. Se tutto va bene, dì allo sviluppatore che possono controllare le loro modifiche. Se
   ci sono problemi con lo sviluppatore che torna al punto 1.

C'è il mondo ideale, e poi c'è il mondo reale.

Nel mondo ideale , tutto il tuo codice è stato testato, quindi puoi essere sicuro che tutto ciò che viene archiviato funzionerà o saprai che è danneggiato perché fallisce uno o più test. Inoltre, chiunque non sia così esperto verrà abbinato a qualcuno che ha esperienza, quindi la revisione del codice viene eseguita al volo (si commette mentre si va).

Nel mondo reale , le cose sono diverse. Business vuole che cambino live ora e ti dirà, con una faccia perfettamente dritta, che sì, avrai tempo per ripulire il codice e aggiungere i casi di test più tardi. Probabilmente non avrai il tempo di rivedere il codice tutto e la percentuale di codice coperta dai test diminuisce continuamente. Il motivo principale per la revisione del codice è che gli sviluppatori junior imparino dagli sviluppatori senior (quando c'è tempo per farlo) facendo in modo che una persona più esperta guardi alle modifiche e suggerisca "modi migliori di fare le cose (TM)". Avrai gli sviluppatori senior che hanno appena eseguito il codice non verificato. Branching solo per una revisione del codice e quindi la fusione è un'enorme perdita di tempo. Un modo per superare questo problema è quello di dichiarare una normale riunione settimanale di 2 ore (o così) in cui si selezionano una o due modifiche recenti che le persone hanno lavorato a breve termine e che i loro autori "presentano" il loro approccio guardando il codice insieme su un proiettore o qualcosa del genere Questo può portare ad alcune discussioni interessanti (tipicamente va fuori argomento un po ') ma generalmente migliora la comprensione di tutti su come farlo nel modo giusto. Inoltre la pressione di possibilmente dover presentare il tuo codice fa sì che alcune persone lo facciano meglio; -)

Oppure potresti essere fortunato e lavorare in un ambiente reale in cui non è così frenetico, i programmatori sono effettivamente apprezzati per quello che fanno invece di abusare, e c'è tempo per fare tutto bene. In questo caso la mia risposta sarebbe: prova alcuni dei diversi metodi suggeriti nelle risposte qui e vedi quale si adatta alla tua squadra e al modo in cui lavori meglio.

Le filiali dovrebbero funzionare correttamente, in base alla mia esperienza di utilizzo delle revisioni precedenti al commit precedenti.

_{Da notare, stavamo usando le revisioni pre-commit solo per le patch critiche per il codice candidato alla release di produzione, quindi non c'erano molti rami (le modifiche di routine venivano passate attraverso recensioni post-commit).}

Poiché sembra che tu stia per utilizzare recensioni pre-commit per tutte le modifiche, potresti dover gestire una grande quantità di filiali. Se ti aspetti che lo sviluppatore effettui una modifica "revisionabile" a settimana, finirai per avere circa 50 filiali ogni anno per ogni sviluppatore del team. Se utilizzi blocchi di lavoro più piccoli, come quelli che richiedono 1, 2, 3 ... giorni, moltiplica 50 per 2, 3, 5 ... di conseguenza.

Di seguito sono riportate alcune altre considerazioni da tenere in considerazione se lo desideri modo migliore .

1. gestione dei casi in cui il codice di blocchi di riesame ritardato è necessario per gli altri membri del team

Stabilire, monitorare e risolvere i conflitti relativi alle scadenze di revisione del codice. Per il mio ricordo delle revisioni preliminari alle modifiche di routine trattate in uno dei progetti precedenti, la scadenza ragionevole è di circa 3 giorni e il tempo di iniziare a preoccuparsi è quando la revisione non viene completata più di 1 giorno dopo la presentazione.

_{Per confronto, nelle revisioni post-commit questi requisiti sono molto più rilassati (sto usando una scadenza di 2 settimane e inizio a preoccuparmi dopo 1 settimana) - ma dal momento che si targetizzano le recensioni pre-commit, probabilmente non è interessante .}

2. unire conflitti quando si invia il codice revisionato

Che cosa fare se il commit per il codice revisionato è bloccato da modifiche in conflitto commesse da qualcun altro mentre il codice era in attesa di revisione?

Un paio di opzioni da considerare sono

• ritorna all'inizio e richiede agli sviluppatori di ri-implementare e riesaminare il cambiamento
  In tal caso, potrebbe essere necessario affrontare un impatto negativo sul morale della squadra che questo (forse!) Avrà.
• passare la responsabilità di unirsi ad altri membri del team ("merge master")
  In tal caso, dovrai anche decidere se le fusioni di per sé dovrebbero passare attraverso la revisione pre-commit o meno - e se sì, allora cosa fare nel caso in cui tale fusione a sua volta incontra un altro conflitto.
• ignora le modifiche apportate al codice revisionato nella fase di unione
  In questo caso potresti dover affrontare un impatto negativo sul morale della squadra legato al fatto che il codice impegnato differisce da quello che è stato revisionato.
• inventare un modo per evitare conflitti
  L'approccio diretto consiste nel consentire a uno sviluppatore di modificare uno specifico set di file, sebbene questo non ti protegga dal tipo di modifiche che non modificano direttamente i file, ma li impattano attraverso le modifiche interne all'API. Potresti anche scoprire che questo tipo di "blocco pessimistico" rende piuttosto problematici cambiamenti a livello di sistema e un profondo refactoring.

_{Per il confronto, non ci sarebbero problemi di questo tipo nelle revisioni post-commit (poiché trattano il codice che è già unito per definizione) - ma dal momento che scegli come target recensioni pre-commit, probabilmente non è interessante .}

3. carica lo sviluppatore che è in attesa di revisione

Stabilisci una politica esplicita in base alla quale lo sviluppatore che ha inviato la recensione deve passare a una nuova attività o fare qualcos'altro (come ad es. il revisore della ricerca).

_{Per fare un confronto, le revisioni post-commit richiedono raramente una politica esplicita (dato che è naturale procedere all'attività successiva dopo aver eseguito il commit del codice e tenere conto della scadenza della revisione è di una o due settimane) - ma dal momento che si target commetti recensioni, probabilmente non è interessante.}

Qualsiasi elemento di sviluppo che richiede una revisione dovrebbe essere in un ramo separato. Quindi, il ramo dovrebbe già esistere prima che venga il momento per la revisione. Quindi il passaggio dovrebbe essere:

1. Recensione
2. Revisiona (possibilmente)
3. ricomincia da Review (possibilmente)
4. Unisci nel tronco

L'unione è il punto difficile. Più a lungo il ramo rimane indipendente, più difficile sarà ridistribuirlo nel bagagliaio. (Potrebbe anche essere più difficile da testare.)

L'unione incrociata è una possibile soluzione. Prima di unire il tronco (passaggio 4 o anche prima, dire prima del punto 3 o del passaggio 1), unire il tronco al ramo. Lo sviluppatore può farlo e testarlo. Poi il ramo raggiunge il tronco e diventa più facile fonderlo nel tronco. L'unione nel bagagliaio è fatta meglio da te o da chi è responsabile del tronco.

Alcune persone provano rebase invece di unione incrociata. Alcune persone sostengono che il rebase è malvagio. Questo è un altro dibattito.