<div dir="ltr"><div dir="ltr"><br clear="all"><div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr">Regards,<br></div><div>Sheetal Pamecha<br></div></div></div></div></div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 15, 2020 at 4:31 PM Amar Tumballi <<a href="mailto:amar@kadalu.io">amar@kadalu.io</a>> 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"><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 <<a href="mailto:xhernandez@redhat.com" target="_blank">xhernandez@redhat.com</a>> 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'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't have this feature (at least I haven't seen it). So I'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'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 "Squash and Merge" 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'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't need './rfc.sh' 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></div></div></blockquote><div><br></div><div>Actually True, Since the migration to github. I have not been using ./rfc.sh and For me it's easier and cleaner. <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 class="gmail_quote"><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" target="_blank">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>
_______________________________________________<br>
<br>
Community Meeting Calendar:<br>
<br>
Schedule -<br>
Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC<br>
Bridge: <a href="https://bluejeans.com/441850968" rel="noreferrer" target="_blank">https://bluejeans.com/441850968</a><br>
<br>
<br>
<br>
<br>
Gluster-devel mailing list<br>
<a href="mailto:Gluster-devel@gluster.org" target="_blank">Gluster-devel@gluster.org</a><br>
<a href="https://lists.gluster.org/mailman/listinfo/gluster-devel" rel="noreferrer" target="_blank">https://lists.gluster.org/mailman/listinfo/gluster-devel</a><br>
<br>
</blockquote></div></div>