From: Junio C Hamano <hidden> Date: 2016-06-15 23:08:22
Fengguang Wu [off-list ref] writes:
Hi Junio,
On Sun, Feb 21, 2016 at 08:19:56PM -0800, Junio C Hamano wrote:
quoted
Xiaolong Ye [off-list ref] writes:
quoted
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 <redacted>
---
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.
quoted
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.
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.
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.
So I dunno.
Hi Junio,
On Sun, Feb 21, 2016 at 08:19:56PM -0800, Junio C Hamano wrote:
quoted
Xiaolong Ye [off-list ref] writes:
quoted
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 <redacted>
---
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
quoted
quoted
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
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2016-02-23 09:23:49
On 02/23/16 01:17, Fengguang Wu wrote:
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
Wouldn't the important part here be the git hash, rather than the tree?
If you have the same hash then it by definition is the same contents?
-hpa
On Tue, Feb 23, 2016 at 01:23:19AM -0800, H. Peter Anvin wrote:
On 02/23/16 01:17, Fengguang Wu wrote:
quoted
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
Wouldn't the important part here be the git hash, rather than the tree?
If you have the same hash then it by definition is the same contents?
Yes. Sorry for the partial wording! We should be talking about the
same thing: the hash of the tree object. The commit SHA1 will also
do the work.
Thanks,
Fengguang
From: Dan Carpenter <hidden> Date: 2016-02-23 10:33:27
So this is the format for the first patch?
base commit: 0233b800c838ddda41db318ee396320b3c21a560
Can we change it to include the name of the public tree we are starting
from?
applies-to: 0233b800c838 git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git#master
Of course, my absolute prefered format would be:
applies-to: net-next 0233b800c838
I don't think that's possible though? I often write that sort of a line
in my emails to Dave already.
Fengguang was suggesting something like this if we have to include
unmerged patches:
applies-to: net-next 0233b800c838
private patchset 1
private patchset 2
I don't think git knows what a patchset is. We would have to include
the subject line for each unmerged patch. I think we should only do
that if there is a cover letter, otherwise the it's too noisy.
regards,
dan carpenter
Hi Dan,
On Tue, Feb 23, 2016 at 01:32:53PM +0300, Dan Carpenter wrote:
So this is the format for the first patch?
base commit: 0233b800c838ddda41db318ee396320b3c21a560
What's in my mind is lines like
base tree/branch: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
base commit: afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc
base patch-id: a849260a843115dbac4b1a330d44256ee6b16d7b
The point is one piece of information per line, so that new lines can
be added trivially in future, like
base patch-subject: Linux 4.4
base tag: v4.4
The exact format can be improved wherever suitable. For example, use
more suitable key name part (eg. "base commit" => "base-commit") or
value part (eg. "$tree_url $branch" to "$tree_url#$branch").
Can we change it to include the name of the public tree we are starting
from?
applies-to: 0233b800c838 git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git#master
No problem, just that I'd prefer breaking up such information into
multi "key: value" lines.
Of course, my absolute prefered format would be:
applies-to: net-next 0233b800c838
I don't think that's possible though? I often write that sort of a line
in my emails to Dave already.
Yeah, that'd be most human readable. It does require people (and
scripts) to reach consensus on the tree/branch name, which may only be
possible for well known trees.
Fengguang was suggesting something like this if we have to include
unmerged patches:
applies-to: net-next 0233b800c838
private patchset 1
private patchset 2
I don't think git knows what a patchset is.
Git may not need to have patchset concept. Suppose a developer's local
branch has
v4.4
private commit 1, subject: do aaa
private commit 2, subject: do bbb
private commit 3, subject: do ccc
private commit 4, subject: do ddd
private commit 5, subject: do eee
If he decided to send commits 1-2 as one patchset, and 3-5 as another
patchset to LKML. The 2 cover letters would look like (only showing
useful fields):
$ git format-patch commit 1..commit 2
[PATCH 0/2]
base commit: afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc
$ git format-patch commit 3..commit 5
[PATCH 0/3]
base patch-subject: do bbb
The 0day robot will be able to find the suitable base and re-create
exactly the same tree object for both the above 2 patchsets based on
the first one's "base commit" and the second one's "base patch-subject".
We would have to include the subject line for each unmerged patch.
That's a good idea!
I think we should only do that if there is a cover letter, otherwise
the it's too noisy.
Or if no cover letter, the information can be included in the first
patch, ie. [PATCH 1/N].
Thanks,
Fengguang
From: Dan Carpenter <hidden> Date: 2016-02-23 13:31:50
Blergh... You want it machine readable and I want it human readable. I
don't care so much about the cover letter but for the first patch then I
really want something minimal (one line) and human readable.
base tree/branch: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
base commit: afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc
base patch-id: a849260a843115dbac4b1a330d44256ee6b16d7b
base patch-subject: Linux 4.4
base tag: v4.4
To me that looks like an unparseable wall of text. My version of that
is:
Applies-to: afd2ff9b7e1b+ origin
As a human all I really want to know is the tree to apply this to. If
it doesn't apply then I don't debug it, I just send an automatic note
"This doesn't apply to staging-next. Please redo."
I think that Applies-to is a better name and also that grepping for
"^base " is less reliable than grepping for ^Applies-to.
I used "origin" because that's the name in Next/Trees. The + means
private patches are applied. That's what we already do in naming the
kernel. If the + matters, then I would include a cover letter.
I have no idea what a "base patch-id" is so that doesn't work at all.
Including the tag is just duplicative since we already have the hash.
In my email, I proposed that we list all the other private patches in a
cover letter, but I think you are saying that we only need to know the
most recent private patch? Another idea would be to list them newest
to oldest (git log order instead of email order) in the cover letter.
Btw, I always work against linux-next and Dave M is always getting
annoyed with me for not marking which patches go to net and which go to
net-next. I don't use git format-patch, but I will probably start using
"Applies-to: net" or "Applies-to: net-next".
regards,
dan carpenter
From: Junio C Hamano <hidden> Date: 2016-02-23 19:51:36
Fengguang Wu [off-list ref] writes:
quoted
quoted
quoted
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.
quoted
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.
On Tue, Feb 23, 2016 at 04:31:35PM +0300, Dan Carpenter wrote:
Blergh... You want it machine readable and I want it human readable. I
Yeah. It's kind of tasting which may differ among people. I'll leave
the judgments to Junio and others, and only add necessary comments to
your points.
don't care so much about the cover letter but for the first patch then I
really want something minimal (one line) and human readable.
base tree/branch: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
base commit: afd2ff9b7e1b367172f18ba7f693dfb62bdcb2dc
base patch-id: a849260a843115dbac4b1a330d44256ee6b16d7b
base patch-subject: Linux 4.4
base tag: v4.4
The necessary lines for the robot are
base commit:
base patch-id:
or
base tree-id:
base patch-id:
The "base tree-id" will be useful if the submitted patchset is based
on a public (maintainer) commit.
The "base patch-id" will be useful if the submitted patchset is based
on another patchset someone (likely the developer himself) posted to
the mailing list.
To me that looks like an unparseable wall of text. My version of that
is:
Applies-to: afd2ff9b7e1b+ origin
As a human all I really want to know is the tree to apply this to. If
it doesn't apply then I don't debug it, I just send an automatic note
"This doesn't apply to staging-next. Please redo."
I think that Applies-to is a better name and also that grepping for
"^base " is less reliable than grepping for ^Applies-to.
Grep reliability should be the same, if you use "^base tree-id" and
"^base patch-id". If necessary, we can avoid white space by naming the
keys base-tree-id and base-patch-id.
I used "origin" because that's the name in Next/Trees. The + means
private patches are applied. That's what we already do in naming the
kernel. If the + matters, then I would include a cover letter.
I have no idea what a "base patch-id" is so that doesn't work at all.
It'll come from this command
man git patch-id
It'll be useful if the patchset's base commit is a private one -- not
in any public maintainer tree, however the developer may have posted
it to LKML before.
The "base patch-id" can more reliably track different versions of
patches than "base patch-subject", and do not have the risk of
information leaking in case it's a confidential patch.
Including the tag is just duplicative since we already have the hash.
That's right. Just in case it's more human readable.
In my email, I proposed that we list all the other private patches in a
cover letter, but I think you are saying that we only need to know the
most recent private patch?
Yes in test robot POV. However it's a general git feature, so I guess
there will be more potential use cases and requirements.
Another idea would be to list them newest
to oldest (git log order instead of email order) in the cover letter.
Btw, I always work against linux-next and Dave M is always getting
annoyed with me for not marking which patches go to net and which go to
net-next. I don't use git format-patch, but I will probably start using
"Applies-to: net" or "Applies-to: net-next".
As for now, I see the netdev ML has the convention
[PATCH net]
[PATCH net-next]
to tell Dave the target tree.
Thanks,
Fengguang
On Tue, Feb 23, 2016 at 11:51:31AM -0800, Junio C Hamano wrote:
Fengguang Wu [off-list ref] writes:
quoted
quoted
quoted
quoted
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.
Problem arises when a developer based his work on a maintainer's topic
branch. The robot doesn't know that and tests the patch on v4.5-rc5,
which may trigger a false error because the patch depends on some
changes in that maintainer's topic branch. In that case, the error
report will be pure noise.
Thanks,
Fengguang