Reapply https://codereview.chromium.org/1257233006/ with conditional eliding.

Reapply https://codereview.chromium.org/1257233006/ with conditional eliding.

- Create logs URL for both gitiles and github.
- Include the number of commits being rolled in in the commit subject, which is
  always useful.
- Make the log inclusion conditional on the repository being rolled in and the
  number of commits.
- Checkout the new commit when using this script, otherwise this is surprising
  to users.
- Add flags to support more use cases.
- Use --quiet when calling git to reduce noise in the output.

R=spang@chromium.org,jochen@chromium.org
BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296924

[M-A Ruel, 2015-09-23 17:52:12]

Patchset #1 (id:1) has been deleted

[M-A Ruel, 2015-09-23 17:58:43]

Patchset #1 (id:20001) has been deleted

[M-A Ruel, 2015-09-23 18:01:30]

Patchset #1 (id:40001) has been deleted

[M-A Ruel, 2015-09-23 18:02:14]

maruel@chromium.org changed reviewers:
+ jochen@chromium.org
- eisinger@chromium.org

[M-A Ruel, 2015-09-23 18:03:00]

I think this change captures the need of everybody here. I tested locally with
src/third_party/catapult and src/v8 and with another report not in the
blacklist.

[spang, 2015-09-23 18:36:46]

On 2015/09/23 18:03:00, M-A Ruel wrote:
> I think this change captures the need of everybody here. I tested locally with
> src/third_party/catapult and src/v8 and with another report not in the
> blacklist.

My ideal is definitely the concise description with just a link, and that we
should adopt that everywhere as a best practice. But it's obvious you find the
logs useful so, thanks for trying to find some middle ground.

For concrete feedback, I think you could probably remove the project-based
exceptions, since most of them have their own tools anyway - having exceptions
here doesn't change anything.

I would suggest a lower threshold, to like 20 commits. I am very skeptical
anyone will read even 50 commit titles in a gardening commit, 100 was an extreme
example to illustrate that this can get really spammy.

[M-A Ruel, 2015-09-23 18:50:11]

On 2015/09/23 18:36:46, spang wrote:
> On 2015/09/23 18:03:00, M-A Ruel wrote:
> > I think this change captures the need of everybody here. I tested locally
with
> > src/third_party/catapult and src/v8 and with another report not in the
> > blacklist.
> 
> My ideal is definitely the concise description with just a link, and that we
> should adopt that everywhere as a best practice. But it's obvious you find the
> logs useful so, thanks for trying to find some middle ground.
> 
> For concrete feedback, I think you could probably remove the project-based
> exceptions, since most of them have their own tools anyway - having exceptions
> here doesn't change anything.

Most of them call roll-dep or roll-dep-svn in a way or another. So I added a
flag in patchset #2 and once/if the tooling starts using the flag, the blacklist
can be removed.

As a counter example, libvpx last roll includes the logs and Johann did it
manually. Same for recent NaCl roll by Mark and buildtools roll by Dirk. This is
clearly not a "skia-only" thing.


> I would suggest a lower threshold, to like 20 commits. I am very skeptical
> anyone will read even 50 commit titles in a gardening commit, 100 was an
extreme
> example to illustrate that this can get really spammy.

I disagree on that point, I do check this out, as I'm mainly a command line
user, so clicking on a link is a dereference forcing me to jump from my console
to my web browser. But if we keep the blacklist, the arbitrary limit is
irrelevant.

[chromium-reviews, 2015-09-23 18:58:20]

On Wed, Sep 23, 2015 at 11:50 AM,  <maruel@chromium.org> wrote:
> On 2015/09/23 18:36:46, spang wrote:
>>
>> On 2015/09/23 18:03:00, M-A Ruel wrote:
>> > I think this change captures the need of everybody here. I tested
>> > locally with
>> > src/third_party/catapult and src/v8 and with another report not in the
>> > blacklist.
>
>
>> My ideal is definitely the concise description with just a link, and that
>> we
>> should adopt that everywhere as a best practice. But it's obvious you find
>> the
>> logs useful so, thanks for trying to find some middle ground.
>
>
>> For concrete feedback, I think you could probably remove the project-based
>> exceptions, since most of them have their own tools anyway - having
>> exceptions
>> here doesn't change anything.
>
>
> Most of them call roll-dep or roll-dep-svn in a way or another. So I added a
> flag in patchset #2 and once/if the tooling starts using the flag, the
> blacklist
> can be removed.
>
> As a counter example, libvpx last roll includes the logs and Johann did it
> manually. Same for recent NaCl roll by Mark and buildtools roll by Dirk.
> This is
> clearly not a "skia-only" thing.

Probably missing a bunch of context but we used to do this automatically-ish:
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx_ne...

That procedure is changing, but I'm hoping to keep something similar.
Maybe I should be using this roll_dep.py script directly?

