git support

[Evan Martin]

I've uploaded a series of patches to make git work with Rietveld, and
in particular, upload.py.

This patch abstracts out the svn support from upload.py:
http://codereview.appspot.com/2589
This (not quite finished) patch builds on the above, using git from
upload.py and making the server's patch parser accept such patches:
http://codereview.appspot.com/2602
Once the server accepts git patches, I'll be able to manage these
sorts of interdependent reviews natively from git. :)

I'm mostly posting here in case anyone else is interested. Is it
worth registering an issue in the issue tracker?

[Guido van Rossum]

Thanks for doing this! I'm very much in favor of supporting this.
There are some other upload.py changes in flight though that should
probably be checked in first. I don't see the need to open a tracker
issue -- the code review issues are enough.

--
--Guido van Rossum (home page: http://www.python.org/~guido/)

[Ondrej Certik]

If you mean the mercurial changes, I will work on this after August
8th, unless someone else will be quicker, so
feel free to merge this git change first.

Ondrej

[Evan Martin]

On Thu, Jul 17, 2008 at 11:26 PM, Evan Martin <mar...@danga.com> wrote:

> I've uploaded a series of patches to make git work with Rietveld, and
> in particular, upload.py.
>
> This patch abstracts out the svn support from upload.py:
> http://codereview.appspot.com/2589
> This (not quite finished) patch builds on the above, using git from
> upload.py and making the server's patch parser accept such patches:
> http://codereview.appspot.com/2602
> Once the server accepts git patches, I'll be able to manage these
> sorts of interdependent reviews natively from git. :)

I've now split this into three reviews which are all ready to go.

This one still abstracts out the svn support:
http://codereview.appspot.com/2589
This one makes the server accept git-style patches:
http://codereview.appspot.com/2598

And this last one does the real git work. Since it depends on 2589,
it can't display side-by-side. Here it is as a patch:
http://codereview.appspot.com/2602/patch/6/24

[Ondrej Certik]

Thanks for the work. I hope either Guido or Andi will have time to
push the patches in, as this is really needed and I think they make a
good ground for implementing support to other VCS.

Ondrej

[Evan Martin]

After some discussion of the patch to make the server accept diffs
from Git, Guido suggests changing upload.py to fake a Subversion-style
diff instead. To do this, I would need to depend on the refactoring
done in the first review listed above. However, Guido said he didn't
have time to review that, and intermediate changes have since rotted.

If I redid the refactoring, would anyone be able to review it?
Otherwise, I will abandon this thread.

[Guido van Rossum]

I'm back and have more time for reviews.

--

[Ondrej Certik]

I reviewed the patch you mentioned, or do you accept only reviews from Guido? :)

Ondrej

[Evan Martin]

Ok, I've uploaded a new patch here:
http://codereview.appspot.com/2589

Again, I made an effort to not make any functional change. It's too
bad the diff can't show that GuessBase was just indented but not
changed... maybe I can look into that in the future. :)

[Evan Martin]

On Tue, Jul 29, 2008 at 6:13 AM, Ondrej Certik <ond...@certik.cz> wrote:
> I reviewed the patch you mentioned, or do you accept only reviews from Guido? :)

I appreciate your comments, but ultimately the patch needs to be
(reviewed and) committed by someone with commit access.

[Guido van Rossum]

I will review it tomorrow, if there are any hours without meetings in my day...

[Evan Martin]

On Tue, Jul 29, 2008 at 9:46 AM, Evan Martin <mar...@danga.com> wrote:
> Ok, I've uploaded a new patch here:
> http://codereview.appspot.com/2589

I've now also updated the followup patch to add Git support here:
http://codereview.appspot.com/2602

As Guido suggested, it was actually pretty easy to massage the patch
into a Subversion-lookalike form that the Rietveld server accepts. So
the upload.py from there can be used against the existing appspot
install for anyone who'd like to develop using Git. (In fact, that
patch was itself uploaded from a Git repo.)

[Evan Martin]

Curiously, I can view the side-by-side diff once before it starts
failing with "Bad content. Try to upload again." Clearly, this
followup patch still needs more work. Maybe later.

[Evan Martin]

Yikes. I was accidentally stripping the last line of the file when I
was doing the Subversion-patch-simulation fixup. I've uploaded a
better patch now as patchset #5. Note that diffing against previous
patch sets will produce an error because of my bug, but also that
there's not much interesting to look at in the previous patch sets
anyway.

[Andi Albrecht]

> Curiously, I can view the side-by-side diff once before it starts
> failing with "Bad content. Try to upload again." Clearly, this
> followup patch still needs more work. Maybe later.

Evan, was that caused by the missing last line? I've seen this
behavior before while I developed something but couldn't reproduce it.
Anyway, you've seen the side-by-side diff once so the "Bad content"
message shouldn't appear.

