neiam /gitgud

Git Gud

public · Issues · Pulls · Labels · Forks · Compare · Actions success · Badges · Packages

Pull requests: merging takes write access, sources must be readable, only writers' reviews count

f5a1289 · Gabriel Morell · 2026-10-18 18:50

30 files +587 -63

Message

{commit_body(@commit)}

Files changed

modified lib/git_gud/branch_protections.ex

+19 −9

@@ -15,7 +15,8 @@ defmodule GitGud.BranchProtections do

15	15	import Ecto.Query, warn: false
16	16
17	17	alias GitGud.BranchProtections.BranchProtection
18		− alias GitGud.PullRequests.PrReview
	18	+ alias GitGud.PullRequests
	19	+ alias GitGud.PullRequests.PullRequest
19	20	alias GitGud.Repo
20	21	alias GitGud.Repositories.Repository
21	22

@@ -95,7 +96,7 @@ defmodule GitGud.BranchProtections do

95	96
96	97	defp evaluate(rule, repo, old_sha, new_sha, opts) do
97	98	with :ok <- check_force_push(rule, repo, old_sha, new_sha),
98		− :ok <- check_approvals(rule, opts) do
	99	+ :ok <- check_approvals(rule, repo, opts) do
99	100	check_merge_statuses(rule, repo, opts)
100	101	end
101	102	end

@@ -193,10 +194,16 @@ defmodule GitGud.BranchProtections do

193	194	end
194	195	end
195	196
196		− defp check_approvals(%BranchProtection{required_approvals: n}, _opts) when n in [nil, 0],
197		− do: :ok
	197	+ defp check_approvals(%BranchProtection{required_approvals: n}, _repo, _opts)
	198	+ when n in [nil, 0],
	199	+ do: :ok
198	200
199		− defp check_approvals(rule, opts) do
	201	+ # Counted as the repo's own required-approvals rule counts them
	202	+ # (`PullRequests.effective_approvals/2`): each reviewer's latest
	203	+ # verdict, from someone with write access, the author's only where the
	204	+ # repo lets authors approve. Counting every approved row let one
	205	+ # reviewer — anyone at all, the author included — approve N times.
	206	+ defp check_approvals(rule, repo, opts) do
200	207	pr_id = Keyword.get(opts, :pull_request_id)
201	208
202	209	if is_nil(pr_id) do

@@ -204,10 +211,13 @@ defmodule GitGud.BranchProtections do

204	211	{:error, :approvals_required}
205	212	else
206	213	approvals =
207		− Repo.aggregate(
208		− from(r in PrReview, where: r.pull_request_id == ^pr_id and r.state == "approved"),
209		− :count
210		− )
	214	+ case Repo.get(PullRequest, pr_id) do
	215	+ %PullRequest{} = pr ->
	216	+ PullRequests.effective_approvals(repo, Repo.preload(pr, reviews: :reviewer))
	217	+
	218	+ nil ->
	219	+ 0
	220	+ end
211	221
212	222	if approvals >= rule.required_approvals,
213	223	do: :ok,

modified lib/git_gud/pull_requests.ex

+100 −32

@@ -174,6 +174,11 @@ defmodule GitGud.PullRequests do

174	174	@doc """
175	175	Open a new PR. Resolves source/target to SHAs, computes the merge base,
176	176	and snapshots both head and base into the PR row.
	177	+
	178	+ The author must be able to read both repositories: a cross-repo PR
	179	+ fetches the source branch into the target, so a source the author
	180	+ can't read would be copied into a repository they can. An unreadable
	181	+ or missing source is `{:error, :source_not_found}` either way.
177	182	"""
178	183	def create_pull_request(%Repository{} = repo, %User{} = author, attrs) do
179	184	if GitGud.Moderation.banned?(repo, author) do

@@ -188,10 +193,10 @@ defmodule GitGud.PullRequests do

