Add perforce support to upload.py.

Committed as http://code.google.com/p/rietveld/source/detail?r=640

[Alex McC]

On 2010/10/23 23:00:21, Alex McC wrote:

I'm not too familiar with Python (I'm no Guido), so feel free to rip into this
change until it meets your standards.

Monday is my first day as a Noogler. Here's hoping we can work together again
soon!

[Alex McC]

Hi Guido,

I know of a few game development companies that are currently evaluating code
review tools, and their reliance on perforce keeps rietveld off of their lists.

Let's put rietveld on their radar :)
-Alex

http://codereview.appspot.com/2635043/diff/1/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode541
upload.py:541: # Perforce-specific
Is there a better way to pass in this perforce-specific information, without
resorting to perforce-only options?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1458
upload.py:1458: def GenerateDiff(self, args):
The diffs generated by this function don't match svn's 100%, but they seem to be
good enough for rietveld to process and use them.

Are completely svn-compatible diffs required?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1488
upload.py:1488: # Replicate svn's list of changed lines
A bit of duplication here and on line 1500. Any recommendations?

[Alex McC]

Reviewers: GvR,



Please review this at http://codereview.appspot.com/2635043/

Affected files:
 M upload.py

[Alex McC]

I'm not too familiar with Python (I'm no Guido), so feel free to rip
into this change until it meets your standards.

Monday is my first day as a Noogler. Here's hoping we can work together
again soon!


http://codereview.appspot.com/2635043/

[Alex McC]

Hi Guido,

I know of a few game development companies that are currently evaluating
code review tools, and their reliance on perforce keeps rietveld off of
their lists.

Let's put rietveld on their radar :)
-Alex


http://codereview.appspot.com/2635043/diff/1/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode541
upload.py:541: # Perforce-specific
Is there a better way to pass in this perforce-specific information,
without resorting to perforce-only options?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1458
upload.py:1458: def GenerateDiff(self, args):
The diffs generated by this function don't match svn's 100%, but they
seem to be good enough for rietveld to process and use them.

Are completely svn-compatible diffs required?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1488
upload.py:1488: # Replicate svn's list of changed lines
A bit of duplication here and on line 1500. Any recommendations?


http://codereview.appspot.com/2635043/

[Andi Albrecht]

[adding Evan]

Looks good in general, just some style nits. But I don't know P4 at all, so I
really can't tell if there are any pitfalls.

http://codereview.appspot.com/2635043/diff/1/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode537
upload.py:537: "already guesses the right VCS)."))
We should add an "except for P4" clause here.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode541
upload.py:541: # Perforce-specific
On 2010/10/23 23:12:26, Alex McC wrote:
> Is there a better way to pass in this perforce-specific information, without
> resorting to perforce-only options?

IMO adding a group for P4 options is fine. But isn't there a way to get these
options from a Perforce command line tool (I should mention that I never worked
with Perforce...)?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode545
upload.py:545: help=("Perforce server and port (perforce only, optional)"))
I don't think that we need "perforce only" for every option. It's already in a
"Peforce-specific options" group :)

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1366
upload.py:1366: failure_ok=True)
Keep lines < 80 chars.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1385
upload.py:1385: description = self.RunPerforceCommand(["describe",
self.p4_changelist], marshal_output=True)
Keep lines < 80 chars

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1388
upload.py:1388: options.message = " ".join(description["desc"].splitlines())
I don't know how a changelist description looks like, but do you really want to
collapse multiple lines into a single line? Wouldn't it be enough to take the
first line then?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1405
upload.py:1405: universal_newlines=universal_newlines)
The indentation looks a bit strange here..

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1413
upload.py:1413: description = self.RunPerforceCommand(["describe",
self.p4_changelist], marshal_output=True)
80 chars max

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1446
upload.py:1446: universal_newlines=not is_binary)
Here looks the indentation strange again..

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1449
upload.py:1449: have_result = self.RunPerforceCommand(["have", filename],
marshal_output=True)
80 chars max

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1458
upload.py:1458: def GenerateDiff(self, args):
On 2010/10/23 23:12:26, Alex McC wrote:
> The diffs generated by this function don't match svn's 100%, but they seem to
be
> good enough for rietveld to process and use them.
> 
> Are completely svn-compatible diffs required?
No, the first one counts: Rietveld must be able to process them :)

[Alex McC]

http://codereview.appspot.com/2635043/diff/1/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode537
upload.py:537: "already guesses the right VCS)."))
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> We should add an "except for P4" clause here.

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode541
upload.py:541: # Perforce-specific
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> On 2010/10/23 23:12:26, Alex McC wrote:
> > Is there a better way to pass in this perforce-specific information, without
> > resorting to perforce-only options?
> 
> IMO adding a group for P4 options is fine. But isn't there a way to get these
> options from a Perforce command line tool (I should mention that I never
worked
> with Perforce...)?

If the user has set some perforce environment variables (which is common for
command line usage), p4 will automagically use the right properties for
everything but the changelist.

GUI users will have to configure a custom tool that passes in the correct
parameters. I should document how to do this in the wiki.

I tried doing some clever probing to extract more info from the command line,
but I couldn't get reliable results without introducing a few seconds of delay
while Perforce waits for a non-existent server to timeout :(

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode545
upload.py:545: help=("Perforce server and port (perforce only, optional)"))
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> I don't think that we need "perforce only" for every option. It's already in a
> "Peforce-specific options" group :)

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode549
upload.py:549: group.add_option("--p4_workspace", action="store",
dest="p4_workspace",
Command-line perforce help refers to workspaces as clients, so I'll use that
term here. Workspace is the term used in the GUI.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1366
upload.py:1366: failure_ok=True)
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> Keep lines < 80 chars.

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1385
upload.py:1385: description = self.RunPerforceCommand(["describe",
self.p4_changelist], marshal_output=True)
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> Keep lines < 80 chars

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1388
upload.py:1388: options.message = " ".join(description["desc"].splitlines())
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> I don't know how a changelist description looks like, but do you really want
to
> collapse multiple lines into a single line? Wouldn't it be enough to take the
> first line then?

