Summary

Use SCMTool IDs to look up repositories when possible.

Review Request #13973 — Created June 11, 2024 and submitted June 12, 2024, 7:33 a.m.

Information

Owner

david

Repository

RBTools

Branch

release-5.x

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This change fixes the RBTools side to not include the Power Pack SCMTool
names, which will avoid the problem for existing servers. For new
servers, we'll be shipping a server-side fix that fixes the crash, adds
the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs
rather than names to the repository list API. If we see the IDs in the
capability blob, we can assume that we can pass the IDs, including
potentially unknown IDs.

This also fixes a problem where we were sometimes accessing the
repository list API twice with exactly the same parameters.

Testing Done


Ran unit tests.
Verified that the repository list API was accessed using SCMTool names

  that did not include potentially missing ones when running against an

  older server.
Verified that the repository list API was accessed with SCMTool IDs

  when running against a server with the new API fixes.

Commits

Summary	ID
Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	69154c85a7d367a8c6389242b658387b79814d26

Summary

Use SCMTool IDs to look up repositories when possible.

Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.

69154c85a7d367a8c6389242b658387b79814d26

Issues

Description	From	Last Updated
Small typo in the description: "Clearcase" -> "ClearCase".	chipx86	June 11, 2024, 8:51 p.m.
'rbtools.api.capabilities.Capabilities' imported but unused Column: 5 Error code: F401	reviewbot	June 11, 2024, 4:34 p.m.
'rbtools.api.capabilities.Capabilities' imported but unused Column: 5 Error code: F401	reviewbot	June 11, 2024, 4:34 p.m.
line too long (80 > 79 characters) Column: 80 Error code: E501	reviewbot	June 11, 2024, 4:34 p.m.
:py:attr for server_tool_names.	chipx86	June 11, 2024, 8:55 p.m.
Can we take the opportunity to make this a list of strings? I know we're just passing this to the …	chipx86	June 11, 2024, 8:55 p.m.
These should be in the same import group. Also, Optional before TYPE_CHECKING.	chipx86	June 11, 2024, 9:13 p.m.
Missing a return type.	chipx86	June 11, 2024, 8:56 p.m.
Let's avoid _, since we're using this for gettext in places. Could just do [0] at the end.	chipx86	June 11, 2024, 8:56 p.m.
whitespace before ',' Column: 55 Error code: E203	reviewbot	June 11, 2024, 9:16 p.m.
I don't know where we landed on this, but if we're postponing any changes to this format, let's update this …	chipx86	June 11, 2024, 10:20 p.m.
Here and the other, TYPE_CHECKING sorts before Tuple.	chipx86	June 12, 2024, 7:29 a.m.
This should be two separate strings.	chipx86	June 11, 2024, 10:19 p.m.

flake8 failed.

JSHint passed.

flake8

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

The issue has been dropped. Show all issues

'rbtools.api.capabilities.Capabilities' imported but unused

Column: 5
Error code: F401

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

The issue has been dropped. Show all issues

'rbtools.api.capabilities.Capabilities' imported but unused

Column: 5
Error code: F401

rbtools/commands/setup_repo.py (Diff revision 1)
The issue has been dropped. Show all issues
```
line too long (80 > 79 characters)

Column: 80
Error code: E501
```

Commits:

	Summary	ID
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	0c315187bed7d11eb3072a00087bfc10f8104be5
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	26ee167ed435deb651643aff6d7d9465d2c2c7b9

Diff:

Revision 2 (+272 -60)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	5 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Small typo in the description: "Clearcase" -> "ClearCase".

rbtools/clients/base/scmclient.py (Diff revision 2)
The issue has been resolved. Show all issues
```
:py:attr for server_tool_names.
```

rbtools/clients/base/scmclient.py (Diff revision 2)

The issue has been resolved. Show all issues

Can we take the opportunity to make this a list of strings?

I know we're just passing this to the API, and saving a split/join, but that's pretty inexpensive, and feels like an implementation detail. It'd be nice to have a formal structured definition of what a list of IDs is, so this is more useful going forward outside this one use case.

rbtools/commands/setup_repo.py (Diff revision 2)

The issue has been resolved. Show all issues

These should be in the same import group.
Also, Optional before TYPE_CHECKING.

