Move commit-id into commit message

RESOLVED FIXED

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

Details

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
We currently define a hidden "commit-id" extra field when submitting to MozReview. There are issues with extra fields getting dropped because not all hg commands (pre 3.7) preserve extras. Even with 3.7, I believe they aren't preserved as part of MQ.

The Git implementation stores the commit id in the commit message. We should change Mercurial to do the same thing both for consistency and for robustness.
(Assignee)

Comment 1

3 years ago
This will enable us to use the function from the Mercurial extension.

MozReview-Commit-ID: FhsgdJY3rGn

Review commit: https://reviewboard.mozilla.org/r/33043/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33043/
Attachment #8714362 - Flags: review?(smacleod)
(Assignee)

Comment 2

3 years ago
Storing the commit ID in extras was littered with problems. First, the
ID wasn't always available from data that Review Board had access to (it
sees the commit message but not the extras by default). Second,
Mercurial doesn't always preserve extras, as the existing `hg graft`
test demonstrated. While there wasn't a test for it, I believe MQ
dropped the custom extras entry as well. Third, Git stores the commit
ID in the commit message. Both implementations should be consistent to
make logic using the commit IDs simpler.

Because we changed the content of the commit, SHA-1s changed. This
resulted in massive test fallout.

MozReview-Commit-ID: 1WyO65ojxuv

Review commit: https://reviewboard.mozilla.org/r/33045/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33045/
(Assignee)

Updated

3 years ago
Assignee: nobody → gps
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
As part of this, I also want to strip the commit id from the bugzilla attachment description, as it constitutes spam. I'm sure Git users will complain about this.
(Assignee)

Comment 4

3 years ago
Comment on attachment 8714362 [details]
MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33043/diff/1-2/
Attachment #8714362 - Attachment description: MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r?smacleod → MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r?dminor
Attachment #8714362 - Flags: review?(smacleod) → review?(dminor)
(Assignee)

Comment 5

3 years ago
We want to strip MozReview-Commit-ID and potentially other future
metadata from Bugzilla attachment descriptions to cut down on spam.
Establish a resuable function for performing the strip.

MozReview-Commit-ID: 7eWSjgyjEbp

Review commit: https://reviewboard.mozilla.org/r/33263/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33263/
Attachment #8714924 - Flags: review?(dminor)
(Assignee)

Comment 6

3 years ago
We don't want to litter the Bugzilla descriptions with metadata that
isn't relevant to Bugzilla. Strip it.

MozReview-Commit-ID: 4rjBfMdpHUF

Review commit: https://reviewboard.mozilla.org/r/33265/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33265/
Attachment #8714925 - Flags: review?(dminor)
(Assignee)

Comment 7

3 years ago
Comment on attachment 8714363 [details]
MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33045/diff/1-2/
(Assignee)

Comment 8

3 years ago
Comment on attachment 8714362 [details]
MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33043/diff/2-3/
(Assignee)

Comment 9

