Closed Bug 1248008 Opened 9 years ago Closed 7 years ago

No obviously sane way to add review comments to the commit message

Categories

(MozReview Graveyard :: General, defect)

Product:
MozReview Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: zalun)

References

Details

Attachments

(6 files, 14 obsolete files)

mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)
(deleted), text/x-review-board-request
smacleod
: review+
Details
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)
(deleted), text/x-review-board-request
smacleod
: review+
Details
MozReview: Specify ordering filediffs by id (Bug 1248008)
(deleted), text/x-review-board-request
smacleod
: review+
Details
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)
(deleted), text/x-review-board-request
smacleod
: review+
Details
MozReview: Add mr:diff-context-loaded event (Bug 1248008)
(deleted), text/x-review-board-request
smacleod
: review+
Details
mozreview: send a series of commands to SQLite running in docker container (bug 1248008)
(deleted), text/x-review-board-request
smacleod
: review+
Details 
I'm doing a review on a commit that has some problems that need fixing in the commit message.  Unlike "edit attachment as comment" in Bugzilla, which allows me to trivially add review comments on the commit message, mozreview doesn't seem to have a decent way to do this.
smacleod: I know we've talked about this. Pretty sure you had an idea. Want to brain dump so someone else can implement it? Also, is this a dupe?
Flags: needinfo?(smacleod)
Product: Developer Services → MozReview
(In reply to Gregory Szorc [:gps] from comment #1)
> smacleod: I know we've talked about this. Pretty sure you had an idea. Want
> to brain dump so someone else can implement it? Also, is this a dupe?

The easiest way we can hack around this is just create a plain text attachment on the review request containing the commit message - this will allow a reviewer to comment on lines like the diff viewer (we could also update this attachment when the message changes, giving you a diff).

A more involved way would be hacking the addition of a "commit message file" into the diffviewer (which could be done by spoofing a file addition)

Review Board 2.6 (release date unknown) adds a mechanism for general review comments with issues which could be used to do something like this as well.
Flags: needinfo?(smacleod)
Yes, this was filed as bug 1194354, but there's now more context here so I'll dupe the other to this one.
Never mind, that other bug is a separate issue.
Some notes:

* The code that creates/updates Review Requests after a push is the BatchReviewRequestResource web API, specifically _process_submission() (http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l352).
* You're going to need to create or update a FileAttachment object (https://github.com/reviewboard/reviewboard/blob/release-2.5.x/reviewboard/attachments/models.py#L55) in that function.
* You will need tests for this, which will involve modifying or creating a new mach command (http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/testing/vcttesting/reviewboard/mach_commands.py).
Assignee: nobody → pzalewa
Adding some info here as it's gonna be easier to refer to.

Commit message is read to set `draft.description` in `update_review_request`: http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l914
It happens on every push.

If a new commit is created `update_review_request` is called from http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l683 This is the place where attachment should be created

If commit is amended (`hg commit --amend`) - from http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/resources/batch_review_request.py#l589 This is the place where attachment should be updated
As for above r/place/case - code should be inside `update_review_request`
I tried to add text attachment to a review and UI did not responded.
MozReview does not have a way to comment on commit message.
This patch creates a plain text attachment so reviewer can comment on it.
If a commit message has been amended attachment will be updated.

Feature is not finished - no snippet is visible (None is printed instead).

Review commit: https://reviewboard.mozilla.org/r/52441/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52441/
Attachment #8752153 - Flags: review?(smacleod)
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/1-2/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review49856

Looks good (although smacleod might be pickier :), but as discussed I think we need (a) tests and (b) support for the on-hover comment button that we implemented for diffs in https://bugzilla.mozilla.org/show_bug.cgi?id=1249647.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:930
(Diff revision 2)
> +    commit_message_temp.flush()
> +    try:
> +        comment_attachment = draft.file_attachments.get(caption='commit_message')
> +    except FileAttachment.DoesNotExist:
> +        draft.file_attachments.create(
> +            caption='commit_message',

This is the caption displayed to users in the UI right?  If so, I think it should be a more human-readable string, e.g. "Commit message".
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/2-3/
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/3-4/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/4-5/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review50422

There are a few problems here:

* The review comments are not mirrored to Bugzilla.  You'll have to update `on_review_request_publishing()` in `pylib/mozreview/mozreview/signal_handlers.py`.
* Updates to a commit message, e.g. after `hg commit --amend`, are not reflected in the attachment.  I pushed up a commit, the amended it to just change the commit message and push it up again, and the Description field was updated but not the contents of the Commit message file attachment.

::: hgext/reviewboard/tests/test-commits-added.t:56
(Diff revision 5)
> +Commit message attachments should be created
> +
> +  $ reviewboard dump-attachments 2
> +  - id: 1
> +    caption: Commit message
> +    filename: tmpjknDs5

You're going to need a * (glob) here because this is a randomly generated filename that will change in each test run.

::: hgext/reviewboard/tests/test-commits-added.t:64
(Diff revision 5)
> +      1 - Foo 1</pre><pre></pre><pre>MozReview-Commit-ID: 124Bxg</pre></div></div>'
> +
> +  $ reviewboard dump-attachments 3
> +  - id: 2
> +    caption: Commit message
> +    filename: tmpn9Kbo7

* (glob) here too.
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/5-6/
Attachment #8752153 - Flags: review?(mcote)
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review50458

Also note there should be tests for both things.
https://reviewboard.mozilla.org/r/52441/#review50422

The only error I've got on --amend is a `ERROR - - Failed to update the following attachments: set([1])` (number changes depending on the review request)
Looks like it's failing when trying to update the comment attachment at bugzilla https://reviewboard-hg.mozilla.org/version-control-tools/file/tip/pylib/mozreview/mozreview/bugzilla/client.py#l200. `ids_to_update` is empty ([]) for the creation of the attachment and equals `[{'comment': u'Review request updated; see interdiff: http://172.16.59.128:55556/r/5/diff/1-2/\n', 'file_name': 'reviewboard-5-url.txt', 'attachment_id': 3, 'summary': u'MozReview Request: NEW K bug 11 r?level1'}]` when attachment is updated. I tried both methods of updating a FileField, presented in the diff and comment_attachment.file.save(att_name, File(commit_message_temp)), both with the same result.

I assumed the attachment should exist only in reviewboard, but now I get that the comments to the message's attachment should be pushed to bugzilla like other comments.

I'm looking now why the commit message attachment is not created for bugzilla in the first place.
https://reviewboard.mozilla.org/r/52441/#review50422

Please specify "review comments". Information about a MozReview request appears in bugzilla as a comment.
Please confirm there is a need to add commit message attachment to bugzilla as well (as explained above).
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/6-7/
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/7-8/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Zalun tells me this is still a work in progress, so clearing review flags.
Attachment #8752153 - Flags: review?(smacleod)
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/8-9/
Attachment #8752153 - Attachment description: MozReview Request: Save commit message as a file attachment (bug 1248008) r?smacleod r?mcote → MozReview Request: Save commit message as a file attachment (bug 1248008)
Attachment #8752153 - Flags: review?(smacleod)
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review51862

I'm going to clear the review flag and let :mcote keep taking care of this until it's further along.
Attachment #8752153 - Flags: review?(smacleod)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/9-10/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Clearing again since this is still in progress.  Zalun, I see that you took out the r?s from the commit message, but could you also clear the reviewers from Review Board via the Review Summary link?  That way I believe it will stop re-requesting me for review until you're ready. :)
Attachment #8752153 - Flags: review?(mcote)
Done
https://reviewboard.mozilla.org/r/52441/#review50422

--amend is now supported - working on the comments
We'll need the docs to be updated for this change too, please.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/10-11/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/11-12/
https://reviewboard.mozilla.org/r/52441/#review50422

Added attachment comments - no diff thought. I think it's not saved. It might be created live (based on FileAttachmentHistory), question is only if it should be done.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/12-13/
Attachment #8752153 - Attachment description: MozReview Request: Save commit message as a file attachment (bug 1248008) → MozReview Request: Save commit message as a file attachment (bug 1248008) r?mcote
Attachment #8752153 - Flags: review?(mcote)
https://reviewboard.mozilla.org/r/52441/#review49856

About the [+]:
I'm searching for a way to add a viewdiff.less to attachment review.

2. 1. I've found that reviewboard/static/rb/pages/diffviewer.less is loaded
2. In reviewboard/staticbundles.py diffviewer.less is mentioned in 'reviews'
3. Adding 'reviewes': { ... viewdiff.less} to mozreview/extensions.py didn't worked (I mean the viewdiff.less is not used to build the css)

I'm looking ...
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review54376

This is good stuff!  A few more things to consider:

As discussed earlier today, our extension customizes the draft banner to read "Finish..." instead of "Edit Review" and "Publish".  We need to do the same thing on file-attachment views as well.  This stuff is done in http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/static/mozreview/js/review.js.  You might be able to just change http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/extension.py#l163 to use `reviewable_url_names` instead, as with the on-hover "+" diff-viewer enhancement that we discussed.

Aside from that, mostly some smaller things:

::: hgext/reviewboard/tests/test-commits-message-attachment.t:13
(Diff revision 13)
> +  $ echo 'foo1' > foo
> +  $ hg commit -A -m 'Bug 1 - Foo 1'
> +  adding foo
> +  $ echo 'foo2' > foo
> +  $ hg commit -m 'Bug 1 - Foo 2'
> +  $ hg push 

Trailing whitespace.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:8
(Diff revision 13)
> +from django.core.files.base import File
> +from django.core.files.base import ContentFile

Combine these two lines.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:925
(Diff revision 13)
>  
>      draft.summary = commit['message'].splitlines()[0]
>      draft.description = commit['message']
>      draft.bugs_closed = commit['bug']
>  
> +    # --- create/update a new commit message attachment

Nit: we try to follow PEP 8 on comments, that is, "Comments should be complete sentences. If a comment is a phrase or sentence, its first word should be capitalized, unless it is an identifier that begins with a lower case letter".

https://www.python.org/dev/peps/pep-0008/#comments

::: pylib/mozreview/mozreview/resources/batch_review_request.py:934
(Diff revision 13)
> +    cm_temp.write(draft.description)
> +    cm_temp.flush()
> +    cm_file = File(cm_temp)
> +    cm_caption = 'Commit message'
> +    comment_atts = draft.file_attachments.filter(caption=cm_caption)
> +    if len(comment_atts) == 0:

Nit: I think `if not comment_atts:` is slightly more Pythonic.

Also we try to follow Review Board's coding standards, which extend PEP 8.  Specifically here (and later), blank lines around "if" blocks.

https://www.reviewboard.org/docs/codebase/dev/coding-standards/#spacing

::: pylib/mozreview/mozreview/resources/batch_review_request.py:953
(Diff revision 13)
> +        try:
> +            latest = att_history.file_attachments.latest()
> +        except FileAttachment.DoesNotExist:
> +            latest = None
> +        if latest is None:
> +            # This should theoretically never happen, but who knows.

We should log an error then.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:959
(Diff revision 13)
> +            att_revision = 1
> +        elif latest.review_request.exists():
> +            # This is a new update in the draft.
> +            att_revision = latest.attachment_revision + 1
> +        else:
> +            # This should actually be the case...

I don't understand this comment.

::: pylib/rbbz/rbbz/diffs.py:326
(Diff revision 13)
> +def render_plain_fileattachment_comment(comment):
> +    """render a comment on a attached file"""
> +    ret = ''
> +    # XXX comment.diff_against_file_attachment is None - should it be like that?
> +    if comment.file_attachment.caption == 'Commit message':
> +        ret = 'Comment to a commit message:\n'
> +    else:
> +        ret = 'Comment to "%s":\n' % comment.file_attachment.caption
> +
> +    return '%s\n%s' % (ret, comment.text)

We should provide some context in the comment.  If it's a review against the file itself, then print out the line(s) that are commented, e.g.

> Update foo (bug 1234); r?level3

And if it's a comment on a commit-message diff, render it as a regular diff (as `render_comment_plain()` does).

Users really like context in the BMO comments. :)

::: pylib/rbbz/rbbz/diffs.py:327
(Diff revision 13)
>      lines.append(u"\n%s" % comment)
>  
>      return u"\n".join(lines)
>  
> +def render_plain_fileattachment_comment(comment):
> +    """render a comment on a attached file"""

Nit: "an attached file".  Also see PEP 8 for comment style.

::: pylib/rbbz/rbbz/diffs.py:329
(Diff revision 13)
>      return u"\n".join(lines)
>  
> +def render_plain_fileattachment_comment(comment):
> +    """render a comment on a attached file"""
> +    ret = ''
> +    # XXX comment.diff_against_file_attachment is None - should it be like that?

That's a good question.  Worth asking smacleod or mconley, although they might not even know.

::: pylib/rbbz/rbbz/diffs.py:331
(Diff revision 13)
> +def render_plain_fileattachment_comment(comment):
> +    """render a comment on a attached file"""
> +    ret = ''
> +    # XXX comment.diff_against_file_attachment is None - should it be like that?
> +    if comment.file_attachment.caption == 'Commit message':
> +        ret = 'Comment to a commit message:\n'

Nit: "Comment on the commit message"
Attachment #8752153 - Flags: review?(mcote) → review-
https://reviewboard.mozilla.org/r/52441/#review54376

> I don't understand this comment.

Copied from reviewboard :)

> We should provide some context in the comment.  If it's a review against the file itself, then print out the line(s) that are commented, e.g.
> 
> > Update foo (bug 1234); r?level3
> 
> And if it's a comment on a commit-message diff, render it as a regular diff (as `render_comment_plain()` does).
> 
> Users really like context in the BMO comments. :)

Good call,
I will have to dig a little further to get the commented line (I think we do store it). The diff however is different story - I would need to create the diff for bugzilla - we don't store it for attachments.
I've left the XXX comment as I haven't found any case in which it is used. I will search more in the reviewboard's code for that a little more.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/13-14/
Attachment #8752153 - Attachment description: MozReview Request: Save commit message as a file attachment (bug 1248008) r?mcote → Save commit message as a file attachment (bug 1248008)
Attachment #8752153 - Flags: review?(smacleod)
Attachment #8752153 - Flags: review?(mcote)
Attachment #8752153 - Flags: review-
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/14-15/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Clearing the flags since there's more to do on this patch still.
Attachment #8752153 - Flags: review?(smacleod)
Attachment #8752153 - Flags: review?(mcote)
https://reviewboard.mozilla.org/r/52441/#review49856

[+] is there, but all comments are marked with number "1" only. I believe a JS file is not included properly
https://reviewboard.mozilla.org/r/52441/#review49856

OK, it's not a number of comments on file in general, but number of comments on the line
https://reviewboard.mozilla.org/r/52441/#review54376

> Good call,
> I will have to dig a little further to get the commented line (I think we do store it). The diff however is different story - I would need to create the diff for bugzilla - we don't store it for attachments.
> I've left the XXX comment as I haven't found any case in which it is used. I will search more in the reviewboard's code for that a little more.

I like that in bugzilla code has different background AFAIK it comes from "> " at the begin of the line. Since I've got no diff here - can I use a "> " and mark commented line with a * ? - mozre in a screenshot.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/15-16/
https://reviewboard.mozilla.org/r/52441/#review54376

> I like that in bugzilla code has different background AFAIK it comes from "> " at the begin of the line. Since I've got no diff here - can I use a "> " and mark commented line with a * ? - mozre in a screenshot.

I'm also looking into finding a way to provide a diff...
https://reviewboard.mozilla.org/r/52441/#review54376

> That's a good question.  Worth asking smacleod or mconley, although they might not even know.

It is set in view when updating. It's just the last attached file (not a diff)
https://reviewboard.mozilla.org/r/52441/#review54376

Added the `reviewable_url_names` in `extension.py`, but nothing changed. Assuming the situation is similar to CSS, I tried TemplateHook - added `mozreview/review-scripts-js.html`, but no change there. If it would work, I'd need to add more complexity as TemplateHook is using `review_request_url_names` which also expands `diffviewer_url_names`.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/16-17/
https://reviewboard.mozilla.org/r/52441/#review54376

I think I've got it in, there is an error, as this html is not exactly as in reviews - `init.rr.js` throws "Could not find a valid id for the parent review request."
https://reviewboard.mozilla.org/r/52441/#review54376

Should I remove the Publish button? I haven't seen it in the `review.js` ...
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/17-18/
Attachment #8752153 - Flags: review?(mcote)
https://reviewboard.mozilla.org/r/52441/#review54376

Yes, it should be removed.  It is not shown in the regular diff view.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/18-19/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/19-20/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review56746

I see the Finish Review button now; however, the dialog still shows the "Ship it" checkbox instead of the review-flag dropdown.  As previously, switching to the Diff or Reviews view works.  So looks like you just need to put that override in the File Attachment view as well.

Also, it seems that when you highlight X lines, the BMO comment only reflects X-1 lines.  For example, if I comment on lines 1-3, the BMO comment only displays lines 1-2.

::: pylib/rbbz/rbbz/diffs.py:359
(Diff revisions 14 - 20)
> +    att_f = comment.file_attachment.file
> +    att_f.open()
> +    lines = list(att_f)
> +
> +    att_f.close()
> +    logger.info(lines)

I don't think we want to be logging this all at info level in production. :)

::: pylib/rbbz/rbbz/diffs.py:364
(Diff revisions 14 - 20)
> +    logger.info(lines)
> +
> +    # adding a ">" sign to commented lines
> +    first_line_index = first_line - 1
> +    last_line_index = min(first_line_index + num_lines, len(lines)) - 1
> +    ret = ['Comment on "%s":\n\n' % caption]

I think we should drop the quotes, so that it looks like

    Comment on Commit Message
    
