Re: [chromium-dev] blink-reviews mailing list

[John Abd-El-Malek]

+blink-dev

I'm seeing that changes from a "fetch blink" checkout aren't cc'ing chromium-reviews for changes that contain files outside third_party/WebKit. I'm guessing the opposite is happening as well: changes to third_party/WebKit from "fetch chromium" checkouts won't cc blink-reviews.

It seems like we need to use one list only to solve these problems.

On Sat, Oct 3, 2015 at 10:54 AM, Thiago Farina <tfa...@chromium.org> wrote:

Hi,

Will we keep blink-reviews mailing list? I'm subscribed to it and have a filter that when an email is sent to it it goes to my label and is archived (removed from my INBOX).

But now that third_party/WebKit was merged into chromium, it is crossing with chromium-reviews mailing list and it is bloating my cr-reviews label.

Is there a way to keep them separated or either stop sending mails to blink-reviews? That way my cr-reviews filter/label will keep working as before since I'm not subscribed to chromium-reviews and just receive emails from the paths I'm subscribed from WATCHLISTS.

Thanks,

--

Thiago Farina

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Paweł Hajdan, Jr.]

+infra-dev

[Aaron Gable]

Whether the checkout was gotten via "fetch chromium" or "fetch blink" doesn't matter. They create completely identical checkouts now (and nearly identical always) with no different codereview configuration.

The difference is in the codereview.settings. "git cl upload" searches for a codereview.settings file, and uses the first one that it finds walking up the directory tree. Unfortunately, it starts searching in the current directory. So if you upload a blink+chromium CL from src/, it will cc chromium-reviews. If you upload a blink+chromium CL from third_party/WebKit, it will cc blink-reviews.

This is pretty obviously undesired behavior.

I see two possible fixes:

1) Delete third_party/WebKit's codereview.settings file, so that all CLs use the Chromium one. All reviews go to chromium-reviews, all CLs have the "chromium" project tag, etc.

2) Change git-cl to search for a codereview.settings file only in a directory which is a parent of all changed files. 

I honestly think that (1) is the better fix here for a couple reasons: It is way easier, and the blink codereview.settings contains nonsense information anyway (like broken svn view-vc urls).

Aaron

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPb8HMWUkzZ8Y%2BN28LzFN69r3vQVz5GkEdUrH6RS%3DOFhng%40mail.gmail.com.

[Dirk Pranke]

On Mon, Oct 5, 2015 at 10:09 AM, Aaron Gable <aga...@chromium.org> wrote:

Whether the checkout was gotten via "fetch chromium" or "fetch blink" doesn't matter. They create completely identical checkouts now (and nearly identical always) with no different codereview configuration.

The difference is in the codereview.settings. "git cl upload" searches for a codereview.settings file, and uses the first one that it finds walking up the directory tree. Unfortunately, it starts searching in the current directory. So if you upload a blink+chromium CL from src/, it will cc chromium-reviews. If you upload a blink+chromium CL from third_party/WebKit, it will cc blink-reviews.

This is pretty obviously undesired behavior.

I see two possible fixes:

1) Delete third_party/WebKit's codereview.settings file, so that all CLs use the Chromium one. All reviews go to chromium-reviews, all CLs have the "chromium" project tag, etc.

2) Change git-cl to search for a codereview.settings file only in a directory which is a parent of all changed files. 

I honestly think that (1) is the better fix here for a couple reasons: It is way easier, and the blink codereview.settings contains nonsense information anyway (like broken svn view-vc urls).

I think we should probably just delete third_party/WebKit/codereview.settings as well. Having two different review lists will become increasingly nonsensical.

-- Dirk

 

[Thiago Farina]

If nobody disagree about removing third_party/WebKit/codereview.settings, I'm happy to send a patch for it.

--

Thiago Farina

[Chris Harrelson]

+1

 

--

Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Thiago Farina]

I have uploaded the CL here -> https://codereview.chromium.org/1387883004. I'm happy to land it tomorrow or another day if there are no complains about it.

--

Thiago Farina

[Thiago Farina]

On Mon, Oct 5, 2015 at 8:03 PM, Thiago Farina <tfa...@chromium.org> wrote:

I have uploaded the CL here -> https://codereview.chromium.org/1387883004. I'm happy to land it tomorrow or another day if there are no complains about it.

The CL has landed, but somehow patches are still being sent to both blink-reviews and chromium-reviews.

Is there something else I can look at to stop this?

--

Thiago Farina

[Colin Blundell]

(1) Those could be CLs that were uploaded from local checkouts that haven't brought in your change (or CLs that were uploaded before your change even landed); IIUC, that setting is looked at as part of initial upload.

(2) It looks like there's at least one WATCHLIST that adds blink-reviews@.

[Thiago Farina]

On Mon, Oct 12, 2015 at 4:18 AM, Colin Blundell <blun...@chromium.org> wrote:

(2) It looks like there's at least one WATCHLIST that adds blink-reviews@.

 

Looks like this is what is causing it.

https://chromium.googlesource.com/chromium/src/+/master/WATCHLISTS#867

--

Thiago Farina

[Thiago Farina]

Would be OK if I remove this watchlist? Otherwise every CL that changes third_party/WebKit will copy blink-reviews, but now every CL goes to chromium-reviews anyway.

Timothy, would you be fine with that? Looks like you added that in https://chromium.googlesource.com/chromium/src/+/d035d57d4feaa4665e4914883ec32d9b2fb32158

--

Thiago Farina

[Timothy Loh]

I find the blink-reviews list useful since I'm interested in changes to Blink but not so much to Chromium. Is the watchlist entry causing any problems?

[Thiago Farina]

On Monday, October 12, 2015, Timothy Loh <tim...@chromium.org> wrote:

I find the blink-reviews list useful since I'm interested in changes to Blink but not so much to Chromium. Is the watchlist entry causing any problems?

Shouldn't you add your email to the watchlist entry if you are interested to changes in third_party/WebKit instead of the review mailing list? What is this causing now, is that I'm subscribed to blink-reviews and now I'm getting blink reviews in my cr-reviews CL.

The workaround for me now might be to unsubscribed from it, but wouldn't like if I don't need.

--

Thiago Farina

[Thiago Farina]

On Tuesday, October 13, 2015, Thiago Farina <tfa...@chromium.org> wrote:

On Monday, October 12, 2015, Timothy Loh <tim...@chromium.org> wrote:

I find the blink-reviews list useful since I'm interested in changes to Blink but not so much to Chromium. Is the watchlist entry causing any problems?

Shouldn't you add your email to the watchlist entry if you are interested to changes in third_party/WebKit instead of the review mailing list? What is this causing now, is that I'm subscribed to blink-reviews and now I'm getting blink reviews in my cr-reviews CL.

s/CL/label.

 

--

Thiago Farina

[Douglas Stockwell]

Please don't break blink-reviews. Seems like unsubscribing from blink-reviews would solve your problem.

We shouldn't need to update watchlists for individual subscriptions. Most blink watchlists entries are mapped to individual mailing lists now.

- Doug

[Nico Weber]

There are a few more team-specific mailing lists in watchlists (perf-dashboard-reviews, tracing-review, etc: https://code.google.com/p/chromium/codesearch#search/&q=%5C-reviews%20file:watchlist&sq=package:chromium&type=cs). I don't think there's anything wrong with that, as long as all patches go to chromium-reviews (which they do now).