Review code and content outside of merge request flow
Problem
Code review (and even reviewing of images https://gitlab.com/gitlab-org/gitlab-ce/issues/24816) occurs outside the "regular" workflow of creating code for a feature/bug, submitting a merge request, and reviewing the diff. Often, people just want to navigate to a file (or a particular version of it on any given branch), i.e. a commit, and review it, collaborating with other people asynchronously. This also includes the scenarios of documentation review where the docs are tracked through source control in a git repo.
Solutions
- https://gitlab.com/gitlab-org/gitlab-ce/issues/24946 Comment on any line in commit / merge request diff
- More to come
Original description
Description including problem, use cases, benefits, and/or goals
To be able to do offline code review or just to review file-by-file, it would be very useful to be able to add line comments to any file when viewing it, not only to diffs.
Note that we link to the full file right now on merge requests, but don't actually allow the person to comment.
Proposal
We add the ability to add line comments to any file.
Ideally the line comment would be linked to the current commit. That means that only if a new commit changes this file, the comment disappears from view for someone viewing
master, for instance.Suggestion by @dzaporozhets:
Probably it would be nice when you browse file to see
There are 3 comments on older version of this file. Show them. So you can easily find it even if file changes frequentlyLinks / references
@DouweM @rspeicher @dzaporozhets @stanhu let me know your thoughts
Activity
mentioned in issue #19049 (closed)
mentioned in issue #14923 (closed)
Mentioned in issue #19049 (closed)
@JobV : Can you provide more context for this proposed feature? Are customers asking for it? Does it solve a particular problem or pain point that is fairly common?
Currently we can code review / collaborate by creating an issue and/or creating a merge request. Discussion is in the threads of both places, with inline code comments in the merge request itself. We can introduce tighter integration here between the two. For example, maybe including the diff in the issue itself, including certain parts of the merge request code portion in the threads, etc. etc. I think there's a lot to explore here. With a Issue + Merge Request paradigm, we can probably solve the problems you are referring to.
If we start introducing comments at the commit level, it introduces a whole new notion of collaboration. With more complexity and a whole new way of using GitLab. I'm not convinced at this point it's worth it because we haven't exhausted the power of Issues and Merge Requests.
Edited by Victor Wu@victorwu agreed to avoid complexity as much as possible.
A subset of our customers does offline code review. So out of merge requests, out of the workflow of merging things. This is a trend that you see in many large organisations as well. For offline code-review to work with a low-threshold, the feature as above would be the ideal.
We can put this in the fridge until we see a stronger demand for it.
@JobV : Sorry for my ignorance. What do you mean by "offline code review"? I understand that some people pair together and stare at a screen and chat in real-time, and do code review. Other folks do it asynchronously by using a tool such as GitLab to leave comments in a merge request. I googled "offline code review", and it refers to the latter. So we're already doing that? What am I missing?
@victorwu Currently you can comment only in merge requests, the idea behind this topic is to allow commenting code in the repository.
Edited by username-removed-630544@mgielda: Thanks for the feedback. Are you folks describing a scenario where you want to make a comment on an existing file on any branch (outside the context of a merge request)? In that case, would a solution that leverages the issues functionality work well in your workflows?
-
Somebody is reviewing documentation that is stored in a GitLab repo.
-
They would like to make a comment and start collaborating on it.
-
There is some UI to automatically create an issue and link to the file and a specific line or section of lines in the file.
-
Folks would be able to collaborate and chat about the file, and leverage the power of issues themselves.
Edited by Victor Wu-
Thanks @VirusAirInfector . So you are saying that leaving comments attached to files directly in the repository would allow you to do collaboration right? Sounds good. Any particular scenarios that you would use that for?
Mentioned in issue #24561 (closed)
Hi @victorwu!
We have an external issue tracker, unfortunately. Besides, I think commenting on the code 'as is' is a really useful option, in some scenarios. You know, when you'd like to tell people 'hey, could you correct this, this and this please' without having to hunt for where the changes were introduced etc.
When I check documentation I typically print out the entire doc and read through it. This is then a 'snapshot' of the docs as they are. There are usually many small changes, so it would be really hard to list them all in one issue. Commenting on the full file would be perfect. In fact, I see it as commenting on a commit, though not on changes introduced by the commit but rather the current status of the code as it is if you checked out exactly that commit = what shows when you click "Browse code" on a specific commit.
If there was then a way, upon entering the view for specific files, to make comments on them (currently there is only a possibility to link to specific lines) and some kind of overview of comments for this particular commit (which would play very very well with !5022 (merged), the ability to mark such comments as resolved once they are taken into account), it would be a very powerful tool for collaboration, especially around text documents, and I don't mean just documentation but also other textual content.
Hi!
I second Michael's presentation. In fact, one way to manage such comments would be to attach them to the last commit which modified the line in question - by this, the system would known whom to send an e-mail... And in file view, you would see the comments from different commits pointing to lines which were not reworked since then. What a nice feature - an overview over all open discussions on a per-file base!
Thanks @mgielda + @pbihler for the extremely clear and thorough feedback. It makes a lot of sense. It sounds like you want the power of line commenting and even discussion resolution, within the simple context of just reviewing a file online. Merge requests are overkill for this scenario. We'll keep this issue updated as we proceed on the discussion and hope to get an opportunity for implementation.
Mentioned in issue #24344 (closed)
Hi @victorwu, that is exactly what I mean. Thanks for listening to our feedback, and looking forward to hearing more!
And keep up the awesome work developing GitLab.
Added code review and removed ~13921 labels
@victorwu offline code review = code review on what's in the repo now, instead of in merge requests. @mgielda describes it well.
In fact @mgielda makes an interesting point. There's a difference between commenting on a commit and on a specific file and a specific file/commit. It's important that we make this clear:
- if you could comment on a file with a specific commit, that comment is gone as soon as a new commit touches this file
- if you could comment on a specific file, but do not link it to a specific commit, I suppose this would be considered an ongoing discussion about a particular file. This might be confusing and hard to implement, but valuable for architecture conversations. I'm not sure if we should ever do that.
Random other thought: if you could comment on a specific file, it might be interesting to be able to 'pin' that comment to the HEAD of the branch, so it remains visible through changes. This would imply that line comments would shift around: we couldn't guarantee the position. But it would make the second option above possible.
Hi @JobV - yes, I meant code review without merge requests. We can agree to call it 'offline code review' even if it is a little bit confusing ;)
As for the distinction on commenting on files/commits: I think that both objectives could be achieved more easily. Basically I think comments should be made on a specific file and commit, as implementing it otherwise would be hard and misleading. But since GitLab offers the possibility to mark conversations as resolved (and so we can consider the rest 'unresolved') we can have a mention in the UI that there are X unresolved conversations related to this file (with a link to a list of those) - then we have the cake and eat it (unless my thinking is flawed, which may well be the case).
If a subsequent change fixes or obsoletes the remark raised in the discussion, then the discussion can be marked as resolved and voila, it does not pop up any more (of course you can still find it if you browse through older commits if you really need it).
Edited by username-removed-18474@JobV Ad 1) It would be great if the comment was associated with the commit which introduced the given line of code and remained visible until the line is changed (i.e. the blame for the line is changed, ideally ignoring whitespace changes).
@jankuca That is exactly what I intended to describe last week - So no new concept is required, only a different view.
Uhh, @pbihler @jankuca, I do not know really, I think you are talking about a different scenario than I am and so your needs are different.
In reviewing text, changing a line is not the same as in code. Someone may change a line but not necessarily fix a comment you made on the text before (a common example is a nomenclature inconsistency which can appear in documentations which grow beyond one file).
For our use cases we would need to have a discussion which would be linked to from the file view (in the commit that you are reviewing, understood as a 'snapshot' of the state of the document, not as a list of changes) until the comment is resolved, not until the line changes. In general I am also a fan of implementing it as a modification of the view, not a new concept.
And I would like it to be associated with the commit you are reviewing, not with the commit that made the last change to the given line since the error you are referring to may well have been introduced in an entirely different commit and then putting the discussion there may even be misleading.
Example:
- commit 1: I have a cats.
- commit 2: I have a cats -> I have a beautiful cats
- commits 1&2 get pushed
- REVIEW STARTS
- commit 3: I have a beautiful cats -> I have a beautiful cats that meows.
- REVIEW ENDS
- commit 3 gets pushed
During the review, I would be looking at commit 2, making comments on this line, but in reality I would be referring to a mistake made in commit 1! I would like the discussion to appear in commit 2 because I am looking at the line in the context of the entire document at the time of the review. But I would not like the discussion to disappear when commit 3 is pushed. The error still persists.
Also, I need to be making comments on the view of the entire file (since I need to make sure the line makes sense in its context, like checking pronouns like "it" referring to a previous line etc.) Often a person changing a line of text in docs makes it inconsistent with the previous line without noticing. Currently it is impossible to catch and report that kind of thing from GitLab.
I am actually quite certain this could be useful to code review as well.
I hope my dumb example explains it ;)
Edited by username-removed-18474Mentioned in issue #24946 (closed)
Thanks for all the feedback. They use cases are extremely clear and we want to move forward with this. There's a few things in our existing GitLab implementation we want to clean up first. The first one is just allowing comments everywhere in a file in a commit. https://gitlab.com/gitlab-org/gitlab-ce/issues/24946
Edited by Victor WuMentioned in issue #25066 (closed)
Mentioned in issue #2641 (closed)
A suggestion I made to @heiko_boettger but might be of use to others:
For merge requests we're working on #24378 (closed) which can be used as a workaround, by starting a discussion and mentioning a the file which wasn't touched or linking to a line in a file. This will then appear in the list of unresolved discussions, ensuring they get dealt with for review.
mentioned in issue #20541 (moved)
Prospective customer may be interested in this feature as well https://gitlab.zendesk.com/agent/tickets/56005.
-
Related issue https://gitlab.com/gitlab-org/gitlab-ce/issues/17542
