Skip to content

Adds hash anchors to diff_files to make possible linking exact file in commit

username-removed-803846 requested to merge YarNayar/gitlab-ce:24010-Add-named-anchors-for-diffs-in-the-commit-page into master

What does this MR do?

1) It changes and unifies the way, how we link exact diff file under merge_requests/{merge_req_id}/diffs page.

There were several forms, how you can link exact diff file under /diffs tab:

  • The first form which was added to diff header link on this page is a #diff-N anchor, where N is just index/ordinal number of diff file in the list. It is not convenient since you can't actually know which number has the file you want to link to outside this page. Example: "..../diffs#diff-42". Maybe, it was done by reason, but from my perspective, I just can't see how it can be handy.

  • Another approach is to link exact line in the file in following format: #hash_lineA_lineB, where the hash is a sha1 encoded file's path, and lineA and lineB are line numbers of exact changes presented in file. Example: "....../diffs#d84b028799a0d15b64d62a1d547297b47bc4b58e_43_43" . It's a good form, but it works only for lines and it's impossible to link the file, just by deleting the line numbers from the end.

  • There is another approach, use id of internal elements on the page(since anchor in HTML link is just an id on the page), which has format #"file-path-"HASH, where hash is again sha1 of file's path. Here's example: ".../diffs#file-path-d84b028799a0d15b64d62a1d547297b47bc4b58e" . Almost okay, but a bit long and very different from the previous form

During my work on the issue raised in #24010 (closed), I realized that this several link formats can be unified into one, simple and intuitive. This MR unifies how we link exact file's diff. Since this request is merged, the way we link exact file's diff is just by SHA1 hash.

So, if we want to link exact file line, we use, as it was mentioned earlier, this form "./diffs#d84b028799a0d15b64d62a1d547297b47bc4b58e_43_43", if we want to link just file, we just get rid of line numbers at the end and link directly by hash, "./diffs#d84b028799a0d15b64d62a1d547297b47bc4b58e"

2) It fixes the bug which was found during work on the main issue.

For each tab on merge_request/{merge_req_id} page, beside "Discussion", there are double xhr request for tab data.

How to reproduce:

  • Open one of the tabs besides discussion, e.g. /diffs

  • Open inspector/dev tool and reload the page

  • Let it load, filter request by XHR and sort them by name

  • You'll find that there are doubled request which ends on .json, the name depends on tab, for /diffs it will be diffs.json.

Both request run simultaneously and returns the same data, there is no reason to do it, we basically asks for same data twice every time we reload the page.

The problem is here https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/merge_request_tabs.js#L148 .

It happens because we trigger 'shown.bs.tab' twice, since function .tab('show') triggers it, and then we do it manually by .trigger()

Are there points in the code the reviewer needs to double check?

n/a

Why was this MR needed?

It resolves issues raised in #24010 (closed)

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

Closes #24010 (closed)

Merge request reports