<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 23, 2017 at 4:21 PM, Niels de Vos <span dir="ltr"><<a href="mailto:ndevos@redhat.com" target="_blank">ndevos@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Fri, Jun 23, 2017 at 03:53:42PM +0530, Pranith Kumar Karampuri wrote:<br>
> On Fri, Jun 23, 2017 at 3:01 PM, Niels de Vos <<a href="mailto:ndevos@redhat.com">ndevos@redhat.com</a>> wrote:<br>
><br>
> > On Fri, Jun 23, 2017 at 01:47:57PM +0530, Pranith Kumar Karampuri wrote:<br>
> > > On Fri, Jun 23, 2017 at 1:30 PM, Niels de Vos <<a href="mailto:ndevos@redhat.com">ndevos@redhat.com</a>> wrote:<br>
> > ><br>
> > > > On Fri, Jun 23, 2017 at 09:15:21AM +0530, Pranith Kumar Karampuri<br>
> > wrote:<br>
> > > > > hi,<br>
> > > > > Now that we are doing backports with same Change-Id, we can<br>
> > find the<br>
> > > > > patches and their backports both online and in the tree without any<br>
> > extra<br>
> > > > > information in the commit message. So shall we stop adding text<br>
> > similar<br>
> > > > to:<br>
> > > > ><br>
> > > > > > Reviewed-on: <a href="https://review.gluster.org/17414" rel="noreferrer" target="_blank">https://review.gluster.org/<wbr>17414</a><br>
> > > > > > Smoke: Gluster Build System <<a href="mailto:jenkins@build.gluster.org">jenkins@build.gluster.org</a>><br>
> > > > > > Reviewed-by: Pranith Kumar Karampuri <<a href="mailto:pkarampu@redhat.com">pkarampu@redhat.com</a>><br>
> > > > > > Tested-by: Pranith Kumar Karampuri <<a href="mailto:pkarampu@redhat.com">pkarampu@redhat.com</a>><br>
> > > > > > NetBSD-regression: NetBSD Build System <<br>
> > <a href="mailto:jenkins@build.gluster.org">jenkins@build.gluster.org</a>><br>
> > > > > > Reviewed-by: Amar Tumballi <<a href="mailto:amarts@redhat.com">amarts@redhat.com</a>><br>
> > > > > > CentOS-regression: Gluster Build System <<br>
> > <a href="mailto:jenkins@build.gluster.org">jenkins@build.gluster.org</a><br>
> > > > ><br>
> > > > > (cherry picked from commit de92c363c95d16966dbcc9d8763fd4<br>
> > 448dd84d13)<br>
> > > > ><br>
> > > > > in the patches?<br>
> > > > ><br>
> > > > > Do you see any other value from this information that I might be<br>
> > missing?<br>
> > > ><br>
> > > > I think it is good practise to mention where the backport comes from,<br>
> > > > who developed and reviewed the original. At least the commit-id is<br>
> > > > important, that way the backport can easily be compared to the<br>
> > original.<br>
> > > > git does not know about Change-Ids, but does know commmit-ids :)<br>
> > > ><br>
> > ><br>
> > > Change-ID captures all this information. You can know the patch in all<br>
> > > branches with Change-ID, now that we are following same Change-ID across<br>
> > > branches.<br>
> ><br>
> > However a Change-Id is not standard git, it is purely a Gerrit thing. I<br>
> > can't cherry-pick or 'git show' a Change-Id, but that works fine with a<br>
> > git-commit.<br>
> ><br>
><br>
> Where do we mention git commit-id now? If we do the backport using gerrit,<br>
> it adds it as "(cherry picked from commit<br>
> de92c363c95d16966dbcc9d8763fd4<wbr>448dd84d13)",<br>
> otherwise I don't think it gets mentioned, right?<br>
<br>
</div></div>Correct, that is just what "git cherry-pick -x" does too. It is one of<br>
the main requirements we check for backports. It is not enforced, but<br>
strongly encouraged to have it. Nothing in the commit message is<br>
enforced, it is up to the maintainers to make sure everything makes<br>
sense there.<br>
<br>
Part of this is also mentioned on<br>
<a href="http://gluster.readthedocs.io/en/latest/Developer-guide/Backport-Guidelines/" rel="noreferrer" target="_blank">http://gluster.readthedocs.io/<wbr>en/latest/Developer-guide/<wbr>Backport-Guidelines/</a><br>
<span class="gmail-"><br>
> > I also like to give credits to the people that originally wrote and<br>
> > reviewed the change. It is not required, but it is nice to have.<br>
> ><br>
><br>
> If you do the backport using standard commands Author and committer are<br>
> correctly shown in the patch, I think the tool already handles it. As for<br>
> the reviewer etc. as this information is available on the actual patches,<br>
> if the person who is checking for it really wants it, can find out.<br>
<br>
</span>Depends, if the patch is not exactly the same, the author should be<br>
changed to whoever did the backport and add a note explaining the<br>
difference. Even cherry-picks can be different, and sending those as an<br>
author who did not do the (incorrect?) work is wrong imho.<br>
<br>
But yes, if a backport is straight forward, the original Author of the<br>
change can surely be kept.<br>
<br>
<br>
Note that all of this is just my opinion, and based on working with many<br>
different projects that use git (or other tools) to identify patches<br>
that could be candidates for backporting. In general, the more details<br>
that are captured in the commit message, the easier it is to get an<br>
understanding of the different patches in different branches.<br></blockquote><div><br></div><div>Yes, I am also for as much information as possible in the commit with least amount of human effort. In time I would like us to get to a point, where we just have to say: backport release-3.12 release-3.11 release-3.10 and the script should clone, send the patches on gerrit and do recheck centos, recheck netbsd, so the only human effort has to be to be merge the patch<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">
<span class="gmail-HOEnZb"><font color="#888888"><br>
Niels<br>
</font></span><div class="gmail-HOEnZb"><div class="gmail-h5"><br>
<br>
> > > > We should try to have all the needed details in the git repository, and<br>
> > > > not rely on Gerrit for patch verification/checking. When I'm working on<br>
> > > > a patch and wonder why/when something related was changed, I'll use the<br>
> > > > local history, and do not want to depend on Gerrit.<br>
> > > ><br>
> > ><br>
> > > Change-ID is mentioned in the commit which is there in the git log, so we<br>
> > > can figure out the information without needing internet/gerrit. So that<br>
> > > part is not a problem.<br>
> ><br>
> > Yes, of course I can figure it out, but it is additional work. Most<br>
> > tools do not know about Change-Ids, like browsing through the code on<br>
> > GitHub; a commit-id will be linked, a Change-Id not.<br>
> ><br>
> ><br>
> We will discuss this further after my question above about commit-id is<br>
> answered.<br>
><br>
><br>
> > > All of this information was important to mention earlier because there<br>
> > was<br>
> > > no common thing binding all together. With same Change-ID across branches<br>
> > > for a patch, it seems unnecessary to mention this information in the<br>
> > commit<br>
> > > message.<br>
> ><br>
> > Not everyone is familiar with how Gerrit handles Change-Ids. A<br>
> > git-commit is something that other (new) contributors understand better.<br>
> > I prefer to make it as easy as possible for people to go through the git<br>
> > log and compare/check/verify whatever they are looking for. Limiting<br>
> > references to Change-Ids makes their task a little more difficult.<br>
> ><br>
><br>
> Same here: We will discuss this further after my question above about<br>
> commit-id is answered.<br>
><br>
><br>
> ><br>
> > Niels<br>
> ><br>
><br>
><br>
><br>
> --<br>
> Pranith<br>
</div></div></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><div dir="ltr">Pranith<br></div></div>
</div></div>