Re: [RFC/PATCH 1/1] format-patch: add an option to record base tree info

[Fengguang Wu]

Hi Junio and All,

CC more relevant people. FYI this thread starts here:

        http://thread.gmane.org/gmane.comp.version-control.git

On Mon, Feb 22, 2016 at 10:54:38PM -0800, Junio C Hamano wrote:
> Fengguang Wu <fengguang.wu@xxxxxxxxx> writes:
> 
> > Hi Junio,
> >
> > On Sun, Feb 21, 2016 at 08:19:56PM -0800, Junio C Hamano wrote:
> >> Xiaolong Ye <xiaolong.ye@xxxxxxxxx> writes:
> >> 
> >> > It would be helpful for maintainers or reviewers to know the base tree
> >> > info of the patches created by git format-patch. Teach git format-patch
> >> > a --base-tree-info option to record these info.
> >> >
> >> > Signed-off-by: Xiaolong Ye <xiaolong.ye@xxxxxxxxx>
> >> > ---
> >> 
> >> I have a mixed feeling about this one, primarily because this was
> >> already tried quite early in the life of "format-patch" command.
> >> 
> >>     http://thread.gmane.org/gmane.comp.version-control.git/9694/focus=9757
> >> 
> >> Only the name is different (it was called "applies-to" and named a
> >> tree object).
> >
> > Either commit or tree object will work for us. We can use it in
> > v2 if you prefer tree object.
> 
> Sorry, I think you misunderstood.  By only the name is different, I
> didn't mean to say that the tree object name should be shown as the
> old proposal did.  What I meant but didn't explicitly say, as I
> thought it was sufficient to point at an old discussion thread, was
> that this was already tried and rejected.  This round uses different
> name but does essentially the same thing as the old proposal, and I
> do not think I heard anything new that supports this patch against
> earlier rejection by Linus.  That is what gave me a mixed feeling.

I can understand the rejection by Linus in development process POV.

However we are facing a new situation: in test robot POV, IMHO there
are values to test exactly the same tree as the patch submitter.
Otherwise the robot risks

- false negative: failing to apply and test some patches
- false positive: sending wrong bug reports due to guessed wrong base tree

> >> Is it your goal to insist on one exact commit the patch is applied
> >> to?
> >
> > Right. Our goal is fully automated patch testing, where the base tree
> > info is required for *reliably* avoid reporting false positives.
> >
> > A clean git-apply does not guarantee the resulted code is logically
> > consistent and hence testable by 3rd party. For a 3rd party tester to
> > provide useful and trustable test reports, he must apply the patch to
> > exactly the same base as the patch submitter.
> 
> The patch submitter (or you as a third party tester) is not in the
> position to dictate the integrator to apply the patch to one
> specific commit and use it from there.  The integrator would pick an
> appropriate base that would be different from the commit where the
> patch was taken from, apply it there, and merge the result to the
> tip of the mainline, or apply the patch directly to the tip of the
> mainline.  Even if the integrator picked the commit the patch was
> taken from, the result would not be used alone without any other
> changes, i.e. before getting merged into the integration branch.

Yeah. Per my understanding the base commit info will be mainly parsed
by test robots instead of integrators.

> So in that sense, any test that is done by the patch submitter and
> the third party tester would not be what will be released to the
> wild *anyway*.  The resulting code will be exercised in a context
> that *is* different from the context the original author had.

That's right. But no worry, when the patch is merged by maintainer,
we'll test it once again in the maintainer tree.

Pre-merge patch testing is useful in 2 ways:

- shift left testing to early review stage

- maintainer trees are typically not rebaseable. When errors are
  discovered there, it's a bit too late: the error will likely remain
  in git history for ever. Which will hurt bisects.

> I can see that recording the exact commit object name allows you to
> claim that you identified the exact commit to apply the patch, and
> that you tested the exact tree contents.  It however is unclear what
> the value of such a claim would be to the project or to the
> integrator.

The value of base commit info is: providing a solid ground to the
tester, to reliably avoid false positive/negatives.

> So I dunno.

FYI, the 0day test robot will be able to work better if provided the
base commit info. It'll work a bit more sophisticated than simply
relying on the base commit info: if it's sure about the tree the patch
is targeted for (or the maintainer would apply to), it'll use that as
base tree[*]; otherwise it'll fall back to using the base commit info
included in the patchset.

[*] For examples,

        [PATCH -mm] ...
        [PATCH net] ...

For such patches we are sure they are targeted for the well known
mm/net trees.

Anyway the worst case of not adopting the discussed patch is, the 0day
test robot continue to work in current heuristic way.

Thanks,
Fengguang