Looks a little nicer.

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:24
(Diff revision 20)
> +  // understand what clicking it does.
> +  $("#review-link").text("Finish Review...");
> +
> +  // Remove "Publish" button
> +  $('#review-banner-publish').remove();
> +

As I remarked in my general comment, you'll need the review-flag dropdown stuff in here as well (ideally this would be in a single JS file that we could load in both places, to avoid copying it).

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:25
(Diff revision 20)
> +  $("#review-link").text("Finish Review...");
> +
> +  // Remove "Publish" button
> +  $('#review-banner-publish').remove();
> +
> +  // No MozReview here

Yeah, I don't think any of this is required.  The first part is for the metadata panel, which isn't displayed in the File Attachment view, and the second part is for the Reviews view specifically.
Attachment #8752153 - Flags: review?(mcote) → review-
https://reviewboard.mozilla.org/r/52441/#review56746

Now this (r+/r-/r?) is slightly more complicated. The problem is with Backbone - the extension `mozreview/js/review_flah.js` is not initialized. I mean its initialize function is not even called, View is created though. I've also added the `mozreview_ready` trigger... I'll deal with it today.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/20-21/
Attachment #8752153 - Flags: review- → review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/21-22/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review58020

Not sure if this is ready for me to review yet, but I just tried it out again.  I like where it's going, although the following jumped out:

* The rendering on the Reviews view seems broken. The commits table doesn't render properly, and I get this error: "Well, this is embarassing... Something's gone wrong in either retrieving or manipulating these review requests. Sorry about that. Please consider filing a bug and including the following information: ..."
* The "Review" link beside description should be a button.
* The "Files" header is still there even though there are no files. It should also be hidden unless there are other file attachments uploaded by the author.

Finally, I was thinking, when you're ready, you could flag davidwalsh for review too, for the front-end stuff.
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/22-23/
Attachment #8752153 - Flags: review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/23-24/
https://reviewboard.mozilla.org/r/52441/#review58020

* I've seen the "embarrassing" bit as well, I thought it was some sort of a glitch... will dive into.
* Button - please show me another one. Like [Collapse changes] ?
* Yes, I was wondering if it's there if no attachments are present. I'll fix it
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/24-25/
https://reviewboard.mozilla.org/r/52441/#review58020

So, there is only button left - how you would like to have it?
https://reviewboard.mozilla.org/r/52439/#review58166

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:3
(Diff revision 28)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = { isParent: false };

Would this be safer?

```
var MozReview = window.MozReview || {};
MozReview.isParent = false;
```

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:9
(Diff revision 28)
> +
> +$(document).ready(function() {
> +  // Workaround until we get a template hook point before
> +  // the draft banner thing.
> +  $("#new-navbar").insertBefore(".box.review-request");
> +  $("#new-navbar").show();

We can probably chain the `show()` onto the `insertBefore()'

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:29
(Diff revision 28)
> +  $("#review-link").text("Finish Review...");
> +
> +  // Remove "Publish" button
> +  $('#review-banner-publish').remove();
> +
> +  $(document).trigger("mozreview_ready");

I don't think we want this; `init_rr.js` is the only script that should be triggering `mozreview_ready`.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:25
(Diff revision 28)
>    $("#review-link").text("Finish Review...");
>  
> +  // Move commit message file attachment's review button to the right
> +  // commit message
> +  // hide the file_attachment
> +  var attachments = $('.file-container');

We can do `$('.file-container').each` instead of your `for` loop.
https://reviewboard.mozilla.org/r/52439/#review58166

> I don't think we want this; `init_rr.js` is the only script that should be triggering `mozreview_ready`.

init_rr.js bases on existense of a parent review DOM object. Hence the special file for file_attachments.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/25-26/
https://reviewboard.mozilla.org/r/52439/#review58166

> init_rr.js bases on existense of a parent review DOM object. Hence the special file for file_attachments.

Please reopen if you've got other solution
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review60838

This is getting closer, but I noticed one bug: the "Review" link is duplicated for all revisions past 1.  In fact, there's one Review link for each revision. :) So revision 3 of a commit will see "ReviewReviewReview".

Also please file a follow-up bug, with the "depends on" field set to this bug, for quoting the diff itself in Bugzilla comments when the reviewer leaves a review on the diff of two revisions of a commit message.
Attachment #8752153 - Flags: review?(mcote) → review-
https://reviewboard.mozilla.org/r/52441/#review60878

As noted in my previous quick review, this is really really close.  Just a few little things to clean up and then we're good to go.

::: pylib/mozreview/mozreview/extension.py:280
(Diff revision 26)
> +        # TODO check if this one is not an overkill
> +        TemplateHook(self, 'base-css', 'mozreview/review-stylings-css.html',
> +                     apply_to=reviewable_url_names)

As you note, I believe you only need this and not the TemplateHook() call above, since reviewable_url_names includes (I believe) all of  review_request_url_names.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:939
(Diff revision 26)
> +    cm_file = File(cm_temp)
> +    cm_caption = 'Commit message'
> +    comment_atts = draft.file_attachments.filter(caption=cm_caption)
> +
> +    if not comment_atts:
> +        # no comment attachment found in review request, create a new one

As previously noted, comments should be complete sentences, starting with a capital and ending with a period (though the period can be omitted for short comments) as per PEP 8.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:947
(Diff revision 26)
> +        # at least one commit attachment found
> +        # find attachment history and update attachment

Proper sentences here too.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:1
(Diff revision 26)
> +/* mozreview viewdiff.less */
> +

I don't think this is needed.

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:6
(Diff revision 26)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = window.MozReview || { isParent: false };
> +
> +$(document).ready(function() {
> +  // Workaround until we get a template hook point before

Now that we know this is working, we should get rid of the duplication between this file and review.js.  It's only a matter of time until someone creates a bug (or spends a lot of time debugging) by updating one file but not the other.

Could you make a common function that both file_attachmen_review.js and review.js call, to avoid the redundancy?

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:27
(Diff revision 26)
> +  // Change string of "Review" button to be a verb so people better
> +  // understand what clicking it does.
> +  $("#review-link").text("Finish Review...");
> +
> +  // Remove "Publish" button
> +  $('#review-banner-publish').remove();

For other views, this is actually removed in css (http://hg.mozilla.org/hgcustom/version-control-tools/file/tip/pylib/mozreview/mozreview/static/mozreview/css/review.less#l49).  Probably best to use that same technique.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:27
(Diff revision 26)
> +  // Move commit message file attachment's review button to the right
> +  // commit message
> +  // hide the file_attachment
> +  var attachments = $('.file-container');
> +  attachments.each(function() {
> +    if ($(this).find('.file-caption a.edit').text() === 'Commit message') {

Just a thought--now that we're removing the attachment UI, is the caption displayed anywhere (like on the file-attachment page)?  If not, I wonder if we should try to make the caption more unique, to avoid someone uploading a file manually with the same caption and then accidentally triggering this code.  It's unlikely, but by making the caption a weird, longer string, it'll be even less likely.

That's only assuming the caption is no longer visible anywhere.

::: pylib/rbbz/rbbz/diffs.py:329
(Diff revision 26)
> +    # Do not provide additional data if not commenting on source
> +    if comment.extra_data['viewMode'] != 'source':
> +        return 'Comment on "%s"\n\n%s' % (comment.file_attachment.caption,
> +                                          comment.text)

I'm not exactly sure what this means.  What is a viewMode that's not source, and do we definitely want to comment in Bugzilla in that case?
https://reviewboard.mozilla.org/r/52441/#review60838

It was picking file attachments mentioned later in "Review request changed" message. We might want to not add these...
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/26-27/
Attachment #8752153 - Flags: review- → review?(mcote)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/27-28/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/28-29/
https://reviewboard.mozilla.org/r/52441/#review60878

> As you note, I believe you only need this and not the TemplateHook() call above, since reviewable_url_names includes (I believe) all of  review_request_url_names.

It does not include it - https://github.com/reviewboard/reviewboard/blob/master/reviewboard/urls.py#L25-L40 
But I think it needs to be just the urls we want...
https://reviewboard.mozilla.org/r/52441/#review60878

> I'm not exactly sure what this means.  What is a viewMode that's not source, and do we definitely want to comment in Bugzilla in that case?

It is another type then - (image, zip)
https://reviewboard.mozilla.org/r/52441/#review60878

> Just a thought--now that we're removing the attachment UI, is the caption displayed anywhere (like on the file-attachment page)?  If not, I wonder if we should try to make the caption more unique, to avoid someone uploading a file manually with the same caption and then accidentally triggering this code.  It's unlikely, but by making the caption a weird, longer string, it'll be even less likely.
> 
> That's only assuming the caption is no longer visible anywhere.

It is visible in the "attachment added message" as in https://reviewboard.mozilla.org/r/52441/file/318/
https://reviewboard.mozilla.org/r/52441/#review60878

> It is visible in the "attachment added message" as in https://reviewboard.mozilla.org/r/52441/file/318/

Hm that's not terribly important.  Then again, with the commit message being automatically uploaded, it's probably not much of an issue.  Let's leave it for now.

> It is another type then - (image, zip)

Ah okay.
https://reviewboard.mozilla.org/r/52441/#review60878

> As previously noted, comments should be complete sentences, starting with a capital and ending with a period (though the period can be omitted for short comments) as per PEP 8.

Doesn't look like you addressed this in the new commit.

> Proper sentences here too.

Doesn't look like you addressed this in the new commit.

> I don't think this is needed.

Doesn't look like you addressed this in the new commit.
https://reviewboard.mozilla.org/r/52441/#review61320

Almost done looking at this again, but so far it looks good aside from the missed issues from the last review, and a couple minor comments below.  Will give this a final look tomorrow.

::: pylib/mozreview/mozreview/extension.py:125
(Diff revision 29)
>      apply_to = review_request_url_names
>  
>  
>  class MRReviewFlagExtension(JSExtension):
>      model_class = 'MRReviewFlag.Extension'
> -    apply_to = review_request_url_names
> +    apply_to = reviewable_url_names

Based on your comment that `reviewable_url_names` does not include everything in `review_request_url_names`, doesn't this (and possibly other places) need to be changed as you do with the `TemplateHooks()` below?

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:4
(Diff revision 29)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = window.MozReview || { isParent: false };
> +

Ah! So in the end we could include `review.js` on all pages?  That's pretty nice. :)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/29-30/
https://reviewboard.mozilla.org/r/52441/#review61320

> Ah! So in the end we could include `review.js` on all pages?  That's pretty nice. :)

Yes, I remember there were issues, but after these were sorted with adding `review_flag.js`
https://reviewboard.mozilla.org/r/52441/#review60878

> Doesn't look like you addressed this in the new commit.

I marked is as fixed before puching the changes... (same for the next comment messages)
https://reviewboard.mozilla.org/r/52441/#review61320

> Based on your comment that `reviewable_url_names` does not include everything in `review_request_url_names`, doesn't this (and possibly other places) need to be changed as you do with the `TemplateHooks()` below?

Good catch, checking the diff
https://reviewboard.mozilla.org/r/52441/#review60878

> I marked is as fixed before puching the changes... (same for the next comment messages)

Okay that's confusing. :)  You pushed several commits and flagged me for review, so I figured it was good to review again.  If you have work in progress, please remove me from the reviewers list until the patch is ready for a proper review again.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/30-31/
Attachment #8752153 - Flags: review?(mcote) → review-
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review61954

Functionally, this appears to be working just fine. \o/  I have one comment about how you're hiding the file list, and a few more little nits.

::: pylib/mozreview/mozreview/extension.py:125
(Diff revision 31)
>      apply_to = review_request_url_names
>  
>  
>  class MRReviewFlagExtension(JSExtension):
>      model_class = 'MRReviewFlag.Extension'
> -    apply_to = review_request_url_names
> +    apply_to = reviewable_url_names + ['file-attachment', 'screenshot']

You have `file_attachment_url_names` defined now, so you might as well use it here.

Also, you switched from `review_request_url_names` to `reviewable_url_names`.  This means that the `review-request-detail` view is no longer included.  I don't think that was intentional, was it?

::: pylib/mozreview/mozreview/extension.py:294
(Diff revision 31)
>          # All of our review request styling is injected via
>          # review-stylings-css, which in turn loads the review.css static
>          # bundle.
>          TemplateHook(self, 'base-css', 'mozreview/review-stylings-css.html',
> -                     apply_to=review_request_url_names)
> +                     apply_to=review_request_url_names + [
> +                         'file-attachment',

Use `file_attachment_url_names` here.  Or, even better, since I think `MRReviewFlagExtension` should be using `review_request_url_names + file_attachment_url_names`, create a new variable containing both those and use that in both places.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:36
(Diff revision 31)
> +      reviewButton.removeClass('file-review')
> +                  .addClass('description-review')
> +                  .text('Review')
> +                  .appendTo(descriptionLabel);
> +      if (attachments.length === 1) {
> +        $('#file-list').parent().hide();

So this causes a momentary flicker, which is kind of annoying.  I think we should do the complement of what you have here, which we do in other places: in CSS, hide `#file-list` by default, and then here unhide it if `attachments.length > 1`.

::: pylib/mozreview/mozreview/static/mozreview/js/review_flag.js
(Diff revision 31)
>      this.model.get('extraData')[this.key] = val;
>      this.save()
>    }
>  });
>  
> -

Leave this blank line in (particularly since you didn't touch this file elsewhere, but also because I believe we keep two lines between JavaScript "class" definitions as well.

::: pylib/rbbz/rbbz/diffs.py:352
(Diff revision 31)
> +        first_line = begin_line_num
> +        num_lines = end_line_num - begin_line_num + 1
> +
> +    caption = comment.file_attachment.caption
> +    # This is the base comment of a review, so we
> +    # should quote the code the comment is adressing.

Nit: "addressing"

::: pylib/rbbz/rbbz/diffs.py:364
(Diff revision 31)
> +    for index in range(len(lines)):
> +        if index >= first_line_index and index <= last_line_index:
> +            prefix = '>  '
> +            ret.append('%s%s' % (prefix, lines[index]))

Here's a simpler way to do this:

    ret += ['> %s' % line for line in lines[first_line_index:last_line_index+1]]
https://reviewboard.mozilla.org/r/52441/#review61980

Oh you'll need to rebase this as well, since rbbz went away.
https://reviewboard.mozilla.org/r/52441/#review61954

> So this causes a momentary flicker, which is kind of annoying.  I think we should do the complement of what you have here, which we do in other places: in CSS, hide `#file-list` by default, and then here unhide it if `attachments.length > 1`.

That was slightly more complicated as we need to hide `#file-list`'s parent. I'm hiding `#review_request_extra div.clearfix`. Also, because it's been shown before by inline CSS (using JS I suppose) I need to remove this class to show the div.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/31-32/
Attachment #8752153 - Flags: review- → review?(mcote)
Blocks: 1287736
Blocks: 1287737
Attachment #8752153 - Flags: review?(mcote) → review+
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review62248

I *think* this is okay now. :) I was just testing out what happens if you manually attach more files, and there was some strange behaviour.  However, that might have nothing to do with your patch; I went back to the parent of your commit and the behaviour continued.

You will still need to rebase your commit on @.  Please give it a quick test after you do, including adding another file attachment manually.  Then you can push it up.  Flag me for review; I'll just give it a quick look and then autoland it.

Other than that, just two very tiny nits.

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:153
(Diff revisions 31 - 32)
>    padding-left: 10px;
>    padding-right: 10px;
>    font-size: 10pt;
>  }
>  
> +

Just one blank line needed here.

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:162
(Diff revisions 31 - 32)
> +}
> +#review_request_extra div.more_attachments {
> +  display: block;
> +}
> +
> +

Just one blank line needed here.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/32-33/
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52441/diff/33-34/
https://reviewboard.mozilla.org/r/52441/#review63076

All tests pass for me. \o/

Giving it to smacleod for another look.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

Bug 1291732 prevented me from flagging you via Review Board.
Attachment #8752153 - Flags: review?(smacleod)
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review66746

::: hgext/reviewboard/tests/test-commits-message-attachment.t:1
(Diff revision 34)
> +#require mozreviewdocker

This isn't testing for any of the cross-posting to bugzilla, that should be tested as well.

::: pylib/mozreview/mozreview/diffs.py:6
(Diff revision 34)
> +import logging
> +logger = logging.getLogger(__name__)

This is unused, please make sure your code additions pass cleanly under pyflakes.

::: pylib/mozreview/mozreview/diffs.py:327
(Diff revision 34)
>      lines.append(u"\n%s" % comment)
>  
>      return u"\n".join(lines)
>  
>  
> +def render_plain_fileattachment_comment(comment):

You're not dealing with replies at all, we should follow the pattern used by diff comments.

::: pylib/mozreview/mozreview/diffs.py:327
(Diff revision 34)
> +def render_plain_fileattachment_comment(comment):
> +    """Render a comment on an attached file"""

So currently this has nothing to do with "diffs" like the name of the file would indicate... we might want to consider moving this, or giving things a rename to indicate the actual overall purpose.

::: pylib/mozreview/mozreview/diffs.py:339
(Diff revision 34)
> +    if begin_line_num == end_line_num:
> +        if begin_line_num < 6:
> +            first_line = 1
> +            num_lines = end_line_num
> +        else:
> +            first_line = begin_line_num - 5
> +            num_lines = 6
> +    else:
> +        first_line = begin_line_num
> +        num_lines = end_line_num - begin_line_num + 1

```Python
if begin_line_num == end_line_num:
    first_line = max(1, begin_line_num - 5)
else:
    first_line = begin_line_num

num_lines = end_line_num - first_line + 1
```

is a bit cleaner IMO. That being said, does 5 lines of context *really* make sense for the commit message? you're generally going to be dealing with a much smaller file than source code, and the vast majority will be english, not code. We should consider reducing the lines, or even not including the context.