188	193	target_ref = attrs["target_ref"] \|\| attrs[:target_ref]
189	194	source_repo_id = attrs["source_repository_id"] \|\| attrs[:source_repository_id]
190	195
191		− {source_repo, cross_repo?} = resolve_source_repo(repo, source_repo_id)
192		−
193	196	result =
194		− with {:ok, head_sha} <- Repositories.resolve(source_repo, source_ref),
	197	+ with :ok <- authorize_read(author, repo),
	198	+ {:ok, source_repo, cross_repo?} <- resolve_source_repo(repo, author, source_repo_id),
	199	+ {:ok, head_sha} <- Repositories.resolve(source_repo, source_ref),
195	200	{:ok, target_sha} <- Repositories.resolve(repo, target_ref),
196	201	# For cross-repo PRs we need to fetch the source's commit into
197	202	# the target's object database before merge_base / merge_tree

@@ -244,6 +249,11 @@ defmodule GitGud.PullRequests do

244	249	err -> {:error, err}
245	250	end
246	251
	252	+ # A cross-repo attempt that fetched and then failed (nothing to
	253	+ # merge, a bad title) leaves its temp ref behind; success already
	254	+ # dropped it when pinning `refs/pull/<N>/head`.
	255	+ if match?({:error, _}, result) and is_binary(source_ref), do: drop_pull_tmp(repo, source_ref)
	256	+
247	257	with {:ok, pr} <- result do
248	258	_ = GitGud.Federation.Publishing.publish_pr_opened(repo, pr, author)
249	259	:ok = PullRequestDispatch.enqueue(pr, "opened", uid)

@@ -452,11 +462,34 @@ defmodule GitGud.PullRequests do

452	462	end
453	463	end
454	464
455		− defp resolve_source_repo(target_repo, nil), do: {target_repo, false}
	465	+ defp authorize_read(user, repo) do
	466	+ if GitGud.Organizations.can?(user, repo, :read), do: :ok, else: {:error, :forbidden}
	467	+ end
	468	+
	469	+ defp resolve_source_repo(target_repo, _author, nil), do: {:ok, target_repo, false}
	470	+
	471	+ defp resolve_source_repo(target_repo, author, id) when is_integer(id) or is_binary(id) do
	472	+ with {n, ""} <- Integer.parse(to_string(id)),
	473	+ %Repository{} = src <- Repo.get(Repository, n),
	474	+ true <- src.id == target_repo.id or GitGud.Organizations.can?(author, src, :read) do
	475	+ {:ok, src, src.id != target_repo.id}
	476	+ else
	477	+ _ -> {:error, :source_not_found}
	478	+ end
	479	+ end
	480	+
	481	+ defp resolve_source_repo(_target_repo, _author, _id), do: {:error, :source_not_found}
	482	+
	483	+ # `source_ref` is whatever the caller sent, so only a well-formed name
	484	+ # under refs/pull-tmp/ is deleted — never `refs/pull-tmp/../heads/x`.
	485	+ defp drop_pull_tmp(repo, source_ref) do
	486	+ ref = "refs/pull-tmp/" <> source_ref
456	487
457		− defp resolve_source_repo(target_repo, id) when is_integer(id) or is_binary(id) do
458		− src = Repositories.get_repository!(id)
459		− {src, src.id != target_repo.id}
	488	+ with {_, 0} <- System.cmd("git", ["check-ref-format", ref], stderr_to_stdout: true) do
	489	+ System.cmd("git", ["-C", repo.disk_path, "update-ref", "-d", ref], stderr_to_stdout: true)
	490	+ end
	491	+
	492	+ :ok
460	493	end
461	494
462	495	defp maybe_fetch_cross_repo(false, _, _, _), do: :ok

@@ -690,7 +723,8 @@ defmodule GitGud.PullRequests do

690	723	Perform a 3-way merge of `pr.head_sha` into the target ref. Atomic via
691	724	`git update-ref <ref> <new> <expected_old>`: if someone else moved the
692	725	ref between probe and merge, we surface a `:concurrent_update` error
693		− and leave the PR untouched.
	726	+ and leave the PR untouched. `{:error, :forbidden}` when `actor` can't
	727	+ write to the target repository.
