This method used to not log anything but I increased the verbosity since it's safer to sift through false positives than to miss an export whose patch didn't apply. IMO not generating any logs in this method would be dangerous.

However, in my experience I haven't seen any obvious way we could differentiate between acceptable patch failures and alertable failures.

I will take a look at the general logging and various messages from importer/exporter later, and see if there's anything we can do about this particular case.

In this particular case, the most obvious options that come to mind are:

 1. Make exportable_commits_over_last_n_commits take a flag that tells it whether to log
 2. Make exportable_commits_over_last_n_commits return a pair (exportable_commits, patch_failures) and make the caller responsible for logging.

The first one seems slightly simpler, but both seem OK to me, and maybe there's a nicer way to do it.

Oops please disregard my last comment, this is for the importer. IMHO I'd prefer Quinten's idea #2, so that by convention (usually) only the main methods are logging and methods on all the utility classes don't log.

So common.exportable_commits_over_last_n_commits is used by both the importer and exporter. Here are my understandings:

* The importer gets the list of exportable-but-not-exported patches to try to apply them on the local WPT repo. If a patch cannot be cleanly applied to local WPT, then it is considered unexportable and hence ignored by the importer, in which case the error logs can be safely ignored.
* The exporter, on the other hand, uses that method to find exportable commits and we would like to be cautious and log all errors during the process to avoid missing some patches unintentionally.

Is that correct? If yes, it sounds reasonable to me to omit the logs in the importer, as we would have the same logs in the exporter anyway.

On a side note, for this particular case (manually reverting a change that was not merged upstream), perhaps we could add "No-Export: true" in the description to prevent confusing the bots. Not sure if it could be done programmatically.

Yep, that's correct!

On the side-note about adding "No-Export": I feel like both checking for "^ Import .*" in the summary and checking for "No-Export" in the footers are both a little bit "fragile" -- checking for "Import" a little bit more-so. Also, it feels silly to have to explain to people that they can add "No-Export" to avoid confusing the exporter. Anyway, that's just a thought.

 Issue 750627  has been merged into this issue.

Attempting to fix this with the second approach proposed by qyearsley@, which I personally agree is a better practice generally, but found out the code might be a bit more convoluted than I'd like: https://chromium-review.googlesource.com/c/596769

See my comment at the bottom and tell me what you think.

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

commit ef41d2376547049d8f4cecb8f9fddb458e949dbc
Author: Robert Ma <robertma@chromium.org>
Date: Wed Aug 09 17:01:21 2017

Suppress logging when patches fail to apply in importer

Refactor log handling when a patch cannot be cleanly applied. All error
messages are now being passed back to high-level callers from utility
functions, leaving the callers to decide how to handle them: the
importer will ignore errors to avoid confusion in the log, and the
exporter will log them more explicitly for examination.

In addition, increase the test coverage regarding patch failures and
improve documentation.

Bug:  748192 
Change-Id: If6eae2f79f3426b3af97e28a4183c1987fac23ad
Reviewed-on: https://chromium-review.googlesource.com/596769
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493028}
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt_unittest.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/ef41d2376547049d8f4cecb8f9fddb458e949dbc/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py