Good point, I'll grab the first non-blank line.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1405
upload.py:1405: universal_newlines=universal_newlines)
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> The indentation looks a bit strange here..

Done, is this more appropriate?

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1413
upload.py:1413: description = self.RunPerforceCommand(["describe",
self.p4_changelist], marshal_output=True)
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> 80 chars max

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1446
upload.py:1446: universal_newlines=not is_binary)
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> Here looks the indentation strange again..

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1449
upload.py:1449: have_result = self.RunPerforceCommand(["have", filename],
marshal_output=True)
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> 80 chars max

Done.

http://codereview.appspot.com/2635043/diff/1/upload.py#newcode1458
upload.py:1458: def GenerateDiff(self, args):
On 2010/10/27 08:21:31, Andi Albrecht wrote:
> On 2010/10/23 23:12:26, Alex McC wrote:
> > The diffs generated by this function don't match svn's 100%, but they seem
to
> be
> > good enough for rietveld to process and use them.
> > 
> > Are completely svn-compatible diffs required?
> No, the first one counts: Rietveld must be able to process them :)

Done.

http://codereview.appspot.com/2635043/diff/10001/upload.py#newcode1366
upload.py:1366: failure_ok=True)
Cleaner than failure_ok

[Andi Albrecht]

Looks good to me, but it would be good to get some feedback from a Perforce
user... :-)

[techtonik]

On 2010/11/03 12:44:03, Andi Albrecht wrote:
> Looks good to me, but it would be good to get some feedback from a Perforce
> user... :-)

Is it possible to hide perforce specific options from help unless specifically
requested? They are not useful for 95% of upload.py users, and they probably
won't be happy to browse multi-page options manual. Also there is a demand for
options specific to other VCS, so it's better to handle this now if possible.

[Alex McC]

On 2010/11/03 13:15:34, techtonik wrote:
> On 2010/11/03 12:44:03, Andi Albrecht wrote:
> > Looks good to me, but it would be good to get some feedback from a Perforce
> > user... :-)

I'll start digging up random programmers!

> Is it possible to hide perforce specific options from help unless specifically
> requested? They are not useful for 95% of upload.py users, and they probably
> won't be happy to browse multi-page options manual. Also there is a demand for
> options specific to other VCS, so it's better to handle this now if possible.

One option: I could disable help printing in the OptionsParser entirely:
http://stackoverflow.com/questions/1707380/how-to-disable-the-optionparser-de...

I could then add a new custom help printer that has more of a tree structure:
"help" lists something like "basic", "vcs", and "monkies", and then you type
"help basic" or "help monkies" for more detailed help.

Alternatively, I could use optparse.SUPPRESS_HELP
(http://docs.python.org/release/2.5.2/lib/optparse-standard-option-actions.html)
to remove help text from the p4 commands, then add a dummy --help_p4 command to
print out p4 help text.

I don't love either of these options: they smell bad, and they seem like a lot
of effort to save people from just piping help's output through more. You guys
are more in touch with Rietveld's community: what do you think?

[Andi Albrecht]

alex.mccarthy@gmail.com writes:

> On 2010/11/03 13:15:34, techtonik wrote:
>> On 2010/11/03 12:44:03, Andi Albrecht wrote:
>> > Looks good to me, but it would be good to get some feedback from a
> Perforce
>> > user... :-)
>
> I'll start digging up random programmers!

Nice, thanks!

>
>> Is it possible to hide perforce specific options from help unless
> specifically
>> requested? They are not useful for 95% of upload.py users, and they
> probably
>> won't be happy to browse multi-page options manual. Also there is a
> demand for
>> options specific to other VCS, so it's better to handle this now if
> possible.
>
> One option: I could disable help printing in the OptionsParser entirely:
>
http://stackoverflow.com/questions/1707380/how-to-disable-the-optionparser-de...
>
> I could then add a new custom help printer that has more of a tree
> structure: "help" lists something like "basic", "vcs", and "monkies",
> and then you type "help basic" or "help monkies" for more detailed help.
>
> Alternatively, I could use optparse.SUPPRESS_HELP
>
(http://docs.python.org/release/2.5.2/lib/optparse-standard-option-actions.html)
> to remove help text from the p4 commands, then add a dummy --help_p4
> command to print out p4 help text.
>
> I don't love either of these options: they smell bad, and they seem like
> a lot of effort to save people from just piping help's output through
> more. You guys are more in touch with Rietveld's community: what do you
> think?

Agreed. I don't think that people use the --help option too often. If
someone's unfamiliar with upload.py then there are already enough
options to digg through so that a few minutes to scan through a somewhat
longer help output should be ok. IMO most users have a look at the help
once or twice and are fine with a daily subset of options. And if we
don't split it up, we have all options in one place.

BTW, if someone feels uncomfortable with the help output, we already
have a Wiki page listing all options (and discussions about them):
http://code.google.com/p/rietveld/wiki/UploadPyUsage

Andi

>
>
> http://codereview.appspot.com/2635043/

[gvrpython]

So, I happen to be a P4 user. :-)

I patched your change into my working copy of Rietveld, ran it
locally, and sent it a simple p4 patch.

- The upload.py part works, but I find it cumbersome to have to
specify both --vcs=p4 and --p4_changelist=12345. Maybe if any of the
--p4* flags is given it should guess p4?

- I ran into a bug when viewing side-by-side diffs that doesn't seem
to be caused directly by your changes but somehow it gets triggered by
p4 patches only:

Traceback:
File "/usr/local/lib/python2.6/dist-packages/django/core/handlers/base.py"
in get_response
  99.                     response = callback(request, *callback_args,
**callback_kwargs)
File "/home/guido/rietveld/codereview/views.py" in issue_wrapper
  633.     return func(request, *args, **kwds)
File "/home/guido/rietveld/codereview/views.py" in patchset_wrapper
  696.     return func(request, *args, **kwds)
