Extend :Gbrowse to support line anchors for commits #1171
Conversation
|
A few things I don't like:
Fixing this would require expanding the handlers interface, not sure what is right here |
|
Relevant |
|
If it's useful, here's a gitlab commit url |
|
It seems like Gitlab format is: where L and R are left and right line numbers. For ambiguous line numbers like L for added lines the (last + 1) number is used. It looks like it doesn't take any other format and we have to always give these two numbers exactly. Which in turn means abusing |
|
Probably for the best, I think jamming things into existing fields was a bit too clever. I'm not sure I understand how opposing line number is computed when you click a |
|
I've made a sample repo and summarized format for GitHub, Bitbucket and GitLab.
GitHubPath function: MD5
BitbucketPath function: literal string
GitLabPath function: SHA1
|
|
So all three use the same logic for picking the side for the path: pick As for line numbers, to support GitLab we need to pass both – line number in the With passing two line numbers a few things need to be resolved. First is for some providers (GitHub and Bitbucket) we still need to pass which side line is selected. Can add extra field like Another problem is for added or removed lines the number on the opposite side is kinda ambiguous – can pick valid number before or after the region. Whatever strategy you pick if the change region is at the beginning or at the end of the file you end up with line numbers out of bounds (0 or NR+1). GitLab logic for ambiguous line numbers is very bad. In general it picks numbers after the region, even if the region is in the end and the number will be out of bounds. But for some reason it special cases completely added and removed files and for corresponding lines uses 0 instead of 1 which would be a correct value with the logic above. removed line, +++ line number is after the region We can pass line numbers using the after logic – having the So to summarize, we need
|
|
One last case to look at: If you rename a file, and then add a new file with the old name but completely different content, how does that look? I'm guessing the reason the Thanks for all the research! I don't think I want to commit to a new API until after 3.0 (hopefully this month), but don't let that stop you from going ahead and taking a stab at adding these new fields. |
This is displayed as plain modification and addition by all providers – GitHub, Bitbucket, GitLab – and by Git itself: No heuristics work for that case by default. This is documented in great detail in None of the providers enable that. In general it looks like they try to be as close to the original diff format as possible – at least I haven't noticed any discrepancies – which makes sense. Anyway, as I described above GitLab has special case for ambiguous line numbers in removed and added paths (uses 0 instead of 1 for them) for some reason. Passing both |
|
So two different hypotheticals: a provider that supports I think using |
|
I was afraid of other hypotheticals which can combine paths in other unpredicted ways. Went through the first list of Git web hostings I could find. The first which worked for me is Beanstalk and it actually doesn't have rename detection in commits at all: So supporting this one would definitely require both paths. Missing path could be represented by either missing key or empty value for the key. As for line numbers, do you want to re-use existing 'line1' and 'line2' keys despite they have different meaning or add new keys? |
|
This screenshot doesn't tell me much, but I take it renames show up as a thousand deleted lines in the old filename and a thousand added lines in the new filename. And if you try to I'm not sure what the API should look like (hence my reluctance to rush it out the door) other than we should not reuse the |
odnoletkov
changed the title
|
Now this should support line ranges: https://github.blog/changelog/2019-07-23-multi-line-diff-selection |
|
Is this supposed to work by shift clicking a second line while viewing a commit? Because that's not working for me. |
|
Yes, shift-clicking. Works in: Doesn't work in: Apparently not everything is 'diff view'. I guess until it is not supported on the commit page it's not very relevant for us. Should we still future-proof the adapters interface and include lines range there? Also this makes me wonder if 'commit in pull request' page may be more useful than 'standalone commit' page for |
If we've decided to send 2 filenames, may as well send 4 line numbers.
I think it should be consistent, not a "use this one page if it exists and this other page if not" deal. |
It looks like this has been fixed since, all these Github pages now support line ranges in url anchors |
|
TODO:
|
odnoletkov
force-pushed
the
master
branch
2 times, most recently
from
7210c11
to
7c481ab
Compare
|
@tpope any other fixes here or is it good to go? |
|
Been struggling to put it into words but my gut reaction is that this change feels very alien, like it was blindly bolted on with zero knowledge of the existing API or of Fugitive in general. All 5 child keys are identifiers that appear nowhere else in the source code. It triples the size of the API, which I would frown upon even if it was necessary, but it doesn't even feel necessary. I think a better design would be to use the "new" file to fill out the This proposal has 2 obvious consequences, both of which I am fine with:
|
|
As described above, having just paths and line numbers for both sides is not enough to unambiguously define position in a diff for all providers. We also need the side of the diff for the line – currently stored in the 'cursor' field with I also feel like a separate and independent 'position in a diff' entity might have other uses – as a uniform way to address a line in a diff. But you know better of course. |
Yes, I was assuming
If it wasn't a potentially reusable abstraction I think I'd object less to it. The design smells weird with its "new" and "old" pseudo-nesting, but I can't really pinpoint how to fix it without another use case or two. So, I don't want the first use case to be an official API. |
|
What about the internal use case in diff --git a/autoload/fugitive.vim b/autoload/fugitive.vim
index 551baa7..21a2fd2 100644
--- a/autoload/fugitive.vim
+++ b/autoload/fugitive.vim
@@ -5705,19 +5705,15 @@ function! s:cfile() abort
elseif getline('.') =~# '^[+-]\{3\} [abciow12]\=/'
let ref = getline('.')[4:]
- elseif getline('.') =~# '^[+-]' && search('^@@ -\d\+\%(,\d\+\)\= +\d\+','bnW')
- let type = getline('.')[0]
- let lnum = line('.') - 1
- let offset = 0
- while getline(lnum) !~# '^@@ -\d\+\%(,\d\+\)\= +\d\+'
- if getline(lnum) =~# '^[ '.type.']'
- let offset += 1
- endif
- let lnum -= 1
- endwhile
- let offset += matchstr(getline(lnum), type.'\zs\d\+')
- let ref = getline(search('^'.type.'\{3\} [abciow12]/','bnW'))[4:-1]
- let dcmds = [offset, 'normal!zv']
+ elseif getline('.') =~# '^[+-]'
+ let pos = s:DiffPosition(line('.'))
+ if type(pos) == type({})
+ let side = {'+':'new','-':'old'}[pos.cursor]
+ let ref = {'+':'b/','-':'a/'}[pos.cursor] . pos[side . 'path']
+ let dcmds = [pos[side . 'line'], 'normal!zv']
+ else
+ let ref = ''
+ endif
elseif getline('.') =~# '^rename from '
let ref = 'a/'.getline('.')[12:]This makes the new/old pseudo-nesting problem you mentioned stand out. What about more nested structure (for single position): {
"sigil": "+",
"+": {
"path": "path/to/new/file",
"line": 1
},
"-": {
"path": "path/to/old/file",
"line": 2
}
}This one would simplify use in both |
|
Hola, any news or update about that feature? |
|
It's very much still on the agenda. It's prioritized below other Fugitive changes I've been banging my head against. This particular PR is too heavy and consequential to consider jumping the queue. |
|
Is this sufficient? Instead of |
|
Yes this is sufficient, updated both PRs. One remaining issue I see is when the range spans multiple files in the diff – we don't detect file name for the line2 properly. Need more sophisticated approach – probably extend iteration logic which calculates offsets to also look further for the file name? |
|
To be clear, we don't actually want to do anything with that second filename, correct? We just want to avoid pretending that second file's line number belongs to the first. |
|
Now that I think about it, a better key name than Either that, or add a key like |
Yes, I don't think we can do anything reasonable with multiple files. We just need to bail out clearly in that case, fallback to main commit view same as
Done |
|
I plan to finally tackle converting commit buffers to the |
In commit buffers :GBrowse with range will include 'diff' dictionary with 'path', 'line1' and 'line2' values for the range on the '---' side of the diff. Root 'path', 'line1' and 'line2' values specify range on the '+++' side. Line number is 0 when position on the corresponding side of the diff is ambiguous because it is inside deleted hunk.
Look for and use path of the +++ side when it exists. For removals it is
/dev/null, so use path of the --- side. For the line range only line1
is set, corresponding to the first line of selected region. If this line
is in the removed part of the hunk, the number is relative to the ---
side and is negative to indicate that to adapters.