Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7568 pragma(msg) segfaults with an aggregate including a class. #774

Merged
merged 1 commit into from
Mar 1, 2012

Conversation

donc
Copy link
Collaborator

@donc donc commented Feb 29, 2012

Fixes issue 7568.

WalterBright added a commit that referenced this pull request Mar 1, 2012
7568 pragma(msg) segfaults with an aggregate including a class.
@WalterBright WalterBright merged commit 4983d32 into dlang:master Mar 1, 2012
@WalterBright
Copy link
Member

github/bugzilla did not recognize the commit message, because it was not of the form:

fix issue 7568

@kennytm
Copy link
Contributor

kennytm commented Mar 1, 2012

@WalterBright probably you should just modify the regex to match /^(?:fix(?:e[ds])?\s(?:issue|bug)\s)?(\d+)/i.

@donc
Copy link
Collaborator Author

donc commented Mar 1, 2012

So the previous regex that Brad posted, no longer works? In the past, it even worked when I wrote "Fixes bug XXX".

@braddr
Copy link
Member

braddr commented Mar 1, 2012

the comment needs to be in the commit message, not the pull request message -- at least from my quick read of the code: https://github.com/github/github-services/blob/master/services/bugzilla.rb

@CyberShadow
Copy link
Member

@WalterBright The form is not the problem - the problem is that the text should have been in the commit message. All of the following forms will be recognized:

Fix bug #123
Fixed issue 42
Closes tickets #123, #456 and #789

(Yes, you can specify multiple bugs.)

@WalterBright
Copy link
Member

It's a waste of time to change the regex, and to enumerate what it accepts and doesn't. There's nothing hard about sticking to:

fix issue nnnn

nor is a leading # recognized. This isn't a creative writing class :-). Just write "fix issue nnnn" on a postit note and stick it to your monitor. Hell, it's "fix" followed by a simple cut & paste of the bugzilla entry. It works.

@CyberShadow
Copy link
Member

I think you are misunderstanding the problem here.

This has nothing to do with a leading #. Don did not include the text "Fixes issue 7568." as the commit message. It is part of the pull request description, but not the commit message. If it were part of the commit message, then according to the link @braddr posted it would have worked. But it did not work because it was not part of the commit message. If it was part of the commit message, it would have been recognized by the above-mentioned script, even if it had a # in it.

Edit: it's the first line of the commit message. (GitHub is inconsistent when it shows the [...] buttons on commit headers.)

@kennytm
Copy link
Contributor

kennytm commented Mar 1, 2012

Speaking of which, @WalterBright could you please label pull requests with a # sign in the commit messages? Like

merge D2 pull #516
              ^

This allows GitHub recognize the commit is related to the pull request, and automatically reference the commit in the corresponding pull request.

@donc
Copy link
Collaborator Author

donc commented Mar 1, 2012

Don did not include the text "Fixes issue 7568."
That is not true. It was part of the commit message. The same wording appeared in the pull request automatically when I created the pull request.
This is what I get locally from git log. OK, I mistyped 'issue' as 'issues', but it should still have matched the regex. I don't understand.

$ git log _error6785
commit 40345d7
Author: Don Clugston dclugston@googlemail.com
Date: Wed Feb 29 22:02:40 2012 +0100

6785 Wrong error message from pragma(msg) of failed instantiation

Fixes issues 6785.

commit 4d6cf6b
Merge: 576f18a 881b661
Author: Walter Bright walter@walterbright.com
Date: Mon Feb 27 19:25:44 2012 -0800

Merge pull request #771 from donc/bug7589

7589 __traits(compiles) does not work with a template that fails to compile

@CyberShadow
Copy link
Member

Yes, see my edit. I looked at previous GitHub automatic actions on Bugzilla, and it seems like it will only work if it is part of the first line of the commit message (aka the commit subject).

@WalterBright
Copy link
Member

merge D2 pull #516

Sure. I didn't know it worked that way.

@donc
Copy link
Collaborator Author

donc commented Mar 2, 2012

it seems like it will only work if it is part of the first line of the commit message (aka the commit subject).

Ah! So it must be in the title, not in the comment section. That's pretty stupid, it wastes valuable space in the change log short description. I wonder if github changed something. I have definitely had some previous commits matched by it, but perhaps none recently, and I don't think I ever put that syntax in the title.

@dnadlinger
Copy link
Member

The needs-to-be-on-first-line thing might be worth discussing with the @github support folks…

@WalterBright
Copy link
Member

I think the convention is fine, we just need to follow it.

@CyberShadow
Copy link
Member

That's pretty stupid, it wastes valuable space in the change log short description.

I believe the shortest accepted form is "Bug NNNN". So, the subject line can be like "Bug NNNN - Bug description here" or "Fix description here (bug NNNN)".

braddr pushed a commit to braddr/dmd that referenced this pull request Oct 22, 2012
snprintf already defined in core.stdc.stdio.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants