New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 808812 link

Starred by 2 users
Status: Duplicate
Merged: issue 767856
Owner: ----
Closed: Mar 2018
Cc:
qyears...@chromium.org
█ emso@chromium.org
mar...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

Allow filtering of comments that are in/out of modified lines of code

Project Member Reported by qyears...@chromium.org, Feb 3 2018 
Sometimes users will only care about comments in modified lines of code. Even if Tricium posts comments about comments in non-modified lines of code, we may want the default behavior for these comments to be FYI/hidden/resolved.

Currently in AnalyzeRequest we get the paths of changed files but not the modified lines. As long as Tricium can get the modified lines, it can do some filtering before posting comments.

Note from maruel@:

> https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-diff should work to get line numbers from tricium. It's downloading the delta lines but at least it's not downloading the whole file, so it's still a net improvement.

Comment 1 by qyears...@chromium.org, Mar 2 2018 

Regarding the get-diff API: It has the advantage that it would work even for the Gerrit "magic paths", (/COMMIT_MSG and /MERGE_LIST).

However, getting the diff for the changes in a patch involves sending one request to Gerrit per file. When there are hundreds to thousands of files in a patch and tens of patches, this could quickly add up.

Another way to get the full patch is the get-patch endpoint:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#get-patch

Yet another way to get the full diff:
In GitFileIsolator, we run `git fetch --depth=1 <repo> <ref>` followed by git checkout FETCH_HEAD -- <files>.
If we change that to --depth=2, then we could get the diff by running `git diff FETCH_HEAD~ FETCH_HEAD`. Again, this doesn't work with special magic Gerrit paths like /COMMIT_MSG but that might be OK.

Comment 2 by mar...@chromium.org, Mar 2 2018 

I'd say get the full patch, then individual diff for the magic paths.

Comment 3 by qyears...@chromium.org, Mar 2 2018

Mergedinto: 767856
Status: Duplicate (was: Available)
►

Sign in to add a comment