>> I would suggest a lower threshold, to like 20 commits. I am very skeptical
>> anyone will read even 50 commit titles in a gardening commit, 100 was an
>
> extreme
>>
>> example to illustrate that this can get really spammy.

I cut it off and 20 and added <...> if there were at least 20 found.

> I disagree on that point, I do check this out, as I'm mainly a command line
> user, so clicking on a link is a dereference forcing me to jump from my
> console
> to my web browser. But if we keep the blacklist, the arbitrary limit is
> irrelevant.
>
> https://codereview.chromium.org/1366493003/

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

[spang, 2015-09-23 18:58:32]

On 2015/09/23 18:50:11, M-A Ruel wrote:
> On 2015/09/23 18:36:46, spang wrote:
> > On 2015/09/23 18:03:00, M-A Ruel wrote:
> > > I think this change captures the need of everybody here. I tested locally
> with
> > > src/third_party/catapult and src/v8 and with another report not in the
> > > blacklist.
> > 
> > My ideal is definitely the concise description with just a link, and that we
> > should adopt that everywhere as a best practice. But it's obvious you find
the
> > logs useful so, thanks for trying to find some middle ground.
> > 
> > For concrete feedback, I think you could probably remove the project-based
> > exceptions, since most of them have their own tools anyway - having
exceptions
> > here doesn't change anything.
> 
> Most of them call roll-dep or roll-dep-svn in a way or another. So I added a
> flag in patchset #2 and once/if the tooling starts using the flag, the
blacklist
> can be removed.
> 
> As a counter example, libvpx last roll includes the logs and Johann did it
> manually. Same for recent NaCl roll by Mark and buildtools roll by Dirk. This
is
> clearly not a "skia-only" thing.
> 

That roll was literally 148 commits long. It was manually abbreviated to exactly
20 with an ellipsis saying ther's tons more stuff. 

> 
> > I would suggest a lower threshold, to like 20 commits. I am very skeptical
> > anyone will read even 50 commit titles in a gardening commit, 100 was an
> extreme
> > example to illustrate that this can get really spammy.
> 
> I disagree on that point, I do check this out, as I'm mainly a command line
> user, so clicking on a link is a dereference forcing me to jump from my
console
> to my web browser. But if we keep the blacklist, the arbitrary limit is
> irrelevant.

Could we include the command line then?

In fact my patch put the rev-list args in the description, so it's directly
usable as an argument to "git log".

[M-A Ruel, 2015-09-23 19:16:42]

On 2015/09/23 18:58:20, Johann wrote:
> Probably missing a bunch of context but we used to do this automatically-ish:
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx_ne...
> 
> That procedure is changing, but I'm hoping to keep something similar.
> Maybe I should be using this roll_dep.py script directly?

Yes, that's the idea.


On 2015/09/23 18:58:32, spang wrote:
> That roll was literally 148 commits long. It was manually abbreviated to
exactly
> 20 with an ellipsis saying ther's tons more stuff. 

I added a new flag --log-limit to trim the list of commits, instead of using a
threshold to remove the list completely. This shouldn't be done manually either.

> Could we include the command line then?

You mean to trim or something else?

> In fact my patch put the rev-list args in the description, so it's directly
> usable as an argument to "git log".

Done.

[spang, 2015-09-23 19:48:35]

On 2015/09/23 19:16:42, M-A Ruel wrote:
> On 2015/09/23 18:58:20, Johann wrote:
> > Probably missing a bunch of context but we used to do this
automatically-ish:
> >
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx_ne...
> > 
> > That procedure is changing, but I'm hoping to keep something similar.
> > Maybe I should be using this roll_dep.py script directly?
> 
> Yes, that's the idea.
> 
> 
> On 2015/09/23 18:58:32, spang wrote:
> > That roll was literally 148 commits long. It was manually abbreviated to
> exactly
> > 20 with an ellipsis saying ther's tons more stuff. 
> 
> I added a new flag --log-limit to trim the list of commits, instead of using a
> threshold to remove the list completely. This shouldn't be done manually
either.

I still think the 100 cutoff is too high as the default, but I'm not getting
much transaction on that point so I'll take what I can get - lgtm.

> 
> > Could we include the command line then?
> 
> You mean to trim or something else?

I just meant the git log command, which is already included. It seems like
providing it covers the "I prefer the command line over a web browser" use case.

> 
> > In fact my patch put the rev-list args in the description, so it's directly
> > usable as an argument to "git log".
> 
> Done.

[M-A Ruel, 2015-09-23 23:50:02]

