support git-style patches

This is much smaller than my full Git patch.

Currently, Rietveld looks for an "Index:" line in the diff, which is the output of "svn diff".  This patch allows you to upload a patch that is the output of "git diff".

[ondrej.certik, 2008-07-19 20:02:46]

I just had a suggestion about a docstring, otherwise a nice patch, +1.

http://codereview.appspot.com/2598/diff/1/2
File codereview/engine.py (right):

http://codereview.appspot.com/2598/diff/1/2#newcode56
Line 56: patch."""
Maybe just a cosmetic issue, but I personally prefer the first line of a
docstring to fit on one line.

Also, could you please put there examples? e.g.

>>> IsPatchSeparator("Index: myfile.py")
myfile.py
>>> IsPatchSeparator("--- a/myfile.py")
myfile.py
>>> IsPatchSeparator("a/myfile.py")
None

[Evan Martin, 2008-07-20 06:13:42]

On 2008/07/19 20:02:46, ondrej.certik wrote:
> I just had a suggestion about a docstring, otherwise a nice patch, +1.
> 
> http://codereview.appspot.com/2598/diff/1/2
> File codereview/engine.py (right):
> 
> http://codereview.appspot.com/2598/diff/1/2#newcode56
> Line 56: patch."""
> Maybe just a cosmetic issue, but I personally prefer the first line of a
> docstring to fit on one line.
> 
> Also, could you please put there examples? e.g.
> 
> >>> IsPatchSeparator("Index: myfile.py")
> myfile.py
> >>> IsPatchSeparator("--- a/myfile.py")
> myfile.py
> >>> IsPatchSeparator("a/myfile.py")
> None

I made it a bit more clear, and documented the return type.  Could you please
take another look?

[ondrej.certik, 2008-07-20 10:58:59]

Now I like it. +1 from me, thanks for the work.

Andi -- will you have time to push this in if you find the patches ok?

[GvR, 2008-07-28 18:13:50]

Is this the only server-side change necessary for Git?

[GvR, 2008-07-28 18:14:28]

[changing email address of reviewer]

[Evan Martin, 2008-07-28 18:19:10]

On 2008/07/28 18:13:50, GvR wrote:
> Is this the only server-side change necessary for Git?

Yes.  The other changes are all for upload.py.

[GvR, 2008-07-28 20:21:12]

On 2008/07/28 18:19:10, Evan Martin wrote:
> On 2008/07/28 18:13:50, GvR wrote:
> > Is this the only server-side change necessary for Git?
> 
> Yes.  The other changes are all for upload.py.

Hm. I just realized something. If I accept this, I'll have to accept similar
patches for every version control system out there. Wouldn't it be much easier
if the upload.py script massages the output from "git diff" slightly so that it
is acceptable to the server without this change? All you need is insert "Index:
<filename>" at the appropriate place. Is there any reason this would be a bad
idea?

[ondrej.certik, 2008-07-28 21:10:16]

On 2008/07/28 20:21:12, GvR wrote:
> On 2008/07/28 18:19:10, Evan Martin wrote:
> > On 2008/07/28 18:13:50, GvR wrote:
> > > Is this the only server-side change necessary for Git?
> > 
> > Yes.  The other changes are all for upload.py.
> 
> Hm. I just realized something. If I accept this, I'll have to accept similar
> patches for every version control system out there. Wouldn't it be much easier
> if the upload.py script massages the output from "git diff" slightly so that
it
> is acceptable to the server without this change? All you need is insert
"Index:
> <filename>" at the appropriate place. Is there any reason this would be a bad
> idea?

Don't have arguments if it's a bad idea, but for example for Mercurial, it can
produce git style patches as well, so it would solve our problems too.

[Evan Martin, 2008-07-28 21:12:14]

On 2008/07/28 20:21:12, GvR wrote:
> On 2008/07/28 18:19:10, Evan Martin wrote:
> > On 2008/07/28 18:13:50, GvR wrote:
> > > Is this the only server-side change necessary for Git?
> > 
> > Yes.  The other changes are all for upload.py.
> 
> Hm. I just realized something. If I accept this, I'll have to accept similar
> patches for every version control system out there. Wouldn't it be much easier
> if the upload.py script massages the output from "git diff" slightly so that
it
> is acceptable to the server without this change? All you need is insert
"Index:
> <filename>" at the appropriate place. Is there any reason this would be a bad
> idea?

On 2008/07/28 20:21:12, GvR wrote:
> Hm. I just realized something. If I accept this, I'll have to accept similar
> patches for every version control system out there. Wouldn't it be much easier
> if the upload.py script massages the output from "git diff" slightly so that
it
> is acceptable to the server without this change? All you need is insert
"Index:
> <filename>" at the appropriate place. Is there any reason this would be a bad
> idea?

That's a good point.  It does mean that upload.py would have to parse the patch
file, though.

How would you feel about a patch that makes this more agnostic to the system? 
All patches include the filename in the patch headers, I think.  E.g. look at 
http://codereview.appspot.com/2598/download/10
and note that patches include the "--- filename" line.

However, git's output is one level deeper (e.g. "--- a/filename") so there'd
still need to be some logic in there that behaves like the "-p" flag to patch.

(But now that I've typed that out, I wonder what "svn diff" does when run from a
subdirectory of the repo root.)

[GvR, 2008-07-28 21:27:07]

On 2008/07/28 21:12:14, Evan Martin wrote:
> That's a good point.  It does mean that upload.py would have to parse the
patch
> file, though.

It does that anyways looking for Index: lines. And it might even be useful to
parse it a bit more so as to catch surprises like issue 30.

> How would you feel about a patch that makes this more agnostic to the system?
> All patches include the filename in the patch headers, I think.  E.g. look at 
> http://codereview.appspot.com/2598/download/10
> and note that patches include the "--- filename" line.
> 
> However, git's output is one level deeper (e.g. "--- a/filename") so there'd
> still need to be some logic in there that behaves like the "-p" flag to patch.
> 
> (But now that I've typed that out, I wonder what "svn diff" does when run from
a
> subdirectory of the repo root.)

It produces patches relative to that subdirectory. Fortunately the output of
"svn info" changes to match, so (as long as you don't depend on the "Repository"
feature) all is well.

All in all I still think that inserting Index: lines in upload.py as needed is
the best thing to do.

[Evan Martin, 2008-07-30 05:11:12]

> All in all I still think that inserting Index: lines in upload.py as needed is
> the best thing to do.

I agree.  I'm deleting this issue.