Skip to content

Linking objects from GFM references #4507

Merged
merged 1 commit into from Aug 26, 2013
@smashwilson

Hello!

This is an implementation in progress of linking issues from comments, also referenced in #2676, #2714, #1170, #302, and probably others. I'm pushing it out now to get early feedback on what I've already done and to hear any thoughts about edge cases I might not be accounting for.

Any mention of issues, merge requests, or commits via GitLab-flavored markdown references in descriptions, titles or follow-on notes to any of the above attaches a system note to the referenced object containing a backtracking link. Furthermore, pushing commits that match a (configurable) regexp to a project's default branch will close any issues mentioned in the closing phrase.

Left to do:

  • Only close issues from commits pushed to the project's default branch -- meaning, actually make it do that thing I said it does :wink:
  • If any Mentionable is edited, create additional cross-references for any references that weren't there to begin with.
  • Merge request and issue creation don't create cross-references correctly, only notes and edits. D'oh.
  • Accepting a merge request that brings in a commit that "fixes" an issue works fine, but if the "fixes" text is in a line other than the first, it happens without warning. Detect and add an "accepting this request will close issues X, Y, and Z" banner on the merge request page.
  • Look into using ActiveRecord::Dirty to implement Mentionable#watch_references, which would let us push reference updating into the observers where it belongs.
  • Duplicate "mentioned in commit..." notes when a commit that mentions a thing is merged into a new branch.
  • Link an issue closed notification back to the commit that closed it, when closing from a commit. "Status changed to closed by..."
  • Catch references in the commits in the first push to a new branch.
  • Do a final rebase and squash on upstream master.
@coveralls

Coverage Status

Coverage decreased (-0%) when pulling 4d70f35db1a775d3a05a160525c543a2d5d0d4a7 on smashwilson:linking-issues into 5315fd5 on gitlabhq:master.

@coveralls

Coverage Status

Coverage remained the same when pulling e44e50bd2da250c493abe7353efaa65982556de6 on smashwilson:linking-issues into 5315fd5 on gitlabhq:master.

@coveralls

Coverage Status

Coverage remained the same when pulling 100fca5bb0b8a76f445448c5114c25456a115113 on smashwilson:linking-issues into 5315fd5 on gitlabhq:master.

@randx
GitLab member
randx commented Jul 12, 2013

@smashwilson cool. Looking forward

@randx randx was assigned Jul 12, 2013
@smashwilson

Some screenshots of an issue cross-reference note:
xref

...and the merge request "will close issues" notification. (The issue ids are proper GFM links.)
issue-close-notification

@coveralls

Coverage Status

Coverage remained the same when pulling f07b2fdf935983be17bf737f2d9e35f656f8ec0b on smashwilson:linking-issues into 79bea31 on gitlabhq:master.

@coveralls

Coverage Status

Coverage decreased (-0%) when pulling ff2c52daafd7d530ffddd1a2de687ac182f321d0 on smashwilson:linking-issues into 79bea31 on gitlabhq:master.

@Razer6
GitLab member
Razer6 commented Aug 2, 2013

:+1:

@archonik
archonik commented Aug 2, 2013

Definitely :+1:

@Morgul
Morgul commented Aug 2, 2013

This looks great. I'm really excited about it!

@jtreml
jtreml commented Aug 2, 2013

:+1:

@smashwilson

Almost ready -- I've got one more bug that keeps poking through the specs. Apparently, faker-generated issue text will very occasionally include a word that the ReferenceExtractor recognizes as a commit when the repository isn't stubbed out to handle it. Also I have problems running more than a few of the Spinach features at a time in my local Vagrant VM so there might be other bugs lurking there.

I'm hoping to have some time to put the finishing touches on this this weekend. Thanks for the encouragement :grinning:

@asyd
asyd commented Aug 3, 2013

:+1:

@smashwilson

Currently flushing out all of the problem spots by temporarily hardcoding the sentence factory to always contain a bad commit reference, and fixing them by stubbing with wild abandon. The specs are clean now, working on the features next.

@coveralls

Coverage Status

Coverage increased (+0%) when pulling ab16034063557bc8debdf2e0c37fea1211b37b5c on smashwilson:linking-issues into 8886b90 on gitlabhq:master.

@smashwilson

Hmm -- that failure looks unrelated to me, and I can't replicate locally.

