malformed patch - where do they come from?

[Maximillian Dornseif]

Hello,

I have a problem with the dif viewer (http://reviewboard/r/58/diff/)
complaining about the fact that patch complains about a malformed
patch at reviewboard/diffviewer/diffutils.py, line 125.

Checking by hand it turs out that the patchfile really is malformed:

review$ patch -l -o _tmp_test /tmp/reviewboard.1PLuNw/tmpFCFZGk < /
tmp/reviewboard.1PLuNw/produktpass.html.diff
Hmm... Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|Index: /branches/production/.../produktpass.html
|===================================================================
|--- /branches/production/2008-05-27/.../produktpass.html (revision
4065)
|+++ /branches/production/2008-05-27/.../produktpass.html (working
copy)
--------------------------
Patching file /tmp/reviewboard.1PLuNw/tmpFCFZGk using Plan A...
patch: **** malformed patch at line 291:

I have tried the patch utils coming with FreeBSD 7.0 (based on GNU
patch 2.1) and GNU patch 2.5.4. and both result in an "malformed
patch" error message. I see no obvious errors in the patchfile. Line
291 is the last (empty) line of the patchfile.

...
00000150 6b 74 70 61 73 73 2e 68 74 6d 6c 09 28 77 6f 72 |
ktpass.html.(wor|
00000160 6b 69 6e 67 20 63 6f 70 79 29 0a 40 40 20 2d 31 |king
copy).@@ -1|
00000170 2c 32 38 36 20 2b 30 2c 30 20 40 40 0a 2d 7b 25 |,286 +0,0
@@.-{%|
00000180 20 65 78 74 65 6e 64 73 20 22 69 6e 74 65 72 6e | extends
"intern|
00000190 2f 62 61 73 65 5f 73 69 74 65 2e 68 74 6d 6c 22 |/
base_site.html"|
000001a0 20 25 7d 0a 2d 7b 25 20 6c 6f 61 64 20 69 31 38 | %}.-{%
load i18|
...
00002d60 0a 2d 3c 70 3e 0a 2d 20 20 20 20 7b 7b 20 70 72 |.-
<p>.- {{ pr|
00002d70 6f 64 75 63 74 2e 61 6e 6e 6f 74 65 20 7d 7d 0a |
oduct.annote }}.|
00002d80 2d 3c 2f 70 3e 0a 2d 0a 2d 3c 2f 64 69 76 3e 0a |-</p>.-.-
</div>.|
00002d90 2d 0a 2d 7b 25 20 65 6e 64 62 6c 6f 63 6b 20 25 |-.-{%
endblock %|
00002da0 7d 0a |}.|
00002da2

The review request was submitted by using post-review on MacOS 10.5
based on a Subversion repository.

Any suggestions where to start debugging? From which sources is /tmp/
reviewboard.1PLuNw/produktpass.html.diff actually generated?

--md

[Maximillian Dornseif]

Seems a line containing

--->

disappears during upload. I'm looking further into that.

--md

[Maximillian Dornseif]

On Nov 9, 6:01 pm, Maximillian Dornseif <m...@hudora.de> wrote:
> Hello,
>
> I have a problem with the dif viewer (http://reviewboard/r/58/diff/)
> complaining about the fact that patch complains about a malformed
> patch at reviewboard/diffviewer/diffutils.py, line 125.

I think I found the problem: post-review is eating vcertian lines.
I created a fix at
http://c0re.23.nu/c0de/misc/reviewboard-r1569_post-review_transparent--upload.patch

I also tried to post a review request at http://reviews.review-board.org/r/624/
but failed to attatch tie diff.

--md

[Christian Hammond]

Make sure you specified the base path correctly. If you generated the diff in the top-level reviewboard directory, this should be "/trunk/reviewboard"

Christian

--
Christian Hammond - chi...@chipx86.com
VMware, Inc.

[Maximillian Dornseif]

On Nov 10, 8:50 am, "Christian Hammond" <chip...@chipx86.com> wrote:
> Make sure you specified the base path correctly. If you generated the diff
> in the top-level reviewboard directory, this should be "/trunk/reviewboard"

That worked! PAtch now is at http://reviews.review-board.org/r/624/diff/#index_header

--md

[Maximillian Dornseif]

Dear Christian,

first of: I'm thankful for you to publish and maintain rewiewboard.
I'm also aware that I might have chosen the wrong channel to submit my
bug report. Finally I also see that you want to spread the practice of
code reviews.

Still my experience with the bug reporting process in quite
unsatisfying.

I found mysterious behavior in post-review and while researching it I
found one way to fix that. Since I want to give back to the community
and didn't like to maintain a separate version of submit-review I
reposted the bug and also send with it a trivial patch to illustrate
the issue.

Probably I erred when also trying to add the patch to the reviewboard
instance at http://reviews.review-board.org/ .

The reviewboard submission did't work as expected. You had to explain
to me how to do it, I had to read that, understand it, redo it.

Now I'm expected to rework the thing to conform the reviewboard coding
standards and make the parser better.

To be frank: I think this is all far to much process. post-review had
strange behavior, I investigated the issue, found a bug, changed the
code to improve the situation and tried to provide a bug report with
an illustrative patch.

I think that can be considered good open source citizenship.
What I don't want is to dive into the reviewboard codebase. I don't
want to write reviewboard coding standards (or PEP 8 or PEP 257)
compliant code at this point, i just want to illustrate an bug.

I expected the maintainers to take the trivial patch and use this as a
pointer where and how to fix their code according to their stylistic
preferences. I was not expecting to be expected to sort imports and
the like.

For me this whole bug is a very frustrating experience. Probably it is
all my error by submitting via reviewboard instead via the bugtracker.

But I wanted to communicate my experience to you - perhaps you might
want the clarify bug submission on the website as a method fto foster
qualified bug reports and keep frustration in the community low.

I have discarded http://reviews.review-board.org//r/624 because it
served it's purpose: making authores aware of an bug in their code and
a possible fix.

Regards

Maximillian

[Christian Hammond]

Hi Maximillian.

This is, of course, the whole point of code review and the point of the project.

Your change is a welcome one and we want it in, but there are things about it that I'm not comfortable with yet. I proposed another potential solution based on other work we've done that should both simplify the code and make it more readable and reliable. This is how a lot of open source projects work, with the contributor going through several revisions of the code until it meets the coding styles and fixes the bugs that the maintainers notice.

The review submission issue you hit is due to the nature of SVN diffs. It's something we cannot work around without someone using post-review (which you could certainly use). You're right in that we should have a guide up somewhere that says exactly how to contribute in order to simplify this process.

I understand your point of view about wanting the just provide something that kinda works that we can use as a basis. We are, however, really busy. It's basically two of us doing the core development and maintenance of the project, and we do this in our spare time between our busy personal lives and our full-time jobs. So we don't necessarily have a lot of time to fix up everyone's patches. This is why we enforce a strict code review standard, so that contributors both learn the general coding style and design philosophies and we have a good record of all final patches that we can apply.

This is not unusual in open source projects. It's just more formalized in ours. As it should be, because we're trying to set an example in this project.

I hope this is not a common frustration. To date, we've had over 80 contributors to the project and I've only heard two who were upset with the process. It's a process that has so far served us very well so we'll be continuing with it.

Thanks for the patch. Maybe someone else will have time to take up the work and finish it so we can get it in to the codebase.

Christian

--
Christian Hammond - chi...@chipx86.com
VMware, Inc.