Change how commit message parsing and diff calls work.
Review Request #5457 — Created Feb. 11, 2014 and submitted
The
SCMClients
had some confusing responsibilities where they attempted
to handle the guessed summary/description logic in the diff methods,
getting and setting data in theoptions
object. This logic had no
business being in thediff
method, or inSCMClients
for that matter, and
should have exclusively been in rbt post.On top of that, we had some variations in how the commit summary and
description were fetched and processed in the differentSCMClients
. This
led to differences in behavior and subtle bugs. For example, Mercurial
was grabbing the summary from the wrong end of a commit range, and
Bazaar was providing extraneous information for the descriptions.Now, instead of
extract_summary()
andextract_description()
methods,
SCMClients
only need to implementget_raw_commit_message()
. This returns
the full set of commit messages for the provided revisions. The base
SCMClient
class then provides aget_commit_message()
that calls that and
splits the result into a summary and a description.The description also no longer includes the summary line, preventing the
first line of the Description field from mirroring the Summary field
unnecessarily.
rbt post
is then responsible for calling this when it needs to generate
the guessed content or match an existing review request in order to
update it.Since both
get_commit_message()
anddiff()
need parsed revisions, it no
longer makes sense fordiff()
to callparse_revision_spec()
. That's now
the job of the caller, which results indiff()
being just a bit simpler.At this point, the
get_diff()
utility function is no longer necessary
either, so it's been removed.Now that
rbt post
owns more of the logic, it can also be smarter about
when things are called. Previously, if using-u
, we'd parse the
revision spec twice (once indiff()
and once when generating the
summary/description for matching).We also would make at least two calls to get the summary and description
(more, if using-g
and the description was blank).
rbt post
now caches the parsed revision spec, and the commit message,
preventing us from having to run as many costly commands.This all results in a cleaner design with less responsibility in
SCMClient
, fewer usages of theoptions
object, and faster posting.
Posted an update to this change using
-g -r
. Then followed it up
with additional updates as I tweaked some comments, using-u
and
-g -u
.Tested posting with
-g
on Bazaar, Git and Mercurial in the following scenarios:
- Single line summary.
- Multi-line summary.
- Single line summary + blank line + description
In the first two, the whole commit message was used for the description.
For the third, only the description was used.
Tested posting updates to changes using -u
on Bazaar, Git and Mercurial.
Tested also with -g -u
.
Unit tests passed. Discovered while updating tests that Mercurial's summary
guessing read from the wrong end of a commit range, and fixed that. Verified
by hand.
No errors or warnings from PyFlakes.
- Change Summary:
-
Hopefully clarified the goals of the change.
- Description:
-
+ When posting a review request with -g, the first line of the commit
+ message would end up appearing in both the Summary field and on the + first line of the Description field. This was redundant, unnecessary, + and redundant. + Running rbt post -g now leaves out the summary when posting the
description for the review request. It only posts the commit body. For Git, we can use Git's own logic to handle this. It allows us to ask
for the body. Previously, we were also asking for the summary, and concatenating. For Mercurial, we have to do a bit more work. If there's 3 or more
lines, and the second line is blank, we assume the first is a summary and the rest is a body. If any of this isn't the case, we just post the entire commit description as the description, as before. Bazaar has not been updated for this logic.
- Commit:
-
a2ebb209a6d39f20021eee2e1b398438b2052b7112901f1d49ceb9b2bed4e84fbde5d5a624d45eb3
-
OK, I think I'm going to give you the same comment that I gave on https://reviews.reviewboard.org/r/5436/:
Because other SCMs have the same issue (git, bazaar, perforce, etc.), I think I'd prefer that this be done in 'rbt post' by checking if the first line of self.options.description is the same as self.options.summary.
- Change Summary:
-
Full rewrite of the change, broadening the scope considerably. Details in the description.
- Summary:
-
When extracting the description, grab the body and not the summary.Change how commit message parsing and diff calls work.
- Description:
-
~ When posting a review request with -g, the first line of the commit
~ message would end up appearing in both the Summary field and on the ~ first line of the Description field. This was redundant, unnecessary, ~ and redundant. ~ The
SCMClients
had some confusing responsibilities where they attempted~ to handle the guessed summary/description logic in the diff methods, ~ getting and setting data in the options
object. This logic had no~ business being in the diff
method, or inSCMClients
for that matter, and+ should have exclusively been in rbt post. ~ Running rbt post -g now leaves out the summary when posting the
~ description for the review request. It only posts the commit body. ~ On top of that, we had some variations in how the commit summary and
~ description were fetched and processed in the different SCMClients
. This+ led to differences in behavior and subtle bugs. For example, Mercurial + was grabbing the summary from the wrong end of a commit range, and + Bazaar was providing extraneous information for the descriptions. ~ For Git, we can use Git's own logic to handle this. It allows us to ask
~ for the body. Previously, we were also asking for the summary, and ~ concatenating. ~ Now, instead of
extract_summary()
andextract_description()
methods,~ SCMClients
only need to implementget_raw_commit_message()
. This returns~ the full set of commit messages for the provided revisions. The base + SCMClient
class then provides aget_commit_message()
that calls that and+ splits the result into a summary and a description. ~ For Mercurial, we have to do a bit more work. If there's 3 or more
~ lines, and the second line is blank, we assume the first is a summary ~ and the rest is a body. If any of this isn't the case, we just post the ~ The description also no longer includes the summary line, preventing the
~ first line of the Description field from mirroring the Summary field ~ unnecessarily. - entire commit description as the description, as before. ~ Bazaar has not been updated for this logic.
~ rbt post
is then responsible for calling this when it needs to generate+ the guessed content or match an existing review request in order to + update it. + + Since both
get_commit_message()
anddiff()
need parsed revisions, it no+ longer makes sense for diff()
to callparse_revision_spec()
. That's now+ the job of the caller, which results in diff()
being just a bit simpler.+ + At this point, the
get_diff()
utility function is no longer necessary+ either, so it's been removed. + + Now that
rbt post
owns more of the logic, it can also be smarter about+ when things are called. Previously, if using -u
, we'd parse the+ revision spec twice (once in diff()
and once when generating the+ summary/description for matching). + + We also would make at least two calls to get the summary and description
+ (more, if using -g
and the description was blank).+ + rbt post
now caches the parsed revision spec, and the commit message,+ preventing us from having to run as many costly commands. + + This all results in a cleaner design with less responsibility in
+ SCMClient
, fewer usages of theoptions
object, and faster posting. - Testing Done:
-
~ Posted this change, and didn't modify the resulting description.
~ Posted an update to this change using
-g -r
. Then followed it up+ with additional updates as I tweaked some comments, using -u
and+ -g -u
.~ Tested posting with Mercurial in the following scenarios:
~ Tested posting with
-g
on Bazaar, Git and Mercurial in the following scenarios:- Single line summary.
- Multi-line summary.
- Single line summary + blank line + description
~ In the first two, the whole commit message was used for the description.
~ In the first two, the whole commit message was used for the description.
+ For the third, only the description was used. ~ For the third, only the description was used.
~ Tested posting updates to changes using
-u
on Bazaar, Git and Mercurial.+ + Tested also with
-g -u
.+ + Unit tests passed. Discovered while updating tests that Mercurial's summary
+ guessing read from the wrong end of a commit range, and fixed that. Verified + by hand. + + No errors or warnings from PyFlakes.
- Commit:
-
12901f1d49ceb9b2bed4e84fbde5d5a624d45eb3a12cc3da2920947890b4b77d47dba79614f45317
- Change Summary:
-
- Fixed up a docstring, bringing it up to date with the realities of the function.
- Brought back the user-visible error when unable to parse a revision, for SCMClients other than Perforce (dictated by a capability).
- Commit:
-
a12cc3da2920947890b4b77d47dba79614f453174228eeec6dae9d39ea86b3f5282f4e7018f375c3