694	728	"""
695	729	def merge!(%PullRequest{} = pr, %User{} = actor, opts \\ []) do
696	730	{message, opts} = pop_message(opts)

@@ -698,7 +732,10 @@ defmodule GitGud.PullRequests do

698	732	repo = Repositories.get_repository!(pr.repository_id)
699	733	path = repo.disk_path
700	734
701		− with :ok <- check_review_block(repo, pr, override?),
	735	+ # Merging writes the target branch, so it takes write access there.
	736	+ # The callers check too; this is the one place none of them can skip.
	737	+ with :ok <- authorize_write(actor, repo),
	738	+ :ok <- check_review_block(repo, pr, override?),
702	739	{:ok, current_target_sha} <- Repositories.resolve(repo, pr.target_ref),
703	740	:ok <-
704	741	BranchProtections.check_ref_update(

@@ -1201,37 +1238,68 @@ defmodule GitGud.PullRequests do

1201	1238	do: where(query, [p], p.repository_id in ^repo_ids)
1202	1239
1203	1240	@doc """
1204		− Reviewers whose standing verdict is "changes requested".
	1241	+ The standing verdicts that count toward `repo`'s review rules, newest
	1242	+ first: each reviewer's latest review (`latest_reviews/1`), left by
	1243	+ someone with write access to `repo`, as on GitHub. Anyone who can read
	1244	+ the pull request may still review it; their verdict is advice.
	1245	+
	1246	+ The author never counts against their own pull request, and counts for
	1247	+ it only where the repo lets authors approve (`block_self_approval`
	1248	+ off). Where the repo blocks self-approval, an approval the author left
	1249	+ before the rule was turned on stops counting too — otherwise enabling
	1250	+ it would leave already-self-approved PRs satisfying a requirement they
	1251	+ were never meant to.
	1252	+
	1253	+ Federated reviews have no local reviewer to hold write access, so
	1254	+ they're shown but don't count.
	1255	+ """
	1256	+ def counting_reviews(%Repository{} = repo, %PullRequest{} = pr) do
	1257	+ pr
	1258	+ \|> latest_reviews()
	1259	+ \|> Enum.filter(&counts?(&1, repo, pr))
	1260	+ end
	1261	+
	1262	+ defp counts?(%PrReview{reviewer_id: rid} = review, repo, pr) when is_integer(rid) do
	1263	+ author_ok? =
	1264	+ rid != pr.author_id or (review.state == "approved" and not repo.block_self_approval)
	1265	+
	1266	+ author_ok? and GitGud.Organizations.can?(review_user(review), repo, :write) == true
	1267	+ end
	1268	+
	1269	+ defp counts?(_review, _repo, _pr), do: false
	1270	+
	1271	+ defp review_user(%PrReview{reviewer: %User{} = user}), do: user
	1272	+ defp review_user(%PrReview{reviewer_id: rid}), do: Repo.get(User, rid)
	1273	+
	1274	+ @doc """
	1275	+ Reviewers whose standing verdict is "changes requested", among those
	1276	+ whose verdict counts (`counting_reviews/2`).
1205	1277
1206	1278	Empty when nobody is blocking. Callers use this both to enforce and
1207	1279	to explain — the PR page names who is waiting on a change.
1208	1280	"""
1209		− def blocking_reviews(%PullRequest{} = pr) do
1210		− pr
1211		− \|> latest_reviews()
	1281	+ def blocking_reviews(%Repository{} = repo, %PullRequest{} = pr) do
	1282	+ repo
	1283	+ \|> counting_reviews(pr)
1212	1284	\|> Enum.filter(&(&1.state == "changes_requested"))
1213	1285	end
1214	1286
1215		− @doc "How many reviewers currently approve — latest verdict each."
	1287	+ @doc """
	1288	+ How many reviewers currently approve — latest verdict each, whoever
	1289	+ left it. What counts toward the rules is `effective_approvals/2`.
	1290	+ """
1216	1291	def approval_count(%PullRequest{} = pr) do
1217	1292	pr \|> latest_reviews() \|> Enum.count(&(&1.state == "approved"))
1218	1293	end
1219	1294
1220	1295	@doc """
1221		− Approvals that count toward the repo's requirement.
1222		−
1223		− Where the repo blocks self-approval, an approval the author left
1224		− before the rule was turned on stops counting too — otherwise
1225		− enabling it would leave already-self-approved PRs satisfying a
1226		− requirement they were never meant to.
	1296	+ Approvals that count toward the repo's requirement, and a protected
	1297	+ branch's: `counting_reviews/2` that approve.
