Summary

Clean up Perforce change description rewriting and fix newline errors.

Review Request #13023 — Created May 8, 2023 and submitted May 15, 2023, 2:50 p.m.

Information

Owner

chipx86

Repository

RBTools

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

We've had code in the tree for a long while to stamp Perforce changes
with a review request URL, but it had a flaw. It left out a newline,
causing the first line to read:

Description:   Here is the first line.
        And here is the second line.

This was due to a missing newline after adding the "Description:" field.

This change fixes this up, reworks the logic and comments to be a bit
more clear about the states being kept and the newlines added, and adds
unit tests.

It also fixes up the amending portion, which didn't include a changenum
in the p4 change command (which meant it couldn't amend anything but
default).

A timeout is now set for the p4 change command, in case anything fails
during a hook script. Errors are outputted if anything goes wrong.

Testing Done

Unit tests passed.

Tested manually with updates to the trigger script.

Commits

Summary	ID
Clean up Perforce change description rewriting and fix newline errors. We've had code in the tree for a long while to stamp Perforce changes with a review request URL, but it had a flaw. It left out a newline, causing the first line to read: ``` Description: Here is the first line. And here is the second line. ``` This was due to a missing newline after adding the "Description:" field. This change fixes this up, reworks the logic and comments to be a bit more clear about the states being kept and the newlines added, and adds unit tests. It also fixes up the amending portion, which didn't include a changenum in the `p4 change` command (which meant it couldn't amend anything but `default`). A timeout is now set for the `p4 change` command, in case anything fails during a hook script. Errors are outputted if anything goes wrong.	174b6711bb9995c787fbae0de53b8386947e8064

Summary

Clean up Perforce change description rewriting and fix newline errors.

We've had code in the tree for a long while to stamp Perforce changes with a review request URL, but it had a flaw. It left out a newline, causing the first line to read: ``` Description: Here is the first line. And here is the second line. ``` This was due to a missing newline after adding the "Description:" field. This change fixes this up, reworks the logic and comments to be a bit more clear about the states being kept and the newlines added, and adds unit tests. It also fixes up the amending portion, which didn't include a changenum in the `p4 change` command (which meant it couldn't amend anything but `default`). A timeout is now set for the `p4 change` command, in case anything fails during a hook script. Errors are outputted if anything goes wrong.

174b6711bb9995c787fbae0de53b8386947e8064

Issues

Description	From	Last Updated
undefined name 'changenum' Column: 12 Error code: F821	reviewbot	May 8, 2023, 12:35 p.m.
undefined name 'changenum' Column: 25 Error code: F821	reviewbot	May 8, 2023, 12:35 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
undefined name 'changenum'

Column: 12
Error code: F821
```
rbtools/clients/perforce.py (Diff revision 1)
The issue has been resolved. Show all issues
```
undefined name 'changenum'

Column: 25
Error code: F821
```

Change Summary:


Added a couple missing changes to the commit for the renamed call and a function definition update.
Fleshed out the description to cover the related changes.

Description:

		We've had code in the tree for a long while to stamp Perforce changes
		with a review request URL, but it had a flaw. It left out a newline,
		causing the first line to read:



		`Description: Here is the first line.`
		And here is the second line.




		This was due to a missing newline after adding the "Description:" field.

		This change fixes this up, reworks the logic and comments to be a bit
		more clear about the states being kept and the newlines added, and adds
		unit tests.
	+
	+	It also fixes up the amending portion, which didn't include a changenum
	+	in the `p4 change` command (which meant it couldn't amend anything but
	+	`default`).
	+
	+	A timeout is now set for the `p4 change` command, in case anything fails
	+	during a hook script. Errors are outputted if anything goes wrong.

Commits:

	Summary	ID
	Clean up Perforce change description rewriting and fix newline errors. We've had code in the tree for a long while to stamp Perforce changes with a review request URL, but it had a flaw. It left out a newline, causing the first line to read: ``` Description: Here is the first line. And here is the second line. ``` This was due to a missing newline after adding the "Description:" field. This change fixes this up, reworks the logic and comments to be a bit more clear about the states being kept and the newlines added, and adds unit tests.	53e9a853d273c8b3516bec66b165bf9b7f144105
	Clean up Perforce change description rewriting and fix newline errors. We've had code in the tree for a long while to stamp Perforce changes with a review request URL, but it had a flaw. It left out a newline, causing the first line to read: ``` Description: Here is the first line. And here is the second line. ``` This was due to a missing newline after adding the "Description:" field. This change fixes this up, reworks the logic and comments to be a bit more clear about the states being kept and the newlines added, and adds unit tests. It also fixes up the amending portion, which didn't include a changenum in the `p4 change` command (which meant it couldn't amend anything but `default`). A timeout is now set for the `p4 change` command, in case anything fails during a hook script. Errors are outputted if anything goes wrong.	174b6711bb9995c787fbae0de53b8386947e8064

Diff:

Revision 2 (+492 -80)

Show changes

	rbtools/clients/perforce.py
	rbtools/clients/tests/test_p4.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-4.x (f6e1d5a)

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

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