File "/home/guido/rietveld/codereview/views.py" in patch_wrapper
  743.     return func(request, *args, **kwds)
File "/home/guido/rietveld/codereview/views.py" in diff
  1994.       rows = _get_diff_table_rows(request, patch, context, column_width)
File "/home/guido/rietveld/codereview/views.py" in _get_diff_table_rows
  2017.   chunks = patching.ParsePatchToChunks(patch.lines, patch.filename)
File "/home/guido/rietveld/codereview/patching.py" in ParsePatchToChunks
  144.         old_i, old_j = old_range

Exception Type: TypeError at
/106/diff/107///depot/google3/apphosting/demos/minishell/app.yaml
Exception Value: 'NoneType' object is not iterable

And when viewing a unified diff I get a *different* tb:

Traceback:
File "/usr/local/lib/python2.6/dist-packages/django/core/handlers/base.py"
in get_response
  99.                     response = callback(request, *callback_args,
**callback_kwargs)
File "/home/guido/rietveld/codereview/views.py" in issue_wrapper
  633.     return func(request, *args, **kwds)
File "/home/guido/rietveld/codereview/views.py" in patchset_wrapper
  696.     return func(request, *args, **kwds)
File "/home/guido/rietveld/codereview/views.py" in patch_wrapper
  723.     return func(request, *args, **kwds)
File "/home/guido/rietveld/codereview/views.py" in patch
  1877.   return patch_helper(request)
File "/home/guido/rietveld/codereview/views.py" in patch_helper
  1896.   parsed_lines = patching.ParsePatchToLines(request.patch.lines)
File "/home/guido/rietveld/codereview/patching.py" in ParsePatchToLines
  246.         result.append((old_ln, 0, line))

Exception Type: UnboundLocalError at /100/patch/101/102
Exception Value: local variable 'old_ln' referenced before assignment

It makes me worry that you are uploading slightly malformed patches
that our patch parser doesn't recognize.

--Guido

On Thu, Nov 4, 2010 at 11:07 PM, Andi Albrecht
<albrecht.andi@googlemail.com> wrote:
> alex.mccarthy@gmail.com writes:
>
>> On 2010/11/03 13:15:34, techtonik wrote:
>>> On 2010/11/03 12:44:03, Andi Albrecht wrote:
>>> > Looks good to me, but it would be good to get some feedback from a
>> Perforce
>>> > user... :-)
>>
>> I'll start digging up random programmers!
>
> Nice, thanks!
>
>>
>>> Is it possible to hide perforce specific options from help unless
>> specifically
>>> requested? They are not useful for 95% of upload.py users, and they
>> probably
>>> won't be happy to browse multi-page options manual. Also there is a
>> demand for
>>> options specific to other VCS, so it's better to handle this now if
>> possible.
>>
>> One option: I could disable help printing in the OptionsParser entirely:
>>
http://stackoverflow.com/questions/1707380/how-to-disable-the-optionparser-de...
>>
>> I could then add a new custom help printer that has more of a tree
>> structure: "help" lists something like "basic", "vcs", and "monkies",
>> and then you type "help basic" or "help monkies" for more detailed help.
>>
>> Alternatively, I could use optparse.SUPPRESS_HELP
>>
(http://docs.python.org/release/2.5.2/lib/optparse-standard-option-actions.html)
>> to remove help text from the p4 commands, then add a dummy --help_p4
>> command to print out p4 help text.
>>
>> I don't love either of these options: they smell bad, and they seem like
>> a lot of effort to save people from just piping help's output through
>> more. You guys are more in touch with Rietveld's community: what do you
>> think?
>
> Agreed. I don't think that people use the --help option too often. If
> someone's unfamiliar with upload.py then there are already enough
> options to digg through so that a few minutes to scan through a somewhat
> longer help output should be ok. IMO most users have a look at the help
> once or twice and are fine with a daily subset of options. And if we
> don't split it up, we have all options in one place.
>
> BTW, if someone feels uncomfortable with the help output, we already
> have a Wiki page listing all options (and discussions about them):
> http://code.google.com/p/rietveld/wiki/UploadPyUsage
>
> Andi
>
>>
>>
>> http://codereview.appspot.com/2635043/
>
> --
> You received this message because you are subscribed to the Google Groups
"codereview-discuss" group.
> To post to this group, send email to codereview-discuss@googlegroups.com.
> To unsubscribe from this group, send email to
codereview-discuss+unsubscribe@googlegroups.com.
> For more options, visit this group at
http://groups.google.com/group/codereview-discuss?hl=en.
>
>



-- 
--Guido van Rossum (python.org/~guido)

[Alex McC]

On 2010/11/05 18:42:41, guido_python.org wrote:
> So, I happen to be a P4 user. :-)
> 
> I patched your change into my working copy of Rietveld, ran it
> locally, and sent it a simple p4 patch.
> 
> - The upload.py part works, but I find it cumbersome to have to
> specify both --vcs=p4 and --p4_changelist=12345. Maybe if any of the
> --p4* flags is given it should guess p4?

Done. Great idea, that's much nicer.

> - I ran into a bug when viewing side-by-side diffs that doesn't seem
> to be caused directly by your changes but somehow it gets triggered by
> p4 patches only:
>
> It makes me worry that you are uploading slightly malformed patches
> that our patch parser doesn't recognize.

Both of those tbs could be called by bad diff headers, but I haven't encountered
any problems with the trivial p4 changes I've tested. I know that the p4 diff
code I added doesn't handle empty lines in the same way that svn does, but that
might be safe. Would you mind mailing me the patch set you tested with?

I've added a print_diffs option to make comparing/debugging diffs simpler.

[Alex McC]

http://codereview.appspot.com/2635043/diff/23001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/23001/upload.py#newcode1424
upload.py:1424: ErrorExit("Got error status from %s:\n%s" % (extra_args, data))
Missed this one while refactoring a few days ago

