Summary

Allow uploading simple linear history with Git.

Review Request #6787 — Created Jan. 16, 2015 and submitted March 3, 2015, 2:33 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

dvcs

Bugs

Depends On

~~6816~~

Commit

5c9866c...

Reviewers

Groups

rbtools

People

chipx86, smacleod

Description

The default behaviour for posting is now to determine the capabilities
of the server to see if it supports creating a review request with
commit history. The user can specify the -S/--squash-history
option or SQUASH_HISTORY config option in .reviewboardrc to
override this. However, the configuration option can itself be
overridden by the -H/--with-history flag.

Currently the only SCMClients that "supports" this flag is Git.

Add the DiffCommitResource and DiffCommitListResource resources to
the RBTools web API resources. The DiffCommitListResource handles
the uploading of commits via the upload_commit method.

Add the create_empty_diffset method to the DiffListResource class.
This method is used to create an empty DiffSet so that a review
request can be created with commit history.

Add the get_history method to the SCMClient class. This method
gets a linear history of commit metadata given some revisions.

The commit histories that can be uploaded by this change are simple
linear ones; that is, they must be linear histories (with no merges)
and diffs in commits should not overlap with respect to files.

Testing Done

Uploaded commits containing diffs to Review Board on the dvcs
branch.

Issues

