From: Martijn van Duren <openbsd+got@list.imperialat.at>
Subject: Re: got branch -d doesn't remove empty parent directory in reference
To: Stefan Sperling <stsp@stsp.name>
Cc: Gameoftrees <gameoftrees@openbsd.org>
Date: Thu, 17 Sep 2026 10:57:21 +0200

Works for me.

I am wondering why you add the ENOENT check? If the parent directory
doesn't exist, wouldn't that be a race against someone who doesn't
adhere to locking policy, or is there a genuine use-case where the
parent directory could vanish before we call rmdir(2)?

On 9/17/26 10:26, Stefan Sperling wrote:
> On Sat, Sep 12, 2026 at 04:42:30PM +0200, Martijn van Duren wrote:
>> martijn@
> 
> Thanks, here is a fix. Does it work for you?
> 
> Please already push your test to got.gameoftrees.org if you have time.
> Otherwise I'll send it along with my fix.
> 
> M  lib/reference.c  |  11+  1-
> 
> 1 file changed, 11 insertions(+), 1 deletion(-)
> 
> commit - 8d7e66e9566a7274917590c6670ea30972d39172
> commit + 12c30af2c8a873c913fd1073d99e2f60c08fb60d
> blob - 40573c4679ab45fe6119a377d17ee557b87498f2
> blob + 50259bff0ca1b0164f56f26dbebb54e36bd1b73d
> --- lib/reference.c
> +++ lib/reference.c
> @@ -1535,7 +1535,7 @@ delete_loose_ref(struct got_reference *ref, struct got
>  {
>  	const struct got_error *err = NULL, *unlock_err = NULL;
>  	const char *name = got_ref_get_name(ref);
> -	char *path_refs = NULL, *path = NULL;
> +	char *path_refs = NULL, *path = NULL, *parent = NULL;
>  	struct got_lockfile *lf = NULL;
>  
>  	path_refs = get_refs_dir_path(repo, name);
> @@ -1549,6 +1549,10 @@ delete_loose_ref(struct got_reference *ref, struct got
>  		goto done;
>  	}
>  
> +	err = got_path_dirname(&parent, path);
> +	if (err)
> +		goto done;
> +
>  	if (ref->lf == NULL) {
>  		err = got_lockfile_lock(&lf, path, -1);
>  		if (err)
> @@ -1563,8 +1567,14 @@ done:
>  	if (ref->lf == NULL && lf)
>  		unlock_err = got_lockfile_unlock(lf, -1);
>  
> +	/* Remove empty directories to make parent reference name available. */
> +	if (err == NULL && unlock_err == NULL &&
> +	    rmdir(parent) == -1 && errno != ENOTEMPTY && errno != ENOENT)
> +		err = got_error_from_errno2("rmdir", path);
> +
>  	free(path_refs);
>  	free(path);
> +	free(parent);
>  	return err ? err : unlock_err;
>  }
>  
>>
>> diff /home/martijn/src/got
>> path + /home/martijn/src/got
>> commit - f85c55bef56ccf3f41b8de4fc0482423541066c7
>> blob - 6b14508b86ae76b06c4b422f9dddcb726e665798
>> file + regress/cmdline/branch.sh
>> --- regress/cmdline/branch.sh
>> +++ regress/cmdline/branch.sh
>> @@ -672,6 +672,45 @@ test_branch_list_worktree_state() {
>>  	test_done "$testroot" "$ret"
>>  }
>>  
>> +test_branch_del_child_create_parent() {
>> +	local testroot=$(test_init branch_del_child_create_parent)
>> +	local wt="$testroot/wt"
>> +
>> +	set -- "$(git_show_head "$testroot/repo")"
>> +
>> +	got checkout "$testroot/repo" "$wt" > /dev/null
>> +	ret=$?
>> +	if [ $ret -ne 0 ]; then
>> +		echo "checkout failed unexpectedly" >&2
>> +		test_done "$testroot" "$ret"
>> +		return 1
>> +	fi
>> +
>> +	(cd "$wt" && got br -n a/b > /dev/null)
>> +	ret=$?
>> +	if [ $ret -ne 0 ]; then
>> +		echo "branch a/b failed unexpectedly" >&2
>> +		test_done "$testroot" "$ret"
>> +		return 1
>> +	fi
>> +	(cd "$wt" && got br -d a/b > /dev/null)
>> +	ret=$?
>> +	if [ $ret -ne 0 ]; then
>> +		echo "branch deletion a/b failed unexpectedly" >&2
>> +		test_done "$testroot" "$ret"
>> +		return 1
>> +	fi
>> +	(cd "$wt" && got br -n a > /dev/null)
>> +	ret=$?
>> +	if [ $ret -ne 0 ]; then
>> +		echo "branch a failed unexpectedly" >&2
>> +		test_done "$testroot" "$ret"
>> +		return 1
>> +	fi
>> +
>> +	test_done "$testroot" "$ret"
>> +}
>> +
>>  test_parseargs "$@"
>>  run_test test_branch_create
>>  run_test test_branch_list
>> @@ -682,3 +721,4 @@ run_test test_branch_show
>>  run_test test_branch_packed_ref_collision
>>  run_test test_branch_commit_keywords
>>  run_test test_branch_list_worktree_state
>> +run_test test_branch_del_child_create_parent
>>
>>