http://codereview.appspot.com/2635043/diff/23001/upload.py#newcode1454
upload.py:1454: if not filename in file_types:
executable text (i.e. script files) were being flagged as binary, and rietveld
would try to display the diff as an image.

http://codereview.appspot.com/2635043/diff/23001/upload.py#newcode1496
upload.py:1496: # We have to replace p4's file status output (the lines starting
with
p4 diff headers had extra prelude lines on some setups. This caused mangled
headers.

http://codereview.appspot.com/2635043/diff/23001/upload.py#newcode1525
upload.py:1525: if is_binary:
oops :(

[Ilia Mirkin]

http://codereview.appspot.com/2635043/diff/26001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode543
upload.py:543: # Perforce-specific
Why are these needed? IIRC p4 will take these as env vars, or out of some
~/.p4config style file.

Oh, I see from previous comments. For GUI users. Well *THAT's* annoying. Didn't
know anyone actually used that thing.

Perhaps change the add_option_group thing to mention the gui thing? People tend
to think options are required even when they're not.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1378
upload.py:1378: self.p4_changelist = options.p4_changelist
Isn't that what --rev is for already? Then the --p4_* stuff never needs to be
specified at all.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1505
upload.py:1505: fstat = self.RunPerforceCommand(["fstat", filename],
It'd be nice if you could also get the source in the move/add/branch cases. IIRC
it involves some kind of p4 integrated/resolved dance (different for committed
and pending CLs). But it's been a while since I've used p4.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1549
upload.py:1549: ErrorExit("No valid patches found in output from p4 diff")
What if it's a mode change? Not sure those are important to handle though.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1666
upload.py:1666: Since local perforce repositories can't be easily detected, this
method
Can't you run something like "p4 info" to see if you're inside a client?

[Ilia Mirkin]

http://codereview.appspot.com/2635043/diff/26001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode543
upload.py:543: # Perforce-specific
On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> Why are these needed? IIRC p4 will take these as env vars, or out of some
> ~/.p4config style file.
> 
> Oh, I see from previous comments. For GUI users. Well *THAT's* annoying.
Didn't
> know anyone actually used that thing.
> 
> Perhaps change the add_option_group thing to mention the gui thing? People
tend
> to think options are required even when they're not.

One more thought on this. People using the GUI are going to have to specify this
one way or the other. Why not make "the way" be to specify the env vars rather
than command-line flags?

[Alex McC]

Thanks for your comments, Ilia! You pointed out some great corner cases. I'll
get some more code uploaded soon.

http://codereview.appspot.com/2635043/diff/26001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode543
upload.py:543: # Perforce-specific
You can switch workspaces/servers/users/etc within the p4 GUIs, so environment
variables won't do :( But the p4 GUIs can pass the correct values to these flags
easily. Env vars are also not ideal for scripts on servers.

Could I make things more clear by making each option's comment say something
like "(optional, overrides P4CLIENT environment variable)"?

On 2010/12/05 21:11:30, Ilia Mirkin wrote:
> On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > Why are these needed? IIRC p4 will take these as env vars, or out of some
> > ~/.p4config style file.
> > 
> > Oh, I see from previous comments. For GUI users. Well *THAT's* annoying.
> Didn't
> > know anyone actually used that thing.
> > 
> > Perhaps change the add_option_group thing to mention the gui thing? People
> tend
> > to think options are required even when they're not.
> 
> One more thought on this. People using the GUI are going to have to specify
this
> one way or the other. Why not make "the way" be to specify the env vars rather
> than command-line flags?

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1378
upload.py:1378: self.p4_changelist = options.p4_changelist
I read http://code.google.com/p/rietveld/wiki/UploadPyUsage as meaning that
--rev is for the base revision, i.e. the state you're synched to or branched off
of. The p4 change is the pending change you'd like to have reviewed.

On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> Isn't that what --rev is for already? Then the --p4_* stuff never needs to be
> specified at all.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1505
upload.py:1505: fstat = self.RunPerforceCommand(["fstat", filename],
I was wondering if my laziness would get called out ;) Good suggestion, I'll try
to hammer out some more code this week!

On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> It'd be nice if you could also get the source in the move/add/branch cases.
IIRC
> it involves some kind of p4 integrated/resolved dance (different for committed

> and pending CLs). But it's been a while since I've used p4.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1549
upload.py:1549: ErrorExit("No valid patches found in output from p4 diff")
Mode changes are just "normal" edits. I expect that each "side" of the diff
should be read as its appropriate file type. I should make sure that I'm
correctly handling empty edits, though!

On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> What if it's a mode change? Not sure those are important to handle though.

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1666
upload.py:1666: Since local perforce repositories can't be easily detected, this
method
p4 info tries to contact the perforce server (which defaults to "perforce:1666"
if P4SERVER isn't set). On my home network (without a p4 server), it takes 21
seconds to timeout :( So it's not a great option.

On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> Can't you run something like "p4 info" to see if you're inside a client?

[Ilia Mirkin]

http://codereview.appspot.com/2635043/diff/26001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode543
upload.py:543: # Perforce-specific
On 2010/12/06 01:21:27, Alex McC wrote:
> You can switch workspaces/servers/users/etc within the p4 GUIs, so environment
> variables won't do :( But the p4 GUIs can pass the correct values to these
flags
> easily. Env vars are also not ideal for scripts on servers.

But what's the difference between running

./upload.py --p4_client=asdf

and

P4CLIENT=asdf ./upload.py

?

> 
> Could I make things more clear by making each option's comment say something
> like "(optional, overrides P4CLIENT environment variable)"?

I meant more just adding something to the group, like "overrides to env vars" or
something along those lines.

> 
> On 2010/12/05 21:11:30, Ilia Mirkin wrote:
> > On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > > Why are these needed? IIRC p4 will take these as env vars, or out of some
> > > ~/.p4config style file.
> > > 
> > > Oh, I see from previous comments. For GUI users. Well *THAT's* annoying.
> > Didn't
> > > know anyone actually used that thing.
> > > 
> > > Perhaps change the add_option_group thing to mention the gui thing? People
> > tend
> > > to think options are required even when they're not.
> > 
> > One more thought on this. People using the GUI are going to have to specify
> this
> > one way or the other. Why not make "the way" be to specify the env vars
rather
> > than command-line flags?
>

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1378
upload.py:1378: self.p4_changelist = options.p4_changelist
Oh, yes. You're right. Ugh. But it's so the logical place to put this sort of
thing. With subversion, there's no concept of a pending CL.

I'd feel comfortable deviating here for p4. What do others think?

On 2010/12/06 01:21:27, Alex McC wrote:
> I read http://code.google.com/p/rietveld/wiki/UploadPyUsage as meaning that
> --rev is for the base revision, i.e. the state you're synched to or branched
off
> of. The p4 change is the pending change you'd like to have reviewed.
> 
> On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > Isn't that what --rev is for already? Then the --p4_* stuff never needs to
be
> > specified at all.
>

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1549
upload.py:1549: ErrorExit("No valid patches found in output from p4 diff")
Oh, you're right. There are still ways of getting empty CLs, if you work hard
enough, but no reason to try to bend over backwards to support them.

On 2010/12/06 01:21:27, Alex McC wrote:
> Mode changes are just "normal" edits. I expect that each "side" of the diff
> should be read as its appropriate file type. I should make sure that I'm
> correctly handling empty edits, though!
> 
> On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > What if it's a mode change? Not sure those are important to handle though.
>

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1666
upload.py:1666: Since local perforce repositories can't be easily detected, this
method
I was thinking that you'd only have p4 installed if you're using it. That 20s
timeout is annoying though. I don't suppose there's a convenient --timeout=1
kind of argument to p4?

On 2010/12/06 01:21:27, Alex McC wrote:
> p4 info tries to contact the perforce server (which defaults to
"perforce:1666"
> if P4SERVER isn't set). On my home network (without a p4 server), it takes 21
> seconds to timeout :( So it's not a great option.
> 
> On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > Can't you run something like "p4 info" to see if you're inside a client?
>

[Alex McC]

I'll upload my changes as soon as I'm done with the branching base file support
(hopefully next week)

http://codereview.appspot.com/2635043/diff/26001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode543
upload.py:543: # Perforce-specific
On 2010/12/06 01:47:00, Ilia Mirkin wrote:
> On 2010/12/06 01:21:27, Alex McC wrote:
> > You can switch workspaces/servers/users/etc within the p4 GUIs, so
environment
> > variables won't do :( But the p4 GUIs can pass the correct values to these
> flags
> > easily. Env vars are also not ideal for scripts on servers.
> 
> But what's the difference between running
> 
> ./upload.py --p4_client=asdf
> 
> and
> 
> P4CLIENT=asdf ./upload.py
> 
> ?

Running "VAR=asdf someApp" leaves the system/env vars in a different state than
when you started, which strikes me as scary. P4 itself supports arguments (as
well as env vars), so arguments seems like a safe and appropriate solution.

Arguments are also portable, whereas setting arguments might not be on
linux/windows/etc (although I'm not sure on this one).
 
> > 
> > Could I make things more clear by making each option's comment say something
> > like "(optional, overrides P4CLIENT environment variable)"?
> 
> I meant more just adding something to the group, like "overrides to env vars"
or
> something along those lines.

Done (locally) :)

> 
> > 
> > On 2010/12/05 21:11:30, Ilia Mirkin wrote:
> > > On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > > > Why are these needed? IIRC p4 will take these as env vars, or out of
some
> > > > ~/.p4config style file.
> > > > 
> > > > Oh, I see from previous comments. For GUI users. Well *THAT's* annoying.
> > > Didn't
> > > > know anyone actually used that thing.
> > > > 
> > > > Perhaps change the add_option_group thing to mention the gui thing?
People
> > > tend
> > > > to think options are required even when they're not.
> > > 
> > > One more thought on this. People using the GUI are going to have to
specify
> > this
> > > one way or the other. Why not make "the way" be to specify the env vars
> rather
> > > than command-line flags?
> > 
>

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1378
upload.py:1378: self.p4_changelist = options.p4_changelist
On 2010/12/06 01:47:00, Ilia Mirkin wrote:
> Oh, yes. You're right. Ugh. But it's so the logical place to put this sort of
> thing. With subversion, there's no concept of a pending CL.

Since p4 operates in a different manner than svn/hg/git (multiple pending cls
being one example), I don't think we should blur these distinct concepts to
force the logical models to be closer. As simple as possible, but no simpler :) 

> I'd feel comfortable deviating here for p4. What do others think?
> 
> On 2010/12/06 01:21:27, Alex McC wrote:
> > I read http://code.google.com/p/rietveld/wiki/UploadPyUsage as meaning that
> > --rev is for the base revision, i.e. the state you're synched to or branched
> off
> > of. The p4 change is the pending change you'd like to have reviewed.
> > 
> > On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > > Isn't that what --rev is for already? Then the --p4_* stuff never needs to
> be
> > > specified at all.
> > 
>

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1549
upload.py:1549: ErrorExit("No valid patches found in output from p4 diff")
Empty diffs (not empty CLs) will work, although file type changes are suspect.

My test cases, changing a text file to a jpg, is now handled equally poorly in
hg and p4 :P (rietveld shows the new jpg, and shows a broken image tag where the
old text should be). Changing a text file's text type also works fine.

On 2010/12/06 01:47:00, Ilia Mirkin wrote:
> Oh, you're right. There are still ways of getting empty CLs, if you work hard
> enough, but no reason to try to bend over backwards to support them.
> 
> On 2010/12/06 01:21:27, Alex McC wrote:
> > Mode changes are just "normal" edits. I expect that each "side" of the diff
> > should be read as its appropriate file type. I should make sure that I'm
> > correctly handling empty edits, though!
> > 
> > On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > > What if it's a mode change? Not sure those are important to handle though.
> > 
>

http://codereview.appspot.com/2635043/diff/26001/upload.py#newcode1666
upload.py:1666: Since local perforce repositories can't be easily detected, this
method
On 2010/12/06 01:47:00, Ilia Mirkin wrote:
> I was thinking that you'd only have p4 installed if you're using it. That 20s
> timeout is annoying though. I don't suppose there's a convenient --timeout=1
> kind of argument to p4?

I use p4 for some projects, and svn/git/hg for others :) I've met dozens of
developers who have a similar setup.

Timeouts might not be the best move: a timeout of 1s will be too long for
someone who isn't using p4 (but has it installed), but it will be too short for
shoddy internet connections across oceans.

Since you have to specify a p4_change anyways, that now implies --vcs=p4, so
there's no big wins for playing with timeouts to do additional detection. 

> On 2010/12/06 01:21:27, Alex McC wrote:
> > p4 info tries to contact the perforce server (which defaults to
> "perforce:1666"
> > if P4SERVER isn't set). On my home network (without a p4 server), it takes
21
> > seconds to timeout :( So it's not a great option.
> > 
> > On 2010/12/05 16:28:44, Ilia Mirkin wrote:
> > > Can't you run something like "p4 info" to see if you're inside a client?
> > 
>

[Alex McC]

Sorry for the big diff. It looks like diffing a branched file against its base
can't be easily done in a cross-platform way.

http://codereview.appspot.com/2635043/diff/35001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode1613
upload.py:1613: #
http://stackoverflow.com/questions/1771314/in-perforce-command-line-how-to-di...
Apparently there's no platform-agnostic way to diff a branched file against its
base revision :(

I'm diffing moves properly now, which is something that Code Collaborator didn't
do when I used it earlier this year.

[Alex McC]

Ilia, Andi, Technonik, or Guido, do you have any other recommendations for this
change?

On 2010/12/13 10:24:20, Alex McC wrote:
> Sorry for the big diff. It looks like diffing a branched file against its base
> can't be easily done in a cross-platform way.
> 
> http://codereview.appspot.com/2635043/diff/35001/upload.py
> File upload.py (right):
> 
> http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode1613
> upload.py:1613: #
>
http://stackoverflow.com/questions/1771314/in-perforce-command-line-how-to-di...
> Apparently there's no platform-agnostic way to diff a branched file against
its
> base revision :(
> 
> I'm diffing moves properly now, which is something that Code Collaborator
didn't
> do when I used it earlier this year.

[Ilia Mirkin]

Sorry, slipped off my radar. I didn't think that fstat had all the necessary
information to figure out where a file came from, but I assume that you've
tested your code and it worked fine, and that's good enough for me. [Please do
confirm ;) ]

In response to the environment thing, running P4CLIENT=asdf ./upload.py would
_not_ set P4CLIENT in the containing environment (feel free to experiment). But
if you're going to have p4_changelist, then you might as well have all the other
ones. I was just hoping that we could avoid having _any_ p4-related options.

Very minor comments below, otherwise looks good.

http://codereview.appspot.com/2635043/diff/35001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode544
upload.py:544: group = parser.add_option_group("Perforce-specific options " +
unnecessary + (python has c-style semantics for string concat)

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode555
upload.py:555: group.add_option("--p4_user", action="store", dest="p4_user",
What about password? Or does p4 ask for a password if one isn't passed in?
(Never had that situation myself... always in a ~/.p4config type thing.)

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode1381
upload.py:1381: ErrorExit("A changelist id is required")
Please do the same if options.rev is specified. Someone could expect that it
would do something.

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode1491
upload.py:1491: # -Or shows information about pending integrations/moves
2-space indent, for consistency, please

[Alex McC]

To be clear: fstat helped me get the (probably) right base for moves (renames),
but NOT for branched (copied) files.

I added a comment to GenerateDiffData explaining how I (currently) don't think
we can get the base for a branch:

      # Is it possible to diff a branched file? Stackoverflow says no:
      #
http://stackoverflow.com/questions/1771314/in-perforce-command-line-how-to-di...

I'm testing my changes on a change containing 11 changes (13 with split moves).
It doesn't cover everything (I don't know how to purge files), but it should be
a good spread:
-add
-branch
-branch + edit
-change type (text -> other text)
-change type (text -> binary)
-delete
-edit
-integrate
-move/add with move/delete
-move/add + edit with move/delete
-re-add deleted

http://codereview.appspot.com/2635043/diff/35001/upload.py
File upload.py (right):

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode544
upload.py:544: group = parser.add_option_group("Perforce-specific options " +
On 2010/12/20 11:07:05, Ilia Mirkin wrote:
> unnecessary + (python has c-style semantics for string concat)

Done, thanks for the tip!

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode555
upload.py:555: group.add_option("--p4_user", action="store", dest="p4_user",
On 2010/12/20 11:07:05, Ilia Mirkin wrote:
> What about password? Or does p4 ask for a password if one isn't passed in?
> (Never had that situation myself... always in a ~/.p4config type thing.)

ConfirmLogin (in PerforceVCS's __init__) makes p4 prompt the user for their
password if they're not logged in. That's mostly to avoid me writing buggy
password handling code that accidentally leaks people's passwords :/

Once a user logs in, most p4 servers won't require a password for several hours.

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode1381
upload.py:1381: ErrorExit("A changelist id is required")
On 2010/12/20 11:07:05, Ilia Mirkin wrote:
> Please do the same if options.rev is specified. Someone could expect that it
> would do something.

Done.

http://codereview.appspot.com/2635043/diff/35001/upload.py#newcode1491
upload.py:1491: # -Or shows information about pending integrations/moves
On 2010/12/20 11:07:05, Ilia Mirkin wrote:
> 2-space indent, for consistency, please

Done.

[imirkin_alum.mit.edu]

On Mon, Dec 20, 2010 at 1:58 PM,  <alex.mccarthy@gmail.com> wrote:
> To be clear: fstat helped me get the (probably) right base for moves
> (renames), but NOT for branched (copied) files.
>
> I added a comment to GenerateDiffData explaining how I (currently) don't
> think we can get the base for a branch:

I believe if you run "p4 resolved" and "p4 integrated", one will give
you the answer for pending CLs, the other for committed ones. [Which
is which, though, I have _no_ recollection.] But this is a fairly
minor point.

>
>      # Is it possible to diff a branched file? Stackoverflow says no:
>      #
>
http://stackoverflow.com/questions/1771314/in-perforce-command-line-how-to-di...
>
> I'm testing my changes on a change containing 11 changes (13 with split
> moves). It doesn't cover everything (I don't know how to purge files),
> but it should be a good spread:
> -add
> -branch
> -branch + edit
> -change type (text -> other text)
> -change type (text -> binary)
> -delete
> -edit
> -integrate
> -move/add with move/delete
> -move/add + edit with move/delete
> -re-add deleted

Wow. Extensive. Have you tested this at all on committed changes?

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[Alex McC]

On Mon, Dec 20, 2010 at 7:04 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:

> On Mon, Dec 20, 2010 at 1:58 PM,  <alex.mccarthy@gmail.com> wrote:
> > To be clear: fstat helped me get the (probably) right base for moves
> > (renames), but NOT for branched (copied) files.
> >
> > I added a comment to GenerateDiffData explaining how I (currently) don't
> > think we can get the base for a branch:
>
> I believe if you run "p4 resolved" and "p4 integrated", one will give
> you the answer for pending CLs, the other for committed ones. [Which
> is which, though, I have _no_ recollection.] But this is a fairly
> minor point.


Sorry, I misspoke: I can get the base(s), but p4 won't generate a diff. I
feel like adding a dependency on a system diff program is the wrong move
(and I doubt it would work on windows), so I'm out of ideas :(

The p4 GUI lets you diff a branched file with its base, but it uses p4 print
to create temporary files, then runs its internal diff program on the
output.

 >
> >      # Is it possible to diff a branched file? Stackoverflow says no:
> >      #
> >
>
http://stackoverflow.com/questions/1771314/in-perforce-command-line-how-to-di...
> >
> > I'm testing my changes on a change containing 11 changes (13 with split
> > moves). It doesn't cover everything (I don't know how to purge files),
> > but it should be a good spread:
> > -add
> > -branch
> > -branch + edit
> > -change type (text -> other text)
> > -change type (text -> binary)
> > -delete
> > -edit
> > -integrate
> > -move/add with move/delete
> > -move/add + edit with move/delete
> > -re-add deleted
>
> Wow. Extensive. Have you tested this at all on committed changes?
>

Not until now, but I just tested it against some minor submitted changes,
and it seems to work fine. Thanks for the reminder :)

-Alex

[imirkin_alum.mit.edu]

On Mon, Dec 20, 2010 at 4:28 PM, Alex McCarthy <alex.mccarthy@gmail.com> wrote:
> On Mon, Dec 20, 2010 at 7:04 PM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>
>> On Mon, Dec 20, 2010 at 1:58 PM,  <alex.mccarthy@gmail.com> wrote:
>> > To be clear: fstat helped me get the (probably) right base for moves
>> > (renames), but NOT for branched (copied) files.
>> >
>> > I added a comment to GenerateDiffData explaining how I (currently) don't
>> > think we can get the base for a branch:
>>
>> I believe if you run "p4 resolved" and "p4 integrated", one will give
>> you the answer for pending CLs, the other for committed ones. [Which
>> is which, though, I have _no_ recollection.] But this is a fairly
>> minor point.
>
> Sorry, I misspoke: I can get the base(s), but p4 won't generate a diff. I
> feel like adding a dependency on a system diff program is the wrong move
> (and I doubt it would work on windows), so I'm out of ideas :(
> The p4 GUI lets you diff a branched file with its base, but it uses p4 print
> to create temporary files, then runs its internal diff program on the
> output.

Ahhh, I see. Annoying.

>>
>> >
>> >      # Is it possible to diff a branched file? Stackoverflow says no:
>> >      #
>> >
>> >
http://stackoverflow.com/questions/1771314/in-perforce-command-line-how-to-di...
>> >
>> > I'm testing my changes on a change containing 11 changes (13 with split
>> > moves). It doesn't cover everything (I don't know how to purge files),
>> > but it should be a good spread:
>> > -add
>> > -branch
>> > -branch + edit
>> > -change type (text -> other text)
>> > -change type (text -> binary)
>> > -delete
>> > -edit
>> > -integrate
>> > -move/add with move/delete
>> > -move/add + edit with move/delete
>> > -re-add deleted
>>
>> Wow. Extensive. Have you tested this at all on committed changes?
>
> Not until now, but I just tested it against some minor submitted changes,
> and it seems to work fine. Thanks for the reminder :)
> -Alex

Great. This all looks good to me. I'll let it sit until ~tonight in
case there are further comments/concerns/etc from others, and then
check it in.

Thanks for bearing with this review... I think it dragged out longer
than it had to, and that's mostly my fault =/

-- 
Ilia Mirkin
imirkin@alum.mit.edu

[Alex McC]

>
> Great. This all looks good to me. I'll let it sit until ~tonight in
> case there are further comments/concerns/etc from others, and then
> check it in.
>

Fantastic! :D Please let me know if you'd like me to change anything else,
I'm happy to spend a little extra time to get this right.

Thanks for bearing with this review... I think it dragged out longer
> than it had to, and that's mostly my fault =/


A rigorous code review seems appropriate when submitting a patch to a code
review tool. Thanks for helping me improve this change :)

[Alex McC]

Ilia, do you want to give rietveld perforce support for Christmas? ;)