@randx and others: ready for any feedback you've got to offer on this, whenever you get the chance.

@MrKeiKun
MrKeiKun commented Aug 8, 2013

Current Status Please?

@smashwilson

@MrKeiKun: Functionally complete, awaiting initial feedback. It's a big PR, too, so I'd be surprised if there weren't a few problems to sort out before it's ready to merge. (I'd also be happy to re-rebase if it drifts too far from master.)

@randx
GitLab member
randx commented Aug 19, 2013

scheduled for 6.1.
After 6.0 being released I'll get back to this PR

@randx
GitLab member
randx commented Aug 19, 2013

I'm highly interested in this PR :)

@randx randx commented on an outdated diff Aug 20, 2013
app/models/commit.rb
@@ -65,6 +68,29 @@ def description
end
end
+ # Regular expression that identifies commit message clauses that trigger issue closing.
+ def issue_closing_regex
+ @@issue_closing_regex ||= Regexp.new(Gitlab.config.gitlab.issue_closing_pattern)
@randx
GitLab member
randx added a note Aug 20, 2013

Please dont use @@ variables

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@randx
GitLab member
randx commented Aug 20, 2013

@smashwilson Looks good. Can you please make it mergeable. After ping me - I'll test it and merge if works.

@smashwilson

Hmmm, I re-rebased, but it looks like the MergeRequest model changes broke some things. I'll be working on it today.

@coveralls

Coverage Status

Coverage decreased (-0.18%) when pulling 5ff44274f6b60413f6def79f28a8dd8c8e7c6b87 on smashwilson:linking-issues into 21e3d84 on gitlabhq:master.

@coveralls

Coverage Status

Coverage decreased (-12.74%) when pulling bdeb7d67d4222814f39fb2d1f3f46843088e45cf on smashwilson:linking-issues into 21e3d84 on gitlabhq:master.

@MrKeiKun

Ready to test if on gitlabhq/gitlabhq:master

@smashwilson

I'm getting some weird issues in the specs and features. It looks like ActivityObserver is expecting Thread.current[:current_user] to be set in a few places where it isn't being populated. What's weirder is that the features occasionally pass anyway.

I'm also not sure yet what's going on with the Event count tests in activity_observer_spec that are creating Events for system notes (but only sometimes). Maybe there are factories being triggered during the block?

@smashwilson

Ah! Yes - I bet the :issue factories are being invoked within that block and creating Events that shouldn't be there. I'll re-push in a minute.

@coveralls

Coverage Status

Coverage decreased (-12.99%) when pulling e57c56c1d448b896de7760fa329ddab8c4a0c808 on smashwilson:linking-issues into 21e3d84 on gitlabhq:master.

@coveralls

Coverage Status

Coverage decreased (-12.79%) when pulling 73d45c2f3cf81f5b35c776b0e21cc7de2e309a49 on smashwilson:linking-issues into 21e3d84 on gitlabhq:master.

@smashwilson

There we go! At least, I think that last failure in the 2.0 build is unrelated. @randx, it should be ready to go, let me know if you find any other problems.

@randx
GitLab member
randx commented Aug 25, 2013

@smashwilson thank you. I'll test it and give you some feedback

@randx randx commented on an outdated diff Aug 25, 2013
app/models/issue.rb
@@ -56,4 +57,10 @@ class Issue < ActiveRecord::Base
# Both open and reopened issues should be listed as opened
scope :opened, -> { with_state(:opened, :reopened) }
+
+ # Mentionable overrides.
+
+ def gfm_reference
+ "issue ##{id}"
@randx
GitLab member
randx added a note Aug 25, 2013

Can you change it to iid please? I made iid for per project id (for issues and MR).

See 125c07f
and 7047a44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@randx randx commented on an outdated diff Aug 25, 2013
app/models/merge_request.rb
@@ -255,6 +255,20 @@ def project
target_project
end
+ # Return the set of issues that will be closed if this merge request is accepted.
+ def closes_issues
+ if target_branch == project.default_branch
+ unmerged_commits.map { |c| c.closes_issues(project) }.flatten.uniq.sort_by(&:id)
+ else
+ []
+ end
+ end
+
+ # Mentionable override.
+ def gfm_reference
+ "merge request !#{id}"
@randx
GitLab member
randx added a note Aug 25, 2013

same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@randx
GitLab member
randx commented Aug 25, 2013

@smashwilson I tested this PR briefly and I really like it.
Ping me if you need help with iid and we will merge this one

@smashwilson

@randx Thanks! This one was fun to code. :grinning:

I changed it to use iid for Issues and MergeRequests and updated the specs accordingly. I think I got them all, but If Travis finds more problems I'll keep at it.

@coveralls

Coverage Status

Coverage increased (+0.22%) when pulling 26a42d16c1c28126e4df5f75e28735dff70f7e46 on smashwilson:linking-issues into 2b36dee on gitlabhq:master.

@smashwilson

@randx The build is green. Let me know if you think of anything else!

@smashwilson smashwilson Link issues from comments and automatically close them
Any mention of Issues, MergeRequests, or Commits via GitLab-flavored markdown
references in descriptions, titles, or attached Notes creates a back-reference
Note that links to the original referencer. Furthermore, pushing commits with
commit messages that match a (configurable) regexp to a project's default
branch will close any issues mentioned by GFM in the matched closing phrase.
If accepting a merge request would close any Issues in this way, a banner is
appended to the merge request's main panel to indicate this.
c8a115c
@smashwilson

Ah, I missed one iid reference: the issue GFM links in the "accepting this merge request will close..." banner.

@coveralls

Coverage Status

Coverage increased (+0.22%) when pulling c8a115c on smashwilson:linking-issues into 2b36dee on gitlabhq:master.

@randx randx merged commit 551946a into gitlabhq:master Aug 26, 2013

1 check passed

Details default The Travis CI build passed
@randx
GitLab member
randx commented Aug 26, 2013

@smashwilson thank you. I've merged this one. If you find any missed referenced to iid - please send PR

@rufinus
rufinus commented Aug 29, 2013

@smashwilson
Hi sorry to write this here, didnt find a other way to contact you.
As written in http://feedback.gitlab.com/forums/176466-general/suggestions/3692754-linking-issues-from-comments-and-automatically-clo?tracking_code=9df9f2ef47771388afbbd2da3e44a130 i want to reward your effort with USD 100 $. How can get the money to you? (PayPal, Skrill, Amazon all fine)

@smashwilson

@rufinus: I appreciate the thought, but I'm just a guy who pokes at things on nights and weekends. I'd rather see the money go to a charity or an open-source project that you use or like, who have needs for hosting and other upkeep... like GitLab :grinning:

@rufinus
rufinus commented Sep 15, 2013

@smashwilson as you wish, I donated it to gitlab via pledgie. ( http://pledgie.com/campaigns/17027 )

@dosire
GitLab member
dosire commented Sep 16, 2013

GitLab received the money, thank you @rufinus and @smashwilson !

@rgbkrk
rgbkrk commented Sep 19, 2013

Wowsers. @rufinus and @smashwilson -- you are both so awesome!

:+1:

@t-oster
t-oster commented Sep 24, 2013

The feature does not seem to be present in 6.1. Did I do anything wrong?

@edude03
@mrPalm
mrPalm commented Sep 25, 2013

Im sorry if this post is in the wrong place.. but

I love this fix/update to close issues thrue commits.

I also altered this after update
issue_closing_pattern: (?i)(([)\s(close|fix)(s|es|d|ed)\s#\d+\s(])|(()\s(close|fix)(s|es|d|ed)\s#\d+\s()))
in commit.rb

and yes it now works with inline..
-m 'this (fixes #12) issue and thats good'

but i also want the multiple issues in one commit fixed.

-m 'this (fixes #32) and also (closes #43)'

Solved it with this.
Anyone know how i can still make use of the preset variable?
this dont work. md = safe_message.scan(Gitlab.config.gitlab.issue_closing_pattern)

 def closes_issues project
    md = safe_message.scan(/(?i)((\[)\s*(close|fix)(s|es|d|ed)*\s*#\d+\s*(\])|(\()\s*(close|fix)(s|es|d|ed)*\s*#\d+\s*(\)))/)
    #md = issue_closing_regex.match(safe_message)
    if md
      extractor = Gitlab::ReferenceExtractor.new
      md.each do |n|
       extractor.analyze(n[0])
      end
      extractor.issues_for(project)
      #extractor = Gitlab::ReferenceExtractor.new
      #extractor.analyze(md[0])
      #extractor.issues_for(project)
    else
      []
    end
  end
@t-oster
t-oster commented Sep 25, 2013

I tried it in a different install and it works like a charm. Thank you for this great feature. I have to check what was the problem... maybe I didn't make the reference in the first line....

@t-oster
t-oster commented Oct 1, 2013

The problem was, that the redis-cli executable was on /usr/local/bin instead of /usr/bin, so gitlab-shell couldn't find it. Since this did not show up in any logs or checks, maybe it's worth adding it to the check-script?

@dosire
GitLab member
dosire commented Oct 1, 2013

@t-oster That is a great idea, feel free to make a merge request for this.

@t-oster
@dosire
GitLab member
dosire commented Oct 1, 2013

@t-oster Ok, great!

@ssehovic

@smashwilson I have one question. Your implementation works on default (eg. master) branch but if I create another one let's say develop branch and trying to close an issuse on that branch closing issue does not work. Is it possible to make issue close on every branch not only master?

@smashwilson smashwilson deleted the smashwilson:linking-issues branch Apr 16, 2014
@smashwilson

@ssehovic The idea is that if you're using something like GitHub flow, you'll be pushing commits on other branches out to GitLab before they've been peer-reviewed and are ready for integration, so you'll have the "fixes" comment on a commit pushed well before you've actually dealt with the issue.

Currently you can't configure it to behave differently. Rather than toggle it to handle issue-closing commits on all branches, it might be interesting to do something like use a regex match against branch names, or handle them on all protected branches...

In the meantime, if you're using a "develop" branch as your integration target, is there any reason you couldn't make "develop" the default branch, instead?

@ssehovic

@smashwilson Thank you for reply. I am using git-flow "philosophy" right now and main branch is develop so I was hoping that this will work.
Best regards.

@dosire
GitLab member
dosire commented Apr 16, 2014

@ssehovic Have you made develop your default branch as @smashwilson asked? (see project => settings)

@ssehovic

@dosire Yes I have and by changing develop to default branch issue closing is working, but I was hoping that issue closing will be available in any branch :-) Btw, is issue always "related" to default branch in gitlab?

@dosire
GitLab member
dosire commented Apr 16, 2014

@ssehovic For now we'll keep it related to the default branch. If you have issues not related to the default branch you'll have to close them manually.

@CodeBrauer

Whats about closing multiple issues by one commit? fixes #39, fixes #36 closed only the first one - the 2nd just got a "mentioned"

@dosire
GitLab member
dosire commented Jun 17, 2014

@GabrielWanzek Feels broken to me, feel free to send an MR to fix this.

@CodeBrauer

@dosire , Isn't it in the code of @mrPalm ?

#4507 (comment)

BTW: I'm not a Ruby man :)

@dosire
GitLab member
dosire commented Jun 17, 2014

@GabrielWanzek Could be, but it is not a regression so there is no designated party to fix it.

@ds82
ds82 commented Jul 22, 2014

Hey guys, I'm sorry if this is the wrong place to post this, but I did not find any better.

While I love the auto-close ./. auto-reference-issues feature of gitlab, I really miss it when working in branches. I agree that issues should not be (auto-)closed if I push a fix to a branch, but they should be referenced.

If I push a commit to the master (aka default branch) of a project gitlab closes the issue, if I use the magic keywords ( close[s], fixe[s|d] ) and a issue number. If I just use the issue number, it creates a reference in the issue linking to the commit.

If I push a commit to a different branch (aka not the default branch) and using the magic keywords - nothing happens at all. If I just use the issue number, gitlab creates a reference to the commit in the branch.

What I'am suggesting here is, that gitlab should always create at least a reference. That means, if I push to a different branch than the default branch using a commit message like "foo bar, fixes #XX", gitlab should create a reference in issue #XX linking to that commit in the branch. When this commit is merged to the master, it should auto-close the issue and link to the commit in the master branch.

@dosire
GitLab member
dosire commented Jul 22, 2014

@ds82 I like your idea, please post to feedback.gitlab.com or create a tested merge request

@rumpelsepp

I also like @ds82 idea. +1

@aminadha
aminadha commented Feb 1, 2015

+1 @ds82 idea

@ambroisemaupate

+1 @ds82 idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.