mr discussion: allow to comment on specific lines in a diff #752

Closed krobelus closed 3 years ago

(the first two commits are independent updates but I included them here to avoid conflicts)

GitLab supports to comment on lines (or even ranges of lines) within a commit diff. This is standard practise when using the web interface, and it's arguably friendlier to the MR author.

Add a bunch of --position-* parameters to "lab mr discussion" to implement this [1]. (This interface is awkward, because almost all of them are needed. Maybe we could fit all of this in one parameter somehow? Then there'd be less room for user errors.)

Given a commit diff like

--- a/README.md
+++ b/README.md
@@ -3,2 +3,5 @@
 line 3
 line 4
+line 5
+line 6
+line 7

this allows to run a command like this to comment on "line 6".

lab mr discussion origin branch --commit=commit-id --position-file=README.md --position-line-type=+ --position-line-number=6 --position-line-number-old=5

The parameters should be fairly self-explanatory, except maybe the line numbers. They are the equivalent to the two columns of line numbers that GitLab/GitHub display to the left of a diff. We should document in the command line help how they are computed, but I wasn't sure where to add it.

This feature is designed to work well with Tig, because Tig readily exposes the correct line number parameters. Adding this to your ~/.tigrc

bind diff C !lab mr discussion %(remote) %(branch) --commit=%(commit) --position-file=%(file) --position-line-type=%(text) --position-line-number=%(lineno) --position-line-number-old=%(lineno_old)

will allow you press C on a line inside a diff hunk to comment on that line. This can be really convenient with Tig. I took the liberty to mention Tig in the documentation; not sure if this is appropriate. (Please note that the --position-file-old which I added to the examples currently requires an unreleased version of Tig, see Tig commit 5a23214c (Expose %(file_old), the filename before a rename or deletion, 2021-10-04).)

One annoyance with this binding is that every "lab mr discussion" touches the network, so can be noticably slow. That's easy to fix though.

Background: I have been using a similar workflow with Tig, see https://github.com/krobelus/gitlab-offline-review I'd love to eventually switch to lab (or something alike)

I only heard about it in the recent Linux Plumber's Conference. Also I discovered today that there is yet another similar project glab. however, they don't have a "mr discussions" command, so I stuck to lab.

(Additionally, there is bichon but that looks less flexible than a command line tool).

Codecov Report

Merging #752 (5205d67) into master (2519cdf) will increase coverage by 0.37%. The diff coverage is 41.97%.

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   54.77%   55.15%   +0.37%     
==========================================
  Files          77       77              
  Lines        5616     5581      -35     
==========================================
+ Hits         3076     3078       +2     
+ Misses       2258     2220      -38     
- Partials      282      283       +1

Impacted Files	Coverage Δ
internal/git/git.go	`54.92% <0.00%> (-1.87%)`	:arrow_down:
cmd/mr_discussion.go	`56.71% <45.94%> (-14.26%)`	:arrow_down:
cmd/ci_trace.go	`69.76% <0.00%> (-1.02%)`	:arrow_down:
cmd/ci_artifacts.go	`91.66% <0.00%> (-0.65%)`	:arrow_down:
cmd/snippet_create.go	`83.51% <0.00%> (-0.36%)`	:arrow_down:
cmd/mr_create.go	`75.00% <0.00%> (-0.23%)`	:arrow_down:
cmd/mr_close.go	`100.00% <0.00%> (ø)`
cmd/mr_reopen.go	`100.00% <0.00%> (ø)`
cmd/issue_close.go	`100.00% <0.00%> (ø)`
cmd/issue_reopen.go	`100.00% <0.00%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2519cdf...5205d67. Read the comment docs.

@krobelus are you planning on submitting a new version?

are you planning on submitting a new version?

yeah, I'll do that this week

commenting on added/deleted/context lines results in a couple of different scenarios to test. So far I tested them manually but I'll try to have a look at automating that.

updated (+ rebased, I hope you don't mind). This should be much more reasonable now. I have submitted the first three commits in separate PRs but also included them here to avoid conflicts.

OOC How does this work for new files

For "+" lines, the parameter is ignored, so the user can put any number there (Edit: added to the docs now). We only need both numbers for context lines, as a workaround for the GitLab API issue.

These should be declared in a single var block.

Done. Sounds like a job for a linter.

zaquestion / lab

mr discussion: allow to comment on specific lines in a diff #752

Codecov Report