[Gluster-devel] reagarding backport information while porting patches

[Niels de Vos]

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.

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