Can't parse the patch

[GoogleCodeExporter]

I get server errors trying to mail comments from this patch:

http://codereview.appspot.com/22052/patch/1004/1005

(as user bradfitz@gmail.com)

Original issue reported on code.google.com by bradfitz on 2 Mar 2009 at 1:10

[GoogleCodeExporter]

I'm not sure, but I think that the problem with that patch is that it doesn't 
have a
space at the start of blank lines.  How did you generate it?

Original comment by gvanrossum@gmail.com on 2 Mar 2009 at 4:29

[GoogleCodeExporter]

No clue how it was generated.  A user sent the patch to me.

But rather than it give *me* a 500 Server Error after I spent time reviewing it 
and went to mail comments, why 
don't you parse the patch at submission time instead, giving the submitting 
user an error message instead of 
me?  Then you could also teach them how to download & use the uploader tool, so 
they don't upload 
malformed patches in the first place.

But I think the bug is that you allowed the malformed patch into the system at 
all.

Original comment by bradfitz on 2 Mar 2009 at 6:21

[GoogleCodeExporter]

Yes, in hindsight that would be a good idea.  I'm not about to make big sweeping
changes myself at this point, still hoping that someone will contribute it 
though.

Original comment by gvanrossum@gmail.com on 2 Mar 2009 at 3:18

• Changed state: Accepted

[GoogleCodeExporter]

I'd propose the following changes to the upload form.                           

- More upload.py propaganda ;-)                                                 

- Turn issue creation into a two step process:                                  
  1.) Set subject, description, base and file/url                               
  2.) Add reviewers, cc, set send_mail and publish the issue                    

After submitting 1) we could show an AJAX-driven page that triggers             
the server to                                                                   

 - parse the patch                                                              
 - download base files                                                          
 - maybe tries to apply them                                                    

Errors that happen during these operation could be shown to the user            
immediately.                                                                    
If everything works well, the user is allowed to add reviewers, cc's            
(plus send_mail flag) and to "publish" the new issue.                           

A similar form could be used to upload new changelists for existing issues.     

This would introduce a new "is draft" flag to the issue and maybe the           
patchset model. Unpublished *and* orphaned issues could be removed after a reas\
onable time span by a scheduled tasks when they're available.                   

Related issues: issue46, issue85 (both have problems fetching the               
correct svn revision) 

Original comment by albrecht.andi on 3 Mar 2009 at 5:42

[GoogleCodeExporter]

That's a lot of work.  Are you signing up?  Personally, I think we should move 
to
upload.py as the *only* way to upload patches.  (And yes, patches should be 
validated
when received.)

As a much more minimal approach, we could easily add more upload.py propaganda 
by
moving that link to the top of the page, or even making the "Create Issue" link 
point
to an intermediate page that strongly recommends upload.py and then gives you a 
link
to the real create page.  (Possibly we could have a per-user bit indicating 
that they
already know about upload.py and shouldn't be bugged any more.)

Original comment by gvanrossum@gmail.com on 3 Mar 2009 at 7:51

[GoogleCodeExporter]

Yes, it's a lot of work and I'm hesitating if it's worth to do. I prefer to 
disable
the upload form in favor of upload.py too.

IMO the minimal approach you've suggested should be the first step. It's easy to
implement and maybe more people will start using upload.py.
The patch validation is needed for both the web form and upload.py to verify 
that
there's only valid data in the system. But I expect that more than one request 
is
required to do a full validation (is it parsable? can we fill content and 
patched
content for all files? calculate deltas...) I think we should first add those
validations for patches uploaded with upload.py as it's much easier to do it 
there.

If after that there's still a need for the web form we could re-consider to 
reuse the
validation for the form - or simply remove it.

A patch for step 1) is uploaded at codereview: 
http://codereview.appspot.com/24051

Original comment by albrecht.andi on 3 Mar 2009 at 11:23

[GoogleCodeExporter]

http://codereview.appspot.com/683041/show has been uploaded with upload.py and 
shows
the same problem.  The same thing with 
http://codereview.appspot.com/636041/show --
but the patch is really simple:
http://codereview.appspot.com/download/issue636041_1_2.diff -- I don't know yet 
which
part of this patch triggers the problem.

Original comment by maciej.b...@gmail.com on 22 Mar 2010 at 9:18

[GoogleCodeExporter]

If I'm not wrong then there's something wrong with your diff. The uploaded diff 
states 
that the removed line starting with "DESCRIPTION =" is in line 3, but in the 
corresponding base file this is line 5.

The chunk that removes the DESCRIPTION line caused the problems (from the logs: 
"Makefile:11: chunk header out of order: u'@@ -2,3 +4,2 @@\n'").

Original comment by albrecht.andi on 22 Mar 2010 at 10:03

[GoogleCodeExporter]

I'm positive I've checked that my subversion client is in sync with the 
repository 
before running upload.py. Perhaps my subversion installation or subversion 
client 
has a problem. I've verified that a patch generated by 'svn diff' applies with 
offsets:

svn revert Makefile
cp Makefile.mod Makefile
svn diff Makefile > 1.patch
svn revert Makefile
gpatch -p0 -i 1.patchReverted 'Makefile'
patching file Makefile
Hunk #2 succeeded at 6 (offset 2 lines).
Hunk #3 succeeded at 18 (offset -2 lines).
Hunk #4 succeeded at 75 (offset 2 lines).

There should be no offsets, right?  Will investigate more.

Original comment by maciej.b...@gmail.com on 22 Mar 2010 at 3:44

[GoogleCodeExporter]

Right, there shouldn't be any offsets. BTW, a similar problem was reported in 
issue28.

Original comment by albrecht.andi on 22 Mar 2010 at 3:50

[GoogleCodeExporter]

Issue 191 has been merged into this issue.

Original comment by albrecht.andi on 21 Apr 2010 at 4:22

[GoogleCodeExporter]

Original comment by albrecht.andi on 6 Apr 2012 at 6:31

• Added labels: Component-Engine