Issue 29336790: Issue 3674 - Add a hg hook that references commits in issues (Closed)
|
Created:
Feb. 22, 2016, 10:17 a.m. by Felix Dahlke Modified:
Feb. 24, 2016, 8:30 p.m. Reviewers:
Wladimir Palant CC:
Sebastian Noack Visibility:
Public. |
DescriptionIssue 3674 - Add a hg hook that references commits in issues
Patch Set 1 : #
Total comments: 15
Patch Set 2 : Address comments #
Total comments: 12
Patch Set 3 : Address comments #Patch Set 4 : Don't use RegexObject.match #
MessagesTotal messages: 11
Philip will be extremely happy about installing yet another Trac plugin ;). I'll look into whether XmlRpcPlugin (and particularly authentication) is sane... https://codereview.adblockplus.org/29336790/diff/29336797/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29336790/diff/29336797/.sitescripts.exampl... .sitescripts.example:166: trac_xmlrpc_url=https://abpbot:abpbot@issues.adblockplus.org/login/xmlrpc Shouldn't this be /login/rpc? That's what it is according to plugin documentation. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:47: comments[issue_id] = comment I'd rather have this logic in a template. This should also make it easier to add additional changeset info (author name, description). https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:60: issue_number_regex = re.compile(r"Issue (\d+) - ") This is too restrictive, see https://hg.adblockplus.org/adblockplus/rev/637c6ebb90c1 for example. It should rather be re.compile(r"\bissue\s+(\d+)\b", re.I) IMHO. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:65: match = issue_number_regex.search(description) What if there multiple issues referenced in the commit? Also, we meant to validate commit messages all along - there should be either an issue reference or Noissue. Maybe now is the right time? https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:79: ui.warn("failed to update %s - does it exist?\n" % \ This backslash is unnecessary.
https://codereview.adblockplus.org/29336790/diff/29336797/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29336790/diff/29336797/.sitescripts.exampl... .sitescripts.example:166: trac_xmlrpc_url=https://abpbot:abpbot@issues.adblockplus.org/login/xmlrpc On 2016/02/22 11:32:33, Wladimir Palant wrote: > Shouldn't this be /login/rpc? That's what it is according to plugin > documentation. Never mind, source code supports both.
https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:56: {"_ts": attributes["_ts"], "action": "leave"}) The ridiculous part about this is: there is a race condition here, if somebody updates the issue after we get our _ts attribute but before we send the actual update request then adding the comment will fail for no good reason. Since the leave action is available regardless of the issue state we could avoid the problem by omitting the _ts attribute. This will currently work but the plugin author wants to deprecate this kind of request. Maybe omit _ts parameter and communicate to the author why deprecating requests without _ts is a bad idea?
https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:56: {"_ts": attributes["_ts"], "action": "leave"}) Sorry about so many mails, I've only noticed some issues while reading XmlRpcPlugin source code. I just realized that this needs one more parameter: notify=True. Otherwise no notification mails will be sent which would be wrong IMHO.
New patch set is up. https://codereview.adblockplus.org/29336790/diff/29336797/.sitescripts.example File .sitescripts.example (right): https://codereview.adblockplus.org/29336790/diff/29336797/.sitescripts.exampl... .sitescripts.example:166: trac_xmlrpc_url=https://abpbot:abpbot@issues.adblockplus.org/login/xmlrpc On 2016/02/22 11:41:37, Wladimir Palant wrote: > On 2016/02/22 11:32:33, Wladimir Palant wrote: > > Shouldn't this be /login/rpc? That's what it is according to plugin > > documentation. > > Never mind, source code supports both. Yup, it's documented too. And since there's also an JsonRpc plugin, I preferred to go for /login/xmlrpc. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:47: comments[issue_id] = comment On 2016/02/22 11:32:33, Wladimir Palant wrote: > I'd rather have this logic in a template. This should also make it easier to add > additional changeset info (author name, description). Done. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:56: {"_ts": attributes["_ts"], "action": "leave"}) On 2016/02/22 12:06:01, Wladimir Palant wrote: > Sorry about so many mails, I've only noticed some issues while reading > XmlRpcPlugin source code. I just realized that this needs one more parameter: > notify=True. Otherwise no notification mails will be sent which would be wrong > IMHO. Done. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:56: {"_ts": attributes["_ts"], "action": "leave"}) On 2016/02/22 12:02:51, Wladimir Palant wrote: > The ridiculous part about this is: there is a race condition here, if somebody > updates the issue after we get our _ts attribute but before we send the actual > update request then adding the comment will fail for no good reason. Since the > leave action is available regardless of the issue state we could avoid the > problem by omitting the _ts attribute. This will currently work but the plugin > author wants to deprecate this kind of request. Maybe omit _ts parameter and > communicate to the author why deprecating requests without _ts is a bad idea? That seems to be quite the hassle for what I think is a fairly improbable edge case. Note that there will be a warning printed saying that the comment couldn't be added, and in that case people can still add a comment referencing the commits manually. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:60: issue_number_regex = re.compile(r"Issue (\d+) - ") On 2016/02/22 11:32:33, Wladimir Palant wrote: > This is too restrictive, see > https://hg.adblockplus.org/adblockplus/rev/637c6ebb90c1 for example. It should > rather be re.compile(r"\bissue\s+(\d+)\b", re.I) IMHO. Done. https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:65: match = issue_number_regex.search(description) On 2016/02/22 11:32:33, Wladimir Palant wrote: > What if there multiple issues referenced in the commit? Never seen that before, but since you've shown a real life example above: Done. > Also, we meant to validate commit messages all along - there should be either an > issue reference or Noissue. Maybe now is the right time? Yeah we should absolutely have that, but so far I've just pointed this out as a potential follow-up in #3674, because it's a bit complicated when multiple changesets are being pushed and I'd rather start simple. I've created a follow-up and added a comment explaining why I think it's complicated: https://issues.adblockplus.org/ticket/3679 Meanwhile, I've added some code below that warns about an invalid commit message format. It's a start... https://codereview.adblockplus.org/29336790/diff/29336797/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:79: ui.warn("failed to update %s - does it exist?\n" % \ On 2016/02/22 11:32:33, Wladimir Palant wrote: > This backslash is unnecessary. Done, also changed the message a bit. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/temp... File sitescripts/hg/template/issue_commit_comment.tmpl (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/temp... sitescripts/hg/template/issue_commit_comment.tmpl:8: https://hg.adblockplus.org/{{repository_name}}/rev/{{change.hex()|truncate(12, True, "")}} I'm not particularly proud of this one but: 1. To my astonishment, I haven't found a nicer way of cutting off a string in Jinja2. 2. The template is a lot more flexible if we just pass in a list of real change context objects. Alternatively, I could simply pass in a list of readily truncated changeset IDs. Seems a bit wrong since then I'm moving view logic back into the hook script, but it would be more readable. Can you think of better options? If not, which of these two do you think is better?
https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:33: template = get_template("hg/template/issue_commit_comment.tmpl") I think you need autoescape=False here. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:50: noissue_regex = re.compile(r"noissue\b", re.I) This should be \bnoissue\b (make sure that the word actually starts with "noissue"). https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:61: # commit message format, see: https://issues.adblockplus.org/ticket/3679 Is that a TODO comment? Maybe it should actually say TODO then. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/temp... File sitescripts/hg/template/issue_commit_comment.tmpl (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/temp... sitescripts/hg/template/issue_commit_comment.tmpl:8: https://hg.adblockplus.org/{{repository_name}}/rev/{{change.hex()|truncate(12, True, "")}} How about: {{change.hex()[:12]}} ;)
New patch set is up. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:33: template = get_template("hg/template/issue_commit_comment.tmpl") On 2016/02/23 10:41:31, Wladimir Palant wrote: > I think you need autoescape=False here. Done. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:50: noissue_regex = re.compile(r"noissue\b", re.I) On 2016/02/23 10:41:31, Wladimir Palant wrote: > This should be \bnoissue\b (make sure that the word actually starts with > "noissue"). I'm using match() so the message has to start with "noissue". We shouldn't match "noissue" somewhere in the message here IMO. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:61: # commit message format, see: https://issues.adblockplus.org/ticket/3679 On 2016/02/23 10:41:31, Wladimir Palant wrote: > Is that a TODO comment? Maybe it should actually say TODO then. Well it's referencing an issue, I normally do just that without writing "TODO". https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/temp... File sitescripts/hg/template/issue_commit_comment.tmpl (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/temp... sitescripts/hg/template/issue_commit_comment.tmpl:8: https://hg.adblockplus.org/{{repository_name}}/rev/{{change.hex()|truncate(12, True, "")}} On 2016/02/23 10:41:31, Wladimir Palant wrote: > How about: > > {{change.hex()[:12]}} > > ;) Well I never, didn't occur to me for some reason to just try Python syntax here :D Done.
https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:50: noissue_regex = re.compile(r"noissue\b", re.I) On 2016/02/23 16:22:59, Felix Dahlke wrote: > I'm using match() so the message has to start with "noissue". We shouldn't match > "noissue" somewhere in the message here IMO. We agreed a while ago that we shouldn't be using match() to avoid exactly that kind of non-obvious behavior. Please use search() and change the regular expression into ^noissue\b.
And another patch set. https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:50: noissue_regex = re.compile(r"noissue\b", re.I) On 2016/02/23 21:33:23, Wladimir Palant wrote: > On 2016/02/23 16:22:59, Felix Dahlke wrote: > > I'm using match() so the message has to start with "noissue". We shouldn't > match > > "noissue" somewhere in the message here IMO. > > We agreed a while ago that we shouldn't be using match() to avoid exactly that > kind of non-obvious behavior. Please use search() and change the regular > expression into ^noissue\b. Fair enough. Tried to be smart, but this comment thread proves that it's a bad idea. Done.
LGTM https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... File sitescripts/hg/bin/update_issues.py (right): https://codereview.adblockplus.org/29336790/diff/29337499/sitescripts/hg/bin/... sitescripts/hg/bin/update_issues.py:50: noissue_regex = re.compile(r"noissue\b", re.I) On 2016/02/23 21:40:58, Felix Dahlke wrote: > Fair enough. Tried to be smart, but this comment thread proves that it's a bad > idea. Done. Yes, particularly with the regexp and re.match() not in the same place this is very confusing. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
