Summary

Implement submitted changeset posting for Perforce.

Review Request #5196 — Created Jan. 5, 2014 and submitted Jan. 7, 2014, 2:55 p.m.

Information

Owner

david

Repository

RBTools

Branch

master

Bugs

921

Depends On

Reviewers

Groups

rbtools

People

Description

Implement submitted changeset posting for Perforce.

This change implements posting for submitted changesets in Perforce. In order
to do this, we fetch the filelog between those revisions, which will give us a
list of every change to every file within those revisions. This is then
processed to accumulate changes together. At the end of this, for each file,
we'll have one of four operations: add, delete, edit, or move. Move operations
can also include edits. This is a little bit complicated to track moves within
the revision range. Just diffing the two trees would handle adds, deletes, and
edits, but not moved files.

Testing Done


Ran unit tests
Posted a variety of revision range changes which included all four of the

  overall change types.

Issues

Description	From	Last Updated
No need for the outer parens.	chipx86	Jan. 6, 2014, 12:53 p.m.
""" on the next line.	chipx86	Jan. 6, 2014, 12:53 p.m.
I know we do this elsewhere, but given that file is a reserved word, we should use filename or something.	chipx86	Jan. 6, 2014, 12:55 p.m.
We should catch and make sure any issues here are at least logged.	chipx86	Jan. 6, 2014, 12:55 p.m.
Can we define this on the class so we don't end up building this list every iteration?	chipx86	Jan. 6, 2014, 7:17 p.m.
Same comment here about catching errors.	chipx86	Jan. 6, 2014, 12:57 p.m.
You can use setdefault here instead.	chipx86	Jan. 7, 2014, 2:07 p.m.
Given that this function doesn't seem to depend on anything in the parent scope, can we move it into its …	chipx86	Jan. 6, 2014, 12:59 p.m.
I'm hoping we can work toward getting rid of die, since it's not exactly safe when called from anything but …	chipx86	Jan. 6, 2014, 1:02 p.m.
Can we raise an exception here?	chipx86	Jan. 6, 2014, 7:55 p.m.
Just realized, this will break on Python 2.4. You can't use super with exceptions.	chipx86	Jan. 7, 2014, 2:16 p.m.

rbtools/clients/perforce.py (Diff revision 1)

Are integrate and branch no longer used at any point?

david Jan. 6, 2014, 1:57 p.m.
```
Didn't mean to do this.
```

rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for the outer parens.
```
rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
""" on the next line.
```

rbtools/clients/perforce.py (Diff revision 1)

The issue has been resolved. Show all issues

I know we do this elsewhere, but given that file is a reserved word, we should use filename or something.

rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We should catch and make sure any issues here are at least logged.
```

rbtools/clients/perforce.py (Diff revision 1)

The issue has been dropped. Show all issues

Can we define this on the class so we don't end up building this list every iteration?

david Jan. 6, 2014, 1:57 p.m.

How is it building the list every iteration? The literal isn't re-parsed for each iteration of the loop.

chipx86 Jan. 6, 2014, 7:57 p.m.

Didn't consider that it just treated it as a literal. You're right.

I ran a test to see just what the difference would be, for my own curiosity. I created two scripts with a simple loop doing an if like above, one with the list inline and one pre-defined. It looped it 50 million times, and the literal was faster by 1.4 seconds.

The more you know.

rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same comment here about catching errors.
```

rbtools/clients/perforce.py (Diff revision 1)

The issue has been resolved. Show all issues

You can use setdefault here instead.

david Jan. 6, 2014, 1:57 p.m.

I'm not sure I understand how that would apply in this situation.

chipx86 Jan. 6, 2014, 7:57 p.m.

changesets.setdefault(cln, {}).set('depot_file', change)
No big deal, just figured it'd save a couple lines.

rbtools/clients/perforce.py (Diff revision 1)

The issue has been resolved. Show all issues

Given that this function doesn't seem to depend on anything in the parent scope, can we move it into its own proper method on the class? The parent function's pretty large as it is.

rbtools/clients/perforce.py (Diff revision 1)

Do we want to break when we find this, or go for the last one?

rbtools/clients/perforce.py (Diff revision 1)

The issue has been resolved. Show all issues

I'm hoping we can work toward getting rid of die, since it's not exactly safe when called from anything but an RBTools command. Can we raise exceptions instead?

There was another somewhere else as well that this also applies to.

david Jan. 6, 2014, 1:57 p.m.

I'll fix the one I'm adding but I don't want to make more, unrelated changes right now.

chipx86 Jan. 6, 2014, 7:57 p.m.
```
Completely fine.
```

rbtools/clients/perforce.py (Diff revision 1)

The issue has been dropped. Show all issues

Can we raise an exception here?

david Jan. 6, 2014, 1:57 p.m.

AssertionError? :P

This really is a code consistency check and not really logic. The various tests in the if/elif/else clause are all data that is just used internally.

That said, looking at this, I did forget to handle a few cases :P

chipx86 Jan. 6, 2014, 7:57 p.m.
```
Sure, that's fine.
```

Diff:

Revision 2 (+274 -33)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/perforce.py

rbtools/clients/errors.py (Diff revision 2)

The issue has been dropped. Show all issues

Just realized, this will break on Python 2.4. You can't use super with exceptions.

david Jan. 6, 2014, 10 p.m.

Can we bump up to 2.5 for rbtools 0.6? 2.5 is over 7 years old now.

Change Summary:

Use setdefault()

Diff:

Revision 3 (+271 -33)

Show changes

	rbtools/clients/errors.py
	rbtools/clients/perforce.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (9ba6c27).