::: pylib/mozreview/mozreview/diffs.py:351
(Diff revision 34)
> +    # This is the base comment of a review, so we
> +    # should quote the code the comment is addressing.

We're not quoting code

::: pylib/mozreview/mozreview/diffs.py:354
(Diff revision 34)
> +    att_f = comment.file_attachment.file
> +    att_f.open()
> +    lines = list(att_f)
> +
> +    att_f.close()

We should use RBs builtin mechanism for grabbing the file contents (it deals with caching etc).

You'll want to go through the Review UI (which generally deals with grabbing the relavant fragment of the file for a comment, but does it in a rendered way we can't use).

Try something like `comment.review_ui.get_text()` which should return the file contents from the cache, or generate it, as a string.

::: pylib/mozreview/mozreview/diffs.py:360
(Diff revision 34)
> +    att_f.open()
> +    lines = list(att_f)
> +
> +    att_f.close()
> +
> +    # adding a ">" sign to commented lines

This block below does more than just this, it's creating the whole rendering of the comment and the quoted section of the attachment.

I think something like "Blockquote the commented lines." is more descriptive of what we're doing as well.

::: pylib/mozreview/mozreview/diffs.py:363
(Diff revision 34)
> +    ret = ['Comment on %s:\n\n' % caption]
> +    ret += ['> %s' % line for line in lines[first_line_index:last_line_index + 1]]
> +    code = u"".join(ret)
> +    return u"%s\n%s" % (code, comment.text)

I don't think we want a blank space between the file the comment's on, and the quoting of the file.

::: pylib/mozreview/mozreview/extension.py:121
(Diff revision 34)
> +# These url names are used to set file_attachment_review JS
> +file_attachment_url_names = [
> +    'file-attachment',
> +    'screenshot',
> +]
> +rr_and_attachment_url_names = review_request_url_names + file_attachment_url_names

I think a more future proof solution would be to use `reviewboard.urls.reviewable_url_names`. This'll make sure we get any future areas which will allow popping up the review dialog etc.

Although, this doesn't include `'review-request-detail'`... How about we define our own constant as `rr_and_reviewable_url_names = reviewable_url_names + ['review-request-detail']`

::: pylib/mozreview/mozreview/extension.py:185
(Diff revision 34)
>                                   'mozreview/js/ui.mozreviewautocomplete.js',
>                                   'mozreview/js/review_flag.js',
>                                   ],
>              'apply_to': review_request_url_names,
>          },
> +        # As HTML is different there is a need to initiate less

Differant than what? in what way?

Is there a real perf gain from splitting things up like this into multiple bundles? you're including the same js file into multiple minified bundles now, which is a separate resource the client needs to pull down and cache.

::: pylib/mozreview/mozreview/extension.py:310
(Diff revision 34)
> +        TemplateHook(self, 'file_attachment_review',
> +                     'mozreview/file_attachment_review-js.html',
> +                     apply_to=file_attachment_url_names)

This isn't actually doing anything, the first argument here is supposed to be the hookpoint where this will execute, but `file_attachment_review` isn't a real hookpoint. Your js gets included just by having the `apply_to` key in the bundle definition. The reason we haven't done that for CSS is historical, and something that should really change.

Also, `file_attachment_review-js.html` doesn't even exist.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:927
(Diff revision 34)
>  
>      draft.summary = commit['message'].splitlines()[0]
>      draft.description = commit['message']
>      draft.bugs_closed = commit['bug']
>  
> +    # Creating/updating a new commit message attachment started

Comments like this should be ended with a period.

I think something like "Create/update the commit message attachment." is more in line with the current style.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:929
(Diff revision 34)
>      draft.description = commit['message']
>      draft.bugs_closed = commit['bug']
>  
> +    # Creating/updating a new commit message attachment started
> +
> +    # Prepare attachment file

period.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:931
(Diff revision 34)
>  
> +    # Creating/updating a new commit message attachment started
> +
> +    # Prepare attachment file
> +    # cm_* means commit_message_*
> +    cm_temp = NamedTemporaryFile(delete=True)

Please don't create a temporary file for the contents - move directly from the string to the format django requires by using `ContentFile`

::: pylib/mozreview/mozreview/resources/batch_review_request.py:935
(Diff revision 34)
> +    cm_caption = 'Commit message'
> +    comment_atts = draft.file_attachments.filter(caption=cm_caption)

I really don't think we should rely on the caption being equal for identifying our specially handled commit message file. We should store information about which id corresponds to our special file inside of our CommitData table.

We should also use that stored data to prevent the author or anyone else from updating / deleting the special commit message attachment, other than our special batch endpoint. This will be important as we start to integrate the attachment review better, possibly adding custom UI and hiding the normal UI, etc.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:936
(Diff revision 34)
> +    cm_temp = NamedTemporaryFile(delete=True)
> +    cm_temp.write(draft.description)
> +    cm_temp.flush()
> +    cm_file = File(cm_temp)
> +    cm_caption = 'Commit message'
> +    comment_atts = draft.file_attachments.filter(caption=cm_caption)

`comment_atts` or `commit_message_atts`?

::: pylib/mozreview/mozreview/resources/batch_review_request.py:939
(Diff revision 34)
> +    cm_file = File(cm_temp)
> +    cm_caption = 'Commit message'
> +    comment_atts = draft.file_attachments.filter(caption=cm_caption)
> +
> +    if not comment_atts:
> +        # There is no comment attachment in review request.

"comment attachment"? "commit message attachment"?

::: pylib/mozreview/mozreview/static/mozreview/js/file_attachment_review.js:1
(Diff revision 34)
> +// A hack to allow review_flag.js work without MozReview object
> +
> +var MozReview = window.MozReview || { isParent: false };
> +
> +$(document).ready(function() {
> +  $(document).trigger("mozreview_ready");
> +});

Why is this hack even a thing? Is there a reason you don't want the MozReview object around? If this is needed it's going to need a good explanation listed.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:22
(Diff revision 34)
> +  // Move commit message file attachment's review button to the right
> +  // commit message
> +  // hide the file_attachment

I don't really understand this comment.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:31
(Diff revision 34)
> +      reviewButton.removeClass('file-review')
> +                  .addClass('description-review')
> +                  .text('Review')
> +                  .appendTo(descriptionLabel);
> +      $(this).hide();

Why exactly are we doing this? It's not clear to me from the screenshots / comments / commit description what the purpose is. Can you elaborate please?

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:39
(Diff revision 34)
> +  if (attachments.length > 1) {
> +    $('#file-list').parent()
> +      .removeClass('clearfix')
> +      .addClass('more_atachments');
> +  }

This too, I'm unsure what's going on.
Attachment #8752153 - Flags: review?(smacleod) → review-
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review66746

> So currently this has nothing to do with "diffs" like the name of the file would indicate... we might want to consider moving this, or giving things a rename to indicate the actual overall purpose.

There is a follow-up bug to add this (bug 1287736), it includes testing in bugzilla, I think I will just change it to duplicate and make it a part of this patch...

> ```Python
> if begin_line_num == end_line_num:
>     first_line = max(1, begin_line_num - 5)
> else:
>     first_line = begin_line_num
> 
> num_lines = end_line_num - first_line + 1
> ```
> 
> is a bit cleaner IMO. That being said, does 5 lines of context *really* make sense for the commit message? you're generally going to be dealing with a much smaller file than source code, and the vast majority will be english, not code. We should consider reducing the lines, or even not including the context.

About the 5 lines around - sure, I was just following the diff rule in code.

> Differant than what? in what way?
> 
> Is there a real perf gain from splitting things up like this into multiple bundles? you're including the same js file into multiple minified bundles now, which is a separate resource the client needs to pull down and cache.

The environment (HTML) is different. Reviewing attachment page has no objet refering to the parent review.

> Why is this hack even a thing? Is there a reason you don't want the MozReview object around? If this is needed it's going to need a good explanation listed.

HTML doesn't have parent review object and MozReview is failing to initiate.
More explanation is needed indeed.

> Why exactly are we doing this? It's not clear to me from the screenshots / comments / commit description what the purpose is. Can you elaborate please?

I'm taking Review link of the file attachment and placing it to the right of the original commit message. Then I'm hiding entire file attachment.

> This too, I'm unsure what's going on.

Because at the beginning there is only one file attachment (commit message one), I needed to hide entire File section, not only the attachment. Since it was blinking (showing <h>Files</h> and hiding it afterwards) @mcote suggested to hide it by default and show it if there is more than 1 attachment present. Adding a class 'more_attachments' is the solution.
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review66746

> This is unused, please make sure your code additions pass cleanly under pyflakes.

pyflakes actually passed it - I might have some very liberal setting
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review66746

> I think a more future proof solution would be to use `reviewboard.urls.reviewable_url_names`. This'll make sure we get any future areas which will allow popping up the review dialog etc.
> 
> Although, this doesn't include `'review-request-detail'`... How about we define our own constant as `rr_and_reviewable_url_names = reviewable_url_names + ['review-request-detail']`

I will need a better explanation of how hooks are working - a diagram or something as long as I will get an answer on this
Comment on attachment 8752153 [details]
Save commit message as a file attachment (bug 1248008)

https://reviewboard.mozilla.org/r/52441/#review66746

> I really don't think we should rely on the caption being equal for identifying our specially handled commit message file. We should store information about which id corresponds to our special file inside of our CommitData table.
> 
> We should also use that stored data to prevent the author or anyone else from updating / deleting the special commit message attachment, other than our special batch endpoint. This will be important as we start to integrate the attachment review better, possibly adding custom UI and hiding the normal UI, etc.

I've written the file attachment id to the extra_data. No idea how to get it out in front-end yet
Attachment #8752153 - Attachment is obsolete: true
Attachment #8752153 - Flags: review?(smacleod)
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review76290

::: pylib/mozreview/mozreview/resources/batch_review_request.py:914
(Diff revision 5)
> +    from reviewboard.diffviewer.diffutils import convert_to_unicode
> +    from reviewboard.scmtools.core import PRE_CREATION, UNKNOWN, FileNotFoundError
> +    from reviewboard.diffviewer.differ import DiffCompatVersion
> +    from django.utils.encoding import smart_unicode

Imports generally belong at the top of the file.  I believe the only time we include them in functions is for mach commands, since they are typically run individually.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review76290

> Imports generally belong at the top of the file.  I believe the only time we include them in functions is for mach commands, since they are typically run individually.

OK, I've copied most stuff from managers, but I didn't like it as well
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review76290

> OK, I've copied most stuff from managers, but I didn't like it as well

"managers"?  Where's that?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review76290

> "managers"?  Where's that?

https://github.com/reviewboard/reviewboard/blob/master/reviewboard/diffviewer/managers.py#L443
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review78004

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1031
(Diff revision 9)
> +        commit_message_name,
> +        len(commit_message_lines),
> +        '%s\n' % '\n'.join(['+%s' % l for l in commit_message_lines]))
> +
> +    # Create file FileDiff containing the commit message
> +    commit_message_filediff = FileDiff.objects.create(

Unfortunately having filediffs in the right order doesn't help. It's working well for loading filediffs from normal request (i.e. /r/2/diff/1-3/), but order becomes acting different if user moves the slider to change revisions. For now I see new file coming to top. 

Sorting has to happen in JS as /api/review-requests/2/diff-context/?revision=1&interdiff-revision=3 provides files in the right order. 

The repo I'm checking contains 2 files and 5 revisiona. First file (foo) is added in root commit, the second (bar) in revision 2. If left slider handle is moved to show [0/1/2] bar becomes the first, however if right slider is moved commit-message is at the top. I'm looking now to find the inconsistency of this behaviour.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review78004

> Unfortunately having filediffs in the right order doesn't help. It's working well for loading filediffs from normal request (i.e. /r/2/diff/1-3/), but order becomes acting different if user moves the slider to change revisions. For now I see new file coming to top. 
> 
> Sorting has to happen in JS as /api/review-requests/2/diff-context/?revision=1&interdiff-revision=3 provides files in the right order. 
> 
> The repo I'm checking contains 2 files and 5 revisiona. First file (foo) is added in root commit, the second (bar) in revision 2. If left slider handle is moved to show [0/1/2] bar becomes the first, however if right slider is moved commit-message is at the top. I'm looking now to find the inconsistency of this behaviour.

I've sorted out the order of the file using a little hackery solution (added `data-extra_data` to difffile's table and checking if it contains "is_commit_message_file" string)
There is `table#diff_index` element which has always the order as RB displays it. It's generated with `diffFileIndexView.js` but I don't see any sorting there.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review78004

> I've sorted out the order of the file using a little hackery solution (added `data-extra_data` to difffile's table and checking if it contains "is_commit_message_file" string)
> There is `table#diff_index` element which has always the order as RB displays it. It's generated with `diffFileIndexView.js` but I don't see any sorting there.

BTW the repo structure in testing is generated using these commands: 

  echo root > foo
  hg commit -A -m 'root commit'
  hg phase --public -r .
  echo foo > foo
  hg commit -m 'Bug 1 - Foo 1'
  hg push -y
  echo foo2 > foo
  hg commit --amend -m 'Bug 1 - Foo 2'
  hg push -y
  echo foo > foo2
  hg commit -A --amend -m 'Bug 1 - Foo2 added'
  hg push -y
Attachment #8792824 - Attachment is obsolete: true
Attachment #8792824 - Flags: review?(smacleod)
Attachment #8793266 - Attachment is obsolete: true
Attachment #8793266 - Flags: review?(smacleod)
Attachment #8793267 - Attachment is obsolete: true
Attachment #8793703 - Flags: review?(smacleod)
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

::: pylib/mozreview/mozreview/resources/batch_review_request.py:914
(Diff revision 18)
>              unrecognized.add(reviewer)
>  
>      return reviewers, unrecognized
>  
>  
> +def create_diffset_from_data(repository, diff_file_contents, request,

I was really hoping we wouldn't need to copy this code, as it's quite large and complex.

I thought you indicated there was another code location sorting was taking place as well? Meaning having the first id wouldn't be enough. What happened to that case? Were we mistaken and having the lower ID will cover it?

Rather than bringing all this code in I think I'd prefer we hack around it slightly (while not as clean DB wise, code wise it will be much better, IMO).

I propose to get our FileDiff to have a lower id number we create it first. Since a filediff requires a DiffSet though, we can create a phony DiffSet (hell, we could even reuse the same one for each repository). The DiffSet we create won't be tied to a DiffSetHistory, so it won't show up for any review requests.

Then, we create the commit FileDiff using the bogus DiffSet and save it, giving it an id. Then once we've called `create_from_data()` we can switch the DiffSet for our commit FileDiff to the result.

It would look something like:
```Python
temp_diffset = get_temp_diffset()
commit_file_diff = FileDiff.objects.create(
    diffset=temp_diffset,
    ...)

    
actual_diffset = DiffSet.objects.create_from_data(...)
commit_file_diff.diffset = actual_diffset
commit_file_diff.save()
```

What do you think about this?

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1020
(Diff revision 18)
> +    # Prepare commit message data
> +    commit_message_name = '000-commit-message-%s' % diffset.base_commit_id[0:5]
> +    commit_message_lines = description.split('\n')
> +    commit_message_diff = 'diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@ -0,0 +1,%s @@\n%s' % (
> +        commit_message_name,
> +        commit_message_name,
> +        commit_message_name,
> +        len(commit_message_lines),
> +        '%s\n' % '\n'.join(['+%s' % l for l in commit_message_lines]))

This method is extremely complex making it very hard to review, especially not knowing what has changed from when it was pulled it. If the alternative solution I've proposed doesn't work I'd much prefer this be pulled in as its own commit. Having the commits structured in a way that lets me review specific pieces you've changed / added will make my life a lot easier.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1157
(Diff revision 18)
> +    commit_data = fetch_commit_data(draft)
> +    commit_data.draft_extra_data.update({
> +        AUTHOR_KEY: commit['author'],
> +        COMMIT_ID_KEY: commit['id'],
> +        FIRST_PUBLIC_ANCESTOR_KEY: commit['first_public_ancestor'],
> +        COMMIT_MESSAGE_ID_KEY: commit_message_filediff.pk

How will this work for previoius diff revisions? we'll need to store a list or some sort of mapping from diff revision to commit message filediff id.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:137
(Diff revision 18)
> +    // Place the container at the top or right after the highlighting DIV.
> +    if (commitMessageFileContainer && !commitMessageFileContainer.is(':first-child')) {
> +      if ($('.diff-highlight').length > 0) {
> +        var highlight = $('.diff-highlight');
> +        if (!commitMessageFileContainer.is(':nth-child(2)')) {
> +          highlight.after(commitMessageFileContainer)
> +        }
> +      } else {
> +        diffs.prepend(commitMessageFileContainer);
> +      }
> +    }
> +  });

Why isn't it the first entry? What is RB doing that causes this and where is it happening?

::: pylib/mozreview/mozreview/templatetags/mozreview.py:52
(Diff revision 18)
> +@register.filter()
> +def commit_message_id(review_request_details):
> +    """Return the id of a commit message file"""
> +    commit_data = fetch_commit_data(review_request_details)
> +    return str(commit_data.get_for(review_request_details, COMMIT_MESSAGE_ID_KEY))
> +
> +@register.filter()
> +def has_commit_message_file(review_request_details):
> +    """Return 'has_commit_message_file' if commit message file has been created
> +    for the review request."""
> +    commit_data = fetch_commit_data(review_request_details)
> +    if commit_data.get_for(review_request_details, COMMIT_MESSAGE_ID_KEY):
> +        return 'has_commit_message_file'
> +    return ''

These don't appear to be used?
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8793703 [details]
MozReview: provide filediff's extra_data as data fields (Bug 1248008)

https://reviewboard.mozilla.org/r/80392/#review83036

::: reviewboard/reviewboard/templates/diffviewer/diff_file_fragment_mozreview.html:53
(Diff revision 3)
> -      data-lines-equal="{{equal_lines}}">
> +  data-lines-equal="{{equal_lines}}"
> +  {% for k, v in file.filediff.extra_data.items %}
> +	data-extra-{{ k }}="{{ v }}"
> +  {% endfor %}>

We should have access to the specific filediff id of the file we're looking for, so I don't think it's needed that we spit out the extra data for everything (especially since Review Board stores a bunch of data in here).

We should include the needed information in our own template of some sort (from the extension) that the javascript can make use of.
Attachment #8793703 - Flags: review?(smacleod) → review-
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review83034

::: reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js:405
(Diff revision 3)
>              if ($anchor.length !== 0) {
>                  this.selectAnchor($anchor);
>                  this._startAtAnchorName = null;
>              }
>          }
>  

