New issue
Advanced search Search tips

Issue 639410 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 474273



Sign in to add a comment

Rebaseline via webkit-patch produces confusing git changes

Project Member Reported by schenney@chromium.org, Aug 19 2016

Issue description

After running
  webkit-patch rebaseline-cl fast/borders/border-antialiasing.html

my git status reports this set of changes

# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	copied:     third_party/WebKit/LayoutTests/platform/linux/fast/borders/border-antialiasing-expected.png -> third_party/WebKit/LayoutTests/platform/android/fast/borders/border-antialiasing-expected.png
#	new file:   third_party/WebKit/LayoutTests/platform/linux-precise/fast/borders/border-antialiasing-expected.png
#	modified:   third_party/WebKit/LayoutTests/platform/linux/fast/borders/border-antialiasing-expected.png
#	new file:   third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/borders/border-antialiasing-expected.png
#	new file:   third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/borders/border-antialiasing-expected.png
#	new file:   third_party/WebKit/LayoutTests/platform/mac-retina/fast/borders/border-antialiasing-expected.png
#	modified:   third_party/WebKit/LayoutTests/platform/mac/fast/borders/border-antialiasing-expected.png
#	modified:   third_party/WebKit/LayoutTests/platform/win/fast/borders/border-antialiasing-expected.png
#	new file:   third_party/WebKit/LayoutTests/platform/win7/fast/borders/border-antialiasing-expected.png
#
# Changes not staged for commit:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#	deleted:    third_party/WebKit/LayoutTests/platform/linux-precise/fast/borders/border-antialiasing-expected.png
#	deleted:    third_party/WebKit/LayoutTests/platform/mac-mac10.10/fast/borders/border-antialiasing-expected.png
#	deleted:    third_party/WebKit/LayoutTests/platform/mac-mac10.9/fast/borders/border-antialiasing-expected.png
#	deleted:    third_party/WebKit/LayoutTests/platform/mac-retina/fast/borders/border-antialiasing-expected.png
#	deleted:    third_party/WebKit/LayoutTests/platform/win7/fast/borders/border-antialiasing-expected.png

I find this very confusing. There are files that are new in the "Changes to be committed" while also "deleted" in "Changes not staged for commit". Not being a git expert I don't know what will happen if I commit this change.

I presume the confusion arises because the tool deletes then adds the files, rather than just leaving them modified in place.

 
Owner: qyears...@chromium.org
Status: Assigned (was: Untriaged)
Ah, good point!

What webkit-patch rebaseline-cl does there is run (in parallel for each builder):
  webkit-patch rebaseline-test-internal --suffixes png \
    --builder <builder> --build-number <build-number> \
    --test fast/borders/border-antialiasing.html

And then:
  webkit-patch optimize-baselines --no-modify-scm --suffixes png \
    fast/borders/border-antialiasing.html

My hypothesis is that it's now calling `git add ...` for each of the intermediary baselines that's added, and then when the baselines are de-duped, it's not doing anything like that.

Maybe if --no-modify-scm is used for the first set of commands, then the git status should make more sense.
Status: Started (was: Assigned)
Blocking: 474273
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 15 2016

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

commit 9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Sep 15 23:36:53 2016

Refactor the way that SCM changes are tracked and aggregated in rebaseline.py.

This is a preliminary refactoring CL which is intended to consolidate related logic into one place, and prepare for the real change that I want to make: That is, apply git changes only once in _rebaseline after all changes are made, to fix  http://crbug.com/639410 .

I want to make two follow-up changes after this:
 (1) Change _rebaseline to apply git changes only once after all commands are run.
 (2) Refactor and simplify ChangeSet, _serial_commands, and _run_in_parallel.

BUG= 639410 

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

[modify] https://crrev.com/9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/optimize_baselines.py
[modify] https://crrev.com/9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[modify] https://crrev.com/9dc06fa0fd9e3ffb68b4feeef5158ab54f2ee6b6/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 22 2016

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

commit 34fdc9df872c6ce13e6c3c546084435321d4e882
Author: qyearsley <qyearsley@chromium.org>
Date: Thu Sep 22 21:24:23 2016

In rebaseline.py, update SCM and expectations all at once after all commands.

This is meant to fix  http://crbug.com/639410  by making it so that if there are files that are both added and deleted (e.g. downloaded and then de-duped), they won't ever be staged since all changes would be aggregated and de-duped before running git add/rm.

BUG= 639410 

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