1227	1298	"""
1228		− def effective_approvals(%Repository{block_self_approval: false}, %PullRequest{} = pr),
1229		− do: approval_count(pr)
1230		−
1231		− def effective_approvals(%Repository{}, %PullRequest{} = pr) do
1232		− pr
1233		− \|> latest_reviews()
1234		− \|> Enum.count(&(&1.state == "approved" and &1.reviewer_id != pr.author_id))
	1299	+ def effective_approvals(%Repository{} = repo, %PullRequest{} = pr) do
	1300	+ repo
	1301	+ \|> counting_reviews(pr)
	1302	+ \|> Enum.count(&(&1.state == "approved"))
1235	1303	end
1236	1304
1237	1305	@doc """

@@ -1244,12 +1312,12 @@ defmodule GitGud.PullRequests do

1244	1312	opt-in per repo and default to off.
1245	1313	"""
1246	1314	def merge_blockers(%Repository{} = repo, %PullRequest{} = pr) do
	1315	+ blocking = if repo.block_merge_on_changes_requested, do: blocking_reviews(repo, pr), else: []
	1316	+
1247	1317	changes =
1248		− if repo.block_merge_on_changes_requested and blocking_reviews(pr) != [] do
1249		− [{:changes_requested, Enum.map(blocking_reviews(pr), &reviewer_label/1)}]
1250		− else
1251		− []
1252		− end
	1318	+ if blocking != [],
	1319	+ do: [{:changes_requested, Enum.map(blocking, &reviewer_label/1)}],
	1320	+ else: []
1253	1321
1254	1322	approvals =
1255	1323	case {repo.required_approvals \|\| 0, effective_approvals(repo, pr)} do

modified lib/git_gud_web/controllers/api/mobile/pull_request_controller.ex

+3 −0

@@ -313,6 +313,9 @@ defmodule GitGudWeb.Api.Mobile.PullRequestController do

313	313	{:ok, merged} ->
314	314	json(conn, %{"pull" => JSON.pull(GitGud.Repo.preload(merged, :merged_by), repo)})
315	315
	316	+ {:error, :forbidden} ->
	317	+ send_error(conn, 403, "you can't merge into this repository")
	318	+
316	319	{:error, :review_blocked} ->
317	320	send_error(conn, 422, "blocked by this repository's review rules")
318	321

modified lib/git_gud_web/live/pr_live/new.ex

+8 −0

@@ -190,8 +190,16 @@ defmodule GitGudWeb.PrLive.New do

190	190	defp normalize_url(nil), do: ""
191	191	defp normalize_url(u), do: u \|> String.trim() \|> String.trim_trailing("/") \|> String.downcase()
192	192
	193	+ # Both sides come from the server's own pool of repositories the
	194	+ # viewer can read, never straight from the client: a forged
	195	+ # `source_repository_id` would otherwise have the source branch of any
	196	+ # repository fetched into the target.
193	197	defp open_local_pr(socket, user, attrs) do
194	198	target = socket.assigns.target_repo
	199	+ source = pick(socket.assigns.candidates, attrs["source_repository_id"], nil)
	200	+
	201	+ attrs =
	202	+ Map.put(attrs, "source_repository_id", (source \|\| socket.assigns.source_repo).id)
195	203
196	204	case PullRequests.create_pull_request(target, user, attrs) do
197	205	{:ok, pr} ->

modified lib/git_gud_web/live/pr_live/reviews.ex

+33 −1

@@ -32,14 +32,34 @@ defmodule GitGudWeb.PrLive.Reviews do

32	32	\|> assign(:pr, pr)
33	33	\|> assign_review_form()
34	34	\|> assign_suggestions()
	35	+ \|> assign_counted()
35	36	\|> assign(:page_title, "Reviews · ##{pr.number} #{pr.title}")}
36	37	end
37	38
38	39	defp reload(socket) do
39	40	pr = PullRequests.get_pull_request!(socket.assigns.repo, socket.assigns.pr.number)
40		− socket \|> assign(:pr, pr) \|> assign_review_form() \|> assign_suggestions()
	41	+ socket \|> assign(:pr, pr) \|> assign_review_form() \|> assign_suggestions() \|> assign_counted()
41	42	end
42	43
	44	+ # Which standing verdicts count toward the repo's rules: a writer's,
	45	+ # not the author's own (`PullRequests.counting_reviews/2`). The rest
	46	+ # are shown, marked as advice.
	47	+ defp assign_counted(socket) do
	48	+ %{repo: repo, pr: pr} = socket.assigns
	49	+
	50	+ socket
	51	+ \|> assign(:counted_ids, MapSet.new(PullRequests.counting_reviews(repo, pr), & &1.id))
	52	+ \|> assign(
	53	+ :viewer_counts?,
	54	+ GitGud.Organizations.can?(viewer(socket), repo, :write) == true
	55	+ )
	56	+ end
	57	+
	58	+ # An approval or a request for changes that the rules ignore. A plain
	59	+ # comment never counts for anything, so it isn't marked.
	60	+ defp advisory?(counted_ids, review),
	61	+ do: review.state != "commented" and not MapSet.member?(counted_ids, review.id)
	62	+
43	63	# Offering the author or someone already asked would be noise, so
44	64	# both are excluded from the suggestions.
45	65	defp assign_suggestions(socket) do

@@ -342,6 +362,14 @@ defmodule GitGudWeb.PrLive.Reviews do

342	362	{reviewer_name(r.reviewer)}
343	363	</.link>
344	364	<span :if={is_nil(profile_handle(r.reviewer))}>{reviewer_name(r.reviewer)}</span>
	365	+ <span
	366	+ :if={advisory?(@counted_ids, r)}
	367	+ id={"review-advisory-#{r.id}"}
	368	+ class="opacity-70"
	369	+ title="Only reviews from people with write access count toward merging."
	370	+ >
	371	+ · doesn't count toward merging
	372	+ </span>
345	373	</p>
346	374	<p :if={r.body}>{r.body}</p>
347	375	</li>

@@ -382,6 +410,10 @@ defmodule GitGudWeb.PrLive.Reviews do

382	410	<p :if={self_approval_blocked?(assigns)} class="text-xs opacity-70">
383	411	This repo doesn't let authors approve their own pull requests.
384	412	</p>
	413	+ <p :if={not @viewer_counts?} id="review-advisory-note" class="text-xs opacity-70">
	414	+ Your review is welcome, but only reviews from people with write access to this
	415	+ repository count toward merging it.
	416	+ </p>
385	417	<.input
386	418	field={@review_form[:state]}
387	419	type="select"

modified lib/git_gud_web/live/pr_live/show.ex

+17 −5

Click to load diff…

modified test/git_gud/branch_protections/status_checks_test.exs

+4 −1

Click to load diff…

added test/git_gud_web/live/pr_live/merge_permission_test.exs

+74 −0

Click to load diff…

added test/git_gud_web/live/pr_live/new_source_access_test.exs

+126 −0

Click to load diff…

modified test/git_gud_web/live/pr_live/required_approvals_test.exs

+6 −6

Click to load diff…

modified test/git_gud_web/live/pr_live/review_block_test.exs

+9 −8

Click to load diff…

added test/git_gud_web/live/pr_live/review_counting_test.exs

+169 −0

Click to load diff…

modified test/git_gud_web/live/pr_live/self_approval_test.exs

+1 −1

Click to load diff…

modified test/support/fixtures/forge_fixtures.ex

+18 −0

Click to load diff…

Parents: 92ad654