(Bogus line for general issue)

Please include a justification for this event in the commit message. Part of me thinks we should be attacking the sorting of the files, rather than trying to move things after render (to avoid jank, and make sure paging won't screw us up). Is there a reason why we can't do that?

::: reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js:406
(Diff revision 3)
> +        // Emit a fileDiffRenderedEvent
> +        this.trigger('fileDiffRendered', diffReviewable.get('fileDiffID'));

I wonder if we should namespace stuff like this so we know it's coming from review board...
Attachment #8791064 - Flags: review?(smacleod) → review-
Comment on attachment 8793700 [details]
MozReview: replace diff_file_fragment (Bug 1248008)

https://reviewboard.mozilla.org/r/80390/#review83038

Clearing until other commits addressed.
Attachment #8793700 - Flags: review?(smacleod)
Comment on attachment 8791063 [details]
MozReview: Add diffViewerPageView_mozreview.js to differentiate commit message file from others (Bug 1248008)

https://reviewboard.mozilla.org/r/78614/#review83040

Clearing until other commits addressed.
Attachment #8791063 - Flags: review?(smacleod)
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review83034

> I wonder if we should namespace stuff like this so we know it's coming from review board...

++ adding this
Comment on attachment 8793703 [details]
MozReview: provide filediff's extra_data as data fields (Bug 1248008)

https://reviewboard.mozilla.org/r/80392/#review83036

> We should have access to the specific filediff id of the file we're looking for, so I don't think it's needed that we spit out the extra data for everything (especially since Review Board stores a bunch of data in here).
> 
> We should include the needed information in our own template of some sort (from the extension) that the javascript can make use of.

That would require us storing somewhere an array of commit message filediff id per revision.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> I was really hoping we wouldn't need to copy this code, as it's quite large and complex.
> 
> I thought you indicated there was another code location sorting was taking place as well? Meaning having the first id wouldn't be enough. What happened to that case? Were we mistaken and having the lower ID will cover it?
> 
> Rather than bringing all this code in I think I'd prefer we hack around it slightly (while not as clean DB wise, code wise it will be much better, IMO).
> 
> I propose to get our FileDiff to have a lower id number we create it first. Since a filediff requires a DiffSet though, we can create a phony DiffSet (hell, we could even reuse the same one for each repository). The DiffSet we create won't be tied to a DiffSetHistory, so it won't show up for any review requests.
> 
> Then, we create the commit FileDiff using the bogus DiffSet and save it, giving it an id. Then once we've called `create_from_data()` we can switch the DiffSet for our commit FileDiff to the result.
> 
> It would look something like:
> ```Python
> temp_diffset = get_temp_diffset()
> commit_file_diff = FileDiff.objects.create(
>     diffset=temp_diffset,
>     ...)
> 
>     
> actual_diffset = DiffSet.objects.create_from_data(...)
> commit_file_diff.diffset = actual_diffset
> commit_file_diff.save()
> ```
> 
> What do you think about this?

Wouldn't it create commit message filefidd as the last file added to the set? Definitely worth a try.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> Why isn't it the first entry? What is RB doing that causes this and where is it happening?

If filediff list is loaded from address, first DIV becomes a highlighting DIV. If it is loaded after moving revisions slider highlight DIV is destroyed and the first filediff becomes the first DIV.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> These don't appear to be used?

Yes, thanks.
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review83034

> ++ adding this

i've used a "mr:" prefix in bug 1253552.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> Wouldn't it create commit message filefidd as the last file added to the set? Definitely worth a try.

Seems fine - the only change in test is the id number of the diffset
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> How will this work for previoius diff revisions? we'll need to store a list or some sort of mapping from diff revision to commit message filediff id.

Previous commit_data will simply not have this. We're currently not using this field, but it might be useful if we will change the method we pick the commit message filediff DOM element. We need to decide if this should happen now (this is possible, there is a need to change template to store the array and listen to review slider's event to find the right pk). This however is only a way to find the element from the same data we've got now.

As of the list - we might store it per review request or generate at will. It's important to store the commit message filediff pk within commit data.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> Seems fine - the only change in test is the id number of the diffset

There is one test which is not working with that method - test-repo-discovery.t
I've got that error:
```
   $ mozreview exec hgrb /var/hg/venv_pash/bin/hg -R /repo/hg/mozilla/autoreview createrepomanifest ${MERCURIAL_URL} ssh://bad.host/
-  36d47cf7c6cbd312de3aeb3b2f770c650b014053 http://$DOCKER_HOSTNAME:$HGPORT/b ssh://bad.host/b
-  4016108b6e06add4c0ddde40dee8c7b9aa410f58 http://$DOCKER_HOSTNAME:$HGPORT/a ssh://bad.host/a
+  abort: repository / not found!
+  [255]
```

The code I'm using:
```python
def get_temp_repository():
    try:
        repository = Repository.objects.get(name='temp repository')
    except Repository.DoesNotExist:
        repository = Repository.objects.create(
            name='temp repository',
            path='/',
            tool=Tool.objects.get(name='Mercurial'))
    return repository


def get_temp_diffset():
    """Get or create a temporary diffset."""
    try:
        diffset = DiffSet.objects.get(name='temp diffset')
    except DiffSet.DoesNotExist:
        diffset = DiffSet.objects.create(
            name='temp diffset',
            revision=0,
            repository=get_temp_repository()
        )
    return diffset

# ... and later in update_review_request
    commit_message_filediff = FileDiff.objects.create(
        diffset=get_temp_diffset(),
        source_file=commit_message_name,
        dest_file=commit_message_name,
        source_revision=PRE_CREATION,
        dest_detail='parent_diff',
        parent_diff='',
        binary=False,
        status='M',
        diff=commit_message_diff)
```
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> There is one test which is not working with that method - test-repo-discovery.t
> I've got that error:
> ```
>    $ mozreview exec hgrb /var/hg/venv_pash/bin/hg -R /repo/hg/mozilla/autoreview createrepomanifest ${MERCURIAL_URL} ssh://bad.host/
> -  36d47cf7c6cbd312de3aeb3b2f770c650b014053 http://$DOCKER_HOSTNAME:$HGPORT/b ssh://bad.host/b
> -  4016108b6e06add4c0ddde40dee8c7b9aa410f58 http://$DOCKER_HOSTNAME:$HGPORT/a ssh://bad.host/a
> +  abort: repository / not found!
> +  [255]
> ```
> 
> The code I'm using:
> ```python
> def get_temp_repository():
>     try:
>         repository = Repository.objects.get(name='temp repository')
>     except Repository.DoesNotExist:
>         repository = Repository.objects.create(
>             name='temp repository',
>             path='/',
>             tool=Tool.objects.get(name='Mercurial'))
>     return repository
> 
> 
> def get_temp_diffset():
>     """Get or create a temporary diffset."""
>     try:
>         diffset = DiffSet.objects.get(name='temp diffset')
>     except DiffSet.DoesNotExist:
>         diffset = DiffSet.objects.create(
>             name='temp diffset',
>             revision=0,
>             repository=get_temp_repository()
>         )
>     return diffset
> 
> # ... and later in update_review_request
>     commit_message_filediff = FileDiff.objects.create(
>         diffset=get_temp_diffset(),
>         source_file=commit_message_name,
>         dest_file=commit_message_name,
>         source_revision=PRE_CREATION,
>         dest_detail='parent_diff',
>         parent_diff='',
>         binary=False,
>         status='M',
>         diff=commit_message_diff)
> ```

No need for a temp repository, just assing the temp diffset to the actual repository for the review request. we'll end up with one temp diffset per repo but that's not a big deal.
Attachment #8793700 - Attachment is obsolete: true
Attachment #8793703 - Attachment is obsolete: true
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> If filediff list is loaded from address, first DIV becomes a highlighting DIV. If it is loaded after moving revisions slider highlight DIV is destroyed and the first filediff becomes the first DIV.

Ya, but we discussed ways to handle this with CSS, why are you needing to add a class instead of using the methods we discussed?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