david June 11, 2024, 8:43 p.m.

One thing I've recently come across is that the standard sort order is supposed to be "CONSTANT_VARIABLE, CamelCaseClass, variable_or_function"

chipx86 June 11, 2024, 8:55 p.m.

Standard, or tool-defined preference?

I can understand the preference for that, but we haven't been doing that, and it requiress more micromanaging. I'd much prefer to be able to continue feeding imports into |sort and have it do the right thing, rather than having to move things around myself.

rbtools/commands/setup_repo.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a return type.
```

rbtools/utils/repository.py (Diff revision 2)

The issue has been resolved. Show all issues

Let's avoid _, since we're using this for gettext in places.

Could just do [0] at the end.

Description:

		Many versions of Review Board currently have a bug where sending an
		unknown value in the tool= parameter to the repository list API would
		cause a crash. We'll be shipping a fix for that, but unfortunately there
~		are a variety of released versions where the Git and Clearcase clients
	~	are a variety of released versions where the Git and ClearCase clients
		can trigger this crash on servers that do not have Power Pack installed.

		This change fixes the RBTools side to not include the Power Pack SCMTool
		names, which will avoid the problem for existing servers. For new
		servers, we'll be shipping a server-side fix that fixes the crash, adds
		the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs
		rather than names to the repository list API. If we see the IDs in the
		capability blob, we can assume that we can pass the IDs, including
		potentially unknown IDs.

		This also fixes a problem where we were sometimes accessing the
		repository list API twice with exactly the same parameters.

Commits:

	Summary	ID
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	26ee167ed435deb651643aff6d7d9465d2c2c7b9
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	9d34d25edd1b0f198fea3d0f52e09e63bb400e3d

Diff:

Revision 3 (+280 -68)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	5 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/clients/git.py (Diff revision 3)
The issue has been dropped. Show all issues
```
whitespace before ','

Column: 55
Error code: E203
```

Commits:

	Summary	ID
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	9d34d25edd1b0f198fea3d0f52e09e63bb400e3d
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	45823b2e84dd8fa8c731335b85c87b1514e6d9bf

Diff:

Revision 4 (+280 -68)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	5 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been resolved. Show all issues

I don't know where we landed on this, but if we're postponing any changes to this format, let's update this and the other imports in the change to be consistent with what we're doing today.

rbtools/clients/clearcase.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should be two separate strings.
```

Commits:

	Summary	ID
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	45823b2e84dd8fa8c731335b85c87b1514e6d9bf
	Use SCMTool IDs to look up repositories when possible. Many versions of Review Board currently have a bug where sending an unknown value in the tool= parameter to the repository list API would cause a crash. We'll be shipping a fix for that, but unfortunately there are a variety of released versions where the Git and Clearcase clients can trigger this crash on servers that do not have Power Pack installed. This change fixes the RBTools side to not include the Power Pack SCMTool names, which will avoid the problem for existing servers. For new servers, we'll be shipping a server-side fix that fixes the crash, adds the SCMTool IDs to the capabilities, and allows us to pass SCMTool IDs rather than names to the repository list API. If we see the IDs in the capability blob, we can assume that we can pass the IDs, including potentially unknown IDs. This also fixes a problem where we were sometimes accessing the repository list API twice with exactly the same parameters. Testing Done: - Ran unit tests. - Verified that the repository list API was accessed using SCMTool names that did not include potentially missing ones when running against an older server. - Verified that the repository list API was accessed with SCMTool IDs when running against a server with the new API fixes.	69154c85a7d367a8c6389242b658387b79814d26

Diff:

Revision 5 (+280 -68)

Show changes

	rbtools/clients/bazaar.py
	rbtools/clients/clearcase.py
	rbtools/clients/cvs.py
	rbtools/clients/git.py
	rbtools/clients/mercurial.py
	rbtools/clients/perforce.py
	rbtools/clients/plastic.py
	rbtools/clients/svn.py
	5 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

rbtools/clients/clearcase.py (Diff revisions 4 - 5)
The issue has been resolved. Show all issues
```
Here and the other, TYPE_CHECKING sorts before Tuple.
```

Status:: Completed
Change Summary:: Pushed to release-5.x (36efa07)