On Mon, Dec 20, 2010 at 9:51 PM, Alex McCarthy <alex.mccarthy@gmail.com>wrote:

> Great. This all looks good to me. I'll let it sit until ~tonight in
>> case there are further comments/concerns/etc from others, and then
>> check it in.
>>
>
> Fantastic! :D Please let me know if you'd like me to change anything else,
> I'm happy to spend a little extra time to get this right.
>
> Thanks for bearing with this review... I think it dragged out longer
>> than it had to, and that's mostly my fault =/
>
>
> A rigorous code review seems appropriate when submitting a patch to a code
> review tool. Thanks for helping me improve this change :)
>

[imirkin_alum.mit.edu]

I actually _just_ committed... wow, what timing :)

On Thu, Dec 23, 2010 at 5:12 AM, Alex McCarthy <alex.mccarthy@gmail.com> wrote:
> Ilia, do you want to give rietveld perforce support for Christmas? ;)
>
> On Mon, Dec 20, 2010 at 9:51 PM, Alex McCarthy <alex.mccarthy@gmail.com>
> wrote:
>>>
>>> Great. This all looks good to me. I'll let it sit until ~tonight in
>>> case there are further comments/concerns/etc from others, and then
>>> check it in.
>>
>> Fantastic! :D Please let me know if you'd like me to change anything else,
>> I'm happy to spend a little extra time to get this right.
>>>
>>> Thanks for bearing with this review... I think it dragged out longer
>>> than it had to, and that's mostly my fault =/
>>
>> A rigorous code review seems appropriate when submitting a patch to a code
>> review tool. Thanks for helping me improve this change :)
>



