This is an archive of the discontinued LLVM Phabricator instance.

stable-merge-request.sh: Add a script for submitting merge requests via bugzilla
ClosedPublic

Authored by tstellar on Mar 13 2017, 11:22 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellar created this revision.Mar 13 2017, 11:22 AM
hans added inline comments.Mar 13 2017, 1:02 PM
utils/release/stable-merge-request.sh
1 ↗(On Diff #91597)

I might have gone for bash just to avoid the worry of whether this works in other shells.

245 ↗(On Diff #91597)

I'm a bit torn on using the Bugzilla for merge requests. On the one hand I can see the benefit of having an issue opened for each merge request to keep track of which ones are open and which ones are done. On the other hand, I don't like the lack of context and visibility. For a bug like "Please merge rXXX", I'll have to search my inbox to see what rXXX is anyway. And the author of rXXX might not be notified about the request. Also, the discussion on that bug does not get sent to any mailing list, so later searches for rXXX will not show the merge discussion.

Having said that, I think this script will help though.

A few ideas:

  1. It would be awesome if the subject of the bug could include the first line of the commit message, and the body of the bug could quote the full commit message. I think this should be farily easy to get with some svn invocation.
  1. Could the commit author be added as a cc on the bug or otherwise get a notification? I realize this might be tricky.. we might want to cc the release person though.

And while I've expressed some skepticism, I think one process is better than two, so maybe the script should simple be called merge-request.sh and be used for the major releases too.

tstellar added inline comments.Mar 13 2017, 4:13 PM
utils/release/stable-merge-request.sh
245 ↗(On Diff #91597)

For me, it's much easier to stay organized when the merge requests are in bugzilla, because there is a clear record of what has been done and what needs to be done. It also allows for other people to check in and easily see which requests are outstanding, without having to look through the mailing list archives. All the bugs have a link to the commit on phabricator, so that should make it easier to track down.

  1. I think adding the commit message is doable.
  1. Automatically adding the author as a cc is tricky because commit emails don't always match the email people use in bugzilla. I can add an option for to add cc's manually, but I think it would be better to do auto-detection as a follow on improvement.
tstellar updated this revision to Diff 92809.Mar 23 2017, 8:51 AM

Rename script to merge-request.sh and add commit summary to bug report.

hans edited edge metadata.Mar 24 2017, 2:34 AM

One thing I just realized: the bugzilla-cli package, at least for Ubuntu, seems only available in the most recent versions: http://packages.ubuntu.com/search?keywords=bugzilla-cli (similar for Debian: https://packages.debian.org/search?keywords=bugzilla-cli)

At least for my work and home machines, that makes it more impractical to use, and I'm guessing that might apply to many other developers.

There seems to be a tool called "bugz" which is more widely available: http://packages.ubuntu.com/search?keywords=bugz
Maybe the script should use that instead, or both?

utils/release/merge-request.sh
2 ↗(On Diff #92809)

nit: drop/change "Test the LLVM release candidates"

35 ↗(On Diff #92809)

What does "Revision project" mean?

This seems to refer to the bugzilla Product field. Do we really care about that? Maybe to simplify this, both for the script and for the user, we could just file these bugs against the "new-bugs" product.

44 ↗(On Diff #92809)

Since "-user" is not spelled "-user-email", I think this could be spelled simply "-assign-to".

224 ↗(On Diff #92809)

This could match other bugs too.. maybe the query should be narrower?

234 ↗(On Diff #92809)

Don't you need to provide the URL to the repository? This seems to assume it's run inside a checked out repository.

236 ↗(On Diff #92809)

I think if we include the svn url above, there is no need for this... it should then work even if the user doesn't have an svn repository checked out or updated past the target revision.

It does assume the user has 'svn' installed, but that's required for git-svn too.

251 ↗(On Diff #92809)

Personally, I prefer this to point to the viewvc, but it's not important.

255 ↗(On Diff #92809)

I assume this is just for your debugging?

tstellar marked 4 inline comments as done.Mar 24 2017, 3:37 AM
In D30905#709621, @hans wrote:

One thing I just realized: the bugzilla-cli package, at least for Ubuntu, seems only available in the most recent versions: http://packages.ubuntu.com/search?keywords=bugzilla-cli (similar for Debian: https://packages.debian.org/search?keywords=bugzilla-cli)

It is available on pypi, so you can install it using pip install. This is recommended anyway, since there is a bug with the older versions.

At least for my work and home machines, that makes it more impractical to use, and I'm guessing that might apply to many other developers.

There seems to be a tool called "bugz" which is more widely available: http://packages.ubuntu.com/search?keywords=bugz
Maybe the script should use that instead, or both?

I can't find the bugz package for fedora or on pypi, so it's going to be pretty hard for me to test / use that one.

utils/release/merge-request.sh
35 ↗(On Diff #92809)

This is optional, and does map to bugzilla Product field. The default is to use the new-bugs product, I thought having a project option would be useful if people wanted to be able to categorize their bugs, but I don't mind dropping this and the -component option if it helps simplify the script.

251 ↗(On Diff #92809)

Is there a way to get viewvc to show you the full diff with all changes on a single page?

255 ↗(On Diff #92809)

Yes, but I think it could be useful for users to verify they've entered the correct options.

tstellar updated this revision to Diff 92916.Mar 24 2017, 3:45 AM

Simplify query for revision commit message, plus some other minor cleanups.

hans added inline comments.Mar 24 2017, 4:00 AM
utils/release/merge-request.sh
241 ↗(On Diff #92916)

I think the URL should just be https://llvm.org/svn/llvm-project/
That way it will work for revisions to any module, not just llvm.

Also, I think the current command will print the *whole* log going back from $revision. I think this is the way to do it:
svn log -r $revision http://llvm.org/svn/llvm-project

35 ↗(On Diff #92809)

I think we should just drop it.

251 ↗(On Diff #92809)

I don't think so, which does make Phabricator seem like a better alternative :-)

255 ↗(On Diff #92809)

Oh right, yes that makes sense.

tstellar updated this revision to Diff 92950.Mar 24 2017, 8:17 AM
tstellar marked 2 inline comments as done.
  • Drop product and component options.
  • Fix svn url.
hans accepted this revision.Mar 24 2017, 8:19 AM

lgtm

utils/release/merge-request.sh
39 ↗(On Diff #92950)

Maybe drop this and list_valid_components now since we're just using 'new-bugs'?

This revision is now accepted and ready to land.Mar 24 2017, 8:19 AM
This revision was automatically updated to reflect the committed changes.