Don't track SCM changes in rebaseline commands.

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
Committed: https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c
Cr-Commit-Position: refs/heads/master@{#424082}

[qyearsley, 2016-10-04 23:23:05]

Description was changed from

==========
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.

Dirk, Walter, do you know if there are any other disadvantages to this approach?

BUG=639410
==========

to

==========
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
==========

[qyearsley, 2016-10-04 23:23:09]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 23:23:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qyearsley, 2016-10-04 23:23:59]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org, wkorman@chromium.org

[qyearsley, 2016-10-04 23:24:00]

Dirk, Walter, can you think of any other disadvantages of trying to remove this
logic from the webkit-patch rebaseline commands?

[commit-bot: I haz the power, 2016-10-05 00:42:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 00:42:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[qyearsley, 2016-10-05 18:39:09]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 18:39:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 19:39:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 19:39:35]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2016-10-06 18:04:59]

The idea sounds good to me; I didn't realize --all had that effect. 

I haven't reviewed this in detail; wkorman@, can I defer that to you?

[wkorman, 2016-10-06 21:10:22]

lgtm

I support landing this and trying it out.

I like the idea, nice win on code cleanup. I'm not immediately sure whether eng
will be uncomfortable with this, but worst case we could revert the patch
(though resolving conflicts for that could be painful).

We could also check and make sure git repo state is clean/empty at start of
script, so that we know --all will only include files that we know were a result
of the script's work.

https://codereview.chromium.org/2397573002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2397573002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:120: def
add_all(self):
What if we allow passing a pathspec, which --all supports, and when we run the
command we only apply the --all to third_party/WebKit/LayoutTests to reduce
likelihood of incorporating files eng may have sitting around elsewhere?

[qyearsley, 2016-10-06 23:05:57]

Making sure there are no unstaged files at start of script sounds like a good
idea, will try that.

Also, I think I should add at least one test that checks the git staging
behavior.

https://codereview.chromium.org/2397573002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2397573002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:120: def
add_all(self):
On 2016/10/06 at 21:10:22, wkorman wrote:
> What if we allow passing a pathspec, which --all supports, and when we run the
command we only apply the --all to third_party/WebKit/LayoutTests to reduce
likelihood of incorporating files eng may have sitting around elsewhere?

Good idea, will add this :-)

https://codereview.chromium.org/2397573002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py
(right):

https://codereview.chromium.org/2397573002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/scm_mock.py:44:
pass
Note, I think it might be nice to actually make this add files to added_paths so
that this can be checked in tests. Will add this in next patchset.

[qyearsley, 2016-10-07 20:20:28]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 20:20:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qyearsley, 2016-10-07 20:24:15]

The CQ bit was unchecked by qyearsley@chromium.org

[qyearsley, 2016-10-07 20:33:25]

Update: This CL now makes it so that:
 - Before starting rebaselining, we check whether there are uncommitted changes
in the layout tests directory, and abort if that's the case.
 - After rebaselining, we add all unstaged changes, but this is limited to the
layout tests directory.

Testing the adding behavior is not really straightforward, since in the tests
for the rebaseline commands nothing is really downloaded or changed in the
MockFileSystem; we just make a bunch of calls to webkit-patch
rebaseline-test-internal etc. on the MockExecutive, which don't actually do
anything.

To be more confident that nothing will go terribly wrong, I'd like to try this
out locally then commit.

[wkorman, 2016-10-07 21:20:26]

lgtm

[qyearsley, 2016-10-07 21:39:37]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-07 21:40:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-07 22:28:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 22:28:33]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-10-08 17:24:27]

Tried it out, didn't discover any problems, now trying to commit

[qyearsley, 2016-10-08 17:24:30]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-10-08 17:24:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-08 17:30:00]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-08 17:32:21]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c
Cr-Commit-Position: refs/heads/master@{#424082}
==========

[commit-bot: I haz the power, 2016-10-08 17:32:22]

Patchset 5 (id:??) landed as
https://crrev.com/ccd977fa5650bf395c59bbbb0afc81f5dbd3531c
Cr-Commit-Position: refs/heads/master@{#424082}