-- 
Ilia Mirkin
imirkin@alum.mit.edu

[Alex McC]

Great, thanks Ilia!

I'll update the docs in the next few days to explain how the new perforce
options are used.
-Alex


On Thu, Dec 23, 2010 at 10:15 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:

> I actually _just_ committed... wow, what timing :)
>
> On Thu, Dec 23, 2010 at 5:12 AM, Alex McCarthy <alex.mccarthy@gmail.com>
> wrote:
> > Ilia, do you want to give rietveld perforce support for Christmas? ;)
> >
> > On Mon, Dec 20, 2010 at 9:51 PM, Alex McCarthy <alex.mccarthy@gmail.com>
> > wrote:
> >>>
> >>> Great. This all looks good to me. I'll let it sit until ~tonight in
> >>> case there are further comments/concerns/etc from others, and then
> >>> check it in.
> >>
> >> Fantastic! :D Please let me know if you'd like me to change anything
> else,
> >> I'm happy to spend a little extra time to get this right.
> >>>
> >>> Thanks for bearing with this review... I think it dragged out longer
> >>> than it had to, and that's mostly my fault =/
> >>
> >> A rigorous code review seems appropriate when submitting a patch to a
> code
> >> review tool. Thanks for helping me improve this change :)
> >
>
>
>
> --
> Ilia Mirkin
> imirkin@alum.mit.edu
>

