Interesting! I checked whether the commit position of the final patch on Gerrit and that of the corresponding commit in Chromium differ and they're the same. Maybe the API call is returning a different commit position? In-progress CLs don't show a commit position anywhere in the UI, so maybe the Gerrit API is returning the commit position of ToT+1 (ToT with the patch applied as one commit).

Cr-Commit-Position is used in webkitpy/w3c/wpt_github.py. Are there bugs that could happen if the number is wrong and doesn't match what landed in Chromium?

In WPTGitHub, there is a pr_with_position method, which is used as a backup for trying to get the corresponding PR for a commit if someone commits via Rietveld.

Recently (in https://chromium-review.googlesource.com/c/558200/) I tweaked it so that commit position is only used as a backup if Change Id is not available, and added a TODO to simplify it after full Gerrit migration.

One possible change we could make here would be to change GerritCL.latest_commit_message_with_footers so that it strips commit position from the footers if the CL is still open.

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py?l=94

Now that this isn't causing any (noticeable) errors anymore, do you think scrubbing false Commit-Positions would be worthwhile?

What do you mean by scrubbing in this case? Removing Commit-Position footers from PR bodies?

Sorry, yes

Yep, sounds OK to me :-) I think removing the commit positions (or always updating the PR after the CL is committed so that the latest commit position is used) would prevent confusion in the future.

Cr-Commit-Position is also used by most_recent_chromium_commit in local_wpt, which is only called by TestExporter.export_first_exportable_commit, which seems to be a dangling method not used anywhere and invokes exportable_commits_over_last_n_commits in a wrong (outdated?) way. All of these could be removed altogether if no longer in use.

I will go ahead and remove the commit positions along with that unused method I found.

By the way,

> In-progress CLs don't show a commit position anywhere in the UI, so maybe the Gerrit API is returning the commit position of ToT+1

I can confirm this is true according to my experiment, didn't find docs though.

Anyway, it is good idea to get rid of incorrect information along with the out-of-date fallback mechanism for Rietveld altogether to minimize the chance of surprise.

Noticed this today in https://github.com/w3c/web-platform-tests/pull/6686

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/041a5e276880155037326ea61f66c6f663f96551

commit 041a5e276880155037326ea61f66c6f663f96551
Author: Robert Ma <robertma@chromium.org>
Date: Fri Aug 04 14:28:07 2017

Strip Cr-Commit-Position from PRs along with all the usage

Cr-{Original-}Commit-Position footers are now stripped from the commit
message before being sent to GitHub. The motivations are:
1. We have fully migrated to Gerrit and can rely on Change ID.
2. Commit positions of in-process CLs are not reliable, leading to
confusion and other issues (e.g.  crbug.com/748876 ).

All the usages of commit positions are removed as well, including
most_recent_chromium_commit and export_first_exportable_commit that are
no longer in use.

Since change ID is the only way to look up corresponding PR now,
WPTGitHub.pr_for_chromium_commit is removed and all previous users now
interact with pr_with_change_id directly.

Bug:  737178 ,  748876 
Change-Id: I32b395c069ddb66ed1ff2412d2cbe2572e7678c4
Reviewed-on: https://chromium-review.googlesource.com/598119
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492024}
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py
[modify] https://crrev.com/041a5e276880155037326ea61f66c6f663f96551/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f1d3ec4530580dabed2db5b681b74005b23236c

commit 8f1d3ec4530580dabed2db5b681b74005b23236c
Author: Robert Ma <robertma@chromium.org>
Date: Fri Aug 04 19:52:10 2017

Revert "Strip Cr-Commit-Position from PRs along with all the usage"

This reverts commit 041a5e276880155037326ea61f66c6f663f96551.

Reason for revert: Rietvield has not been completely deprecated. Existing CLs can still be landed via Rietveld, so we need to keep the fallback mechanism a little longer.

Original change's description:
> Strip Cr-Commit-Position from PRs along with all the usage
> 
> Cr-{Original-}Commit-Position footers are now stripped from the commit
> message before being sent to GitHub. The motivations are:
> 1. We have fully migrated to Gerrit and can rely on Change ID.
> 2. Commit positions of in-process CLs are not reliable, leading to
> confusion and other issues (e.g.  crbug.com/748876 ).
> 
> All the usages of commit positions are removed as well, including
> most_recent_chromium_commit and export_first_exportable_commit that are
> no longer in use.
> 
> Since change ID is the only way to look up corresponding PR now,
> WPTGitHub.pr_for_chromium_commit is removed and all previous users now
> interact with pr_with_change_id directly.
> 
> Bug:  737178 ,  748876 
> Change-Id: I32b395c069ddb66ed1ff2412d2cbe2572e7678c4
> Reviewed-on: https://chromium-review.googlesource.com/598119
> Commit-Queue: Robert Ma <robertma@chromium.org>
> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
> Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#492024}

TBR=qyearsley@chromium.org,jeffcarp@chromium.org,robertma@chromium.org

Change-Id: If92c76683417d795e82f68e1f46ba25210577a86
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  737178 ,  748876 
Reviewed-on: https://chromium-review.googlesource.com/602787
Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492106}
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_mock.py
[modify] https://crrev.com/8f1d3ec4530580dabed2db5b681b74005b23236c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py

Re-opening this bug because I found out we were under the wrong assumption. Rietveld is not yet readonly, and pending CLs on Rietveld can still be landed through there. It's just that new CLs cannot be created on Rietveld.

Therefore, the patch needs to be reverted and reworked to keep the fallback mechanism around. Commit positions from Gerrit (provisional CLs) are still to be stripped. Expecting this to be a small modification and the overall approach remains the same.

Sorry for the hassle.

Here's an example exportable CL on Rietveld: https://codereview.chromium.org/2973963003 (which was already exported)

I checked the PRs created today after the patch was landed, and also https://chromium.googlesource.com/chromium/src/+log/master/third_party/WebKit/LayoutTests/external/wpt. There is no exportable commits landed from Rietveld today, so we are good.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78df37d1618c8db03c47d950b355fb3e3fa9a384

commit 78df37d1618c8db03c47d950b355fb3e3fa9a384
Author: Robert Ma <robertma@chromium.org>
Date: Tue Aug 08 19:20:52 2017

Strip Cr-Commit-Position from provisional PRs

Cr-{Original-}Commit-Position footers are now stripped from the
description of provisional CLs (those created from Gerrit in-flight CLs).
The motivations are:
1. All Gerrit CLs have change IDs, which act sufficiently as unique IDs.
2. Commit positions of in-flight CLs are not reliable (ToT+1), leading to
confusion and other issues (e.g.  crbug.com/748876 ).

Also remove most_recent_chromium_commit and export_first_exportable_commit
that are no longer in use.

This is a rework of I32b395c069ddb66ed1ff2412d2cbe2572e7678c4 which was
reverted in If92c76683417d795e82f68e1f46ba25210577a86 due to some issues.


Bug:  737178 ,  748876 
Change-Id: I33588ef09b3bd6d8a22870eaf222a2f17080d4a6
Reviewed-on: https://chromium-review.googlesource.com/602617
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492723}
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/common/system/executive_mock.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py
[modify] https://crrev.com/78df37d1618c8db03c47d950b355fb3e3fa9a384/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github_unittest.py