::: pylib/mozreview/mozreview/resources/batch_review_request.py:918
(Diff revisions 18 - 21)
>  
>  
> -def create_diffset_from_data(repository, diff_file_contents, request,
> -                             base_commit_id, description):
> -    """Create a DiffSet from raw diff data
> -
> +def get_temp_diffset(repository):
> +    """Get or create a temporary diffset."""
> +    try:
> +        diffset = DiffSet.objects.get(name='temp diffset', repository__pk=repository.pk)

I'm not 100% sure on how RB uses the diffset name, it also doesn't have a database index, so I don't think that's what we should rely on to find the temp diffset (this could perform really poorly with the large production DB).

Lets store the ID number of our temp diffset somewhere and use that instead.

::: pylib/mozreview/mozreview/extension.py:302
(Diff revision 21)
> +        TemplateHook(self, 'base-scripts-post', 'mozreview/viewdiff-commit-message-file-js.html',
> +                     apply_to=diffviewer_url_names)

JS should be included as part of a bundle (see how that's done above) - that being said though, this should just follow the conventions of other things and be added to the markup as a data attribute, not a script tag.

::: pylib/mozreview/mozreview/extra_data.py:36
(Diff revision 21)
>  FIRST_PUBLIC_ANCESTOR_KEY = MOZREVIEW_KEY + '.first_public_ancestor'
>  IDENTIFIER_KEY = MOZREVIEW_KEY + '.identifier'
>  SQUASHED_KEY = MOZREVIEW_KEY + '.is_squashed'
>  UNPUBLISHED_KEY = MOZREVIEW_KEY + '.unpublished_rids'
>  PUBLISH_AS_KEY = MOZREVIEW_KEY + '.publish_as'
> +COMMIT_MESSAGE_ID_KEY = MOZREVIEW_KEY + '.commit_message_id'

`COMMIT_MSG_FILEDIFF_IDS_KEY`

::: pylib/mozreview/mozreview/resources/batch_review_request.py:42
(Diff revision 21)
>      ReviewRequest,
>      ReviewRequestDraft,
>  )
>  from reviewboard.scmtools.models import (
>      Repository,
> +    Tool,

You don't appear to use this? please make sure you're running pyflake on your code and not adding any extra errors.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:915
(Diff revision 21)
> +def get_temp_diffset(repository):
> +    """Get or create a temporary diffset."""

Please expand the docstring to include a detailed explanation of why this is needed.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:970
(Diff revision 21)
> +    # get temp diffset
> +    temp_diffset = get_temp_diffset(rr.repository)
> +    # Prepare commit message data
> +    base_commit_id = commit.get('base_commit_id')
> +    commit_message_name = '000-commit-message-%s' % base_commit_id[0:5]
> +    commit_message_lines = draft.description.split('\n')

You should take this from the commit structure passed in, not the draft description (we may change how we use description after this case, it might not always be the commit message).

::: pylib/mozreview/mozreview/resources/batch_review_request.py:971
(Diff revision 21)
> +    temp_diffset = get_temp_diffset(rr.repository)
> +    # Prepare commit message data
> +    base_commit_id = commit.get('base_commit_id')
> +    commit_message_name = '000-commit-message-%s' % base_commit_id[0:5]
> +    commit_message_lines = draft.description.split('\n')
> +    commit_message_diff = 'diff --git a/%s b/%s\nnew file mode 100644\n--- /dev/null\n+++ b/%s\n@@ -0,0 +1,%s @@\n%s' % (

Can you define this as a constant multiline string (with named format identifiers) at the top of the file. ex:
```Python
COMMIT_MSG_DIFF_FORMAT='''
diff --git a/
...
...
...
'''.lsplit()
```

::: pylib/mozreview/mozreview/resources/batch_review_request.py:978
(Diff revision 21)
> +        commit_message_name,
> +        commit_message_name,
> +        len(commit_message_lines),
> +        '%s\n' % '\n'.join(['+%s' % l for l in commit_message_lines]))
> +
> +    # create commit message filediff using temp diffset

Please document that this *must* come before the full diffset creation for the id ordering.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:984
(Diff revision 21)
> +    commit_message_filediff = FileDiff.objects.create(
> +        diffset=temp_diffset,
> +        source_file=commit_message_name,
> +        dest_file=commit_message_name,
> +        source_revision=PRE_CREATION,
> +        dest_detail='parent_diff',

Why are you setting this to parent_diff? this isn't a parent_diff at all, and this field is described as  "destination file details"

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1014
(Diff revision 21)
> -            base_commit_id=commit.get('base_commit_id'),
> +            base_commit_id=base_commit_id,
>              save=True,
>          )
> +
>          update_diffset_history(rr, diffset)
> +        # diffset.extra_data['commit-message-id'] = commit_message_filediff.pk

Leftover?

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1023
(Diff revision 21)
> +    commit_message_filediff.diffset = diffset
> +    commit_message_filediff.save()

This could probably use a comment expalining how we're switching it over from the temporary now that the actual diffset is created.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1027
(Diff revision 21)
> +    commit_message_dict = json.loads(commit_data.draft_extra_data.get(COMMIT_MESSAGE_ID_KEY, '{}'))
> +    commit_message_dict[str(diffset.revision)] = commit_message_filediff.pk

I'd say this variable name is misleading.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:116
(Diff revision 21)
> +  var diffs = $('#diffs');
> +  var currentCommitMessageIds = [-1, commitMessageIds[currentRevisionNumber]];
> +  page.on('mr:revisionSelected', function(revisions) {
> +    // We never know which one will be used to display the commit
> +    // message filediff...
> +    currentCommitMessageIds = [commitMessageIds[revisions[0]], commitMessageIds[revisions[1]]];
> +  });
> +  var commitMessageFileContainer;
> +  var diffFileInfoAnchorTr;
> +  page.on('mr:fileDiffRendered', function(fileDiffID) {
> +    if (currentCommitMessageIds.indexOf(fileDiffID) >= 0) {
> +      // Add class to the TABLE and select its container.
> +      var commitMessageFile = $('#file' + fileDiffID).addClass('commit-message-file');
> +      commitMessageFileContainer = commitMessageFile.parents('.diff-container');
> +      // remove 000 and hashtag from commit message file name
> +      var fileNameRow = commitMessageFile.find('.filename-row th');
> +      var fileNameRowText = fileNameRow.text().trim();
> +      var fileNameRowTextArr = fileNameRowText.split('-');
> +      var newFileNameRowText = fileNameRowTextArr.slice(1, fileNameRowTextArr.length - 1).join('-');
> +      fileNameRow.contents().last()[0].textContent = newFileNameRowText;
> +      var diffFileInfoAnchor = $('.diff-file-info a:contains(' + fileNameRowText + ')')
> +      diffFileInfoAnchorTr = diffFileInfoAnchor.parent().parent();
> +      diffFileInfoAnchor.text(newFileNameRowText);
> +    }
> +    if (diffFileInfoAnchorTr && !diffFileInfoAnchorTr.is(':first-child')) {
> +      diffFileInfoAnchorTr.parent().prepend(diffFileInfoAnchorTr);
> +    }
> +    // Place the container at the top or right after the highlighting DIV.
> +    if (commitMessageFileContainer && !commitMessageFileContainer.is(':first-child')) {
> +      if ($('.diff-highlight').length > 0) {
> +        var highlight = $('.diff-highlight');
> +        if (!commitMessageFileContainer.is(':nth-child(2)')) {
> +          highlight.after(commitMessageFileContainer)
> +        }
> +      } else {
> +        diffs.prepend(commitMessageFileContainer);
> +      }
> +    }
> +  });

I still haven't been convinced any of this is needed? Please either fully document why our commit message would be sorted out of order (the highlight element can be dealt with by css). Or switch to the css method.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:52
(Diff revision 21)
>      """Return the commit id of a review request or review request draft"""
>      commit_data = fetch_commit_data(review_request_details)
>      return str(commit_data.get_for(review_request_details, COMMIT_ID_KEY))
>  
> +@register.filter()
> +def commit_message_ids(review_request_details):

Very misleading function name.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:55
(Diff revision 21)
> +    commit_message_ids = commit_data.get_for(review_request_details, COMMIT_MESSAGE_ID_KEY)
> +    if not commit_message_ids:
> +        return '[]'

`comit_data.get_for()` takes a `default=` kwarg you can use instead of the conditional here.
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review83034

> (Bogus line for general issue)
> 
> Please include a justification for this event in the commit message. Part of me thinks we should be attacking the sorting of the files, rather than trying to move things after render (to avoid jank, and make sure paging won't screw us up). Is there a reason why we can't do that?

You explain what you're doing in the commit message now, but the justification "There is a need to create an event when user switches revisions as different
fildiff might be loaded" doesn't make sense to me. Why would there be a different filediff loaded first? what cases does this happen in?
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review88504

r- until comment on previous review is addressed again.
Attachment #8791064 - Flags: review?(smacleod) → review-
Comment on attachment 8791063 [details]
MozReview: Add diffViewerPageView_mozreview.js to differentiate commit message file from others (Bug 1248008)

https://reviewboard.mozilla.org/r/78614/#review88506

Clearing flag until next update.
Attachment #8791063 - Flags: review?(smacleod)
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> Ya, but we discussed ways to handle this with CSS, why are you needing to add a class instead of using the methods we discussed?

CSS would require to redefine all designs changed for the commit message filediff. If we would declare that a title is italic we would need to redeclare non-italic for others. Which is problematic and would need to be changed every time diff style has changed.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> CSS would require to redefine all designs changed for the commit message filediff. If we would declare that a title is italic we would need to redeclare non-italic for others. Which is problematic and would need to be changed every time diff style has changed.

This is the good CSS solution - https://jsfiddle.net/wwu1uska/15/
To use it we have to always have a commit message filediff (it needs to be created for old diffsets on read or migrate database).
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Why are you setting this to parent_diff? this isn't a parent_diff at all, and this field is described as  "destination file details"

I've printed out other filediffs from the diffset and blindly repeated what was there. 

I've now found in reviewboard that it is a destination revision (and source_revision is a source revision, PRE_CREATION only for the first time) used later in diff: 
    # ...
    source_revision='6bba278',
    dest_detail='465d217',
        diff=(
            b'diff --git a/diffutils.py b/diffutils.py\n'
            b'index 6bba278..465d217 100644\n'
    # ...
    
To make it properly I'll need to find latest revision with commit message file and change accordingly. Is that right?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Please document that this *must* come before the full diffset creation for the id ordering.

Do you remember why we had to make it as a first one?
I'm not so sure we still need to make it as a first file. We do know it's ordered alphabetically. Maybe it is not needed in the end.
I'll check it after finishing other issues.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> I'd say this variable name is misleading.

Changing to commit_message_ids
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Very misleading function name.

I don't know why it's misleading - it actually returns a list of commit message filediff ids...
I've changed it to commit_message_filediff_ids for now.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> This is the good CSS solution - https://jsfiddle.net/wwu1uska/15/
> To use it we have to always have a commit message filediff (it needs to be created for old diffsets on read or migrate database).

Instead of forcing the commit message creation, a class might be added to the body tag - hasCommitMsgFilediff
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> JS should be included as part of a bundle (see how that's done above) - that being said though, this should just follow the conventions of other things and be added to the markup as a data attribute, not a script tag.

Changed to a data attribute.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> I still haven't been convinced any of this is needed? Please either fully document why our commit message would be sorted out of order (the highlight element can be dealt with by css). Or switch to the css method.

Reordering seems to happen somewhere in JS - https://www.dropbox.com/s/xu1avtvkn36e86s/commit-message-default-ordering.mov?dl=0
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> Instead of forcing the commit message creation, a class might be added to the body tag - hasCommitMsgFilediff

We don't want to create filediffs going backwards. We can either use the class method you describe, or stick the css in it's own bundle which we optionally include in a templatehook. Either should be fine. As for the name, please use dash seperated lowercase (or underscores, we mix both :/) rather than camel case.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Do you remember why we had to make it as a first one?
> I'm not so sure we still need to make it as a first file. We do know it's ordered alphabetically. Maybe it is not needed in the end.
> I'll check it after finishing other issues.

Ordering is not done alphabetically, it's done by primary key order - except in the case of header vs source files of the same name. This is why we need to create it first.

> I've printed out other filediffs from the diffset and blindly repeated what was there. 
> 
> I've now found in reviewboard that it is a destination revision (and source_revision is a source revision, PRE_CREATION only for the first time) used later in diff: 
>     # ...
>     source_revision='6bba278',
>     dest_detail='465d217',
>         diff=(
>             b'diff --git a/diffutils.py b/diffutils.py\n'
>             b'index 6bba278..465d217 100644\n'
>     # ...
>     
> To make it properly I'll need to find latest revision with commit message file and change accordingly. Is that right?

Since the commit message file is considered a new file, I believe what you want here is probably the value of PRE_CREATION. Please look into what RB does here though.

> Changing to commit_message_ids

please include `filediff` in the name.

> Reordering seems to happen somewhere in JS - https://www.dropbox.com/s/xu1avtvkn36e86s/commit-message-default-ordering.mov?dl=0

Can you investigate where Review Board is doing this, it might be a quick fix either in the fork or by poking at the js initially so that we don't have to run this and move our file everytime. I'd also like to be sure it's not possible our commit message ends up on the second page so a JS fix like this wouldn't catch it.

> I don't know why it's misleading - it actually returns a list of commit message filediff ids...
> I've changed it to commit_message_filediff_ids for now.

It's not clear what the id is for. Is it the sha of a commit that has a commit message we want? is it a filediff? etc. It'll be much easier to understand later if we explicitly call out `filediff` here.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Ordering is not done alphabetically, it's done by primary key order - except in the case of header vs source files of the same name. This is why we need to create it first.

We've changed the name of the file with a 000- prefix to be sure it comes first.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> We've changed the name of the file with a 000- prefix to be sure it comes first.

I'll keep it, just trying to understand where it matters.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> We don't want to create filediffs going backwards. We can either use the class method you describe, or stick the css in it's own bundle which we optionally include in a templatehook. Either should be fine. As for the name, please use dash seperated lowercase (or underscores, we mix both :/) rather than camel case.

className it is then.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> className it is then.

No, the opposite, I specifically *don't* want camelCase. please use lowercase with dashes seperating the words.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review83032

> No, the opposite, I specifically *don't* want camelCase. please use lowercase with dashes seperating the words.

yes yes - I was referring that the solution would involve setting the class name.
className is the attribute/function which is setting it - I even had with dashes in the code already :)
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Since the commit message file is considered a new file, I believe what you want here is probably the value of PRE_CREATION. Please look into what RB does here though.

I've been logging this value and in local repo new files have it empty...
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Can you investigate where Review Board is doing this, it might be a quick fix either in the fork or by poking at the js initially so that we don't have to run this and move our file everytime. I'd also like to be sure it's not possible our commit message ends up on the second page so a JS fix like this wouldn't catch it.

It happens here - https://reviewboard-hg.mozilla.org/reviewboard/file/0b7119c8649a699c739f308cb39b429183734a8c/reviewboard/reviewboard/static/rb/js/pages/models/diffViewerPageModel.js#l49
`attrs.files.models` is ordered as received from server and after set is called `this.attributes.files` is ordered as we see in UI
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> I'll keep it, just trying to understand where it matters.

It all came from the fact when we found that RB was ordering by primary key on initial load (with the exception of swapping header / source files with the same name). This corresponds to the same order the files appear in the generated diff... which I guess is why it does this? We've known about this for many weeks - are you indicating you've found something that makes us incorrect?

> It happens here - https://reviewboard-hg.mozilla.org/reviewboard/file/0b7119c8649a699c739f308cb39b429183734a8c/reviewboard/reviewboard/static/rb/js/pages/models/diffViewerPageModel.js#l49
> `attrs.files.models` is ordered as received from server and after set is called `this.attributes.files` is ordered as we see in UI

So you're saying it's coming from the server sorted differently when it's an API request, rather than the initial rendering? Where in the backend is it doing the sorting?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> So you're saying it's coming from the server sorted differently when it's an API request, rather than the initial rendering? Where in the backend is it doing the sorting?

Server is providing a desired order (alphabetical). In file mentioned above `set` function is called to set the model's `attributes.files` - `this.attributes.files.set(attrs.files.models)`. Unfortunately Backbone Collection's `set` function is not updating objects which already are present and adds the other objects at the end of the set. This changes the order of filediffs.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Server is providing a desired order (alphabetical). In file mentioned above `set` function is called to set the model's `attributes.files` - `this.attributes.files.set(attrs.files.models)`. Unfortunately Backbone Collection's `set` function is not updating objects which already are present and adds the other objects at the end of the set. This changes the order of filediffs.

To be more precise, The server provides the `FileDiff`s sorted by primary key
in ascending order. This happens to be alphabetical as a coincidence rather
than purposefully (The `FileDiff`s are created in the order they appear in the
diff file itself, which is coming alphabetically from mercurial). The
distinction is quite important as it's why we use the temporary `DiffSet` to
create our commit `FileDiff` first.

With respect to the ordering when updating the slider, you can take a look at
the source code for backbone's set function[1] to see that by default it
performs a merge of the existing models, rather than fulling replacing them
(New items are added, missing items are removed, and existing items are merged).
It appears this merge operation does not preserve the order of the models
which are passed into set (instead the existing items end up before the others).

So, with all that being said, I'd call Review Board's ordering of the
`FileDiff`s after the slider move a bug. This is definitely something we'll need
to fix ourselves as part of this change so it can be deployed ASAP.

Please investigate a solution for fixing this in the fork. My gut tells me we
might just be able to turn off the merge feature of the `set()` by passing
that option in - I'm not sure if that tells it to fully replace the models or
to not bother touching existing ones. Another option would be to actually
perform a sort on the models, using something that will preserve the order we
want (ascending `FileDiff` primary key).


[1] http://backbonejs.org/docs/backbone.html#section-111
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> To be more precise, The server provides the `FileDiff`s sorted by primary key
> in ascending order. This happens to be alphabetical as a coincidence rather
> than purposefully (The `FileDiff`s are created in the order they appear in the
> diff file itself, which is coming alphabetically from mercurial). The
> distinction is quite important as it's why we use the temporary `DiffSet` to
> create our commit `FileDiff` first.
> 
> With respect to the ordering when updating the slider, you can take a look at
> the source code for backbone's set function[1] to see that by default it
> performs a merge of the existing models, rather than fulling replacing them
> (New items are added, missing items are removed, and existing items are merged).
> It appears this merge operation does not preserve the order of the models
> which are passed into set (instead the existing items end up before the others).
> 
> So, with all that being said, I'd call Review Board's ordering of the
> `FileDiff`s after the slider move a bug. This is definitely something we'll need
> to fix ourselves as part of this change so it can be deployed ASAP.
> 
> Please investigate a solution for fixing this in the fork. My gut tells me we
> might just be able to turn off the merge feature of the `set()` by passing
> that option in - I'm not sure if that tells it to fully replace the models or
> to not bother touching existing ones. Another option would be to actually
> perform a sort on the models, using something that will preserve the order we
> want (ascending `FileDiff` primary key).
> 
> 
> [1] http://backbonejs.org/docs/backbone.html#section-111

Ah, right, the Web API endpoint being used for the no reload diff revision
update *is* sorting alphabetically. I wonder if it's worth "fixing" that
so it's consistent with the full page load behaviour? Either way this
shouldn't affect the fix that's needed for the backbone stuff.
Attachment #8791063 - Attachment is obsolete: true
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Ah, right, the Web API endpoint being used for the no reload diff revision
> update *is* sorting alphabetically. I wonder if it's worth "fixing" that
> so it's consistent with the full page load behaviour? Either way this
> shouldn't affect the fix that's needed for the backbone stuff.

Ordering is now done in the fork. This file and events are now used only to rename the commit message filename. 
If there would be a way to mark the `DiffFile` commit message object, collection could be sorted by that field as well. That would give us an opportunity to remove the prefix from filename and remove this modification (delete events as well).
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Ordering is now done in the fork. This file and events are now used only to rename the commit message filename. 
> If there would be a way to mark the `DiffFile` commit message object, collection could be sorted by that field as well. That would give us an opportunity to remove the prefix from filename and remove this modification (delete events as well).

Couldn't we just have it sort by `FileDiff` id as well (which would keep thinks consistent)? I think that solves all the issues.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> It all came from the fact when we found that RB was ordering by primary key on initial load (with the exception of swapping header / source files with the same name). This corresponds to the same order the files appear in the generated diff... which I guess is why it does this? We've known about this for many weeks - are you indicating you've found something that makes us incorrect?

Filename would be irrelevant if order by id would be identical to order by destFilename with the exception of injected commit-msg-filediff as a first element. But it's as we said before and we're keeping the prefix.
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review83034

> You explain what you're doing in the commit message now, but the justification "There is a need to create an event when user switches revisions as different
> fildiff might be loaded" doesn't make sense to me. Why would there be a different filediff loaded first? what cases does this happen in?

I guess it should read
"different filediff might be loaded for the commit message in chosen revision."
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> I'm not 100% sure on how RB uses the diffset name, it also doesn't have a database index, so I don't think that's what we should rely on to find the temp diffset (this could perform really poorly with the large production DB).
> 
> Lets store the ID number of our temp diffset somewhere and use that instead.

Re-opening, you have marked this as fixed but did not fix it.

> `COMMIT_MSG_FILEDIFF_IDS_KEY`

Re-opening, you've added an S, but did not include "FILEDIFF" in the name. Please go with the name I've suggested.

> Couldn't we just have it sort by `FileDiff` id as well (which would keep thinks consistent)? I think that solves all the issues.

re-opening this issue. As I said, we should just be able to make the files sort by ID in the fork, which means we can get rid of the commit message file prefix, as well as the javascript hook you're using to rename it. Please make this change.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

Please ensure you're not closing issues which have not been fixed.

::: hgext/reviewboard/tests/test-commit-message-as-file.t:17
(Diff revision 23)
> +  $ echo foo1 > foo.h
> +  $ hg commit -m 'Bug 1 - Foo 1'
> +  $ hg push
> +  pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
> +  (adding commit id to 1 changesets)
> +  saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg (glob)

you have a `(glob)` on this but you're not doing any globbing? Please ensure the identifiers of the strip backup are stable or properly glob the parts that aren't.

::: hgext/reviewboard/tests/test-commit-message-as-file.t:89
(Diff revision 23)
> +    - +foo1
> +    - ''
> +  approved: false
> +  approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +Amended commit message should result with a changed attachment

Changed attachment? You're not looking at attachments after this.

::: hgext/reviewboard/tests/test-commit-message-as-file.t:92
(Diff revision 23)
> +  approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +Amended commit message should result with a changed attachment
> +
> +  $ hg commit --amend -m 'Bug 1 - Foo 2 - amended'
> +  saved backup bundle to $TESTTMP/client/.hg/strip-backup/4a9cf7e91820-f76b5126-amend-backup.hg (glob)

`(glob)` again.

::: hgext/reviewboard/tests/test-commit-message-as-file.t:96
(Diff revision 23)
> +  $ hg commit --amend -m 'Bug 1 - Foo 2 - amended'
> +  saved backup bundle to $TESTTMP/client/.hg/strip-backup/4a9cf7e91820-f76b5126-amend-backup.hg (glob)
> +  $ hg push
> +  pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
> +  (adding commit id to 1 changesets)
> +  saved backup bundle to $TESTTMP/client/.hg/strip-backup/2358ed0ad9b5-42479a45-addcommitid.hg (glob)

`(glob)` again.

::: hgext/reviewboard/tests/test-commit-message-as-file.t:117
(Diff revision 23)
> +  (review requests lack reviewers; visit review url to assign reviewers)
> +  
> +  publish these review requests now (Yn)?  y
> +  (published review request 1)
> +
> +Test commit message file creation

Please be more specific, what is this next part of the test actually attempting to verify?

::: pylib/mozreview/mozreview/extension.py:302
(Diff revision 23)
> +        TemplateHook(self, 'base-scripts-post', 'mozreview/viewdiff-commit-msg-file-data.html',
> +                     apply_to=diffviewer_url_names)

You generally use `'base-scripts-post'` for javascript rather than html. `'base-after-content'` is a better location for this.

Also, since we're adding this new template for data related to the diff, it's probably better to give it a generic name like "diff-data.html", to match the "user-data.html" that already exists. This way if we have anymore diff data to inject we have a nice place for it and don't create extra TemplateHooks or changes.

::: pylib/mozreview/mozreview/extra_data.py:36
(Diff revision 23)
>  FIRST_PUBLIC_ANCESTOR_KEY = MOZREVIEW_KEY + '.first_public_ancestor'
>  IDENTIFIER_KEY = MOZREVIEW_KEY + '.identifier'
>  SQUASHED_KEY = MOZREVIEW_KEY + '.is_squashed'
>  UNPUBLISHED_KEY = MOZREVIEW_KEY + '.unpublished_rids'
>  PUBLISH_AS_KEY = MOZREVIEW_KEY + '.publish_as'
> +COMMIT_MESSAGE_IDS_KEY = MOZREVIEW_KEY + '.commit_message_ids'

I've reopened the original issue for this name, please take a look.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:918
(Diff revision 23)
> +    A diffset is needed to create a commit message filediff as the first one
> +    of the set. It is returned here and created if none found. Relation to
> +    temp diffset is gonna be changed to the right one after its creation."""

```
Creating a FileDiff requires an already existing DiffSet
(There is a non-nullable foreign key). Since we must
create a particular FileDiff before its DiffSet is created
we first create a temporary DiffSet to point at.

Create this temporary DiffSet, or return it if it has been
created in the past."""
```

