Message-ID: <87d1rnjil4.fsf@x220.int.ebiederm.org>
Date:	Tue, 23 Feb 2016 14:08:07 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Junio C Hamano <gitster@...ox.com>
Cc:	Fengguang Wu <fengguang.wu@...el.com>,
	Xiaolong Ye <xiaolong.ye@...el.com>, git@...r.kernel.org,
	ying.huang@...el.com, philip.li@...el.com, julie.du@...el.com,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Christoph Hellwig <hch@....de>,
	"H. Peter Anvin" <hpa@...or.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC/PATCH 1/1] format-patch: add an option to record base tree info

Junio C Hamano <gitster@...ox.com> writes:

> Fengguang Wu <fengguang.wu@...el.com> writes:
>
>>> >> 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
>
> I always get negatives and positives confused, so let me think aloud
> with an example.  Let's say that somebody worked on adding a new
> feature based on v4.2 codebase and sent in a patch series.  The
> series touched files in quiescent part of the system, these files
> are identical between v4.2 and the current codebase at v4.5-rc5, and
> the series applies cleanly to a "wrong" base tree at the tip of
> 'master'.  But it turns out that the series uses an old API that was
> removed in the meantime.  The test robot may say "the result of
> applying the series does not even build" and the developer would
> complain to you saying "You tested with a wrong version".
>
> I've already said that I can see the value this approach has for
> you.  By having the developer state which commit the series was
> based on, it will shield you from such a complaint, because you
> would not use closer-to-tip 'master' as the base, but instead use
> v4.2 codebase for the test.
>
> As I said, what is unclear to me is what value this apporach gives
> to the project.
>
>>> 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.
>
> It is valuable for a testing organization to say "We tested this
> series on top of version X.  We know it works, we have tested on a
> lot more hardware than the original developer had, we know this is
> good to go."  It is a valuable service.
>
> But that is valuable only if version X is still relevant, isn't it?
>
> Is the relevance of a version something that is decided by a
> developer who submits a patch series, or is it more of an attribute
> of the project and where the current integration is happening?
> Judging from the responses from Dan to this thread, I think the
> answer is the latter, and for the purpose of identifying the
> relevant version(s), the project does not even care about the exact
> commit, but it wants to know more about which branch the series is
> targetted to.
>
> With that understanding, I find it hard to believe that it buys the
> project much for the "base" commit to be recorded in a patch series
> and automated testing is done by applying the patches to that exact
> commit, which possibly is no-longer-relevant, even though it may
> help shielding the testing machinery from "you tested with a wrong
> version" complaints.
>
> Isn't it more valuable for the test robot to say "this may or may
> not have worked well with whatever old version the patch series was
> based on, but it no longer is useful to the current tip of the
> 'master'"?  If you consider what benefit the project would gain by
> having such a robot, that is the conclusion I have to draw.
>
> So I still am not convinced that this "record base commit" is a
> useful thing to do.

So I don't know what value this has to the git project.

The value of Fengguag's automated testing to the kernel project is to
smoke out really stupid things.

- A developer forgot to actually compile test their code.
  An error report that the code does not even compile from Fengguag
  allows the patch to be dropped without spending any time on it.

- A developer failed to test some weird configuration option in
  like !CONFIG_SYSCTL.  Which results in build errors for people
  who use weird configurations but not for everyone.

- Similarly for changes to non-x86 architectures when making a cross
  tree change.  It is valueable to get feedback on architectures you
  can not compile yourself.

I have not see any great value attached to passing Fengguag's tests but
I have seen a lot of value where the tests don't pass for some stupid
reason and people can go fix it up.

The kernel is a weird project where it actually is a bit tricky to
compile test everything.

Eric

Powered by blists - more mailing lists