Bug 354077 - ConfluenceLanguage does not use internalLinkPattern
Summary: ConfluenceLanguage does not use internalLinkPattern
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Mylyn (show other bugs)
Version: 1.5   Edit
Hardware: All All
: P3 normal with 1 vote (vote)
Target Milestone: 1.6.0   Edit
Assignee: patrick.boisclair CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2011-08-06 14:32 EDT by patrick.boisclair CLA
Modified: 2011-08-10 15:00 EDT (History)
3 users (show)

See Also:


Attachments
mylyn/context/zip (14.05 KB, application/octet-stream)
2011-08-08 12:26 EDT, David Green CLA
no flags Details
Proposed patch (13.55 KB, patch)
2011-08-09 09:07 EDT, patrick.boisclair CLA
greensopinion: iplog+
Details | Diff
mylyn/context/zip (28.44 KB, application/octet-stream)
2011-08-09 20:46 EDT, David Green CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description patrick.boisclair CLA 2011-08-06 14:32:33 EDT
Build Identifier: 3.6.0

ConfluenceLanguage.java does not use the internalLinkPattern and therefore the resulting links are not decorated as expected. All other wiki language are implementing this feature. The ConfluenceLanguageTest.java is also missing a test for it.

Reproducible: Always

Steps to Reproduce:
1. Create a wiki page with Confluence markup
2. Add a link such as [MyPage]
2. Use setInternalLinkPattern ("/something/{0}");
3. Render HTML.
4. Notice that the rendered link is <a href="MyPage" ...> and not <a href="/something/MyPage" ...> as expected.
Comment 1 David Green CLA 2011-08-08 12:26:48 EDT
Thanks for the bug!  Feel free to submit a patch.
Comment 2 David Green CLA 2011-08-08 12:26:49 EDT
Created attachment 201092 [details]
mylyn/context/zip
Comment 3 patrick.boisclair CLA 2011-08-09 09:07:06 EDT
Created attachment 201137 [details]
Proposed patch

