<div dir="ltr"><div>Thanks for taking time on this, and sending this note Xavi!</div><div><br></div>Some comments inline!<div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 15, 2020 at 4:03 PM Xavi Hernandez &lt;<a href="mailto:xhernandez@redhat.com">xhernandez@redhat.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi all,<div><br></div><div>after the recent switch to GitHub, I&#39;ve seen that reviews that require multiple iterations are hard to follow using the old workflow we were using in Gerrit.</div><div><br></div><div>Till now we basically amended the commit and pushed it again. Gerrit had a feature to calculate diffs between versions of the patch, so it was relatively easy to follow the changes between iterations (unless there was a big change in the base branch and the patch was rebased).</div><div><br></div><div>In GitHub we don&#39;t have this feature (at least I haven&#39;t seen it). So I&#39;m proposing to change this workflow.</div><div><br></div><div>The idea is to create a PR with the initial commit. When a modification needs to be done as a result of the review, instead of amending the existing commit, we should create a new commit. From the review tool in GitHub it&#39;s very easy to check individual commits.</div><div><br></div></div></blockquote><div><br></div><div>+1<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>Once the review is finished, the patch will be merged with the &quot;Squash and Merge&quot; option, that will combine all the commits into a single one before merging, so the end result will be exactly the same we had with Gerrit.</div><div><br></div></div></blockquote><div><br></div><div>+1 </div><div>Just a note to the maintainers who are merging PRs to have patience and check the commit message when there are more than 1 commits in PR.</div><div> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>Another thing to consider is that rfc.sh script always does a rebase before pushing changes. This rewrites history and changes all commits of a PR. I think we shouldn&#39;t do a rebase in rfc.sh. Only if there are conflicts, I would do a manual rebase and push the changes.</div><div><br></div></div></blockquote><div><br></div><div>With github workflow, we don&#39;t need &#39;./rfc.sh&#39; in my personal opinion. I ported it to new branch and github considering the number of developers who are used to it.  If you do the changes as per github, then you would have a separate branch per PR (ie, feature/bug), so you are at your own to decide when to rebase.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>What do you think ?</div><div><br></div></div></blockquote><div><br></div><div><div>I agree, we can remove -f option of ./rfc.sh and also the rebase part in ./rfc.sh!</div><div> </div></div><div>Regards,</div><div>Amar</div><div>--</div><div><a href="https://kadalu.io">https://kadalu.io</a></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>Regards,</div><div><br></div><div>Xavi</div></div></blockquote></div><div><br></div></div></div>