Skip to content
21 changes: 21 additions & 0 deletions assets/css/app.css
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,29 @@ input.search-input:focus {
display: none;
}

.ghd-chunk-header {
height: 44px;
}
.ghd-chunk-header:first-child, .ghd-chunk-header:last-child {
height: 0;
}

.ghd-chunk-header .ghd-line-number {
background-color: #f8fafd;
flex-direction: column;
}

.expanded, .expanded .ghd-line-number{
background-color: #f5f5f5;
}

.ghd-chunk-header .ghd-line-number div{
background: #dbedff;
width: 100%;
text-align: center;
}
.ghd-chunk-header .ghd-line-number div:hover{
background: #95c6fe;
}

.ghd-file-status {
Expand Down
73 changes: 73 additions & 0 deletions assets/js/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,76 @@ fileHeaders.forEach(header => {
const scrollIfNeeded = elem => {
elem.getBoundingClientRect().top < 0 && elem.scrollIntoView(true)
}

const MAX_EXPAND_CONTEXT_LINES = 20

document.addEventListener('DOMContentLoaded', e => {
document.querySelectorAll('.ghd-expand-up').forEach(expandUp => {
expandUp.addEventListener('click', e => {
const {fileName, packageName, version, lineAfter, lineBefore, patch} = gatherInfo(expandUp)

// expandUp always follows by diff line.. so we take the number
const toLine = numberFrom(lineAfter) - 1

const _fromLine = lineBefore ? numberFrom(lineBefore) + 1 : 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid the underscored variable names. Please pick a different name or don't use const.

const fromLine = Math.max(toLine - MAX_EXPAND_CONTEXT_LINES, _fromLine)

const _rightLine = lineBefore ? numberTo(lineBefore) + 1 : 1
const rightLine = Math.max(toLine - MAX_EXPAND_CONTEXT_LINES, _rightLine)

fetchChunkAndInsert({target: lineAfter, packageName, version, fromLine, toLine, rightLine, fileName, patch})
})
})

document.querySelectorAll('.ghd-expand-down').forEach(expandDown => {
expandDown.addEventListener('click', e => {
const {fileName, packageName, version, lineAfter, lineBefore, patch} = gatherInfo(expandDown)

const fromLine = numberFrom(lineBefore) + 1
const rightLine = numberTo(lineBefore) + 1

const _toLine = lineAfter ? numberFrom(lineAfter) - 1 : Infinity
const toLine = Math.min(fromLine + MAX_EXPAND_CONTEXT_LINES, _toLine)

fetchChunkAndInsert({target: expandDown.closest('tr'), packageName, version, fromLine, toLine, rightLine, fileName, patch})

})
})
})

const numberFrom = line => parseInt(line.querySelector('.ghd-line-number .ghd-line-number-from').textContent.trim())
const numberTo = line => parseInt(line.querySelector('.ghd-line-number .ghd-line-number-to').textContent.trim())

const gatherInfo = line => {
const patch = line.closest('.ghd-file')
const {fileName, packageName, version} = patch.querySelector('.ghd-file-header').dataset

const lineAfter = line.closest('tr').nextElementSibling
const lineBefore = line.closest('tr').previousElementSibling

return {fileName, packageName, version, lineAfter, lineBefore, patch}
}

const fetchChunkAndInsert = params => {
if(!params.fromLine || !params.toLine || (params.fromLine >= params.toLine)){
return
}

const path = `/diff/${params.packageName}/${params.version}/expand/${params.fromLine}/${params.toLine}/${params.rightLine}`
const url = new URL(path, window.location)
url.searchParams.append('file_name', params.fileName)

fetch(url)
.then(response => response.json())
.then(({chunk, lines, errors}) => {
if(errors) return
const context = document.createElement('template')
context.innerHTML = chunk.trim()
const patchBody = params.patch.querySelector('tbody')

Array.prototype.reduceRight.call(context.content.childNodes, (target, line) => {
return patchBody.insertBefore(line, target)
}, params.target)
})
.catch(console.error)
}
112 changes: 112 additions & 0 deletions lib/diff/hex/chunk_extractor.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
defmodule Diff.Hex.ChunkExtractor do
@enforce_keys [:file_path, :from_line, :to_line, :right_line]
defstruct Enum.map(@enforce_keys, &{&1, nil}) ++ [errors: [], raw: nil, parsed: nil]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use a map internally and only return the parsed part, we don't need the struct.


def run(params) do
params
|> cast()
|> parse_integers([:right_line, :from_line, :to_line])
|> normalize([:from_line, :right_line])
|> validate_to_line()
|> system_read_raw_chunk()
|> parse_chunk()
|> case do
%{errors: []} = chunk -> {:ok, chunk}
%{errors: _} = chunk -> {:error, chunk}
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having every function match on %{errors: ...} use with instead.


defp cast(params) do
struct_keys =
struct(__MODULE__)
|> Map.from_struct()
|> Enum.map(fn {key, _val} -> key end)

struct!(__MODULE__, Map.take(params, struct_keys))
end

defp parse_integers(chunk, fields) do
Enum.reduce(fields, chunk, &parse_integer/2)
end

defp parse_integer(field, chunk) do
value = chunk |> Map.get(field) |> parse_value()

case value do
:error -> %{chunk | errors: [{:parse_integer, "#{field} must be a number"} | chunk.errors]}
integer -> Map.put(chunk, field, integer)
end
end

defp parse_value(number) when is_integer(number), do: number

defp parse_value(number) when is_binary(number) do
with {int, _} <- Integer.parse(number), do: int
end

defp normalize(%{errors: [_ | _]} = chunk, _), do: chunk

defp normalize(chunk, fields) do
Enum.reduce(fields, chunk, &normalize_field/2)
end

defp normalize_field(field, chunk) do
case Map.fetch!(chunk, field) do
value when value > 0 -> chunk
_ -> Map.put(chunk, field, 1)
end
end

defp validate_to_line(%{errors: [_ | _]} = chunk), do: chunk

defp validate_to_line(%{from_line: from_line, to_line: to_line} = chunk)
when from_line < to_line do
chunk
end

defp validate_to_line(chunk) do
error = {:param, "from_line parameter must be less than to_line"}
%{chunk | errors: [error | chunk.errors]}
end

defp system_read_raw_chunk(%{errors: [_ | _]} = chunk), do: chunk

defp system_read_raw_chunk(chunk) do
path = chunk.file_path
from_line = to_string(chunk.from_line)
to_line = to_string(chunk.to_line)

case System.cmd("sed", ["-n", "#{from_line},#{to_line}p", path], stderr_to_stdout: true) do
{raw_chunk, 0} ->
%{chunk | raw: raw_chunk}

{error, code} ->
error = {:system, "System command exited with a non-zero status #{code}: #{error}"}
%{chunk | errors: [error | chunk.errors]}
end
end

defp parse_chunk(%{errors: [_ | _]} = chunk), do: chunk

defp parse_chunk(chunk) do
parsed =
chunk.raw
|> String.split("\n")
|> remove_trailing_newline()
|> Enum.with_index(chunk.from_line)
|> Enum.with_index(chunk.right_line)
# prepend line with whitespace to compensate the space introduced by leading + and - in diffs
|> Enum.map(fn {{line, from}, to} ->
%{text: " " <> line, from_line_number: from, to_line_number: to}
end)

%{chunk | parsed: parsed}
end

defp remove_trailing_newline(lines) do
lines
|> Enum.reverse()
|> tl()
|> Enum.reverse()
end
end
35 changes: 34 additions & 1 deletion lib/diff/hex/hex.ex
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,18 @@ defmodule Diff.Hex do
end

def unpack_tarball(tarball, path) when is_binary(path) do
do_unpack_tarball(tarball, :all_files, path)
end

def unpack_tarball(tarball, file_list, path) when is_binary(path) do
file_list = Enum.map(file_list, &to_charlist/1)
do_unpack_tarball(tarball, file_list, path)
end

def do_unpack_tarball(tarball, file_list, path) do
path = to_charlist(path)

with {:ok, _} <- :hex_tarball.unpack(tarball, path) do
with {:ok, _} <- :hex_tarball.unpack(tarball, file_list, path) do
:ok
end
end
Expand Down Expand Up @@ -100,6 +109,30 @@ defmodule Diff.Hex do
end
end

def get_chunk(params) do
path = tmp_path("chunk")

chunk_extractor_params = Map.put(params, :file_path, Path.join(path, params.file_name))

try do
with {:ok, tarball} <- get_tarball(params.package, params.version),
:ok <- unpack_tarball(tarball, [params.file_name], path),
{:ok, %{parsed: parsed_chunk}} <- Diff.Hex.ChunkExtractor.run(chunk_extractor_params) do
{:ok, parsed_chunk}
else
{:error, %Diff.Hex.ChunkExtractor{errors: errors} = chunk} ->
Logger.error(inspect(errors))
{:error, chunk}

error ->
Logger.error(inspect(error))
{:error, error}
end
after
File.rm_rf(path)
end
end

defp git_diff(path_from, path_to, path_out) do
case System.cmd("git", [
"-c",
Expand Down
53 changes: 52 additions & 1 deletion lib/diff_web/controllers/page_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,27 @@ defmodule DiffWeb.PageController do
end
end

def expand_context(conn, %{"file_name" => _file_name} = params) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is file_name the only required parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually since file_name is required parameter - makes sense to add it to "path" rather that "query_params" that way we won't need to handle that missing file_name in the controller. Additionally, I see some other parameters might be not strictly required and pronbably makes sense to have them in query params..

I'll revisit this part

case parse_version(params["version"]) do
{:ok, version} ->
version = to_string(version)
params = Map.update!(params, "version", fn _ -> version end)

do_expand_context(conn, params)

:error ->
conn
|> put_status(400)
|> json(%{error: "Bad Request"})
end
end

def expand_context(conn, _params) do
conn
|> put_status(400)
|> json(%{error: "missing query parameter: file_name"})
end

defp maybe_cached_diff(conn, _package, version, version) do
render_error(conn, 400)
end
Expand Down Expand Up @@ -70,6 +91,32 @@ defmodule DiffWeb.PageController do
end
end

defp do_expand_context(conn, params) do
chunk_extractor_params =
for {key, val} <- params, into: %{} do
{String.to_existing_atom(key), val}
end

case Diff.Hex.get_chunk(chunk_extractor_params) do
{:ok, chunk} ->
rendered_chunk =
Phoenix.View.render_to_string(
DiffWeb.RenderView,
"render_context_chunk.html",
chunk: chunk
)

conn
|> put_status(200)
|> json(%{chunk: rendered_chunk, lines: length(chunk)})

{:error, %{errors: errors}} ->
conn
|> put_status(400)
|> json(%{errors: Enum.into(errors, %{})})
end
end

defp do_diff(conn, package, from, to) do
case Diff.Hex.diff(package, from, to) do
{:ok, stream} ->
Expand Down Expand Up @@ -134,7 +181,11 @@ defmodule DiffWeb.PageController do
Enum.each(stream, fn
{:ok, patch} ->
html_patch =
Phoenix.View.render_to_iodata(DiffWeb.RenderView, "render.html", patch: patch)
Phoenix.View.render_to_iodata(DiffWeb.RenderView, "render.html",
patch: patch,
package: package,
from_version: from
)

IO.binwrite(file, html_patch)

Expand Down
5 changes: 5 additions & 0 deletions lib/diff_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ defmodule DiffWeb.Router do
pipe_through :browser

live "/", SearchLiveView

get "/diff/:package/:version/expand/:from_line/:to_line/:right_line/",
PageController,
:expand_context

get "/diff/:package/:versions", PageController, :diff
get "/diffs", PageController, :diffs
end
Expand Down
18 changes: 14 additions & 4 deletions lib/diff_web/templates/render/render.html.eex
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% status = patch_status(@patch) %>
<div class="ghd-file">
<div class="ghd-file-header">
<div class="ghd-file-header" data-file-name="<%= @patch.from %>" data-package-name="<%= @package %>" data-version="<%= @from_version %>" >
<span class="ghd-file-status ghd-file-status-<%= status %>">
<%= status %>
</span>
Expand All @@ -10,11 +10,13 @@
</div>
<div class="ghd-diff">
<table class="ghd-diff">
<%= for chunk <- @patch.chunks do %>
<%= for {chunk, index} <- Enum.with_index(@patch.chunks) do %>
<tr class="ghd-chunk-header">
<td class="ghd-line-number">
<div class="ghd-line-number-from">&nbsp;</div>
<div class="ghd-line-number-to"></div>
<%= unless index == 0 do %>
<div class="ghd-expand-down"><svg class="octicon octicon-fold-down" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M4 11l3 3 3-3H8V5H6v6H4zm-4 0c0 .55.45 1 1 1h2.5l-1-1h-1l2-2H5V8H3.5l-2-2H5V5H1c-.55 0-1 .45-1 1l2.5 2.5L0 11zm10.5-2H9V8h1.5l2-2H9V5h4c.55 0 1 .45 1 1l-2.5 2.5L14 11c0 .55-.45 1-1 1h-2.5l1-1h1l-2-2z"></path></svg></div>
<% end %>
<div class="ghd-expand-up"><svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 14 16" width="14" height="16"><path fill-rule="evenodd" d="M10 6L7 3 4 6h2v6h2V6h2zm4 0c0-.55-.45-1-1-1h-2.5l1 1h1l-2 2H9v1h1.5l2 2H9v1h4c.55 0 1-.45 1-1l-2.5-2.5L14 6zM3.5 8H5v1H3.5l-2 2H5v1H1c-.55 0-1-.45-1-1l2.5-2.5L0 6c0-.55.45-1 1-1h2.5l-1 1h-1l2 2z"></path></svg></div>
</td>
<td class="ghd-text">
<div class="ghd-text-internal"><%= chunk.header %></div>
Expand All @@ -36,6 +38,14 @@
</tr>
<% end %>
<% end %>
<tr class="ghd-chunk-header">
<td class="ghd-line-number">
<div class="ghd-expand-down"><svg class="octicon octicon-fold-down" viewBox="0 0 14 16" version="1.1" width="14" height="16" aria-hidden="true"><path fill-rule="evenodd" d="M4 11l3 3 3-3H8V5H6v6H4zm-4 0c0 .55.45 1 1 1h2.5l-1-1h-1l2-2H5V8H3.5l-2-2H5V5H1c-.55 0-1 .45-1 1l2.5 2.5L0 11zm10.5-2H9V8h1.5l2-2H9V5h4c.55 0 1 .45 1 1l-2.5 2.5L14 11c0 .55-.45 1-1 1h-2.5l1-1h1l-2-2z"></path></svg></div>
</td>
<td class="ghd-text">
<div class="ghd-text-internal"></div>
</td>
</tr>
</table>
</div>
</div>
Loading