Hi Ilpo-
On Feb 10, 2009, at 8:12 AM, Ilpo Järvinen wrote:
On Mon, 9 Feb 2009, Chuck Lever wrote:
On Feb 8, 2009, at 12:25 AM, J. Bruce Fields wrote:
On Sat, Feb 07, 2009 at 06:56:03PM +0200, Ilpo Järvinen wrote:
I personally think that people were honestly just mislead to
think that
diff-funcs output is part of the patch
Yes, exactly, in my case, that's all it was--the "diff" I thought
I saw
in my 2-second skim looked odd, and I figured a comment might have
gotten dropped off in the process of replying and adding cc's--
hence the
original question.
I apologize if my request for "English" seemed Anglo-centric. What
I intended
was "non-machine-generated" and expressed in the natural language
that is
usual for Linux kernel patch descriptions, which appears to be
English.
...I still argue that if title gives the same information as that
sentence would, the sentence is optional.
I'm OK with that. In the original patch, I think both were somewhat
difficult to parse. My request is to try harder next time to make
them clear. It sounds like we agree here.
Some patches don't even have
patch description beyond title and there's very little problem with
that.
As I admitted earlier the title wasn't as good as it should have
(and I
even sent a followup to please all including even the in body sentence
plus the fixed title). Not that I find the title really a problem to
sane
people who understand that code-duplication is bad and getting rid
of it
is what one should do in general, as long as it's possible without
making
a mess out of it.
Like all things software, absolute statements are usually always
proven incorrect (eg "goto is spawn of the devil and should never be
used"). Code duplication is _usually_ bad. Sometimes there is a
reason for it. We want to avoid a knee-jerk response of eliminating
it wherever we find it, without another thought.
I agree with you that the diff output in the patch description was
confusing,
and yes, after Dave's first comment I understood what the
description meant.
I believe patch authors owe it to maintainers, who have dozens of
patches to
review a day, to keep it as simple and easy as possible. I have
tried to work
with individual maintainers where possible to improve the
legibility of my
patches, though at times I have fallen short.
Although interesting, the tool output doesn't seem necessary to
justify the
proposed change.
I don't understand why a tool output cannot "say" the same as some
English
sentence...
It can. But it's interesting to observe that the point of your
proposed change was to reduce lines of code and code obscurity. Your
description, however, was much longer and more obscure than it needed
to be. :-)
I was too harsh to ask for no machine-generated patch descriptions.
The point is, is the description meaningful and concise?
And if a proof that the code blocks that were merged were
infact identical (instead of having some hard to spot difference)
doesn't
seem like useful information to reviewer I don't really know what to
say.
Verification of your patch is different than justification. But
that's a minor point.
In addition to crafting a legible description, my suggestions were
about
cc'ing the proper mailing list, and routing this patch directly to
the sunrpc
kernel maintainers in the first place, all of which are listed
within easy
reach in MAINTAINERS.
Please note that I couldn't guess that I'd have to look for another
entry
after first SUNRPC entry in the MAINTAINERS. So asking outsider to
know
who is who in sunrpc realm is not very likely to be a practical
requirement either. And even to add into confusion the latest commits
in that particular directory had the last S-o-B line as Bruce's. How
could
I have guessed? Forgetting to lookup the address from MAINTAINERS
into the
first mail is something which just happened and I'm certainly guilty
to
that :-).
Yes, we should fix the SUNRPC entries in MAINTAINERS.
...I've had enough down turning experiences with random dev lists
listed
in the MAINTAINERS (mostly spammy subsriber only ones, nearly infinite
response time, etc things) that I mostly ignore them nowadays though
this
time it would have been vger list so no subscription required madness.
Instead I either select lkml or netdev, this time netdev because of
the obvious presence under net/ hierarcy.
My experience is usually the opposite. I post a patch directly to a
maintainer and get a "please use such-and-such list" response. The
exception is usually very specific device drivers.
But you're right, it would be nice to have some consistency here
across the kernel.
But I wonder how we ended up with these two copies of the same
logic. It may
be that at some point in the past we had more different logic in
the two
write_space functions, but I suppose we can split it out again if
we ever need
to. Thinking aloud, at some point we may have lost a little
history here, or
we may have perhaps lost any implicit operational dependencies on
other parts
of xprtsock.c (like the TCP state logic) that are not expressed in
the source
code or comments.
Well, this kind of thinking is beyond me and requires more knowledge
about
internals of the code.
My concern with immediately accepting a patch that appears to be
solely machine-generated is exactly this. The submission didn't leave
me with the impression that there was a human who had taken the time
to understand history (git log/annotate) and how the code works, what
the source code was attempting to express, or whether the resulting
patch was even tested. Usually you can glean a bit of this from a
lengthier patch description.
Yes, yours was a simple and proper change in the end, but still.
Given the long "dialog" just had on lkml about compiler (machine-
generated) optimizations, I think we would all be just a little
skeptical (or perhaps careful) about such automated transformations.
IMHO, this kind of speculation changes nothing
really, and besides the functions _were_ different to begin with as
was
proven in the patch description :-). ...Would they have been fully
identical you would have seen a bit different patch ;-).
If you think there's something wrong, please fix, you're free to do
so,
I don't understand enough. But we don't keep all those #if 0 blocks
around either just because somebody thinks that something will get
implemented one day (not that you directly made such assertion,
probably
saying mildy the opposite instead). Ironically, you would have been
perfectly happy with the particular functions unless a cleanup patch
against them gets made... :-)
You are correct, the analysis doesn't change anything, but the
analysis is worth at least noting during review. It helps us
understand where we have to make the code more clear.
The question is whether the code itself documents dependencies on
other parts of the code, whether such dependencies are only documented
by comments, or whether it is entirely implicit and up to the folks
who work on it to understand and remember. This is usually only an
issue when someone who has never worked on the code submits a patch.
The goal of open source code is to facilitate other folks modifying
our code. For open source especially it is a larger issue than simple
code maintenance. It is not enough for open source code simply to
work. If people outside our team (or outside the larger Linux
community) can't easily understand the important features of our open
source code, then we've still failed, even if the legal barriers have
been lifted by the GPL.
Thus I'm especially sensitive to areas where our source code does not
express itself well to folks who are not familiar. We want to invite
wider review, and that's simply not possible if nobody understands
this code base but ourselves.
The newly shared code appears to be socket-specific, so it does not
belong in the shared function we already have, xprt_write_space(),
which
is meant to be transport-generic.
IMHO, this is the most useful response so far! :-) Usually the hardest
obstacles I face with non-familiar code is its structure and naming.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
