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