On 2015/09/23 19:48:35, spang wrote:
> On 2015/09/23 19:16:42, M-A Ruel wrote:
> > On 2015/09/23 18:58:20, Johann wrote:
> > > Probably missing a bunch of context but we used to do this
> automatically-ish:
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/libvpx_ne...
> > > 
> > > That procedure is changing, but I'm hoping to keep something similar.
> > > Maybe I should be using this roll_dep.py script directly?
> > 
> > Yes, that's the idea.
> > 
> > 
> > On 2015/09/23 18:58:32, spang wrote:
> > > That roll was literally 148 commits long. It was manually abbreviated to
> > exactly
> > > 20 with an ellipsis saying ther's tons more stuff. 
> > 
> > I added a new flag --log-limit to trim the list of commits, instead of using
a
> > threshold to remove the list completely. This shouldn't be done manually
> either.
> 
> I still think the 100 cutoff is too high as the default, but I'm not getting
> much transaction on that point so I'll take what I can get - lgtm.
> 
> > 
> > > Could we include the command line then?
> > 
> > You mean to trim or something else?
> 
> I just meant the git log command, which is already included. It seems like
> providing it covers the "I prefer the command line over a web browser" use
case.

Done.

I also made the fetch quiet.

Examples:
------

~/src/chromium/src> roll-dep src/v8
Found old revision 38c6221febba5b87d833ce563b06077629d208f4
Found new revision 7a7b692b30d50daacc1d054281f723d6c2255ccf
Commit message:
    Roll src/v8/ 38c6221fe..7a7b692b3 (34 commits).
    
    https://chromium.googlesource.com/v8/v8.git/+log/38c6221febba..7a7b692b30d5
    
    $ git log 38c6221fe..7a7b692b3 --date=short --format='%ad %ae %s'
    

You forgot to pass -r, make sure to insert a R=foo@example.com line
to the commit description before emailing.

Run:
  git cl upload --send-mail

-----

~/src/chromium/src> roll-dep src/tools/swarming_client                          
                                                                  
Found old revision f71d1a3fbe207d91bb2f0bd96c8821ff90bb7c28
Found new revision 6e5d2b21f0ac98396cd736097a985346feed1328
Commit message:
    Roll src/tools/swarming_client/ f71d1a3fb..6e5d2b21f (2 commits).
    
   
https://chromium.googlesource.com/external/swarming.client.git/+log/f71d1a3fb...
    
    $ git log f71d1a3fb..6e5d2b21f --date=short --format='%ad %ae %s'
    2015-09-23 maruel Transparently retry 404s when served from CloudEndpoints
and not as json.
    2015-09-21 maruel Recreate the work dir in run_isolated.py.
    

You forgot to pass -r, make sure to insert a R=foo@example.com line
to the commit description before emailing.

Run:
  git cl upload --send-mail

-----

[M-A Ruel, 2015-09-24 18:14:04]

Anyone has inputs on this?

[jochen (gone - plz use gerrit), 2015-09-25 11:10:42]

what about a test? :)

[M-A Ruel, 2015-09-25 20:00:11]

On 2015/09/25 11:10:42, jochen wrote:
> what about a test? :)

I always test this tool manually, which is also a reason why I didn't provide
much options in the first place and kept it very linear. Obviously it'd be
better to have it be tested automatically but I don't think it's worth the
trade-off atm.

[Johann, 2015-09-25 22:00:41]

On 2015/09/24 18:14:04, M-A Ruel wrote:
> Anyone has inputs on this?

No particular inputs on this change but:
"Currently this script will always roll to the tip of to origin/master."
makes it tricky for me to use this. I frequently (almost always) want something
other than origin/master for libvpx.

[M-A Ruel, 2015-09-28 17:24:05]

On 2015/09/25 22:00:41, Johann wrote:
> On 2015/09/24 18:14:04, M-A Ruel wrote:
> > Anyone has inputs on this?
> 
> No particular inputs on this change but:
> "Currently this script will always roll to the tip of to origin/master."
> makes it tricky for me to use this. I frequently (almost always) want
something
> other than origin/master for libvpx.

Added --roll-to for this use case in patchset #7.

[Johann, 2015-09-28 17:52:13]

On 2015/09/28 17:24:05, M-A Ruel wrote:
> On 2015/09/25 22:00:41, Johann wrote:
> > On 2015/09/24 18:14:04, M-A Ruel wrote:
> > > Anyone has inputs on this?
> > 
> > No particular inputs on this change but:
> > "Currently this script will always roll to the tip of to origin/master."
> > makes it tricky for me to use this. I frequently (almost always) want
> something
> > other than origin/master for libvpx.
> 
> Added --roll-to for this use case in patchset #7.

Thanks!

[jochen (gone - plz use gerrit), 2015-09-29 07:35:33]

lgtm

[M-A Ruel, 2015-09-29 12:25:08]

The CQ bit was checked by maruel@chromium.org

[M-A Ruel, 2015-09-29 12:25:08]

The patchset sent to the CQ was uploaded after l-g-t-m from spang@chromium.org
Link to the patchset: https://codereview.chromium.org/1366493003/#ps170001
(title: "Add --roll-to")

[commit-bot: I haz the power, 2015-09-29 12:25:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1366493003/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1366493003/170001

[commit-bot: I haz the power, 2015-09-29 12:27:26]

Committed patchset #7 (id:170001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296924