::: pylib/mozreview/mozreview/resources/batch_review_request.py:922
(Diff revision 23)
> +
> +    A diffset is needed to create a commit message filediff as the first one
> +    of the set. It is returned here and created if none found. Relation to
> +    temp diffset is gonna be changed to the right one after its creation."""
> +    try:
> +        diffset = DiffSet.objects.get(name='temp diffset', repository__pk=repository.pk)

Please see re-opened issue about this line.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:969
(Diff revision 23)
> +    # Commit message filediff needs to be created as a first filediff in a set
> +    # Get temp diffset for commit_message_filediff creation.
> +    temp_diffset = get_temp_diffset(rr.repository)

> Commit message filediff needs to be created as a first filediff in a set

In the future someone reading this code would want to know *why* not just that it has to be. Please document this in detail.

> Get temp diffset for commit_message_filediff creation.

The reasoning for this should also be rolled into the comment.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:973
(Diff revision 23)
> +    base_commit_id = commit.get('base_commit_id')
> +    commit_message_name = '000-commit-message-%s' % base_commit_id[0:5]

This is going to change when the commit series has been rebased, which will manifest really strangely in the interdiff (We'll show the previous commit message file as deleted, and a completely new one as added, instead of properly showing the difference). You'll need to use something stable for this commit throughout the entire series.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:982
(Diff revision 23)
> +    # Create commit message filediff using temp diffset
> +    commit_message_filediff = FileDiff.objects.create(
> +        diffset=temp_diffset,
> +        source_file=commit_message_name,
> +        dest_file=commit_message_name,
> +        source_revision=PRE_CREATION,
> +        dest_detail='',
> +        parent_diff='',
> +        binary=False,
> +        status='M',
> +        diff=commit_message_diff)

Creating the commit message filediff should only be done if this is a completely new commit review request, or if the review request we're updating already had commit filediffs.

We don't want to all of a sudden add commit message files to review requests that didn't start with them.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1002
(Diff revision 23)
>      for user in sorted(reviewer_users):
>          draft.target_people.add(user)
>          logger.debug('adding reviewer %s to #%d' % (user.username, rr.id))
>  
>      try:
> +        # Create the diffset from raw data

This comment doesn't really add any extra understanding to the code.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1025
(Diff revision 23)
> +    # Assign the commit message filediff to the right diffset.
> +    commit_message_filediff.diffset = diffset
> +    commit_message_filediff.save()

Something like the following would be much more informative:
```
# Now that the proper DiffSet has been created, re-assign
# the commit message FileDiff we created to the new DiffSet.
```

(Side note, please properly capatalize the python class names when you are referring to them - this should be done throughout the entire patch, not just this line)

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:92
(Diff revision 23)
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr td, 
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.insert tr td,
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.replace tr td, 
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.replace tr td {
> +  background-color: #ffdfd7;
> +  border-color: #bdd8b7;
> +}
> +
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr.selected, 
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.insert tr.selected, 
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.replace tr.selected, 
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.replace tr.selected {
> +  background-color: #e9bdac;
> +}
> +
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr th, 
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.insert tr th, 
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.replace tr th, 
> +.has-commit-msg-file :not(.diff-container) + .diff-container .sidebyside tbody.replace tr th {
> +  background-color: #ddb8b7;

Please use the features of less to get rid of all the redundancy here.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:92
(Diff revision 23)
>  }
>  .comment-block-container-draft {
>    border-top: 2px dotted #aceb6f;  // @comment-flag-draft-color;
>  }
> +
> +.has-commit-msg-file .diff-container:first-child .sidebyside tbody.insert tr td, 

Please ensure there is no trailing whitespace. You have it on several of the lines you've added here.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:96
(Diff revision 23)
> +  background-color: #ffdfd7;
> +  border-color: #bdd8b7;

Please include some screenshots of what the new styling looks like in your next update.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:124
(Diff revision 23)
> +  if (currentRevisionNumber) {
> +    var commitMessageIds = commitMessageIdsData.data('commitMessageIds');
> +  }
> +  if (currentRevisionNumber && commitMessageIds[currentRevisionNumber]) {
> +    // Revision has commit message file element. Mark body accordingly.
> +    $('body').addClass('has-commit-msg-file');

Will this result in a flicker when the page is loading? You could use a TemplateHook to conditionally load your CSS depending on the request, or two template hooks (one before content, one after) to wrap things in an element conditionally (not sure if this will mess up RBs CSS though).

If this doesn't flicker because of the order the JS runs, I'd prefer we add the class to an element much closer to the FileDiffs, rather than the body.

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:127
(Diff revision 23)
> +    page.on('mr:revisionSelected', function(revisions) {
> +      // Revision slider triggers an event giving two revision numbers.
> +      // Displayed commit message file will be related to one of them.
> +      currentCommitMessageIds = [commitMessageIds[revisions[0]], commitMessageIds[revisions[1]]];
> +    });
> +
> +    // Rename the commit message filename every time it loads.
> +    page.on('mr:fileDiffRendered', function(fileDiffID) {
> +      // Check if rendered filediff is a commit message file related to
> +      // current revision
> +      if (currentCommitMessageIds.indexOf(fileDiffID) >= 0) {
> +        var commitMessageFile = $('#file' + fileDiffID);
> +        // Remove 000 and the hashtag from commit message file name
> +        var fileNameRow = commitMessageFile.find('.filename-row th');
> +        var fileNameRowText = fileNameRow.text().trim();
> +        var fileNameRowTextArr = fileNameRowText.split('-');
> +        var newFileNameRowText = fileNameRowTextArr.slice(1, fileNameRowTextArr.length - 1).join('-');
> +        fileNameRow.contents().last()[0].textContent = newFileNameRowText;
> +        // Rename commit message filename in the file info list
> +        var diffFileInfoAnchor = $('.diff-file-info a:contains(' + fileNameRowText + ')')
> +        var diffFileInfoAnchorTr = diffFileInfoAnchor.parent().parent();
> +        diffFileInfoAnchor.text(newFileNameRowText);
> +      }
> +    });

Please see reopened issue in my previous review talking about how we can get rid of these listeners.
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8807138 [details]
Specify ordering filediffs by destFilename (Bug 1248008)

https://reviewboard.mozilla.org/r/90402/#review91976

::: reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js:6
(Diff revision 1)
>  /*
>   * A collection of files.
>   */
>  RB.DiffFileCollection = Backbone.Collection.extend({
> -    model: RB.DiffFile
> +    model: RB.DiffFile,
> +    comparator: 'destFilename'

comparing by id should get rid of a bunch of the sorting issues - see review request in our extension for more information.
Attachment #8807138 - Flags: review?(smacleod) → review-
Comment on attachment 8791064 [details]
MozReview: Trigger an event on commit message file related action  (Bug 1248008)

https://reviewboard.mozilla.org/r/78616/#review91980

in the extension review request I talk about how we don't need this.
Attachment #8791064 - Flags: review?(smacleod) → review-
Comment on attachment 8807137 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)

https://reviewboard.mozilla.org/r/90400/#review91984

::: reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js:1
(Diff revision 1)
> +/*
> + * A collection of files.
> + */
> +RB.DiffFileCollection = Backbone.Collection.extend({
> +    model: RB.DiffFile
> +});

This file should be marked as a copy of the old file. Please use "hg cp" instead  of just creating a new file.
Attachment #8807137 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Re-opening, you have marked this as fixed but did not fix it.

I had a broken repository and this slipped from copying, sorry.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> you have a `(glob)` on this but you're not doing any globbing? Please ensure the identifiers of the strip backup are stable or properly glob the parts that aren't.

I got it from other test, I think it was `test-commit-s-added.t` - doesn't `$TESTTMP` need to be accompanied with `(glob)`?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Please be more specific, what is this next part of the test actually attempting to verify?

I've removed it after fixing the "attachment" bit
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> ```
> Creating a FileDiff requires an already existing DiffSet
> (There is a non-nullable foreign key). Since we must
> create a particular FileDiff before its DiffSet is created
> we first create a temporary DiffSet to point at.
> 
> Create this temporary DiffSet, or return it if it has been
> created in the past."""
> ```

Thanks
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> > Commit message filediff needs to be created as a first filediff in a set
> 
> In the future someone reading this code would want to know *why* not just that it has to be. Please document this in detail.
> 
> > Get temp diffset for commit_message_filediff creation.
> 
> The reasoning for this should also be rolled into the comment.

I've written this instead:
    # As commit message filediff has to be displayed as the first file it
    # needs to be created before other filediffs in the DiffSet.
    # FileDiff object has a required `DiffSet` field. Since before commit
    # filediff is created we must create a temporary DiffSet.
    # Later in the code it is replaced with the target one.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> This is going to change when the commit series has been rebased, which will manifest really strangely in the interdiff (We'll show the previous commit message file as deleted, and a completely new one as added, instead of properly showing the difference). You'll need to use something stable for this commit throughout the entire series.

What do you think about storing the commit_message_name (with first commit_id) in extra_data?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Something like the following would be much more informative:
> ```
> # Now that the proper DiffSet has been created, re-assign
> # the commit message FileDiff we created to the new DiffSet.
> ```
> 
> (Side note, please properly capatalize the python class names when you are referring to them - this should be done throughout the entire patch, not just this line)

Do you think I should fix entire file? There is a lot of that outside of the patch.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Will this result in a flicker when the page is loading? You could use a TemplateHook to conditionally load your CSS depending on the request, or two template hooks (one before content, one after) to wrap things in an element conditionally (not sure if this will mess up RBs CSS though).
> 
> If this doesn't flicker because of the order the JS runs, I'd prefer we add the class to an element much closer to the FileDiffs, rather than the body.

It doesn't flicker, the class is set before the filediff is loaded (even on local network)
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Please see reopened issue in my previous review talking about how we can get rid of these listeners.

When it's gonna be ordered by ID we can remove the '000-' prefix, but we need to decide what to do with the suffix.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Creating the commit message filediff should only be done if this is a completely new commit review request, or if the review request we're updating already had commit filediffs.
> 
> We don't want to all of a sudden add commit message files to review requests that didn't start with them.

I've got it done, I'm creating the commit msg filediff in such case:

    if (commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False) or not rr.get_latest_diffset()):

I have trouble with testing.
I wanted to remove the `p2rb.commit_message_ids` from database using direct call to sqlite3, but there is a problem with parentheses.

This is the SQL I'm trying to run within test:

    UPDATE mozreview_commitdata SET extra_data='{p2rb.first_public_ancestor: 'df67364c205763de5ad1d2c33fa78f87f6618289', p2rb.is_squashed: false, p2rb.author: 'test', p2rb.identifier: 'bz://1/mynick', p2rb.commit_id: 'ecfec372b48ce7b49eccf655e422acb4283283e0', p2rb: true}' WHERE review_request_id=2
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> re-opening this issue. As I said, we should just be able to make the files sort by ID in the fork, which means we can get rid of the commit message file prefix, as well as the javascript hook you're using to rename it. Please make this change.

`DiffViewerView` is extending `TemplateView`. It's `get` method is called every time server is preparing the diffsets list. The `get_context_data` is calling `diffviewer.diffutils.get_diff_files` which is sorting the result using `get_sorted_filediffs` from the same module.

If the dataflow is changed (`get_diff_files` returns unsorted list), server is responding with the order by id fashion, however in the front-end an alphabetical order is forced by JS. To fix this I've changed the `comparator` field in `reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js`[1] from `destFilename` to `id`. 

Interdiff `0-3` has desired order, but for interdiff `1-3` the order is `[commit-message, foo, bar]` which relates to their ids `[2, 3, 10]`. This is no longer alphabetical and is changing across other interdiffs. I don't think it's a desired solution.

Note: The change in JS would be enough to place the commit msg filediff on top of the list. There could be a potential issue in paging if many files would come before commit message filediff filename as ordering on the server would remain by `dest_filename`.

[1] https://reviewboard.mozilla.org/r/90402/diff/1#0
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> It doesn't flicker, the class is set before the filediff is loaded (even on local network)

The `.has-commit-msg-file` is now added to `#diffs`
Blocks: 1296135
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> `DiffViewerView` is extending `TemplateView`. It's `get` method is called every time server is preparing the diffsets list. The `get_context_data` is calling `diffviewer.diffutils.get_diff_files` which is sorting the result using `get_sorted_filediffs` from the same module.
> 
> If the dataflow is changed (`get_diff_files` returns unsorted list), server is responding with the order by id fashion, however in the front-end an alphabetical order is forced by JS. To fix this I've changed the `comparator` field in `reviewboard/reviewboard/static/rb/js/diffviewer/collections/diffFileCollection_mozreview.js`[1] from `destFilename` to `id`. 
> 
> Interdiff `0-3` has desired order, but for interdiff `1-3` the order is `[commit-message, foo, bar]` which relates to their ids `[2, 3, 10]`. This is no longer alphabetical and is changing across other interdiffs. I don't think it's a desired solution.
> 
> Note: The change in JS would be enough to place the commit msg filediff on top of the list. There could be a potential issue in paging if many files would come before commit message filediff filename as ordering on the server would remain by `dest_filename`.
> 
> [1] https://reviewboard.mozilla.org/r/90402/diff/1#0

We agreed on following steps:

Check if pagination is indeed an issue if ordering by id is done in front-end only. That would eliminate the need to hack the `get_diff_files` in the fork.

There is no problem if order of files changes in interdiffs as long as paging is still right.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> What do you think about storing the commit_message_name (with first commit_id) in extra_data?

Try to use a plain "commit-message" as a filenam. An additional check is needed on interdiffs.

> I've got it done, I'm creating the commit msg filediff in such case:
> 
>     if (commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False) or not rr.get_latest_diffset()):
> 
> I have trouble with testing.
> I wanted to remove the `p2rb.commit_message_ids` from database using direct call to sqlite3, but there is a problem with parentheses.
> 
> This is the SQL I'm trying to run within test:
> 
>     UPDATE mozreview_commitdata SET extra_data='{p2rb.first_public_ancestor: 'df67364c205763de5ad1d2c33fa78f87f6618289', p2rb.is_squashed: false, p2rb.author: 'test', p2rb.identifier: 'bz://1/mynick', p2rb.commit_id: 'ecfec372b48ce7b49eccf655e422acb4283283e0', p2rb: true}' WHERE review_request_id=2

Create a SQL fixtures containing an entire structure using the code created before this task. Inject the file and test against old-style data.

> When it's gonna be ordered by ID we can remove the '000-' prefix, but we need to decide what to do with the suffix.

This might be solved in 2 ways without using JavaScript. 
1. It might be possible to change the destFilename to plain 'commit-message' (requires additional tests mentioned above)
2. The name might be chanegd using CSS
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> The `.has-commit-msg-file` is now added to `#diffs`

There is an issue in the condition under this class is added. It is added regardless of the page. This has to happen only on the first page of diffs.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> I got it from other test, I think it was `test-commit-s-added.t` - doesn't `$TESTTMP` need to be accompanied with `(glob)`?

Just to get it written down, we discussed this in our 1-1, and you do not need `(glob)` for just using the `$<name>` variables the test provides.

> I've written this instead:
>     # As commit message filediff has to be displayed as the first file it
>     # needs to be created before other filediffs in the DiffSet.
>     # FileDiff object has a required `DiffSet` field. Since before commit
>     # filediff is created we must create a temporary DiffSet.
>     # Later in the code it is replaced with the target one.

Sounds good.

> Do you think I should fix entire file? There is a lot of that outside of the patch.

Keeping it to new code is fine.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Try to use a plain "commit-message" as a filenam. An additional check is needed on interdiffs.

Interdiffs fail if the name is the same. Unique name needs to be stored in extra_data.

> This might be solved in 2 ways without using JavaScript. 
> 1. It might be possible to change the destFilename to plain 'commit-message' (requires additional tests mentioned above)
> 2. The name might be chanegd using CSS

CSS is the way
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Just to get it written down, we discussed this in our 1-1, and you do not need `(glob)` for just using the `$<name>` variables the test provides.

using hg push in test is printing (glob) out:
    
    $ hg push
    pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
    (adding commit id to 1 changesets)
    -  saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg
    +  saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg (glob)
Comment on attachment 8807137 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)

https://reviewboard.mozilla.org/r/90400/#review91984

> This file should be marked as a copy of the old file. Please use "hg cp" instead  of just creating a new file.

I'm wondering how to achieve that in MozReview - should I discard the review request and create a new one?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Create a SQL fixtures containing an entire structure using the code created before this task. Inject the file and test against old-style data.

I've got trouble entering the sql file with commands to sqlite, tried piping subprocesses, but no luck
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> We agreed on following steps:
> 
> Check if pagination is indeed an issue if ordering by id is done in front-end only. That would eliminate the need to hack the `get_diff_files` in the fork.
> 
> There is no problem if order of files changes in interdiffs as long as paging is still right.

Server provides the `FileDiff`s in primary key order, but it sorts them if paging is involved.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Server provides the `FileDiff`s in primary key order, but it sorts them if paging is involved.

I'm sorry - my last comment might be misunderstood. 
If `get_diff_files` is not changed paging becomes an issue. Which means that commit message `FileDiff` might be displayed on a page other than the first one.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> CSS is the way

I think it's hard/impossible to do without adding <span> around the name in `FileDiff` row and list.
So some additional change in the fork is needed.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> I'm sorry - my last comment might be misunderstood. 
> If `get_diff_files` is not changed paging becomes an issue. Which means that commit message `FileDiff` might be displayed on a page other than the first one.

Okay, so we discussed in IRC and you've let me know the sorting will happen server side if pagination is required. Meaning we'll need to modify the fork to prevent this sort and ensure our commit message gets put on the first page and is first.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> using hg push in test is printing (glob) out:
>     
>     $ hg push
>     pushing to ssh://$DOCKER_HOSTNAME:$HGPORT6/test-repo
>     (adding commit id to 1 changesets)
>     -  saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg
>     +  saved backup bundle to $TESTTMP/client/.hg/strip-backup/8d901902ff4b-c4c7c89c-addcommitid.hg (glob)

Huh, strange - I don't know what causes the glob to be required...

> I think it's hard/impossible to do without adding <span> around the name in `FileDiff` row and list.
> So some additional change in the fork is needed.

Hard to have the CSS do it without new markup? if you post a small fiddle or something like with the selecting first child I could take a look - I guess if we're already making fork changes for the sorting and js and stuff a little markup change wouldn't be a huge deal though.
Comment on attachment 8807137 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)

https://reviewboard.mozilla.org/r/90400/#review91984