Let me know if this is the proper way of submitting a patch and if it works for you.
Comment 4 David Green CLA 2011-08-09 20:46:26 EDT
(In reply to comment #3)
> Created attachment 201137 [details]
> Proposed patch

Patrick, thanks for the patch!  The patch is difficult to consume because it's not rooted at the root of the Git repository.  Also it contains several unrelated changes.  For future, this reference might help you to produce the patch: http://wiki.eclipse.org/Git_for_Committers#Generating_patches_for_Bugzilla or if you're using EGit: http://wiki.eclipse.org/EGit/User_Guide#Creating_Patches

Note that I cut out the @STANDARD_EXTERNAL_LINK_FORMAT@ since it was not covered by unit tests and I didn't see a use case for it -- please let me know if I've missed something.

Pushed the fix as commit 67ba4643a612a2d6209f0cf46d568e36ebeea48c
Comment 5 David Green CLA 2011-08-09 20:46:27 EDT
Created attachment 201202 [details]
mylyn/context/zip
Comment 6 patrick.boisclair CLA 2011-08-09 22:40:14 EDT
(In reply to comment #5)

David, I reviewed the code and everything looks good and more concise. Thank you for your help.
Comment 7 Steffen Pingel CLA 2011-08-10 06:07:40 EDT
David, when applying contributions, please always enter the name of the contributor as the author of the commit. To track authorship of changes please always commit the unaltered patch and then make another commit with your changes. All files altered by a contributor must also have a note in the copyright header and should have an @author tag if the change was significant. 

Reopening, since the copyright header is missing. Sorry, if the process wasn't clear. We are still in the process of updating the documentation.
Comment 8 David Green CLA 2011-08-10 13:14:24 EDT
(In reply to comment #7)

Apologies for incorrect application of our process.

> David, when applying contributions, please always enter the name of the
> contributor as the author of the commit. 

Do you mean Git author?  If so, this sounds new -- where can find the process for that?

> To track authorship of changes please
> always commit the unaltered patch and then make another commit with your
> changes.

The patch had a lot of things that needed changing, I was not prepared to apply the patch in its entirety -- however the unit tests were great.  In discussions before we discussed how teams often mark patches as iplog+ even if when applied they were altered.  I'm reluctant to commit a portion of the patch if it causes unit tests to fail -- and given that Patrick is a new contributor wanted to make the process as easy as possible without round-tripping the patch too many times.  Do we really want to make contributing that hard?

> All files altered by a contributor must also have a note in the
> copyright header and should have an @author tag if the change was significant.
>
> Reopening, since the copyright header is missing. Sorry, if the process wasn't
> clear. We are still in the process of updating the documentation.

My mistake, fixed in the latest push.
Comment 9 Steffen Pingel CLA 2011-08-10 14:20:16 EDT
(In reply to comment #8)
> > David, when applying contributions, please always enter the name of the
> > contributor as the author of the commit.
> 
> Do you mean Git author?  If so, this sounds new -- where can find the process
> for that?

http://wiki.eclipse.org/Development_Resources/Handling_Git_Contributions
 
> > To track authorship of changes please
> > always commit the unaltered patch and then make another commit with your
> > changes.
> 
> The patch had a lot of things that needed changing, I was not prepared to apply
> the patch in its entirety -- however the unit tests were great.  In discussions
> before we discussed how teams often mark patches as iplog+ even if when applied
> they were altered.  I'm reluctant to commit a portion of the patch if it causes
> unit tests to fail -- and given that Patrick is a new contributor wanted to make
> the process as easy as possible without round-tripping the patch too many times.
> Do we really want to make contributing that hard?

Yes, I think it's perfectly reasonable to round-trip a patch until it's close to what ends up in the repository. This should be the norm and hopefully we'll be able leverage better tools in the future that simplify the review and feedback process.

In either case I think it's the right thing to make an unaltered commit that is based on the original patch (I don't see any problem with committing a portion of a patch). Based on that you can always make additional commits to improve tests etc. and then push all commits at once to avoid having the master repository in an inconsistent state. We weren't able to do this before since CVS didn't allow commiting without making changes visible immediately. Does that make sense?
Comment 10 David Green CLA 2011-08-10 14:34:16 EDT
(In reply to comment #9)
> > Do you mean Git author?  If so, this sounds new -- where can find the process
> > for that?
> 
> http://wiki.eclipse.org/Development_Resources/Handling_Git_Contributions

Great, that's perfect.  Note that it doesn't discuss accepting patches and setting the commit author.

> Yes, I think it's perfectly reasonable to round-trip a patch until it's close to
> what ends up in the repository. This should be the norm and hopefully we'll be
> able leverage better tools in the future that simplify the review and feedback
> process.

Sounds good

> In either case I think it's the right thing to make an unaltered commit that is
> based on the original patch (I don't see any problem with committing a portion
> of a patch). 

In principle I agree -- however do we take "unaltered" literally, or do we mean with minimal changes?  Getting a patch in a form that can be applied unaltered sounds like it could discourage contribution.

> Based on that you can always make additional commits to improve
> tests etc. and then push all commits at once to avoid having the master
> repository in an inconsistent state. 

Would it make sense to put all of these on a separate (topic) branch, with a merge commit to master?  That way the grouping of commits would be traceable.  Having multiple commits per issue goes against some peoples ideas of an optimal git workflow.

> We weren't able to do this before since CVS
> didn't allow commiting without making changes visible immediately. Does that
> make sense?

Yes, though perhaps we should move this discussion to the mailing list to iron out any wrinkles.
Comment 11 Steffen Pingel CLA 2011-08-10 15:00:44 EDT
(In reply to comment #10)
> Great, that's perfect.  Note that it doesn't discuss accepting patches and
> setting the commit author.

Fair enough, but it seems sensible to apply the same policy to patches.

> > In either case I think it's the right thing to make an unaltered commit that
> is
> > based on the original patch (I don't see any problem with committing a portion
> > of a patch).
> 
> In principle I agree -- however do we take "unaltered" literally, or do we mean
> with minimal changes?  Getting a patch in a form that can be applied unaltered
> sounds like it could discourage contribution.

It's more overhead but it's usually a valuable learning experience that will encourage contributors to provide patches in a way that are easy to consume by the project. From my experience contributors have an interest in getting their patches applied and are perfectly willing to iterate. If not, you can always consider merging what is there.

> > Based on that you can always make additional commits to improve
> > tests etc. and then push all commits at once to avoid having the master
> > repository in an inconsistent state.
> 
> Would it make sense to put all of these on a separate (topic) branch, with a
> merge commit to master?  That way the grouping of commits would be traceable.
> Having multiple commits per issue goes against some peoples ideas of an optimal
> git workflow.

To me it's sufficient if the commit message references the same task but if anyone has concerns with that we could make that a policy. 

> > We weren't able to do this before since CVS
> > didn't allow commiting without making changes visible immediately. Does that
> > make sense?
> 
> Yes, though perhaps we should move this discussion to the mailing list to iron
> out any wrinkles.

I'll update the wiki page with the policy that comes out of this bug and notify the mailing list to discuss further.