try emails should include commit messages from pushed commits

RESOLVED FIXED

Status

Release Engineering
General Automation
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: froydnj, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
I often push several different sets of patches to try, and then don't look at them until a couple of days later (or I need to go back and look at previous pushes that I have since closed, or whatever).  While I filter my try email into a separate folder, it's a difficult task to remember "Hm, is $RANDOM_HASH1 the push with such-and-such changes in it, or is it $RANDOM_HASH2 ?"  And since I have not gotten any better at it, I wind up opening numerous try tabs to find the right push.

It'd be great if the try emails included a short human-readable summary, like the commit messages of the top N commits.  I was going to say the heuristic should be "top N commits whose committer's email matches the pusher's email", but I'm not sure if that does the right thing for when you're pushing somebody else's patches to try for them.

Comment 1

3 years ago
Picking the non-public changesets should be sufficient.
Emails are sent at two points...

1) Immediately after submission:
https://hg.mozilla.org/build/buildbotcustom/file/default/status/generators.py#l5

2) In response to jobs completing (if people have enabled this; off by default):
https://hg.mozilla.org/build/buildbotcustom/file/default/bin/try_mailer.py#l56
Component: Repos and Hooks → General Automation
QA Contact: hwine → catlee
Oh! This is a nice idea, and would solve a problem I have that I hadn't even realized was a problem.
Created attachment 8474191 [details] [diff] [review]
Include commit messages in Try submission emails

This has been in the back of my mind for a while, too.

With this patch, the subject will change to "Try submission rev: <title>", where <title> is the title of the topmost commit. If the topmost commit starts with "try:", the title of the 2nd topmost commit is used instead.

The email body will also have something like:

  Summary:
    * try: -b d -p linux64 -u all -t none
    * Bug N - Fix everything
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8474191 - Flags: review?(emorley)
Comment on attachment 8474191 [details] [diff] [review]
Include commit messages in Try submission emails

I'm not a peer for this code - someone from releng will need to review (perhaps catlee, given hg log for this file?).
That said, looks reasonable & I think this is a great feature to add.
Similarly, TBPL recently adding something like this - though it cleans the titles more and handles using later commits messages if the topmost wasn't useful. TBPL's algorithm is at:
https://hg.mozilla.org/webtools/tbpl/file/470fce047d3b/js/UserInterface.js#l236
Though I'd take the patch as implemented over the current situation any day :-)
Attachment #8474191 - Flags: review?(emorley) → feedback+
Created attachment 8474686 [details] [diff] [review]
Include commit messages in Try submission emails

(In reply to Ed Morley [:edmorley] from comment #5)
> That said, looks reasonable & I think this is a great feature to add.
> Similarly, TBPL recently adding something like this - though it cleans the
> titles more and handles using later commits messages if the topmost wasn't
> useful. TBPL's algorithm is at:
> https://hg.mozilla.org/webtools/tbpl/file/470fce047d3b/js/UserInterface.
> js#l236

Thanks, I updated the patch to use the same algorithm.
Attachment #8474191 - Attachment is obsolete: true
Attachment #8474686 - Flags: review?(catlee)
Comment on attachment 8474686 [details] [diff] [review]
Include commit messages in Try submission emails

Review of attachment 8474686 [details] [diff] [review]:
-----------------------------------------------------------------

looks pretty good overall, thanks for the patch! just a few minor fixes and it should be good to land.

::: changes/hgpoller.py
@@ +365,5 @@
>                                     when=change["date"],
>                                     branch=self.branch)
> +                if self.mergePushChanges:
> +                    c.properties.setProperty('commit_titles', commit_titles,
> +                                             'BaseHgPoller')

There's a limit to how much data can be stored in properties, it's around 1k of json. Can you add some checks in here to make sure we're not trying to store too much data in this property?

::: status/generators.py
@@ +44,3 @@
>      return msgdict
> +
> +def getSensibleTitle(titles):

some comments/docstring here about what this function does would be great. could you also please add some tests for this function?
Attachment #8474686 - Flags: review?(catlee) → review-
Created attachment 8474791 [details] [diff] [review]
Include commit messages in Try submission emails
Attachment #8474686 - Attachment is obsolete: true
Attachment #8474791 - Flags: review?(catlee)
Comment on attachment 8474791 [details] [diff] [review]
Include commit messages in Try submission emails

Review of attachment 8474791 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

I think we need to be truncating long titles instead of skipping over them.

::: changes/hgpoller.py
@@ +317,5 @@
> +                    # order to avoid insert/update failures, we enforce a cap
> +                    # on the total length with enough room for JSON overhead.
> +                    if commit_titles_total_length + len(title) <= 800:
> +                        commit_titles_total_length += len(title)
> +                        commit_titles.append(title)

Would it be better to add truncated titles instead of ignoring any titles that exceed the limit?

Or perhaps just include the first 100 characters of each title. Nobody's going to want a 800 char email subject!
Attachment #8474791 - Flags: review?(catlee) → review-
Created attachment 8479063 [details] [diff] [review]
Include commit messages in Try submission emails

(In reply to Chris AtLee [:catlee] from comment #9)
> Or perhaps just include the first 100 characters of each title.

Done. I made it trim at the last space before the 100th char to avoid cutting words.
Attachment #8474791 - Attachment is obsolete: true
Attachment #8479063 - Flags: review?(catlee)

Updated

3 years ago
Attachment #8479063 - Flags: review?(catlee) → review+
Comment on attachment 8479063 [details] [diff] [review]
Include commit messages in Try submission emails

https://hg.mozilla.org/build/buildbotcustom/rev/f57b59955738
Attachment #8479063 - Flags: checked-in+
patch(es) in production :)
Depends on: 1061359

Updated

3 years ago
Depends on: 1062557
No longer depends on: 1061359
(In reply to Ed Morley [:emorley] from comment #2)
> 2) In response to jobs completing (if people have enabled this; off by
> default):
> https://hg.mozilla.org/build/buildbotcustom/file/default/bin/try_mailer.
> py#l56

This still needs to be done.
Assignee: birunthan → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 798619

Updated

4 months ago
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.