> I'm wondering how to achieve that in MozReview - should I discard the review request and create a new one?

Ah, nope, no need to discard the review request. You'll just need to change it so the commit is created by using `hg cp ...` instead of manually copying the file contents. There might be a way to automatically recognise this, I think :gps has mentioned it before. But after you've done that as long as it has the same MozReview-Commit-Id when you push it will be fine.
Attachment #8791064 - Attachment is obsolete: true
Attachment #8807138 - Attachment is obsolete: true
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Huh, strange - I don't know what causes the glob to be required...

Dropping it then. We can create another bug as it touches other tests as well
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Hard to have the CSS do it without new markup? if you post a small fiddle or something like with the selecting first child I could take a look - I guess if we're already making fork changes for the sorting and js and stuff a little markup change wouldn't be a huge deal though.

Minimal example: https://jsfiddle.net/zalun/o733uyvs/2/
Attachment #8807137 - Attachment is obsolete: true
Attachment #8807137 - Flags: review?(smacleod)
Attachment #8824353 - Attachment is obsolete: true
Attachment #8824353 - Flags: review?(smacleod)
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> Minimal example: https://jsfiddle.net/zalun/o733uyvs/2/

And the solution - https://jsfiddle.net/o733uyvs/6/
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> And the solution - https://jsfiddle.net/o733uyvs/6/

I've got it working for the DiffFile, but #diff_index is more complicated (filename sits within an anchor). :davidwalsh implemented a mr:diffview-loaded event which does exactly the same as our former mr:FileDiffRendered [1]. I think it would be OK to create another event called mr:diff_context-loaded whih would be placed in the same file after context is loaded [2]. 

Are you OK with that?

[1] https://hg.mozilla.org/webtools/reviewboard/rev/8c09c3912c62
[2] https://reviewboard-hg.mozilla.org/reviewboard/file/tip/reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js#l735
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review88476

> Okay, so we discussed in IRC and you've let me know the sorting will happen server side if pagination is required. Meaning we'll need to modify the fork to prevent this sort and ensure our commit message gets put on the first page and is first.

Fixed in https://reviewboard.mozilla.org/r/102916/diff/2
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review91938

> I've got it working for the DiffFile, but #diff_index is more complicated (filename sits within an anchor). :davidwalsh implemented a mr:diffview-loaded event which does exactly the same as our former mr:FileDiffRendered [1]. I think it would be OK to create another event called mr:diff_context-loaded whih would be placed in the same file after context is loaded [2]. 
> 
> Are you OK with that?
> 
> [1] https://hg.mozilla.org/webtools/reviewboard/rev/8c09c3912c62
> [2] https://reviewboard-hg.mozilla.org/reviewboard/file/tip/reviewboard/reviewboard/static/rb/js/pages/views/diffViewerPageView_mozreview.js#l735

Ya I guess it'd be fine to do that. Thanks for the links :)
Comment on attachment 8824386 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) Bug 1248008

https://reviewboard.mozilla.org/r/102916/#review105770

Just a nit about the comment, should be r+ after that's updated.

::: reviewboard/reviewboard/diffviewer/diffutils.py:407
(Diff revision 2)
> -    if len(files) == 1:
> +    # MozReview is adding a commit message filediff which has to be the first
> +    # from the set. Sorting by dest_filename would be causing issues when
> +    # paging is involved.

We should mention that since the incoming diffs are already sorted in a logical way (by hg when it generates them) changing this won't really matter.

Something like:
```
# MozReview is adding a commit message filediff which has to be the first
# from the set. Sorting by dest_filename would cause issues when paging
# is involved. Since the filediff's are already in a logical order when
# sorted by id (which they should be here) removing this sort shouldn't
# make UX worse.
```
Attachment #8824386 - Flags: review?(smacleod) → review-
Comment on attachment 8824452 [details]
Add diffFileCollection_mozreview.js to specify sorting by id (Bug 1248008)

https://reviewboard.mozilla.org/r/102954/#review105772
Attachment #8824452 - Flags: review?(smacleod) → review+
Comment on attachment 8824453 [details]
MozReview: Specify ordering filediffs by id (Bug 1248008)

https://reviewboard.mozilla.org/r/102956/#review105774
Attachment #8824453 - Flags: review?(smacleod) → review+
Comment on attachment 8824386 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) Bug 1248008

https://reviewboard.mozilla.org/r/102916/#review105946
Attachment #8824386 - Flags: review?(smacleod) → review+
Comment on attachment 8824386 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) Bug 1248008

https://reviewboard.mozilla.org/r/102916/#review105946

Actually, can you post a new revision that fixes the commit message summary line, it's missing the brackets.
Comment on attachment 8827471 [details]
MozReview: Add mr:diff-context-loaded event (Bug 1248008)

https://reviewboard.mozilla.org/r/105140/#review105960

Please post another revision with the bug bracketed in the commit message.
Attachment #8827471 - Flags: review?(smacleod) → review+
Blocks: 1331311
Attachment #8824453 - Attachment is obsolete: true
Attachment #8824386 - Attachment is obsolete: true
Attachment #8827471 - Attachment is obsolete: true
Comment on attachment 8828063 [details]
MozReview: Specify ordering filediffs by id (Bug 1248008)

https://reviewboard.mozilla.org/r/105576/#review106722
Attachment #8828063 - Flags: review?(smacleod) → review+
Comment on attachment 8828064 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)

https://reviewboard.mozilla.org/r/105578/#review106724

This commit message still doesn't have the `Bug <#>` in parenthesis`[1][2]`. Also, what happened to the previous review requests? why are they discarded and these are new ones?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1248008#c251
[2] https://reviewboard.mozilla.org/r/102916/
Attachment #8828064 - Flags: review?(smacleod) → review-
Comment on attachment 8828065 [details]
MozReview: Add mr:diff-context-loaded event (Bug 1248008)

https://reviewboard.mozilla.org/r/105580/#review106728
Attachment #8828065 - Flags: review?(smacleod) → review+
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review106926

Typo in your commit message, last paragraph "DileDiff"

::: hgext/reviewboard/tests/sql/remove-commit-message-file.sql:1
(Diff revision 24)
> +--- commit message filediff
> +DELETE FROM diffviewer_filediff WHERE id=2;
> +
> +--- temp diffset
> +DELETE FROM diffviewer_diffset WHERE id=2;
> +
> +--- remove extra_data fields from commitdata
> +UPDATE "mozreview_commitdata" SET 
> +	extra_data='{"p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.temp_diffset_id": 2, "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.identifier": "bz://1/mynick", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb": true}', 
> +	draft_extra_data = '{"p2rb.temp_diffset_id": 2, "p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb.identifier": "bz://1/mynick", "p2rb": true}' 
> +	WHERE review_request_id=2;

This is very specific to the single test, but it's named in a generic way. How about you just fold this into the test, cat it into a file or something as part of the test. You should also document how this is generated, as a small change to the test will require regeneration.

::: hgext/reviewboard/tests/sql/remove-commit-message-file.sql:8
(Diff revision 24)
> +UPDATE "mozreview_commitdata" SET 
> +	extra_data='{"p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.temp_diffset_id": 2, "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.identifier": "bz://1/mynick", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb": true}', 
> +	draft_extra_data = '{"p2rb.temp_diffset_id": 2, "p2rb.first_public_ancestor": "df67364c205763de5ad1d2c33fa78f87f6618289", "p2rb.is_squashed": false, "p2rb.author": "test", "p2rb.commit_id": "4a9cf7e9182050f2125ae6c10b21cfc78f6b25ef", "p2rb.identifier": "bz://1/mynick", "p2rb": true}' 

Trailing whitespace

::: hgext/reviewboard/tests/test-auth.t:179
(Diff revision 24)
> +
> +
> +

This is really strange... does the test fail if you don't do this? if not you should remove this (and the others like it)

::: hgext/reviewboard/tests/test-commit-message-as-file.t:191
(Diff revision 24)
> +    - +foo1
> +    - ''
> +  approved: false
> +  approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +ReviewRequest created before implementing this change remains intacted.

"intacted"?

::: hgext/reviewboard/tests/test-review-request-delegation.t:87
(Diff revision 24)
>        reviewer2:
>          review_flag: r?
>          ship_it: false
>      diff:
>        delete: 1
> -      insert: 1
> +      insert: 4