[Alex McC]

I've added a comment to http://code.google.com/p/rietveld/wiki/UploadPyUsage
documenting the new p4 options, and explaining how to use upload.py from the P4V
gui. It looks like I can't edit the wiki, would one of you guys mind folding in
my changes?

On 2010/12/23 11:01:46, Alex McC wrote:
> Great, thanks Ilia!
> 
> I'll update the docs in the next few days to explain how the new perforce
> options are used.
> -Alex

[Alex McC]

Andi, Evan, Guido, Ilia, or Techtonik: would one of you be able to update
the UploadPyUsage wiki, or give me edit access to it?

The page should be updated to include perforce documentation.
-Alex


On Sun, Dec 26, 2010 at 10:13 AM, <alex.mccarthy@gmail.com> wrote:

> I've added a comment to
> http://code.google.com/p/rietveld/wiki/UploadPyUsage documenting the new
> p4 options, and explaining how to use upload.py from the P4V gui. It
> looks like I can't edit the wiki, would one of you guys mind folding in
> my changes?
>
>
> On 2010/12/23 11:01:46, Alex McC wrote:
>
>> Great, thanks Ilia!
>>
>
>  I'll update the docs in the next few days to explain how the new
>>
> perforce
>
>> options are used.
>> -Alex
>>
>
> http://codereview.appspot.com/2635043/
>

