brennie got a fish trophy!

Summary

Generate commit histories with Mercurial

Review Request #10101 — Created July 24, 2018 and submitted Oct. 25, 2018, 10:58 p.m.

Information

Owner

brennie

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

7b3cc01...

Reviewers

Groups

rbtools

People

Description

The Mercurial SCM tool can now generate commit histories so that it can
be used to create review requests with history.

Testing Done

Posted review requests with history.

Issues

Description	From	Last Updated
E303 too many blank lines (2)	reviewbot	July 24, 2018, 9:48 p.m.
F821 undefined name 'NO_PARENT'	reviewbot	July 24, 2018, 9:48 p.m.
Byte string prefix? We'd need it above as well.	chipx86	Aug. 17, 2018, 7:43 p.m.
Byte string prefix?	chipx86	Aug. 17, 2018, 7:43 p.m.
Alphabetical order.	chipx86	Aug. 30, 2018, 8:06 p.m.
This should say that it raises NotImplementedError under the conditions.	chipx86	Aug. 30, 2018, 8:07 p.m.
It'd be nice to know where the revisions come from.	chipx86	Aug. 30, 2018, 8:08 p.m.
Just to check, is \\x1f right, or would \x1f be right?	chipx86	Aug. 30, 2018, 8:11 p.m.
We use these special markers in several places. Let's make them constants so it's readable and consistent.	chipx86	Aug. 30, 2018, 8:11 p.m.
This was reordered but wasn't put in alphabetical order.	chipx86	Sept. 4, 2018, 12:30 p.m.
I think the function name changed in a parent change. Can you retest with that?	chipx86	Sept. 4, 2018, 12:29 p.m.
Must be the full path: rbtools.clients.errors.SCMError.	chipx86	Sept. 4, 2018, 12:30 p.m.
It'd be nice to have these as constants like with Git, to help with readability.	chipx86	Sept. 4, 2018, 12:34 p.m.
Is there any reason why we couldn't just do if entry['parent2'] != self.NO_PARENT? We don't use it elsewhere, and it …	david	Sept. 27, 2018, 9:21 a.m.
Missing period.	chipx86	Oct. 25, 2018, 9:56 p.m.

flake8 failed.

JSHint passed.

flake8

rbtools/clients/mercurial.py (Diff revision 1)
Show all issues
```
E303 too many blank lines (2)
```
rbtools/clients/mercurial.py (Diff revision 1)
Show all issues
```
F821 undefined name 'NO_PARENT'
```

Commit:

-	b2a208ea1a19d697590737059ae9eaa13b035601
+	a14e648241fa0e519dcb47bb34963af172cafb7b

Diff:

Revision 2 (+62)

Show changes

rbtools/clients/mercurial.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/mercurial.py (Diff revision 2)
Show all issues
```
Byte string prefix?
We'd need it above as well.
```
rbtools/clients/mercurial.py (Diff revision 2)
Show all issues
```
Byte string prefix?
```

Change Summary:

Address issues

Commit:

-	a14e648241fa0e519dcb47bb34963af172cafb7b
+	d2c58677e2c59dd1f7b795a7b4ff11a0ca9a406a

Diff:

Revision 3 (+64)

Show changes

rbtools/clients/mercurial.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

rebase

Commit:

-	d2c58677e2c59dd1f7b795a7b4ff11a0ca9a406a
+	f8cad25d8e5d065c87f4609dba1becb601b3fb00

Diff:

Revision 4 (+64)

Show changes

rbtools/clients/mercurial.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/mercurial.py (Diff revision 4)
Show all issues
```
Alphabetical order.
```

rbtools/clients/mercurial.py (Diff revision 4)

Show all issues

This should say that it raises NotImplementedError under the conditions.

rbtools/clients/mercurial.py (Diff revision 4)

Show all issues

It'd be nice to know where the revisions come from.

rbtools/clients/mercurial.py (Diff revision 4)

Show all issues

Just to check, is \\x1f right, or would \x1f be right?

brennie Aug. 30, 2018, 8:11 p.m.

\ is correct. We are giving the string "\x1f" (instead of the character 0x1F) to Hg. Ill switch it to a raw string.

rbtools/clients/mercurial.py (Diff revision 4)

Show all issues

We use these special markers in several places. Let's make them constants so it's readable and consistent.

brennie Aug. 30, 2018, 8:11 p.m.

We actually use \x1f (the character) and \\x1f (a string which contains a python escape sequence).

Change Summary:

addressed feedback

Commit:

-	f8cad25d8e5d065c87f4609dba1becb601b3fb00
+	b9538d038a1ccab87978e6f62e6169a3f22e2f9d

Diff:

Revision 5 (+70 -1)

Show changes

rbtools/clients/mercurial.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/clients/mercurial.py (Diff revision 5)

Show all issues

This was reordered but wasn't put in alphabetical order.

rbtools/clients/mercurial.py (Diff revision 5)

Show all issues

I think the function name changed in a parent change. Can you retest with that?

brennie Sept. 4, 2018, 12:30 p.m.

it did but the call sites didnt change. Ill fix that up.

rbtools/clients/mercurial.py (Diff revision 5)

Show all issues

Must be the full path: rbtools.clients.errors.SCMError.

rbtools/clients/mercurial.py (Diff revision 5)

Show all issues

It'd be nice to have these as constants like with Git, to help with readability.

Change Summary:

addressed feedback

Commit:

-	b9538d038a1ccab87978e6f62e6169a3f22e2f9d
+	9ef4b914a78636778f6ea6d1a1a912655d16798c

Diff:

Revision 6 (+91 -2)

Show changes

rbtools/clients/mercurial.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

Is there any reason why we couldn't just do if entry['parent2'] != self.NO_PARENT? We don't use it elsewhere, and it seems like it's always going to be present (pop will throw an exception by default if not).

brennie Sept. 25, 2018, 3:34 p.m.

It will always be present, but we do not want it present in the history entry.

david Sept. 25, 2018, 3:56 p.m.
```
Can you add a comment explaining that?
```

Change Summary:

addressed feedback

Commit:

-	9ef4b914a78636778f6ea6d1a1a912655d16798c
+	9b3cdd22774f16cfc676cec183eef9d893428b43

Diff:

Revision 7 (+92 -2)

Show changes

rbtools/clients/mercurial.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

Change Summary:


Post with author date
Do not include base commit (hg log base..tip includes base, which we didn't want)
Add with_parent_diff to MercurialClient.diff
Fix KeyError for parent vs parent_id
Do not attempt to parse an empty log entry (because each entry is suffixed with the record sep)