Hrmmm, I didn't even think that we'd be messing with this size measure... my inital reaction was that we should filter it out, but I guess the commit message is fine counting into size. That being said it might be better to be split out into it's own thing. Not something that we need to/should address now though, just thought I'd note it.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:927
(Diff revision 24)
> +def get_temp_diffset(id, repository):
> +    """Get or create a temporary diffset.

This should really only need the repository (see my other comment about using one diffset per repository)

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1014
(Diff revision 24)
> +        temp_diffset = get_temp_diffset(commit_data.draft_extra_data.get(
> +            TEMP_DIFFSET_ID_KEY, None), rr.repository)

You're still creating multiple diffsets per repository, since you're storing the id in the commit extra data, rather than something global - one per repostiory. Please change this so the DiffSet is created once per repository.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:110
(Diff revision 24)
> +.has-commit-msg-file {
> +	.diff-container:first-child .sidebyside,
> +   	:not(.diff-container) + .diff-container .sidebyside {

This *really* ought to be documented, explaining how it works and why we need such a strange selector.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:112
(Diff revision 24)
>    border-top: 2px dotted #aceb6f;  // @comment-flag-draft-color;
>  }
> +
> +.has-commit-msg-file {
> +	.diff-container:first-child .sidebyside,
> +   	:not(.diff-container) + .diff-container .sidebyside {

Is there a tab character inserted here? RB is complaining... Please make sure you're not mixing tabs and spaces.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:113
(Diff revision 24)
> +		tbody.insert tr td,
> +		tbody.replace tr td {
> +			background-color: #ffdfd7;

Ah, are you changing the commit messages addition to red-ish? I think it should either stay green for the insertion, or just be made white. The issue comes about though when dealing with an interdiff where colouring is important.

It's probably better to use something else to differentiate the commit message.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:54
(Diff revision 24)
>      return str(commit_data.get_for(review_request_details, COMMIT_ID_KEY))
>  
>  
> +@register.filter()
> +def commit_message_filediff_ids(review_request_details):
> +    """Return the commit id of a review request or review request draft"""

it's not a commit id, it's a filediff id for the commit message.
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review106926

> Trailing whitespace

Huh - switching back to VIM
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review106926

> Ah, are you changing the commit messages addition to red-ish? I think it should either stay green for the insertion, or just be made white. The issue comes about though when dealing with an interdiff where colouring is important.
> 
> It's probably better to use something else to differentiate the commit message.

I had a request to make commit message FileDiff different from others.
Making id red-ish was more to see how (and if) these might be changed. I left it like this as screenshots were not commented. I'd go with same color, but italic font instead of current bold one. It would make it less important and slightly different.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review106926
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review106926

> I had a request to make commit message FileDiff different from others.
> Making id red-ish was more to see how (and if) these might be changed. I left it like this as screenshots were not commented. I'd go with same color, but italic font instead of current bold one. It would make it less important and slightly different.

I actually think the text should be left the same, but doing something like changing the title bar styling, or the "filename" which is "Commit message" styling, to make that stand out. The actual diff itself is probably best left alone.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review106926

> I actually think the text should be left the same, but doing something like changing the title bar styling, or the "filename" which is "Commit message" styling, to make that stand out. The actual diff itself is probably best left alone.

Sorry, I wasn't clear on this - I wanted to leave colors on text and change style on filename
Comment on attachment 8828064 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)

https://reviewboard.mozilla.org/r/105578/#review106724

I've commited all review requests, reviewboard automatically discarded the other ones
Comment on attachment 8828064 [details]
MozReview: Return `FileDiff` list sorted by default model sorting (pk) (Bug 1248008)

https://reviewboard.mozilla.org/r/105578/#review107908
Attachment #8828064 - Flags: review?(smacleod) → review+
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review108444

::: hgext/reviewboard/tests/test-commit-message-as-file.t:189
(Diff revision 25)
> +ReviewRequest created before implementing this change remains untouched.
> +No commit message filediff is created.
> +SQL="\"UPDATE mozreview_commitdata SET extra_data='{p2rb.first_public_ancestor: 'df67364c205763de5ad1d2c33fa78f87f6618289', p2rb.is_squashed: false, p2rb.author: 'test', p2rb.identifier: 'bz://1/mynick', p2rb.commit_id: 'ecfec372b48ce7b49eccf655e422acb4283283e0', p2rb: true}' WHERE review_request_id=2\""
> +mozreview exec rbweb "sqlite3 /reviewboard/data/reviewboard.db $SQL"

There's nothing actually being tested here? You don't do anything after this...

::: hgext/reviewboard/tests/test-commit-message-file-old-structure.t:179
(Diff revision 25)
> +    - +foo2
> +    - ''
> +  approved: false
> +  approval_failure: A suitable reviewer has not given a "Ship It!"
> +
> +Entirely new review request within the same parent has a commit-message FileDiff.

Review requests with the same parent *shouldn't* get commit message FileDiffs. This will be really strange since you'll have to look at two different locations for commit messages.

::: pylib/mozreview/mozreview/extra_data.py:48
(Diff revision 25)
>      AUTHOR_KEY,
>      FIRST_PUBLIC_ANCESTOR_KEY,
>      IDENTIFIER_KEY,
>      COMMIT_ID_KEY,
> +    COMMIT_MSG_FILEDIFF_IDS_KEY,
> +    TEMP_DIFFSET_ID_KEY,

This isn't part of commit data and shouldn't be in here.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:24
(Diff revision 25)
>  from reviewboard.accounts.backends import (
>      get_enabled_auth_backends,
>  )
>  from reviewboard.accounts.errors import (
>      UserQueryError,
>  )
>  from reviewboard.diffviewer.models import (
>      DiffSet,
> +    FileDiff,
>  )
>  from reviewboard.reviews.models import (
>      ReviewRequest,
>      ReviewRequestDraft,
>  )
>  from reviewboard.scmtools.models import (
>      Repository,
>  )
>  from reviewboard.webapi.decorators import (
>      webapi_check_local_site,
>  )
>  from reviewboard.webapi.encoder import (
>      status_to_string,
>  )
>  from reviewboard.webapi.resources import (
>      WebAPIResource,
>  )
> +from reviewboard.scmtools.core import (
> +    PRE_CREATION,
> +)
>  
>  from mozreview.errors import (
>      REVIEW_REQUEST_UPDATE_NOT_ALLOWED,
>  )
>  from mozreview.extra_data import (
>      AUTHOR_KEY,
>      BASE_COMMIT_KEY,
>      COMMITS_KEY,
>      COMMIT_ID_KEY,
>      DISCARD_ON_PUBLISH_KEY,
>      fetch_commit_data,
>      FIRST_PUBLIC_ANCESTOR_KEY,
>      IDENTIFIER_KEY,
>      MOZREVIEW_KEY,
>      SQUASHED_KEY,
>      UNPUBLISHED_KEY,
> +    COMMIT_MSG_FILEDIFF_IDS_KEY,
> +    COMMIT_MSG_FILENAME_KEY,
> +    TEMP_DIFFSET_ID_KEY,

Alphabetical. including inside the import statements.

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:119
(Diff revision 25)
> +   We use an ugly CSS hack to rename unique filename "commit-message-[hash]"
> +   visually to "commit-message"
> + */
> +.has-commit-msg-file {
> +    .diff-container:first-child .sidebyside,
> +   	:not(.diff-container) + .diff-container .sidebyside {

You're mixing tabs and spaces or something again. Please make sure these don't exist in your patch before asking for review.

::: testing/vcttesting/mozreview_mach_commands.py:143
(Diff revision 25)
>          print('export ADMIN_PASSWORD=%s' % mr.admin_password)
>          print('export HGSSH_HOST=%s' % mr.ssh_hostname)
>          print('export HGSSH_PORT=%d' % mr.ssh_port)
>          print('export PULSE_HOST=%s' % mr.pulse_host)
>          print('export PULSE_PORT=%d' % mr.pulse_port)
> +        print('export RBWEB_ID=%s' % mr.rbweb_id)

Why are you printing this?

::: testing/vcttesting/mozreview_mach_commands.py:382
(Diff revision 25)
> +        """Run sequence of commands stored in file on a database on docker
> +        container.
> +        """

Summary line of a docstring should not wrap. Expansion on the summary can take place in lines below it, keep the summary short.

::: testing/vcttesting/mozreview_mach_commands.py:390
(Diff revision 25)
> +        # TODO Make it work on other containers if needed
> +        if name != 'rbweb':
> +            raise NotImplementedError('Only `rbweb` container is implemented')
> +
> +        mr = self._get_mozreview(None)
> +        cid = getattr(mr, '%s_id' % name, None)

please use a less cryptic name.
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review108444

> There's nothing actually being tested here? You don't do anything after this...

I've removed it - it is all moved to another test.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review108444

> Why are you printing this?

left from a docker cp in the test file, removing
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review108966

::: pylib/mozreview/mozreview/resources/batch_review_request.py:101
(Diff revisions 25 - 26)
>  You must reopen the review request before it can be updated.
>  Review requests should only be reopened if your changes have not landed or have
>  been backed out - file new bugs for follow-up work.
>  '''.strip()
>  
> +TEMP_DIFFSET_ID_KEY = MOZREVIEW_KEY + '.temp_diffset_id'

This should still go in the extra_data file, just not be marked as drafted, and be put with a line between it in it's own new group rather than the commitdata constants.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1008
(Diff revisions 25 - 26)
> -    if (commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False)
> -        or not rr.get_latest_diffset()):
> +    sibling_generator = gen_child_rrs(get_parent_rr(rr))
> +    a_sibling = _get_sibling(sibling_generator, rr)
> +    should_create_commit_msg_filediff = (
> +        not a_sibling and (
> +            # Check if created after commit message filediff implementation.
> +            commit_data.draft_extra_data.get(COMMIT_MSG_FILEDIFF_IDS_KEY, False)
> +            # Check if it's a first push.
> +            or not rr.get_latest_diffset()
> +        ) or
> +        # Check if sibling is created after commit msg FileDiff implementation.
> +        a_sibling and fetch_commit_data(a_sibling).extra_data.get(
> +            COMMIT_MSG_FILEDIFF_IDS_KEY, False)
> +    )

It would be *way* simpler to just check an extra_data field on the parent, and set it when a commit message is created on the initial push of new stuff.

::: hgext/reviewboard/tests/test-commit-message-as-file.t:1
(Diff revision 26)
> +#require mozreviewdocker

(Bogus diff comment so I can open an issue).

Your commit message doesn't follow the conventions`[1]` for version-control-tools.

>Save commit message as a file (bug 1248008) r=smacleod

You're missing the `mozreview:` to start this. Might also be a better summary to say "mozreview: inject commit message as a FileDiff to allow reviewing it..."


>MozReview does not have a way to comment on commit message. This patch is solving the issue by adding a commit message FileDiff to the DiffSet.  It is using a standard FileDiff and appears only in ReviewBoard.  In every other aspect it works as a file from repository.
> 
>Commit message file is given a name "commit-message" with a suffix (4 characters sliced from mercurial hashtag) to make it unique.
> 
>DOM "#review_request" element has a "has-commit-msg-file" class to add style.
> 
>The unique commit message with a suffix filename is changed to "commit-message" by a CSS hack.
> 
>There is a listener for a mr:diff-context-loaded event which changes the filename of the commit message FileDiff in #diff_index list.
> 
>The change doesn't affect existing ReviewRequests. No commit message FileDiff is added in that case.

This reads as a bunch of disjointed list items. It would be much more understandable if written in paragraphs, with the why included rather than just what is changed.


`[1]`https://mozilla-version-control-tools.readthedocs.io/en/latest/devguide/contributing.html#commit-creation-guidelines
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review109248

::: pylib/mozreview/mozreview/resources/batch_review_request.py:68
(Diff revisions 25 - 27)
> +    COMMIT_MSG_FILENAME_KEY,
>      DISCARD_ON_PUBLISH_KEY,
>      fetch_commit_data,
>      FIRST_PUBLIC_ANCESTOR_KEY,
> +    get_parent_rr,
> +    gen_child_rrs,

I've left this one by mistake. 
I'll wait for the review to avoid creating an almost empty revision.
We need to take bug 1335246 into consideration. We're changing the sorting as decided here in comment 229.
Comment on attachment 8831953 [details]
mozreview: send a series of commands to SQLite running in docker container (bug 1248008)

https://reviewboard.mozilla.org/r/108422/#review111592

::: testing/vcttesting/mozreview_mach_commands.py:377
(Diff revision 1)
> +                     choices={'bmoweb', 'bmodb', 'pulse', 'rbweb', 'hgrb',
> +                              'autoland', 'hgweb', 'treestatus'})

If `rbweb` is the only container we support we probably shouldn't have the other containers as choices here.

::: testing/vcttesting/mozreview_mach_commands.py:397
(Diff revision 1)
> +        if not container_id:
> +            print('No container for %s was found running' % name)
> +            return 1
> +
> +        # read SQL commands from stdin and store in temp file
> +        f = tempfile.NamedTemporaryFile('w+b')

use `with`

::: testing/vcttesting/mozreview_mach_commands.py:400
(Diff revision 1)
> +
> +        # read SQL commands from stdin and store in temp file
> +        f = tempfile.NamedTemporaryFile('w+b')
> +        for line in sys.stdin:
> +            f.write(line)
> +        f.flush()

>flush() does not necessarily write the file’s data to disk.

https://docs.python.org/2/library/stdtypes.html?highlight=file%20flush#file.flush

::: testing/vcttesting/mozreview_mach_commands.py:402
(Diff revision 1)
> +        f = tempfile.NamedTemporaryFile('w+b')
> +        for line in sys.stdin:
> +            f.write(line)
> +        f.flush()
> +
> +        args = '' if 'TESTTMP' in os.environ else '-it'

Put this just before it is used, not with things in between.

::: testing/vcttesting/mozreview_mach_commands.py:409
(Diff revision 1)
> +        # Copy file to the container.
> +        subprocess.check_call(
> +            'docker cp %(file)s %(container_id)s:/temp_query_file.sql' %
> +            {'file': f.name, 'container_id': container_id}, shell=True)
> +
> +        # Delete local file as it's no longer needed

"Closing a NamedTemporaryFile will delete  it by default"
Attachment #8831953 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review111614

::: hgext/reviewboard/tests/test-commit-message-file-old-structure.t:42
(Diff revision 28)
> +
> +System is recognizing if a ReviewRequest has been created before implementation
> +of the commit-message FileDiff feature by fields added to Repository and
> +CommitData. To make above ReviewRequest look like an old one we need to
> +remove commit message entries from database.
> +  $ mozreview sql rbweb <<SQL

It's standard to use `EOF`, meaing "end of file" like the character, rather than `SQL`.

::: hgext/reviewboard/tests/test-commits-deleted-obsolescence.t:1035
(Diff revision 28)
>    commit_extra_data:
>      p2rb: true
>      p2rb.author: test
>      p2rb.commit_id: a27a94c54524d4331dec2f92f647067bfd6dfbd4
> +    p2rb.commit_message_filename: commit-message-71387
> +    p2rb.commit_message_ids: '{"1": 12, "3": 31, "2": 27}'

uhg, I really hope this is deterministic between our machines. We might have to end up sorting this like we've done elsewhere just so that the test output is consistent. Lets see what happens but just keep that in mind if it comes up.

::: pylib/mozreview/mozreview/extra_data.py:33
(Diff revision 28)
>  # CommitData field keys:
>  AUTHOR_KEY = MOZREVIEW_KEY + '.author'
>  BASE_COMMIT_KEY = MOZREVIEW_KEY + '.base_commit'
>  COMMIT_ID_KEY = MOZREVIEW_KEY + '.commit_id'
>  COMMITS_KEY = MOZREVIEW_KEY + '.commits'
> +COMMIT_MSG_FILEDIFF_IDS_KEY = MOZREVIEW_KEY + '.commit_message_ids'

This should also have "filediff" in the actual key value itself.

::: pylib/mozreview/mozreview/extra_data.py:34
(Diff revision 28)
>  AUTHOR_KEY = MOZREVIEW_KEY + '.author'
>  BASE_COMMIT_KEY = MOZREVIEW_KEY + '.base_commit'
>  COMMIT_ID_KEY = MOZREVIEW_KEY + '.commit_id'
>  COMMITS_KEY = MOZREVIEW_KEY + '.commits'
> +COMMIT_MSG_FILEDIFF_IDS_KEY = MOZREVIEW_KEY + '.commit_message_ids'
> +COMMIT_MSG_FILENAME_KEY = MOZREVIEW_KEY + '.commit_message_filename'

Hypothetically, what happens if a user take the commit message filename, and *intentionally* adds a file in the next revision with the same name? Can you check the breakage here?

I know it's not likely to be hit by a normal user, but I'd like to know what happens if someone is being malicious.

::: pylib/mozreview/mozreview/extra_data.py:49
(Diff revision 28)
>  # draft_extra_data to extra_data when a review request is published.
>  DRAFTED_COMMIT_DATA_KEYS = (
>      AUTHOR_KEY,
> +    COMMIT_ID_KEY,
> +    COMMIT_MSG_FILEDIFF_IDS_KEY,
> +    COMMIT_MSG_FILENAME_KEY,

Things that are constant should not be drafted, they should just be set for the request itself, not the draft.

If we aren't doing any changing of this value it should not be here.

::: pylib/mozreview/mozreview/extra_data.py:51
(Diff revision 28)
>      AUTHOR_KEY,
> +    COMMIT_ID_KEY,
> +    COMMIT_MSG_FILEDIFF_IDS_KEY,
> +    COMMIT_MSG_FILENAME_KEY,
>      FIRST_PUBLIC_ANCESTOR_KEY,
> +    HAS_COMMIT_MSG_FILEDIFF,

This as well, if it's constant for the review request, we shouldn't be using the draft mechanism.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:50
(Diff revision 28)
> +from reviewboard.scmtools.core import (
> +    PRE_CREATION,
> +)

This still isn't properly alphabetized.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:997
(Diff revision 28)
> +    # Commit msg filediff should be created only if this is a completely new
> +    # commit review request, or if the review request we're updating already
> +    # had commit filediffs.
> +    base_commit_id = commit.get('base_commit_id')
> +    commit_message_filediff = None
> +    parent_commit_data = fetch_commit_data(get_parent_rr(rr))

Instead of regrabbing these things from the DB, you should really just pass it into the function. The caller of this function already has all of the squashed data - in fact it could pass in *only* what you need, something like `create_commit_msg_filediff` - if you wanted to go that route.

::: pylib/mozreview/mozreview/resources/batch_review_request.py:1070
(Diff revision 28)
> +
> +        commit_msg_filediff_ids = json.loads(commit_data.draft_extra_data.get(
> +            COMMIT_MSG_FILEDIFF_IDS_KEY, '{}'))
> +        commit_msg_filediff_ids[str(diffset.revision)] = commit_message_filediff.pk
> +        # Store commit message filediffs in extra_data
> +        commit_data.draft_extra_data[COMMIT_MSG_FILEDIFF_IDS_KEY] = json.dumps(commit_msg_filediff_ids)

This line looks much longer than 79 characters. Again, please ensure all of your additions are passing flake8.

::: pylib/mozreview/mozreview/templatetags/mozreview.py:54
(Diff revision 28)
> +    """Return the commit message filediff ids of a review request or review
> +    request draft"""

Again, "Summary line of a docstring should not wrap. Expansion on the summary can take place in lines below it, keep the summary short."
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review111614

> Things that are constant should not be drafted, they should just be set for the request itself, not the draft.
> 
> If we aren't doing any changing of this value it should not be here.

If user would do it interdiffs are broken (left - commit-message, right - file).
For some revision ranges the filediff list shows only one "commit-message" filediff.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review111614

> If user would do it interdiffs are broken (left - commit-message, right - file).
> For some revision ranges the filediff list shows only one "commit-message" filediff.

what? I don't understand what you're talking about?
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review111614

> Hypothetically, what happens if a user take the commit message filename, and *intentionally* adds a file in the next revision with the same name? Can you check the breakage here?
> 
> I know it's not likely to be hit by a normal user, but I'd like to know what happens if someone is being malicious.

If user would do it interdiffs are broken (left - commit-message, right - file).
For some revision ranges the filediff list shows only one "commit-message" filediff.
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review111614

> If user would do it interdiffs are broken (left - commit-message, right - file).
> For some revision ranges the filediff list shows only one "commit-message" filediff.

We discussed this over IRC, and while it will cause a bug in the interdiff it's really not that bad, and the normal diff looks fine. We think it is safe to drop this issue.
Comment on attachment 8831953 [details]
mozreview: send a series of commands to SQLite running in docker container (bug 1248008)

https://reviewboard.mozilla.org/r/108422/#review112976

Just a few last things.

::: testing/vcttesting/mozreview_mach_commands.py:385
(Diff revision 2)
> +
> +        A tempporary file is created in local filesystem and then copied to the
> +        docker container. It is used by sqlite to run stored command sequence.
> +        After that file is deleted on the container.
> +        """
> +        mozr = self._get_mozreview(None)

Why did you change this to "mozr"? The standard convention is "mr", as you had in the last iteration.

::: testing/vcttesting/mozreview_mach_commands.py:388
(Diff revision 2)
> +        After that file is deleted on the container.
> +        """
> +        mozr = self._get_mozreview(None)
> +        container_id = getattr(mozr, '%s_id' % name, None)
> +        if not container_id:
> +            print 'No container for %s was found running' % name

Why have you changed this to the print statement, instead of the function call?

::: testing/vcttesting/mozreview_mach_commands.py:392
(Diff revision 2)
> +        if not container_id:
> +            print 'No container for %s was found running' % name
> +            return 1
> +
> +        # Read SQL commands from stdin and store in temp file which will be
> +        # deleted automatically.

nit, for clarity, "deleted when closed."
Attachment #8831953 - Flags: review?(smacleod) → review-
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review112984

Very close, few small changes and we should be good to go.

::: pylib/mozreview/mozreview/extra_data.py:37
(Diff revision 29)
>  COMMITS_KEY = MOZREVIEW_KEY + '.commits'
> +COMMIT_MSG_FILEDIFF_IDS_KEY = MOZREVIEW_KEY + '.commit_message_filediff_ids'
> +COMMIT_MSG_FILENAME_KEY = MOZREVIEW_KEY + '.commit_message_filename'
>  DISCARD_ON_PUBLISH_KEY = MOZREVIEW_KEY + '.discard_on_publish_rids'
>  FIRST_PUBLIC_ANCESTOR_KEY = MOZREVIEW_KEY + '.first_public_ancestor'
> +HAS_COMMIT_MSG_FILEDIFF = MOZREVIEW_KEY + '.has_commit_message_filediff'

"_KEY"

::: pylib/mozreview/mozreview/static/mozreview/css/viewdiff.less:110
(Diff revision 29)
> +/* Select commit message FileDiff.
> +   It is a div.diff-container. It is not always first element of #diffs.
> +   If any FileDiff is selected, there is a div.diff-highlight injected before.

This still doesn't quite explain what our selector is doing, justy by reading this comment I should be able to understand what our selector is doing and why - we aren't even mentioning that while it isn't the always the first element, it *is* the first filediff (whatever class that is).

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js
(Diff revision 29)
>      });
>      document.getElementById('file' + fileDiffID)
>        .parentElement
>        .appendChild(reviewButton);
>    };
> -

Is there a reason you're removing the line here?

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:111
(Diff revision 29)
> +  // Reading message ids and revisions number to identify review requests
> +  // with commit message filediff. There is no commit message filediff added
> +  // to the review requests created before this code was implemented.
> +  var commitMessageIdsData = $('#commit-message-filediff-data');
> +  var currentRevisionNumber = commitMessageIdsData.data('currentRevisionNumber');
> +  if (currentRevisionNumber) {
> +    var commitMessageIds = commitMessageIdsData.data('commitMessageIds');
> +  }
> +  if (currentRevisionNumber && commitMessageIds[currentRevisionNumber]) {
> +    // Revision has commit message file.
> +    // As commit message filediff is displayed on the first page only we
> +    // need to check if the right one is displayed.
> +    function changeDiffIndexFilename() {
> +      // Change the filename of the commit message index
> +      $('.has-commit-msg-file #diff_index tbody tr:first td a').text('commit-message');
> +    }
> +    function hasCommitMsgFile(page) {
> +      // Set has-commit-msg-file class if on first page.
> +      if (page == 1) {
> +        $('#review_request').addClass('has-commit-msg-file');
> +        // Set the diff_index's commit message fileName on page (re)load.
> +        changeDiffIndexFilename();
> +        return;
> +      }
> +      $('#review_request').removeClass('has-commit-msg-file');
> +    }
> +    // Check the initial stage of the page.
> +    if ($('#pagination1').children().length == 0 || $('#pagination1 span.paginate-current').text() == '1') {
> +      hasCommitMsgFile(1);
> +    }
> +    // Listen to pagination events.
> +    page._paginationView1.on('pageSelected', hasCommitMsgFile);
> +    page._paginationView2.on('pageSelected', hasCommitMsgFile);
> +    // Listen to context loaded event and change name.
> +    $(document).on('mr:diff-context-loaded', changeDiffIndexFilename);
> +  }

This block of js is quite hard to read (To the point I'm less confident in my review of it, which took much longer than other parts of the change). I can't quite put my finger on why, maybe it needs to be factored into some functions? or the functions you define in the if block clutter it up? Could you take a stab at cleaning this up a bit while you make the other changes?

::: pylib/mozreview/mozreview/static/mozreview/js/init_filediffreviewer.js:127
(Diff revision 29)
> +    // need to check if the right one is displayed.
> +    function changeDiffIndexFilename() {
> +      // Change the filename of the commit message index
> +      $('.has-commit-msg-file #diff_index tbody tr:first td a').text('commit-message');
> +    }
> +    function hasCommitMsgFile(page) {

This is a misleading name for a function that has the side effect of updating the dom - it gives the impression it is just a check with no side effects.
Attachment #8786836 - Flags: review?(smacleod) → review-
Comment on attachment 8831953 [details]
mozreview: send a series of commands to SQLite running in docker container (bug 1248008)

https://reviewboard.mozilla.org/r/108422/#review114190
Attachment #8831953 - Flags: review?(smacleod) → review+
Comment on attachment 8786836 [details]
mozreview: allow reviewing a commit message by injecting it as a FileDiff (bug 1248008)

https://reviewboard.mozilla.org/r/75722/#review114196
Attachment #8786836 - Flags: review?(smacleod) → review+
Pushed by smacleod@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/6a7fac88d5b0
Add diffFileCollection_mozreview.js to specify sorting by id r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/75a7c1033075
MozReview: Specify ordering filediffs by id r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/dfbc927623b7
MozReview: Return `FileDiff` list sorted by default model sorting (pk) r=smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/bc491b5efdb7
MozReview: Add mr:diff-context-loaded event r=smacleod
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
reopening - looks like this was partially landed (and deployed!)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/50d5b6611155
mozreview: send a series of commands to SQLite running in docker container r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d73acaccd1d9
mozreview: allow reviewing a commit message by injecting it as a FileDiff r=smacleod
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: