New issue
Advanced search Search tips

Issue 679951 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocked on:
issue 685326

Blocking:
issue 707006



Sign in to add a comment

[WPT Export] Handle reverted changes

Project Member Reported by jeffcarp@chromium.org, Jan 11 2017

Issue description

- [x] Create a tiny change in external/wpt
  - https://codereview.chromium.org/2651533003
- [x] Revert it
  - https://codereview.chromium.org/2657613002
- [ ] Run script and make necessary fixes
 
Strategy:

1. Create an insignificant change (e.g. whitespace) in external/wpt
2. Revert it
3. Make necessary fixes to the script in order to handle this case
  - This might involve making two passes on the list of exportable commits
Description: Show this description
Labels: -Pri-3 Pri-2
Description: Show this description
Project Member

Comment 5 by bugdroid1@chromium.org, Jan 24 2017

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

commit b156b16a00574a89d85ccb034b30cdfd9cc5676a
Author: jeffcarp <jeffcarp@chromium.org>
Date: Tue Jan 24 04:54:57 2017

Create a change in external/wpt meant to be reverted

This is to test reverted changes in the WPT Export process.

BUG= 679951 

Review-Url: https://codereview.chromium.org/2651533003
Cr-Commit-Position: refs/heads/master@{#445651}

[modify] https://crrev.com/b156b16a00574a89d85ccb034b30cdfd9cc5676a/third_party/WebKit/LayoutTests/external/wpt/html/semantics/forms/the-input-element/checkbox.html

Project Member

Comment 6 by bugdroid1@chromium.org, Jan 24 2017

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

commit 0702925bc3519350a1f578c40de225e45da52717
Author: jeffcarp <jeffcarp@chromium.org>
Date: Tue Jan 24 18:19:10 2017

Revert of Create a change in external/wpt meant to be reverted (patchset #1 id:1 of https://codereview.chromium.org/2651533003/ )

Reason for revert:
This commit was intended to be reverted to test how WPT Export handles reverted changes. See the bug for more context.

Original issue's description:
> Create a change in external/wpt meant to be reverted
>
> This is to test reverted changes in the WPT Export process.
>
> BUG= 679951 
>
> Review-Url: https://codereview.chromium.org/2651533003
> Cr-Commit-Position: refs/heads/master@{#445651}
> Committed: https://chromium.googlesource.com/chromium/src/+/b156b16a00574a89d85ccb034b30cdfd9cc5676a

TBR=qyearsley@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 679951 

Review-Url: https://codereview.chromium.org/2657613002
Cr-Commit-Position: refs/heads/master@{#445762}

[modify] https://crrev.com/0702925bc3519350a1f578c40de225e45da52717/third_party/WebKit/LayoutTests/external/wpt/html/semantics/forms/the-input-element/checkbox.html

Quinten, I'm looking for input on how to deal with reverted changes. I have two ideas so far, both of which involve doing two passes on the list of exportable commits:

1. Look to see if any commit in exportable_commits has a revert commit later on
2. Diff each commit against chromium ToT, if it doesn't make a patch then that commit has been reverted
Description: Show this description
I was thinking as I was working on this - what if we just didn't "handle" reverted changes? Both the original commit and the revert commit would go through the export process normally, resulting in a little bit more noise in the IRC channel. This would eliminate a good amount of complexity added by revert checking. And also, if a commit has already been exported, then we'd need to export the revert commit anyway, so this feature would only catch reverted commits within a ~20-30 minute window.

I think that the tradeoff leans toward not building this feature to ignore reverted commits in this way, but I'm open to feedback.

What's more worrisome for me is how to deal with conflicts upstream. I'll create a tracking bug for that now.
Yeah, I agree -- the simpler the better, and even if it were fairly common for people to commit and then revert right afterwards, it probably wouldn't be extremely troublesome or unexpected for both the original commit and then revert to go through the export pipeline.

Maybe we'll put this as a P3, a potential feature to consider at some point in the future?

Re #7: The way I was imagining this might be done is #1 -- if, when the export script is run, there is both a commit and its revert commit, that are both exportable and not exported, then they cancel each other out, and neither are exported.
Labels: -Type-Bug -Pri-2 Pri-3 Type-Feature
Blockedon: 685326
Blocking: 707006
Blocking: -657117
Status: WontFix (was: Assigned)
In the 198 export commits we've seen about 4 reverts go through so far:
https://github.com/w3c/web-platform-tests/pulls?utf8=%E2%9C%93&q=is%3Apr%20label%3Achromium-export%20%22Revert%20of%22%20in%3Atitle%20

My opinion is that this is an optimization that we can do if people start complaining about revert commits being PR'd upstream, in which case we can re-open this bug.
Components: Blink>Infra>Ecosystem
Components: -Blink>Infra>Predictability

Sign in to add a comment