[Gluster-devel] reagarding backport information while porting patches

Fri Jun 23 11:00:24 UTC 2017

On Fri, Jun 23, 2017 at 4:21 PM, Niels de Vos <ndevos at 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 <ndevos at 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 <ndevos at 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 <jenkins at build.gluster.org>
> > > > > >     > Reviewed-by: Pranith Kumar Karampuri <pkarampu at redhat.com>
> > > > > >     > Tested-by: Pranith Kumar Karampuri <pkarampu at redhat.com>
> > > > > >     > NetBSD-regression: NetBSD Build System <
> > > jenkins at build.gluster.org>
> > > > > >     > Reviewed-by: Amar Tumballi <amarts at redhat.com>
> > > > > >     > CentOS-regression: Gluster Build System <
> > > jenkins at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20170623/a1f41659/attachment-0001.html>