On Fri, Jun 13, 2025 at 02:00:12PM +0200, Martijn van Duren wrote:
> Hello all,
> 
> For libopensmtpd[0] I tried to merge the mdoc_lb branch into main, but
> when trying to send it to upstream I'm greeted with a "reference is
> protected". When doing some printf debugging I found myself in
> protect_refs_from_moving(), which is only called from a single place,
> when a modification is requested without a packfile.
> So it appears I've hit another issue with protected branches and
> modifications. :-)

Thanks for reporting this! The following patch fixes this issue:


fix gotd branch protection rejecting commits that already exist on server

When a reference update sends an empty pack file because the objects
already exist on a different branch, gotd was wrongly rejecting the
reference update. The logic which protects references from moving
kicked in, but didn't account for this particular case. Run a YCA
check to ensure the client is only appending new commits to the
branch and allow the ref update to proceed if so. Add test coverage.

Problem reported by martijn@

M  gotd/repo_write.c                     |  16+  6-
M  regress/gotd/repo_write_protected.sh  |  45+  0-

2 files changed, 61 insertions(+), 6 deletions(-)

commit - 1247ca2fed601968ea335d0623ffac2973b1b5bc
commit + fe389d9e9b32033b8ea536ca04677a62243d42a7
blob - 76f58c2c3fdb86ee2eaef3de5de9245899f628b0
blob + 29c0f8bde535b60c18b781ecf842b2684f50cc7c
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -471,7 +471,8 @@ protect_require_yca(struct got_object_id *tip_id,
 			break;
 		}
 
-		if (got_object_idset_num_elements(traversed_set) >=
+		if (max_commits_to_traverse > 0 &&
+		    got_object_idset_num_elements(traversed_set) >=
 		    max_commits_to_traverse)
 			break;
 
@@ -1542,17 +1543,26 @@ protect_refs_from_moving(void)
 
 		RB_FOREACH(pe, got_pathlist_head,
 		    &repo_write.protected_branch_namespaces) {
-			err = protect_ref_namespace(refname, pe->path);
+			if (strcmp(pe->path, refname) != 0)
+				continue;
+
+			err = protect_require_yca(&ref_update->new_id, 0,
+			    NULL, NULL, ref_update->ref);
 			if (err)
 				return err;
+			break;
 		}
 
 		RB_FOREACH(pe, got_pathlist_head,
 		    &repo_write.protected_branches) {
-			if (strcmp(refname, pe->path) == 0) {
-				return got_error_fmt(GOT_ERR_REF_PROTECTED,
-				    "%s", refname);
-			}
+			if (strcmp(pe->path, refname) != 0)
+				continue;
+
+			err = protect_require_yca(&ref_update->new_id, 0,
+			    NULL, NULL, ref_update->ref);
+			if (err)
+				return err;
+			break;
 		}
 	}
 
blob - d1e70129d2724139bccbc574d043f24c9ec5c7cd
blob + d0f1d1411c0193288b09c815534328a836e872ab
--- regress/gotd/repo_write_protected.sh
+++ regress/gotd/repo_write_protected.sh
@@ -306,6 +306,51 @@ test_modify_protected_branch() {
 		diff -u $testroot/stdout.expected $testroot/stdout
 	fi
 
+	# Put the tip commit back.
+	got ref -r $testroot/repo-clone -c $commit_id refs/heads/main
+
+	# Try adding a new commit to 'main' while sending an empty pack file.
+	# First, create a new branch 'bar' and send it.
+	got branch -r $testroot/repo-clone -c $commit_id bar > /dev/null
+	(cd $testroot/wt && got up -q -b bar > /dev/null)
+	echo "change beta" > $testroot/wt/beta
+	(cd $testroot/wt && got commit -m 'change beta' > /dev/null)
+	got send -q -r $testroot/repo-clone  -b bar
+	# Now merge branch 'bar' into 'main' and send the 'main' branch.
+	# Because the server already has the revelant commit, we will send
+	# an empty pack file during our second send operation.
+	# This would cause a bogus 'reference is protected' error.
+	(cd $testroot/wt && got up -q -b main > /dev/null)
+	(cd $testroot/wt && got merge bar > /dev/null)
+	local commit_id=`git_show_head $testroot/repo-clone`
+	got send -q -r $testroot/repo-clone -b main
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got send failed unexpectedly" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	# Verify that the send operation worked fine.
+	got clone -l ${GOTD_TEST_REPO_URL} | grep '^refs/heads/' \
+		> $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone -l failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	local commit_id_foo=`git_show_branch_head $testroot/repo-clone foo`
+	echo "refs/heads/bar: $commit_id" > $testroot/stdout.expected
+	echo "refs/heads/foo: $commit_id_foo" >> $testroot/stdout.expected
+	echo "refs/heads/main: $commit_id" >> $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+
 	test_done "$testroot" $ret
 }