Bug 434841 - Support fields for linking to Gerrit and Git commits
Summary: Support fields for linking to Gerrit and Git commits
Status: VERIFIED FIXED
Alias: None
Product: Community
Classification: Eclipse Foundation
Component: Bugzilla (show other bugs)
Version: unspecified   Edit
Hardware: PC Mac OS X
: P3 normal with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: Eclipse Webmaster CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 434811 435599
  Show dependency tree
 
Reported: 2014-05-14 07:41 EDT by Steven Spungin CLA
Modified: 2015-02-05 11:49 EST (History)
17 users (show)

See Also:


Attachments
screenshot (554.58 KB, image/png)
2014-11-13 11:02 EST, Steven Spungin CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Spungin CLA 2014-05-14 07:41:57 EDT
It would be helpful to have these additional fields for bugs:

1.  ChangeId [space separated list of Gerrit review changeIds that aim to solve the bug]
2.  CommitId [space separated list of GIT commitIds that resolved the bug

These would be useful for the following reasons:

1. Developers would not have to add an additional comment for these links
2. It would make the process of finding this information consistent
3. Accessing those fields from XMLRPC would be more reliable than trying to parse the comments.
4. If the ChangeID was in the bug, Gerrit could more easily provide efficient means to close the bug.
5. It would make it clearer if there was a solution in the works, or if a solution (partial solution) was merged, regardless of the status field.
Comment 1 Marc Khouzam CLA 2014-05-15 09:14:02 EDT
+1

In CDT we used to have a genie that would detect commits that started with "Bug xyz" and add a comment to the bug.  Something like that would be nice feature to auto-fill the proposed CommitId field.

But let's start with getting the fields ;-)
Comment 2 Denis Roy CLA 2014-11-12 16:58:32 EST
In the end, we follow cues from the community.  Right now the bug has 2 comments on it.  While the idea makes sense, we want to make sure it's the best solution for everyone (as additional fields will "pollute" everyone's UI).

Typically, we suggest someone discuss with other committers on the eclipse.org-committers list and rally up some support for the change  :)
Comment 3 Matthias Sohn CLA 2014-11-13 03:12:53 EST
+1 for adding these fields