Description	From	Last Updated
One blank line.	chipx86	Jan. 16, 2015, 1:07 p.m.
Is it commits or diff-commits? I thought we had the issue of conflicts in naming.	chipx86	Jan. 16, 2015, 1:10 p.m.
That's just too new!	chipx86	Jan. 16, 2015, 1:01 p.m.
This is the bit that's all placeholders I'm guessing. Maybe add a big # XXX This is all bogus data. …	chipx86	Jan. 16, 2015, 1:05 p.m.
This can end up returning two different results. We should pre-calculate once.	chipx86	Jan. 16, 2015, 1:04 p.m.
% on the next line (same line as the variable).	chipx86	Jan. 16, 2015, 1:05 p.m.
Might be worth adding a config_key for this, in case any projects or users want to default to this behavior.	chipx86	Jan. 22, 2015, 4:23 p.m.
No blank line here.	chipx86	Feb. 11, 2015, 1:49 p.m.
Looks like this will always be True if --squash-history is not set.	chipx86	Jan. 22, 2015, 2:13 p.m.
undefined name 'merge_parent_id'	reviewbot	Feb. 11, 2015, 1:49 p.m.
Col: 13 E265 block comment should start with '# '	reviewbot	Feb. 11, 2015, 1:49 p.m.
Col: 23 W292 no newline at end of file	reviewbot	Feb. 11, 2015, 1:49 p.m.
'datetime' imported but unused	reviewbot	Feb. 11, 2015, 1:49 p.m.
Col: 26 E225 missing whitespace around operator	reviewbot	Feb. 11, 2015, 1:49 p.m.
"committer"	chipx86	Feb. 24, 2015, 10:41 a.m.
Worth maybe just doing a full-on strip()?	chipx86	Feb. 24, 2015, 11:12 a.m.
Trailing period.	chipx86	Feb. 24, 2015, 11:09 a.m.
This can be: for sha in reversed(history_lines): That's a little bit faster, since it can just iterate instead of rebuilding …	chipx86	Feb. 24, 2015, 10:43 a.m.
Can we get a list of all SHAs we need, and then do a single execute() for this, to reduce …	chipx86	Feb. 24, 2015, 11:09 a.m.
This will also result in a lot of slow, expensive calls. Can we grab everything we need for all SHAs …	chipx86	Feb. 24, 2015, 11:09 a.m.
You can probably just put the dictionary right in the append call instead.	chipx86	Feb. 24, 2015, 11:09 a.m.
'datetime' imported but unused	reviewbot	Feb. 11, 2015, 1:52 p.m.
Col: 26 E225 missing whitespace around operator	reviewbot	Feb. 11, 2015, 1:52 p.m.
A nice Python utility function is enumerate. You can use that here to get both the index and the history …	chipx86	Feb. 24, 2015, 11:13 a.m.
not diff will be the same as len(diff) == 0.	chipx86	Feb. 24, 2015, 11:11 a.m.
This is kind of hard to follow (and sometimes can lead to unexpected values). Can you break this out into …	chipx86	Feb. 24, 2015, 11:12 a.m.
We probably need to check for NotImplementedError, right?	chipx86	Feb. 27, 2015, 8:38 p.m.
We ignore the item, but then we try fetching it in several places below. We should just use the item …	chipx86	Feb. 27, 2015, 8:42 p.m.
Should be two blank lines.	david	March 3, 2015, 2:31 p.m.
Blank line after this.	david	March 3, 2015, 2:27 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Testing Done:

	+	Uploaded a commit containing a diff to Review Board (see:
	+	https://reviews.reviewboard.org/r/6779/)

Depends On:

+	~~6779 - Initial backend support for review requests with commit history.~~

Summary:

-	Allow uploading of a single (squashed) commit with Git.
+	[WIP] Allow uploading of a single (squashed) commit with Git.

I'm excited! I know it's too early, but just even seeing a prototype on the RBTools side is cool.

rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
One blank line.
```

rbtools/api/resource.py (Diff revision 1)

The issue has been resolved. Show all issues

Is it commits or diff-commits? I thought we had the issue of conflicts in naming.

brennie Jan. 16, 2015, 1:11 p.m.

I'll fix this in https://reviews.reviewboard.org/r/6779/.

rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
That's just too new!
```

rbtools/commands/post.py (Diff revision 1)

The issue has been resolved. Show all issues

This is the bit that's all placeholders I'm guessing. Maybe add a big # XXX This is all bogus data. Don't pay too much attention to it. or something. (I almost commented on the commit IDs before I realized :)

rbtools/commands/post.py (Diff revision 1)

The issue has been resolved. Show all issues

This can end up returning two different results. We should pre-calculate once.

rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
% on the next line (same line as the variable).
```

Commit:

-	7f0cb6920e72641acf26e167593e2f0b29b3ff68
+	588ed00337b72f34e7ee47efa49da7727e2cc7ff

Diff:

Revision 2 (+108 -6)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Change Summary:

Add -S flag and SQUASH_HISTORY option.

Description:

~		Add the `-H`/`--with-history` option to the `rbt post` command. This
~		flag allows for review requests with commit history to be posted.
~		That is, instead of review requests consisting of only a single diff,
~		they can span multiple commits (i.e., diffs), each of which can be
~		inspected and reviewed. Currently, the only SCMTool that supports this
~		flag is Git.
	~	The default behaviour for posting is now to determine the capabilities
	~	of the server to see if it supports creating a review request with
	~	commit history. The user can specify the `-S`/`--squash-history`
	~	option or `SQUASH_HISTORY` config option in `.reviewboardrc` to
	~	override this. However, the configuration option can itself be
	~	overridden by the `-H`/`--with-history` flag.
	+
	+	Currently the only `SCMClient`s that "supports" this flag is Git.

		Add the `DiffCommitResource` and `DiffCommitListResource` resources to
		the RBTools web API resources. The `DiffCommitListResource` handles
		the uploading of commits via the `upload_commit` method.

		Add the `create_empty_diffset` method to the `DiffListResource` class.
		This method is used to create an empty `DiffSet` so that a review
		request can be created with commit history.

		NB:
		- The commit metadata information generated by this patch is
		bogus.
		- The commit will contain the squashed diff of the entire revision
		range.

		This is currently a proof of concept.

Commit:

-	588ed00337b72f34e7ee47efa49da7727e2cc7ff
+	35429c7f5e59f90ae20245bf2d6d70195577aa5a

Diff:

Revision 3 (+130 -6)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Change Summary:

Use the right capability

Commit:

-	35429c7f5e59f90ae20245bf2d6d70195577aa5a
+	5c9866caeed32f08731853866e40d9f00038b899

Diff:

Revision 4 (+131 -6)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Change Summary:

Update depends on field

Depends On:

-	~~6779 - Initial backend support for review requests with commit history.~~
+	~~6816 - Add initial web API for commit histories.~~

rbtools/commands/post.py (Diff revision 4)

The issue has been dropped. Show all issues

Might be worth adding a config_key for this, in case any projects or users want to default to this behavior.

brennie Jan. 21, 2015, 4:22 p.m.

I made that the default behaviour by checking if the server supports commit histories via the API capabilities.

rbtools/commands/post.py (Diff revision 4)
The issue has been resolved. Show all issues
```
No blank line here.
```

rbtools/commands/post.py (Diff revision 4)

The issue has been dropped. Show all issues

Looks like this will always be True if --squash-history is not set.

brennie Jan. 21, 2015, 4:22 p.m.

The commit history posting is intended to be the default behaviour unless one of -S/--squash-history/SQUASH_HISTORY=True is set. In case that IS true, we let -H override it.

Change Summary:

squashed diffs -> linear history

Summary:

-	[WIP] Allow uploading of a single (squashed) commit with Git.
+	Allow uploading linear history with Git.

Description:

		The default behaviour for posting is now to determine the capabilities
		of the server to see if it supports creating a review request with
		commit history. The user can specify the `-S`/`--squash-history`
		option or `SQUASH_HISTORY` config option in `.reviewboardrc` to
		override this. However, the configuration option can itself be
		overridden by the `-H`/`--with-history` flag.

		Currently the only `SCMClient`s that "supports" this flag is Git.

		Add the `DiffCommitResource` and `DiffCommitListResource` resources to
		the RBTools web API resources. The `DiffCommitListResource` handles
		the uploading of commits via the `upload_commit` method.

		Add the `create_empty_diffset` method to the `DiffListResource` class.
		This method is used to create an empty `DiffSet` so that a review
		request can be created with commit history.

~		NB:
~		- The commit metadata information generated by this patch is
	~	Add the `get_history` method to the `SCMClient` class. This method
	~	gets a linear history of commit metadata given some revisions.
-		bogus.
-		- The commit will contain the squashed diff of the entire revision
-		range.

~		This is currently a proof of concept.
	~	NB: This requires some overhauls to the current backend that is up for review.

Diff:

Revision 5 (+238 -14)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

rbtools/api/resource.py (Diff revision 5)
The issue has been resolved. Show all issues
```
 undefined name 'merge_parent_id'
```
rbtools/clients/git.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 13
 E265 block comment should start with '# '
```
rbtools/clients/git.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 23
 W292 no newline at end of file
```
rbtools/commands/post.py (Diff revision 5)
The issue has been resolved. Show all issues
```
 'datetime' imported but unused
```
rbtools/commands/post.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 26
 E225 missing whitespace around operator
```

Change Summary:

Update to reflect changes to r/6816

Testing Done:

		Uploaded a commit containing a diff to Review Board (see:
~		https://reviews.reviewboard.org/r/6779/)
	~	https://reviews.reviewboard.org/r/6816/)

Diff:

Revision 6 (+253 -16)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

rbtools/commands/post.py (Diff revision 6)
The issue has been resolved. Show all issues
```
 'datetime' imported but unused
```
rbtools/commands/post.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 26
 E225 missing whitespace around operator
```

A few comments on performance, and some small things here and there, but this looks like it's basically good to go :)

rbtools/api/resource.py (Diff revision 6)
The issue has been resolved. Show all issues
```
"committer"
```
rbtools/clients/git.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Worth maybe just doing a full-on strip()?
```
rbtools/clients/git.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Trailing period.
```

rbtools/clients/git.py (Diff revision 6)

The issue has been resolved. Show all issues

This can be:

for sha in reversed(history_lines):

That's a little bit faster, since it can just iterate instead of rebuilding the list.

rbtools/clients/git.py (Diff revision 6)

The issue has been resolved. Show all issues

Can we get a list of all SHAs we need, and then do a single execute() for this, to reduce the time needed?

rbtools/clients/git.py (Diff revision 6)

The issue has been resolved. Show all issues

This will also result in a lot of slow, expensive calls. Can we grab everything we need for all SHAs in a single call, in some serialized format we can parse?

rbtools/clients/git.py (Diff revision 6)

The issue has been resolved. Show all issues

You can probably just put the dictionary right in the append call instead.

rbtools/commands/post.py (Diff revision 6)

The issue has been resolved. Show all issues

A nice Python utility function is enumerate. You can use that here to get both the index and the history item:

for i, item in enumerate(history):

rbtools/commands/post.py (Diff revision 6)
The issue has been resolved. Show all issues
```
not diff will be the same as len(diff) == 0.
```

rbtools/commands/post.py (Diff revision 6)

The issue has been resolved. Show all issues

This is kind of hard to follow (and sometimes can lead to unexpected values). Can you break this out into explicit if/else blocks?

Change Summary:

Address Christian's issues.

We only have to execute git log once due to new better parsing.

Diff:

Revision 7 (+249 -16)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Diff:

Revision 8 (+251 -16)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Diff:

Revision 9 (+253 -16)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Change Summary:

Rewording. master -> dvcs.

Summary:

-	Allow uploading linear history with Git.
+	Allow uploading simple linear history with Git.

Description:

		The default behaviour for posting is now to determine the capabilities
		of the server to see if it supports creating a review request with
		commit history. The user can specify the `-S`/`--squash-history`
		option or `SQUASH_HISTORY` config option in `.reviewboardrc` to
		override this. However, the configuration option can itself be
		overridden by the `-H`/`--with-history` flag.

		Currently the only `SCMClient`s that "supports" this flag is Git.

		Add the `DiffCommitResource` and `DiffCommitListResource` resources to
		the RBTools web API resources. The `DiffCommitListResource` handles
		the uploading of commits via the `upload_commit` method.

		Add the `create_empty_diffset` method to the `DiffListResource` class.
		This method is used to create an empty `DiffSet` so that a review
		request can be created with commit history.

		Add the `get_history` method to the `SCMClient` class. This method
		gets a linear history of commit metadata given some revisions.

~		NB: This requires some overhauls to the current backend that is up for review.
	~	The commit histories that can be uploaded by this change are simple
	+	linear ones; that is, they must be linear histories (with no merges)
	+	and diffs in commits should not overlap with respect to files.

Branch:

-	master
+	dvcs

rbtools/commands/post.py (Diff revision 9)

The issue has been dropped. Show all issues

We probably need to check for NotImplementedError, right?

brennie Feb. 27, 2015, 8:38 p.m.

It should never be the case that with_history is True and self.tool.get_history would throw a NotImplementedError. Earlier in the code (lines 645--652) we check if we have specified the -H option and the tool doesn't support history (via the SCMTool's supports_post_with_history field.

rbtools/commands/post.py (Diff revision 9)

The issue has been resolved. Show all issues

We ignore the item, but then we try fetching it in several places below. We should just use the item that we fetch here, to avoid all the extra lookups.

Branch:

-	dvcs
+	master

Diff:

Revision 10 (+252 -16)

Show changes

	rbtools/api/resource.py
	rbtools/clients/__init__.py
	rbtools/clients/git.py
	rbtools/commands/post.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Change Summary:

Update testing done.

Testing Done:

~		Uploaded a commit containing a diff to Review Board (see:
~		https://reviews.reviewboard.org/r/6816/)
	~	Uploaded commits containing diffs to Review Board on the `dvcs`
	~	branch.

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/clients/__init__.py
    rbtools/api/resource.py
    rbtools/clients/git.py

Change Summary:

master -> dvcs

Branch:

-	master
+	dvcs

Ship it!

rbtools/api/resource.py (Diff revision 10)
The issue has been dropped. Show all issues
```
Should be two blank lines.
```
rbtools/api/resource.py (Diff revision 10)
The issue has been resolved. Show all issues
```
Blank line after this.
```

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (12b1d7e)