On Fri, Nov 02, 2018 at 12:21:13PM +0000, Lorenzo Pieralisi wrote:
> On Fri, Nov 02, 2018 at 12:23:12AM +0000, Trent Piepho wrote:
> > On Thu, 2018-11-01 at 13:55 +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Oct 31, 2018 at 07:49:59PM +0000, Trent Piepho wrote:
> > > > This fixes a regression introduced in merge 562df5c8521e.
> > >
> > > A merge being a commits collection, the regression was certainly
> > > introduced by a specific commit in it, not the merge itself.
> >
> > In this case it really is merge commit.
> >
> > While the problem and fix are relatively obvious, finding how it came
> > to be broken was a challenge. Most of the commit message is my proof
> > that this is a bug and not something done intentionally, by tracking
> > back the complicated route that caused the code to be in its current
> > state.
> >
> > Basically there are two commits, on both branches of the merge, neither
> > of which caused a problem nor were they incorrect in any way at the
> > time they were committed. When that merge combined this collection of
> > multiple commits, it did not do so correctly and that is the point a
> > bug appeared.
> >
> > Or put another way, this problem was already fixed some time ago, but
> > in the merge commit the fix was dropped.
> >
> > It seems like the proper way to attribute my fix is to the merge, as
> > that is what caused the regression. There is no prior commit where one
> > can observe the regression.
>
> You are right, I went through git history and I agree with your
> summary, that's what happened.
>
> > > Also for the Fixes: tag and all references.
> >
> > Would it be ok to refer to the commit this way the first time, then use
> > a shortened method for subsequent usages? Otherwise talking about two
> > commits becomes very long. This it what I have mostly done, other than
> > the "Fixes" line. I'm surprised checkpatch didn't catch that.
>
> I would like to ask Bjorn's opinion on this, I do not know if adding
> a Fixes: tag with a merge commit in it is common practice but that
> summarizes what happened so I assume it should be fine.
That sounds good to me.
> As for the shortened commit format I understand your point, I do not
> know if that's "official" from a kernel process standpoint.
It took me a while to figure out what '886 meant because it's unusual and
the "'" usually goes where the *missing* characters are. Personally I
would use the full
886bc5ceb5cc ("PCI: designware: Add generic dw_pcie_wait_for_link()")
at the first reference and just 886bc5ceb5cc for subsequent references.
Thanks for chasing this down! It's really a hassle to figure out things
that work separately but not together.
Bjorn