we could also consider to use the Gerrit Bugzilla integration (https://gerrit.googlesource.com/plugins/its-bugzilla/+/master/src/main/resources/Documentation/about.md)
Comment 4 Christian Schneider CLA 2014-11-13 03:16:36 EST
I would rather propose to add the issue number in the commit and then have an automatic integration in the issue system that shows commits that are related to issues.

We use this at apache. There we prepend the commit comment with e.g. "FELIX-4689" which is a jira issue id. Jira integrates with the svn and git repos and automatically shows related commits.

A similar solution would surely also work for eclipse.
Comment 5 Lars Vogel CLA 2014-11-13 03:30:44 EST
(In reply to Christian Schneider from comment #4)
> I would rather propose to add the issue number in the commit and then have
> an automatic integration in the issue system that shows commits that are
> related to issues.

That is not the scope of this bug. See Bug 435599 for additional suggestions.
Comment 6 Mickael Istria CLA 2014-11-13 04:10:29 EST
Big +1 for dedicated (manual) fields mapping from Bugzillas to Gerrit reviews and actual commits. This would make the bug report very useful, even after it has been closed.
It's also seems to me as a first necessary iteration towards a more automated linkage between those tools.
Comment 7 Jay Billings CLA 2014-11-13 05:42:33 EST
+1. I think it would be very helpful/useful and worth the "pollution."
Comment 8 Gunnar Wagenknecht CLA 2014-11-13 07:07:43 EST
(In reply to Steven Spungin from comment #0)
> 1. Developers would not have to add an additional comment for these links

That argument is broken. I have to enter a comment anyway in order to resolve a bug. Thus, I'd prefer continue doing just that instead of *additionally* having to populate more fields.

> 2. It would make the process of finding this information consistent

That's true.

> 3. Accessing those fields from XMLRPC would be more reliable than trying to
> parse the comments.

Yes, but I don't to see which tools would parse those? (see next argument re: tools)

> 4. If the ChangeID was in the bug, Gerrit could more easily provide
> efficient means to close the bug.

I don't get that. All integrations of SCM and issue tracking typically work by referencing the bugs in SCM commits. Gerrit can just use that number and do whatever it wants with it. In fact, there is a Gerrit Bugzilla plug-in available.

> 5. It would make it clearer if there was a solution in the works, or if a
> solution (partial solution) was merged, regardless of the status field.

That I don't get either. The proposal says it's a list of ids. Nothing would be clearer by looking at a list of chars. Plus, why would it by the change id and not the review number?


(In reply to Lars Vogel from comment #5)
> That is not the scope of this bug. See Bug 435599 for additional suggestions.

Lars, I'm sorry but I also see to fail how this bug contributes to bug 435599. Having to populate additional fields manually is by no means a simplification of any committer workflow.

My guess is that there will be only a few teams populating the additional fields. I'd favor a solution which automatically scans Git for related commits and displays them in Bugzilla and/or populates such fields automatically.
Comment 9 Lars Vogel CLA 2014-11-13 07:18:17 EST
(In reply to Gunnar Wagenknecht from comment #8)

> (In reply to Lars Vogel from comment #5)
> > That is not the scope of this bug. See Bug 435599 for additional suggestions.
> 
> Lars, I'm sorry but I also see to fail how this bug contributes to bug
> 435599. Having to populate additional fields manually is by no means a
> simplification of any committer workflow.

In Platform we have the policy to always add the Gerrit review and the link to the commit to the bug report. This helps our contributors and committers to find the relevant information. At the moment we have to do that manually, having fields would allow to automate that.

You do not have to populate these fields if that is not part of your project culture. The question is more, would you mind having two additional fields in the UI.
Comment 10 Gunnar Wagenknecht CLA 2014-11-13 07:46:24 EST
(In reply to Lars Vogel from comment #9)
> You do not have to populate these fields if that is not part of your project
> culture. The question is more, would you mind having two additional fields
> in the UI.

I would mind if there is no functionality. Is the integration to populate the fields ready to deploy? If yes, go add the fields and enable it.
Comment 11 Christian Schneider CLA 2014-11-13 07:52:30 EST
One thing to consider is multiplicity. The reason why we chose to add the issue to the commit in apache and not the other way round is that sometimes an issue is solved by more than one commit.

So for the proposed fields it should be possible to add more than one commit. I also think it would be good to then create links to the commits in the UI. So a user can easily navigate to the commit diff.
Comment 12 Lars Vogel CLA 2014-11-13 08:00:27 EST
(In reply to Gunnar Wagenknecht from comment #10)
> I would mind if there is no functionality. Is the integration to populate
> the fields ready to deploy? If yes, go add the fields and enable it.

AFAIK having these fields would allow us to try out the integration, see the link from Matthias in Comment 3. I'm not a Bugzilla nor Gerrit developer so I can't judge that but I know that we could use these fields already in platform instead of polluting the message text with this information.
Comment 13 Mickael Istria CLA 2014-11-13 08:00:59 EST
(In reply to Christian Schneider from comment #11)
> I would mind if there is no functionality. Is the integration to populate
> the fields ready to deploy? If yes, go add the fields and enable it.

Even if there is no automation yet, those fields is functional value for projects that use them, it's the same as for URL, Whiteboard, Keywords, Tags and other flags.
Comment 14 Christian Campo CLA 2014-11-13 08:14:58 EST
I don't mind having the extra two UI fields. However I wouldn't use them for myself. As others have already said, it makes a hell lot more sense to reference the bug in the commit than the other way round. Maybe we are all spoiled by what Jira and Stash does buts thats what they do and its super handy.

Maybe in a second step, Denis could add a post-commit step in Git on the server that parses the commits and adds the info to bugzilla. (or someone else writes that code)

I am a little uncertain what the "commitId" is. If its the Git hash than its certainly not enough, since you also need a reference to the git repo don't you. Most projects have more than one.

Last not least I think to make this useful each changeId and commit should be shown as a link on the UI. (maybe not at the beginning but at some stage). If the changeId is just text and I have manually find the commit in the correct repo, you find many people using it.

In that light wouldn't it make more sense if a Git-post hook automatically adds a comment to the bug that links to a browser view that shows the diff of the commit that solved this bug ? (just saying)
Comment 15 Lars Vogel CLA 2014-11-13 08:24:24 EST
(In reply to Christian Campo from comment #14)
> it makes a hell lot more sense to
> reference the bug in the commit than the other way round. 

That works already, if you put Bug 123456 into your commit messages, the Bug link is generated in git.eclipse.org/c
Comment 16 Christian Campo CLA 2014-11-13 08:32:31 EST
(In reply to Lars Vogel from comment #15)
> (In reply to Christian Campo from comment #14)
> > it makes a hell lot more sense to
> > reference the bug in the commit than the other way round. 
> 
> That works already, if you put Bug 123456 into your commit messages, the Bug
> link is generated in git.eclipse.org/c

No it doesn't work the way I meant it. I don't wanna reference the bug in the commit and then the commit in the bug. That seems a redundant effort.

I want to reference the bug in the commit and then automatically the commit is referenced in the bug. And I think I would like that reference in the bug to be a hyperlink the the commit diff.....
Comment 17 Christian Schneider CLA 2014-11-13 08:38:21 EST
(In reply to Christian Campo from comment #16)
> (In reply to Lars Vogel from comment #15)
> > (In reply to Christian Campo from comment #14)
> > > it makes a hell lot more sense to
> > > reference the bug in the commit than the other way round. 
> > 
> > That works already, if you put Bug 123456 into your commit messages, the Bug
> > link is generated in git.eclipse.org/c
> 
> No it doesn't work the way I meant it. I don't wanna reference the bug in
> the commit and then the commit in the bug. That seems a redundant effort.
> 
> I want to reference the bug in the commit and then automatically the commit
> is referenced in the bug. And I think I would like that reference in the bug
> to be a hyperlink the the commit diff.....

+1 fully agree
Comment 18 Thomas Watson CLA 2014-11-13 08:43:39 EST
(In reply to Christian Campo from comment #16)
> (In reply to Lars Vogel from comment #15)
> > (In reply to Christian Campo from comment #14)
> > > it makes a hell lot more sense to
> > > reference the bug in the commit than the other way round. 
> > 
> > That works already, if you put Bug 123456 into your commit messages, the Bug
> > link is generated in git.eclipse.org/c
> 
> No it doesn't work the way I meant it. I don't wanna reference the bug in
> the commit and then the commit in the bug. That seems a redundant effort.
> 
> I want to reference the bug in the commit and then automatically the commit
> is referenced in the bug. And I think I would like that reference in the bug
> to be a hyperlink the the commit diff.....

+1, I agree with Christian, that is exactly what I would like.  No other steps by the committer to reference the commit in the bug.
Comment 19 Ed Merks CLA 2014-11-13 08:48:03 EST
Currently for the projects I work on I include [<bugzilla-id>] in the commit message and I include the full commit link in the bugzilla so there are two way references between the two. 

https://bugs.eclipse.org/bugs/show_bug.cgi?id=450056#c7

Note in this case there were two commits because it was addressed for both maintenance and master.

Something to formalize this and/or automate this would be great!
Comment 20 Lars Vogel CLA 2014-11-13 08:51:43 EST
(In reply to Thomas Watson from comment #18)
> +1, I agree with Christian, that is exactly what I would like.  No other
> steps by the committer to reference the commit in the bug.

Great, everyone agrees here. 

This is the target and the additional field in Bugzilla is a prerequisite for that (so that we have something to place the link to the commit). The automatic update to this field is handled by Bug 434811.
Comment 21 Christian Campo CLA 2014-11-13 08:53:47 EST
(In reply to Lars Vogel from comment #20)
> (In reply to Thomas Watson from comment #18)
> > +1, I agree with Christian, that is exactly what I would like.  No other
> > steps by the committer to reference the commit in the bug.
> 
> Great, everyone agrees here. 
> 
> This is the target and the additional field in Bugzilla is a prerequisite
> for that (so that we have something to place the link to the commit). The
> automatic update to this field is handled by Bug 434811.

The addition field could hold multiple pointers to git commits and represent them as hyperlinks ?
Comment 22 Lars Vogel CLA 2014-11-13 09:02:37 EST
(In reply to Christian Campo from comment #21)
> The addition field could hold multiple pointers to git commits 

See the original request description. Yes. I think it should be similar to attachments.

> and represent them as hyperlinks ?

I don't know if Bugzilla can do this, would be definitely desired if possible.
Comment 23 Christian Campo CLA 2014-11-13 09:31:06 EST
(In reply to Lars Vogel from comment #22)
> (In reply to Christian Campo from comment #21)
> > The addition field could hold multiple pointers to git commits 
> 
> See the original request description. Yes. I think it should be similar to
> attachments.
> 
> > and represent them as hyperlinks ?
> 
> I don't know if Bugzilla can do this, would be definitely desired if
> possible.

I am not against additional fields. My fear simply is that if we put the links to commits in additional fields that a comfortable hyperlink back to Git will require extra work. And for me its not a nice-to-have but a prereq to make it usable. (or a prereq for me to use it)

On the other hand linking from comments is something that we know that it works.

Lars what is downside from your POV of putting the links in comments ?
Comment 24 Lars Vogel CLA 2014-11-13 09:38:47 EST
(In reply to Christian Campo from comment #23)
> Lars what is downside from your POV of putting the links in comments ?

I agree with Mickael on this. See his points https://bugs.eclipse.org/bugs/show_bug.cgi?id=434841#c13
Comment 25 Gunnar Wagenknecht CLA 2014-11-13 10:04:34 EST
(In reply to Lars Vogel from comment #12)
> AFAIK having these fields would allow us to try out the integration, see the
> link from Matthias in Comment 3.

The integration works without additional fields. It publishes comments which (based on this request) isn't what you want.

> but I know that we could use these fields already in
> platform instead of polluting the message text with this information.

So there is value in having the fields for some teams. That is a valid argument.
Comment 26 Konstantin Komissarchik CLA 2014-11-13 10:29:18 EST
I am in favor of automatic detection of Bugzilla references in Git and display of corresponding Git links in Bugzilla, but I am unclear why adding the proposed field is the first step towards that goal. Surely, the automated system would not be implemented by writing to the Bugzilla database when a Git commit is detected.

In Sapphire, when a fix is pushed to multiple branches, we annotate in the commit message which commit goes with which branch. Without such annotation, a list of commits would be less useful.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=449989#c1

Also, I saw others raise this question, but I didn't see an adequate answer... A bare commit id is pretty useless. One also needs to specify the repo and it needs to function as a link. If we are pasting multiple links into a single one-line text field, that would be quite unwieldy.
Comment 27 Lars Vogel CLA 2014-11-13 10:40:51 EST
(In reply to Konstantin Komissarchik from comment #26)

> Also, I saw others raise this question, but I didn't see an adequate
> answer... A bare commit id is pretty useless. One also needs to specify the
> repo and it needs to function as a link. If we are pasting multiple links
> into a single one-line text field, that would be quite unwieldy.

We (platform.ui) also use the same format as you, e.g. https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=e7c04bd2443cf702e7bde556ef77f9fcb67f0692 and this is what should IMHO store in the additional fields. Sorry if that was unclear, I guess I assumed everyone uses this format.
Comment 28 Denis Roy CLA 2014-11-13 10:43:30 EST
Here are a few thoughts:

> Maybe in a second step, Denis could add a post-commit step in Git

We have Git, and we have Gerrit, and those two don't use the same post-commit hooks.  If we write a hook, it will be for Gerrit only.

That said, +1 on a post-commit step which parses the commit and posts to Bugzilla automatically.


In terms of fields, we currently have two fields which are generally underused:

- whiteboard
- See Also

The current See Also field is made to reference a specific set of sites.  It may be possible to alter its functionality to allow links to Gerrit.  Since the field can contain multiple entries, and creates clickable links automatically, we could get much of the desired functionality for free without adding any more UI pollution.
Comment 29 Lars Vogel CLA 2014-11-13 10:49:04 EST
(In reply to Denis Roy from comment #28)

> The current See Also field is made to reference a specific set of sites.  It
> may be possible to alter its functionality to allow links to Gerrit.  Since
> the field can contain multiple entries, and creates clickable links
> automatically, we could get much of the desired functionality for free
> without adding any more UI pollution.

+1, I like this better than the original proposal to create new fields.
Comment 30 Denis Roy CLA 2014-11-13 10:56:26 EST
Here's a reference that gives me clues how to patch into the See Also field:

Bug 704999 - Add support for GitHub for the 'See Also' field 
https://bugzilla.mozilla.org/show_bug.cgi?id=704999

Since Bugzilla is using the See Also field for referencing GitHub and Pull Requests, I think using the See Also for Gerrit and git.eclipse.org/c references is the way to go.
Comment 31 Steven Spungin CLA 2014-11-13 11:02:25 EST
Created attachment 248640 [details]
screenshot
Comment 32 Steven Spungin CLA 2014-11-13 11:02:44 EST
When I originaly filed this, I had already written tooling that lists all gerrit reviews for a committer.   For each review, all the bugzilla bugs in the commit messages are cross referenced.  See attachment.

If the review was submitted, the tooling alerts the committer if 
1. bug is still open 
2. the bug does not reference the review/commit.

With the press of a button, the commiter can close the bug, and leave a message with a link to gerrit/git.  

My tooling uses REST calls, and is independent from both Gerrit and Bugzilla sites;

I posted this bug because there was no standard way to cross reference between the issue and the solution.

Furthermore, if a bug is reopened, there are often several referenced to patches that may or may not have solved the issue. 

Having a field would provide a consistent and dynamic place for the reference.
Comment 33 Denis Roy CLA 2015-01-29 15:32:39 EST
> The current See Also field is made to reference a specific set of sites.  It
> may be possible to alter its functionality to allow links to Gerrit.  Since
> the field can contain multiple entries, and creates clickable links
> automatically, we could get much of the desired functionality for free
> without adding any more UI pollution.
Comment 34 Denis Roy CLA 2015-01-30 11:10:46 EST
As you can see, the See Also field will now accept Gerrit URLs as well as links to commits on cGit.

Il see if I can pretty them up using the Templates.
Comment 35 Lars Vogel CLA 2015-01-30 11:17:22 EST
(In reply to Denis Roy from comment #34)
> As you can see, the See Also field will now accept Gerrit URLs as well as
> links to commits on cGit.
> 
> Il see if I can pretty them up using the Templates.

Thanks Denis, that is very useful (and cool). In case the Templates are flexible enough we would suggest to use a prefix for the different types, for exmaple:

Gerrit 40155
Commmit 98011c093 -> 8 or 10 entries should be sufficient
Bug 1212312
Comment 36 Steven Spungin CLA 2015-01-30 11:19:00 EST
+1 for standardizing 'See Also' content format
Comment 37 Denis Roy CLA 2015-01-30 13:23:45 EST
If you refresh this bug, the See Also should be much better.  I probably can match the commit ID and change number and display it, but let's start with this.

Closing as fixed. This paves the way for automation via bug 434811 and 435599.
Comment 38 Denis Roy CLA 2015-01-30 13:39:07 EST
Now if only Bugzilla could set "resolution"=> fixed when the comment says so.

Apologies for the noise.
Comment 39 Lars Vogel CLA 2015-01-31 06:23:03 EST
Thanks a bunch Denis, this will help to keep the bug reports much cleaner and improve their readability.
Comment 40 Lars Vogel CLA 2015-02-02 11:49:58 EST
Thanks Denis, works fine and this will hopefully help us to keep the bug discussion to the content.

On minor tweak maybe: If I press "add" under the See also its says:

Add Bug URLs: 

Maybe this can be changed to:

Add Bug, Gerrit or Commit URLs:
Comment 41 Sam Davis CLA 2015-02-02 13:43:12 EST
This looks really useful. With bug 307729, I hope we'll soon support this in the Mylyn connector as well.
Comment 42 Denis Roy CLA 2015-02-02 14:52:39 EST
> On minor tweak maybe: If I press "add" under the See also its says:
> 
> Add Bug URLs: 
> 
> Maybe this can be changed to:
> 
> Add Bug, Gerrit or Commit URLs:

Thanks, Lars. I've changed the label.
Comment 43 Lars Vogel CLA 2015-02-03 03:46:20 EST
Thanks Denis. One last tweak if possible.

If I push to Gerrit the response ULR is at the moment not valid for Bugzilla. For example if I push a new change to Gerrit I receive the following URL:

https://git.eclipse.org/r/40952/

If I enter this is Gerrit, Gerrit forwards to https://git.eclipse.org/r/#/c/40952/.

Would it be possible to allow "https://git.eclipse.org/r/40952/" in Bugzilla? I want to paste the URL directly into Bugzilla without the need to edit it.
Comment 44 Denis Roy CLA 2015-02-05 09:58:23 EST
> https://git.eclipse.org/r/40952/

Testing...
Comment 45 Denis Roy CLA 2015-02-05 09:59:33 EST
Lars, it works.  I needed that for bug 434811, which I will be updating soon.
Comment 46 Lars Vogel CLA 2015-02-05 11:08:28 EST
Verified, thanks Denis.