[imirkin_alum.mit.edu]

On Sun, Jan 9, 2011 at 5:58 PM, Alex McCarthy <alex.mccarthy@gmail.com> wrote:
> Andi, Evan, Guido, Ilia, or Techtonik: would one of you be able to update

Done. Let me know if I messed something up.

  -ilia

[Alex McC]

Looks perfect, thanks!
-Alex


On Mon, Jan 10, 2011 at 4:23 AM, Ilia Mirkin <imirkin@alum.mit.edu> wrote:

> On Sun, Jan 9, 2011 at 5:58 PM, Alex McCarthy <alex.mccarthy@gmail.com>
> wrote:
> > Andi, Evan, Guido, Ilia, or Techtonik: would one of you be able to update
>
> Done. Let me know if I messed something up.
>
>  -ilia
>

[techtonik]

Can we close this issue?

It was committed as http://code.google.com/p/rietveld/source/detail?r=640

[gvrpython]

Yes; only Alex can close it.

On Tue, Mar 29, 2011 at 11:44 AM,  <techtonik@gmail.com> wrote:
> Can we close this issue?
>
> It was committed as
> http://code.google.com/p/rietveld/source/detail?r=640
>
>
>
> http://codereview.appspot.com/2635043/
>



-- 
--Guido van Rossum (python.org/~guido)

[Alex McC]

Closed!
-Alex


On Tue, Mar 29, 2011 at 7:50 PM, Guido van Rossum <guido@python.org> wrote:

> Yes; only Alex can close it.
>
> On Tue, Mar 29, 2011 at 11:44 AM,  <techtonik@gmail.com> wrote:
> > Can we close this issue?
> >
> > It was committed as
> > http://code.google.com/p/rietveld/source/detail?r=640
> >
> >
> >
> > http://codereview.appspot.com/2635043/
> >
>
>
>
> --
> --Guido van Rossum (python.org/~guido)
>