>
> >
>

[Evan Martin]

Yes. I didn't understand the full execution flow, but it ended up
that this part in patching.PatchChunks() was failing:
# Check that the patch matches the target file
if old_lines[old_i:old_j] != old_chunk:
yield ("error: old chunk mismatch", old_lines[old_i:old_j], old_chunk)
return
Both sides of the if are arrays of lines, but when I had dropped the
last line, the last entry in old_chunk was missing the trailing
newline.

This error was getting up to views._get_diff_table_rows and this code path:
rows = list(engine.RenderDiffTableRows(request, content.lines,
chunks, patch,
context=context))
if rows and rows[-1] is None:
del rows[-1]
# Get rid of content, which may be bad
if content.is_uploaded:
# Don't delete uploaded content, otherwise get_content()
# will fetch it.
content.is_bad = True
content.text = None
content.put()

[Andi Albrecht]

> Yes. I didn't understand the full execution flow, but it ended up
> that this part in patching.PatchChunks() was failing:
> # Check that the patch matches the target file
> if old_lines[old_i:old_j] != old_chunk:
> yield ("error: old chunk mismatch", old_lines[old_i:old_j], old_chunk)
> return
> Both sides of the if are arrays of lines, but when I had dropped the
> last line, the last entry in old_chunk was missing the trailing
> newline.

Ah, I understand... The "old chunk mismatch" is correct if the actual
lines doesn't match the line counts provided by the diff. The diff
mentioned in Issue 28
(http://code.google.com/p/rietveld/issues/detail?id=28) has the same
problem (but maybe another one with svn:keywords). But I'm still
curios why you viewed it once...

[Evan Martin]

On Wed, Jul 30, 2008 at 10:28 AM, Andi Albrecht
<albrec...@googlemail.com> wrote:
>
>> Yes. I didn't understand the full execution flow, but it ended up
>> that this part in patching.PatchChunks() was failing:
>> # Check that the patch matches the target file
>> if old_lines[old_i:old_j] != old_chunk:
>> yield ("error: old chunk mismatch", old_lines[old_i:old_j], old_chunk)
>> return
>> Both sides of the if are arrays of lines, but when I had dropped the
>> last line, the last entry in old_chunk was missing the trailing
>> newline.
>
> Ah, I understand... The "old chunk mismatch" is correct if the actual
> lines doesn't match the line counts provided by the diff.

Just to be completely clear: I was dropping the trailing newline on
the last line of the diff, which meant the test looked like
if ["foo\n", "bar\n"] != ["foo\n", "bar"]:
That is, the line counts matched -- it was just the trailing newline
that was throwing the comparison off.

> The diff
> mentioned in Issue 28
> (http://code.google.com/p/rietveld/issues/detail?id=28) has the same
> problem (but maybe another one with svn:keywords). But I'm still
> curios why you viewed it once...

Now that I've slept on it, I think this is what happens.
When I click "view" the first time, the differ generates the diff view
including an "error" tag produced from this block of code. It then
displays the diff with the "CHUNK ERROR" marker at the end of the
page. But as it does that (see the above code in
views._get_diff_table_rows()) it sets the "is_bad" flag in the
datastore.

It's a little confusing that a views "get" function modifies the
datastore. It causes us to get confused about these sorts of things.
:)
Perhaps I'll look into rearranging that at some point.

[dacresni]

where's the work on the git compatible version of upload.py? where can
i look/work on it

On Jul 30, 1:07 pm, "Evan Martin" <mart...@danga.com> wrote:
> On Wed, Jul 30, 2008 at 10:28 AM, Andi Albrecht
>

[Raghuram Devarakonda]

On Mon, Aug 11, 2008 at 2:08 PM, dacresni <vivac...@gmail.com> wrote:
>
> where's the work on the git compatible version of upload.py? where can
> i look/work on it

I think you can look at http://codereview.appspot.com/2602.

[dacresni]

thanks

On Aug 11, 1:10 pm, "Raghuram Devarakonda" <draghu...@gmail.com>
wrote:

[Andi Albrecht]

Git is now supported by upload.py: http://codereview.appspot.com/static/upload.py

Thanks, Evan!

[Ondrej Certik]

On Tue, Aug 19, 2008 at 10:39 PM, Andi Albrecht
<albrec...@googlemail.com> wrote:
>
> Git is now supported by upload.py: http://codereview.appspot.com/static/upload.py

I uploaded a test patch with this new upload.py using git and got:

http://codereview.appspot.com/2993/patch/1/2

Ondrej

[Andi Albrecht]

Ondrej, can you try again?

[Ondrej Certik]

On Wed, Aug 20, 2008 at 10:38 AM, Andi Albrecht
<albrec...@googlemail.com> wrote:
>
> Ondrej, can you try again?

Ok, all is working now!

Ondrej