3 years ago
Comment on attachment 8714924 [details]
MozReview Request: mozautomation: function to strip commit metadata from string (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33263/diff/1-2/
(Assignee)

Comment 10

3 years ago
Comment on attachment 8714925 [details]
MozReview Request: rbbz: strip commit metadata from Bugzilla descriptions (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33265/diff/1-2/
(Assignee)

Updated

3 years ago
Attachment #8714363 - Attachment description: MozReview Request: INCOMPLETE reviewboard: store commit ID in commit messages instead of extras (bug 1243486) → MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r?dminor
Attachment #8714363 - Flags: review?(dminor)
(Assignee)

Comment 11

3 years ago
Comment on attachment 8714363 [details]
MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33045/diff/2-3/

Updated

3 years ago
Attachment #8714362 - Flags: review?(dminor) → review+
Comment on attachment 8714362 [details]
MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor

https://reviewboard.mozilla.org/r/33043/#review30021
Comment on attachment 8714924 [details]
MozReview Request: mozautomation: function to strip commit metadata from string (bug 1243486); r=dminor

https://reviewboard.mozilla.org/r/33263/#review30029

::: pylib/mozautomation/mozautomation/commitparser.py:204
(Diff revision 2)
> +    lines = [l for l in s.splitlines() if not METADATA_RE.match(l)]

This could be made stronger: metadata is always preceded by metadata or a blank line, and is always followed by metadata or EOF. You could use a multline regex or iterate on the lines and maintain state variables.

That said, I'm not sure this is worth doing at the moment. Maybe add a TODO?
Attachment #8714924 - Flags: review?(dminor) → review+

Updated

3 years ago
Attachment #8714925 - Flags: review?(dminor) → review+
Comment on attachment 8714925 [details]
MozReview Request: rbbz: strip commit metadata from Bugzilla descriptions (bug 1243486); r=dminor

https://reviewboard.mozilla.org/r/33265/#review30033
Comment on attachment 8714363 [details]
MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor

https://reviewboard.mozilla.org/r/33045/#review30103

Looks good to me.
Attachment #8714363 - Flags: review?(dminor) → review+
(Assignee)

Updated

3 years ago
Attachment #8714362 - Attachment description: MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r?dminor → MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor
(Assignee)

Comment 16

3 years ago
Comment on attachment 8714362 [details]
MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33043/diff/3-4/
(Assignee)

Comment 17

3 years ago
Comment on attachment 8714924 [details]
MozReview Request: mozautomation: function to strip commit metadata from string (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33263/diff/2-3/
Attachment #8714924 - Attachment description: MozReview Request: mozautomation: function to strip commit metadata from string (bug 1243486); r?dminor → MozReview Request: mozautomation: function to strip commit metadata from string (bug 1243486); r=dminor
(Assignee)

Updated

3 years ago
Attachment #8714925 - Attachment description: MozReview Request: rbbz: strip commit metadata from Bugzilla descriptions (bug 1243486); r?dminor → MozReview Request: rbbz: strip commit metadata from Bugzilla descriptions (bug 1243486); r=dminor
(Assignee)

Comment 18

3 years ago
Comment on attachment 8714925 [details]
MozReview Request: rbbz: strip commit metadata from Bugzilla descriptions (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33265/diff/2-3/
(Assignee)

Comment 19

3 years ago
Comment on attachment 8714363 [details]
MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33045/diff/3-4/
(Assignee)

Updated

3 years ago
Blocks: 1245589
(Assignee)

Comment 20

3 years ago
Comment on attachment 8714362 [details]
MozReview Request: reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33043/diff/4-5/
(Assignee)

Comment 21

3 years ago
Comment on attachment 8714924 [details]
MozReview Request: mozautomation: function to strip commit metadata from string (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33263/diff/3-4/
(Assignee)

Comment 22

3 years ago
Comment on attachment 8714925 [details]
MozReview Request: rbbz: strip commit metadata from Bugzilla descriptions (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33265/diff/3-4/
(Assignee)

Comment 23

3 years ago
Comment on attachment 8714363 [details]
MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33045/diff/4-5/
Attachment #8714363 - Attachment description: MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r?dminor → MozReview Request: reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/hgcustom/version-control-tools/rev/eb9e43ce04570bc08cfb4bd08f0b9ddbe0fe79c9
reviewboard: move commit message rewriting function into library (bug 1243486); r=dminor

https://hg.mozilla.org/hgcustom/version-control-tools/rev/1af421167a356a8d008529139ad791dd00a3655a
mozautomation: function to strip commit metadata from string (bug 1243486); r=dminor

https://hg.mozilla.org/hgcustom/version-control-tools/rev/eef5eefc3b2c015a517e1f9e7b53159ac15a2524
mozreview: strip commit metadata from Bugzilla descriptions (bug 1243486); r=dminor

https://hg.mozilla.org/hgcustom/version-control-tools/rev/dba9648638d5ec2afa1ed6c32df7778fc5014ae0
reviewboard: store commit ID in commit messages instead of extras (bug 1243486); r=dminor
(Assignee)

Comment 25

3 years ago
Will deploy this soon. (Deploy will prevent Review Board from posting "MozReview-Commit-ID" lines in Bugzilla attachment descriptions.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.