[modify] https://crrev.com/34fdc9df872c6ce13e6c3c546084435321d4e882/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/optimize_baselines_unittest.py
[modify] https://crrev.com/34fdc9df872c6ce13e6c3c546084435321d4e882/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[modify] https://crrev.com/34fdc9df872c6ce13e6c3c546084435321d4e882/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/34fdc9df872c6ce13e6c3c546084435321d4e882/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 28 2016

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

commit 48a6db917e5860b4798c5c16565bbc03304433d3
Author: qyearsley <qyearsley@chromium.org>
Date: Wed Sep 28 17:27:42 2016

Revert of In rebaseline.py, update SCM and expectations all at once after all commands. (patchset #2 id:20001 of https://codereview.chromium.org/2341643005/ )

Reason for revert:
Caused rebaseline-o-matic to fail: http://crrev.com/650944

Could be re-landed with a fix.

Original issue's description:
> In rebaseline.py, update SCM and expectations all at once after all commands.
>
> This is meant to fix  http://crbug.com/639410  by making it so that if there are files that are both added and deleted (e.g. downloaded and then de-duped), they won't ever be staged since all changes would be aggregated and de-duped before running git add/rm.
>
> BUG= 639410 
>
> Committed: https://crrev.com/34fdc9df872c6ce13e6c3c546084435321d4e882
> Cr-Commit-Position: refs/heads/master@{#420461}

TBR=dpranke@chromium.org,wkorman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 639410 

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

[modify] https://crrev.com/48a6db917e5860b4798c5c16565bbc03304433d3/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/optimize_baselines_unittest.py
[modify] https://crrev.com/48a6db917e5860b4798c5c16565bbc03304433d3/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[modify] https://crrev.com/48a6db917e5860b4798c5c16565bbc03304433d3/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/48a6db917e5860b4798c5c16565bbc03304433d3/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py

Update about why rebaseline was trying to add files that don't exist ( bug 650944 ): The overall process of what was happening was:

 1. Existing baselines may be copied by copy-existing-baselines-internal; this may add new files but not remove any.
 2. Baselines may be downloaded by rebaseline-test-internal. This may add new files or modify existing files, but not remove any. In this step and the previous one, the list of files to call git add for is growing.
 3. Finally, baselines may be moved or removed by optimize-baselines. If the file has not yet been added with git add, then it's just removed from the filesystem without recording the fact that it is removed, so the list of files to delete is not updated.

This way, as the copy-existing-baselines-internal, rebaseline-test-internal, and optimize-baselines are run, the list of files to "add to SCM" (i.e. arguments to git add) keeps growing, and the list of files to remove stays empty.

Then, at the end, the list of files to add includes all of the files that were temporarily added then removed.


One possible solution for this would be to make it so that in optimize-baselines, the list of files to remove is always updated, so that by the time all of the changes are made, the list of files to add should only include files that were added but never removed. This was what I originally intended to do.

But another approach would be: Assuming that we're only using git and we're not adding any files in the repo that we don't actually want to commit, we could just call `git add -A` after all of the rebaselining commands, and forget about tracking SCM changes altogether. This would be simpler and easier to understand.
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 8 2016

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

commit ccd977fa5650bf395c59bbbb0afc81f5dbd3531c
Author: qyearsley <qyearsley@chromium.org>
Date: Sat Oct 08 17:29:31 2016

Don't track SCM changes in rebaseline commands.

There's a fair amount of logic in the rebaseline-related command
which is all for the purpose of tracking which files to add and remove
to the source control management system.

However, since we're now only using git, one possible simpler route
is to just call `git add --all`.

Pros:
 - Much simpler, fewer chances for bugs in SCM change tracking logic.
Cons:
 - This would make rebaseline commands stage all new files from the user
   in the working tree even if they didn't intend to stage these files.
   One possible way to compensate for this would be to *not* call
   `git add --all` after all rebaselining, but instead call this seperately
   when (1) doing a w3c test auto-update or (2) doing a auto-rebaseline
   on rebaseline-o-matic.

This is an alternative to http://crrev.com/2396433004.

BUG= 639410 

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

[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/baselineoptimizer_unittest.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/analyze_baselines.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/auto_rebaseline_unittest.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/optimize_baselines.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/optimize_baselines_unittest.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py
[modify] https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c/third_party/WebKit/Tools/Scripts/webkitpy/w3c/update_w3c_test_expectations.py

Status: Fixed (was: Started)
I believe with that with the above change, this should be fixed now.

Sign in to add a comment