From: Omar Polo <op@omarpolo.com>
Subject: Re: attempt at respecting umask
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Sat, 29 Oct 2022 16:13:28 +0200

On 2022/10/28 23:52:35 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Thu, Oct 27, 2022 at 08:16:28PM +0200, Omar Polo wrote:
> > This is a (sad) attempt at making `got update' follow umask(2).
> > 
> > Under most circumstances we shouldn't do any extra work: when
> > checking out file open(2) does the work for us; the issue lays when
> > updating files.
> > 
> > In those cases, we need to either set or clear the xbit, but we
> > can't directly do that.  When updating we're using a fd provided
> > by mkstemp (via got_opentemp_named_fd) and it's likely to be 0600...
> > I assume that's why after open(2) we do an fchmod(2) to reset the
> > permission.  Loosing umask tho!
> > 
> > Diff below moves the fchmod(2) only in the update case and does an
> > (IMHO ugly) umask dance to compute the correct value.  Includes a
> > matching test case; existing tests are passing.
> > 
> > Suggestions are welcome!
> 
> This does not look very bad to me.

I don't think we can do much better.  open(2) applies the umask
transparently, mkstemp also, but starts with 0600...

> Something you could is move the umask dance into a separate function,
> such that you can write code like:
> 
> 			if (fchmod(fd, apply_umask(mode)) == -1) {
> 
> This works because umask(2) never fails, thus no error handling
> is needed.
> 
> Do all other code paths which modify files come through here?

nope!  It's obvlious, but I realized it only after starting to write
the test cases.  Most commands (including `patch') do tempfile +
rename and so more places needs to be tweaked.

> It would be good to have umask test cases for checkout, add, revert,
> commit, rebase, histedit, cherrypick, backout, merge, and unstage
> commands (that's a lot of commands, but corresponding test cose should
> hopefully be relatively quick to copy-paste?)

It wasn't hard to do, just a bit boring :)

diff below includes new regress tests for backout, checkout,
cherrypick histedit, merge, patch, rebase and revert.  I've skipped
add, commit and unstage from your list, and added patch too.

I had to duplicate apply_umask because patch.c also needs it and
moving `apply_patch' to worktree.c isn't that easy.  (I probably
did a mistake developing the patch stuff in its own file, but was
easy for me to do so when I started and worktree.c scared me a bit
back than ;-)

diff 86769de8751a920ee4288ec91157066d6f098bfc e0dd859e4a5061670f1f6d7ca59d7dd64a884e35
commit - 86769de8751a920ee4288ec91157066d6f098bfc
commit + e0dd859e4a5061670f1f6d7ca59d7dd64a884e35
blob - 14024d2a98a433e4feb490708b08e72e42f96f75
blob + 8647dea45a83ad12f49ae63960dd903bd55fde98
--- lib/patch.c
+++ lib/patch.c
@@ -88,6 +88,16 @@ static const struct got_error *
 	struct got_patch_hunk_head *head;
 };
 
+static mode_t
+apply_umask(mode_t mode)
+{
+	mode_t um;
+
+	um = umask(000);
+	umask(um);
+	return mode & ~um;
+}
+
 static const struct got_error *
 send_patch(struct imsgbuf *ibuf, int fd)
 {
@@ -916,7 +926,7 @@ apply_patch(int *overlapcnt, struct got_worktree *work
 		goto done;
 	}
 
-	if (fchmod(outfd, mode) == -1) {
+	if (fchmod(outfd, apply_umask(mode)) == -1) {
 		err = got_error_from_errno2("chmod", tmppath);
 		goto done;
 	}
blob - d0e5af7c33c6ca8a4ba2e9dc621a494ed26dda57
blob + 438973a5f89e071064f01cb39023043b4337946a
--- lib/worktree.c
+++ lib/worktree.c
@@ -64,6 +64,8 @@ static const struct got_error *
 #define GOT_MERGE_LABEL_MERGED	"merged change"
 #define GOT_MERGE_LABEL_BASE	"3-way merge base"
 
+static mode_t		 apply_umask(mode_t);
+
 static const struct got_error *
 create_meta_file(const char *path_got, const char *name, const char *content)
 {
@@ -703,7 +705,7 @@ merge_file(int *local_changes_subsumed, struct got_wor
 			goto done;
 	}
 
-	if (fchmod(fileno(f_merged), st_mode) != 0) {
+	if (fchmod(fileno(f_merged), apply_umask(st_mode)) != 0) {
 		err = got_error_from_errno2("fchmod", merged_path);
 		goto done;
 	}
@@ -776,7 +778,7 @@ install_symlink_conflict(const char *deriv_target,
 	if (err)
 		goto done;
 
-	if (fchmod(fileno(f), GOT_DEFAULT_FILE_MODE) == -1) {
+	if (fchmod(fileno(f), apply_umask(GOT_DEFAULT_FILE_MODE)) == -1) {
 		err = got_error_from_errno2("fchmod", path);
 		goto done;
 	}
@@ -1124,9 +1126,19 @@ get_ondisk_perms(int executable, mode_t st_mode)
 		return st_mode | xbits;
 	}
 
-	return (st_mode & ~(S_IXUSR | S_IXGRP | S_IXOTH));
+	return st_mode;
 }
 
+static mode_t
+apply_umask(mode_t mode)
+{
+	mode_t um;
+
+	um = umask(000);
+	umask(um);
+	return mode & ~um;
+}
+
 /* forward declaration */
 static const struct got_error *
 install_blob(struct got_worktree *worktree, const char *ondisk_path,
@@ -1388,9 +1400,11 @@ install_blob(struct got_worktree *worktree, const char
 	size_t len, hdrlen;
 	int update = 0;
 	char *tmppath = NULL;
+	mode_t mode;
 
+	mode = get_ondisk_perms(te_mode & S_IXUSR, GOT_DEFAULT_FILE_MODE);
 	fd = open(ondisk_path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW |
-	    O_CLOEXEC, GOT_DEFAULT_FILE_MODE);
+	    O_CLOEXEC, mode);
 	if (fd == -1) {
 		if (errno == ENOENT) {
 			char *parent;
@@ -1403,7 +1417,7 @@ install_blob(struct got_worktree *worktree, const char
 				return err;
 			fd = open(ondisk_path,
 			    O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC,
-			    GOT_DEFAULT_FILE_MODE);
+			    mode);
 			if (fd == -1)
 				return got_error_from_errno2("open",
 				    ondisk_path);
@@ -1425,17 +1439,17 @@ install_blob(struct got_worktree *worktree, const char
 				if (err)
 					goto done;
 				update = 1;
+
+				if (fchmod(fd, apply_umask(mode)) == -1) {
+					err = got_error_from_errno2("fchmod",
+					    tmppath);
+					goto done;
+				}
 			}
 		} else
 			return got_error_from_errno2("open", ondisk_path);
 	}
 
-	if (fchmod(fd, get_ondisk_perms(te_mode & S_IXUSR, st_mode)) == -1) {
-		err = got_error_from_errno2("fchmod",
-		    update ? tmppath : ondisk_path);
-		goto done;
-	}
-
 	if (progress_cb) {
 		if (restoring_missing_file)
 			err = (*progress_cb)(progress_arg, GOT_STATUS_MISSING,
@@ -4590,7 +4604,10 @@ create_patched_content(char **path_outfile, int revers
 			goto done;
 
 		if (!S_ISLNK(sb2.st_mode)) {
-			if (fchmod(fileno(outfile), sb2.st_mode) == -1) {
+			mode_t mode;
+
+			mode = apply_umask(sb2.st_mode);
+			if (fchmod(fileno(outfile), mode) == -1) {
 				err = got_error_from_errno2("fchmod", path2);
 				goto done;
 			}
blob - 71210cda27e4286934db7aa6882ed109ca1ad397
blob + 02acb5b14f9b116de16f3ff0fe380929aada9a65
--- regress/cmdline/backout.sh
+++ regress/cmdline/backout.sh
@@ -208,7 +208,42 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_backout_umask() {
+	local testroot=`test_init backout_umask`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+	echo "edit alpha" >$testroot/wt/alpha
+	(cd "$testroot/wt" && got commit -m 'edit alpha') >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	local commit=`git_show_head "$testroot/repo"`
+
+	(cd "$testroot/wt" && got update) >/dev/null
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && got backout $commit) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-------; then
+		echo "alpha is not 0600 after backout" >&2
+		ls -l "$testroot/wt/alpha" >&2
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_backout_basic
 run_test test_backout_edits_for_file_since_deleted
 run_test test_backout_next_commit
+run_test test_backout_umask
blob - fa55d96f05c0ea98d084505524dc36353abc0120
blob + 3ceb6774f7bce8a411b960566268aea4c45554cc
--- regress/cmdline/checkout.sh
+++ regress/cmdline/checkout.sh
@@ -854,6 +854,41 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_checkout_umask() {
+	local testroot=`test_init checkout_umask`
+
+	# using a subshell to avoid clobbering global umask
+	(umask 044 && got checkout "$testroot/repo" "$testroot/wt") \
+		>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	for f in alpha beta epsilon/zeta gamma/delta; do
+		ls -l "$testroot/wt/$f" | grep -q ^-rw-------
+		if [ $? -ne 0 ]; then
+			echo "$f is not 0600 after checkout" >&2
+			ls -l "$testroot/wt/$f" >&2
+			test_done "$testroot" 1
+			return 1
+		fi
+	done
+
+	for d in epsilon gamma; do
+		ls -ld "$testroot/wt/$d" | grep -q ^drwx--x--x
+		if [ $? -ne 0 ]; then
+			echo "$d is not 711 after checkout" >&2
+			ls -ld "$testroot/wt/$d" >&2
+			test_done "$testroot" 1
+			return 1
+		fi
+	done
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_checkout_basic
 run_test test_checkout_dir_exists
@@ -868,3 +903,4 @@ run_test test_checkout_quiet
 run_test test_checkout_symlink_relative_wtpath
 run_test test_checkout_repo_with_unknown_extension
 run_test test_checkout_quiet
+run_test test_checkout_umask
blob - ed1cc69eb0e97b9e276519a28388cd22e2ec82dc
blob + 464cd2159b38557b4fa61c2ac402ba344ba16d43
--- regress/cmdline/cherrypick.sh
+++ regress/cmdline/cherrypick.sh
@@ -1692,6 +1692,39 @@ test_parseargs "$@"
 	test_done "$testroot" "0"
 }
 
+test_cherrypick_umask() {
+	local testroot=`test_init cherrypick_umask`
+
+	got checkout $testroot/repo $testroot/wt >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	(cd "$testroot/wt" && got branch newbranch) >/dev/null
+	echo "modified alpha on branch" > $testroot/wt/alpha
+	(cd "$testroot/wt" && got commit -m 'edit alpha') >/dev/null
+	(cd "$testroot/wt" && got update -b master) >/dev/null
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && got cherrypick newbranch) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-------; then
+		echo "alpha is not 0600 after cherrypick!" >&2
+		ls -l "$testroot/wt/alpha" >&2
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_cherrypick_basic
 run_test test_cherrypick_root_commit
@@ -1709,3 +1742,4 @@ run_test test_cherrypick_binary_file
 run_test test_cherrypick_same_branch
 run_test test_cherrypick_dot_on_a_line_by_itself
 run_test test_cherrypick_binary_file
+run_test test_cherrypick_umask
blob - 1264347441d2f3f6859d520ab1230f6161a9d666
blob + ed77df82520d57a35def63dbe04ca0b4cc145444
--- regress/cmdline/histedit.sh
+++ regress/cmdline/histedit.sh
@@ -2157,6 +2157,62 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_histedit_umask() {
+	local testroot=`test_init histedit_umask`
+	local orig_commit=`git_show_head "$testroot/repo"`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+
+	echo "modified alpha" > $testroot/wt/alpha
+	(cd "$testroot/wt" && got commit -m 'edit #1') >/dev/null
+	local commit1=`git_show_head "$testroot/repo"`
+
+	echo "modified again" > $testroot/wt/alpha
+	(cd "$testroot/wt" && got commit -m 'edit #2') >/dev/null
+	local commit2=`git_show_head "$testroot/repo"`
+
+	echo "modified again!" > $testroot/wt/alpha
+	echo "modify beta too!" > $testroot/wt/beta
+	(cd "$testroot/wt" && got commit -m 'edit #3') >/dev/null
+	local commit3=`git_show_head "$testroot/repo"`
+
+	(cd "$testroot/wt" && got update -c "$orig_commit") >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "update to $orig_commit failed!" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	echo fold $commit1 >$testroot/histedit-script
+	echo fold $commit2 >>$testroot/histedit-script
+	echo pick $commit3 >>$testroot/histedit-script
+	echo mesg folding changes >>$testroot/histedit-script
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && \
+		got histedit -F "$testroot/histedit-script") >/dev/null
+	ret=$?
+
+	if [ $ret -ne 0 ]; then
+		echo "histedit operation failed" >&2
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	for f in alpha beta; do
+		ls -l "$testroot/wt/$f" | grep -q ^-rw-------
+		if [ $? -ne 0 ]; then
+			echo "$f is not 0600 after histedi" >&2
+			ls -l "$testroot/wt/$f" >&2
+			test_done "$testroot" 1
+			return 1
+		fi
+	done
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_histedit_no_op
 run_test test_histedit_swap
@@ -2179,3 +2235,4 @@ run_test test_histedit_resets_committer
 run_test test_histedit_prepend_line
 run_test test_histedit_mesg_invalid
 run_test test_histedit_resets_committer
+run_test test_histedit_umask
blob - 3f0578e460b5ab26c3e92ecb652e7a6e21f4cb65
blob + 43fc62a9959f02a646f8a0777654f8bce35c3878
--- regress/cmdline/merge.sh
+++ regress/cmdline/merge.sh
@@ -1394,6 +1394,37 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_merge_umask() {
+	local testroot=`test_init merge_umask`
+
+	(cd $testroot/repo && git checkout -q -b newbranch)
+	echo "modified alpha on branch" >$testroot/repo/alpha
+	git_commit "$testroot/repo" -m "committing alpha on newbranch"
+	echo "modified delta on branch" >$testroot/repo/gamma/delta
+	git_commit "$testroot/repo" -m "committing delta on newbranch"
+
+	# diverge from newbranch
+	(cd "$testroot/repo" && git checkout -q master)
+	echo "modified beta on master" >$testroot/repo/beta
+	git_commit "$testroot/repo" -m "committing zeto no master"
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && got merge newbranch) >/dev/null
+
+	for f in alpha gamma/delta; do
+		ls -l "$testroot/wt/$f" | grep -q ^-rw-------
+		if [ $? -ne 0 ]; then
+			echo "$f is not 0600 after merge" >&2
+			ls -l "$testroot/wt/$f" >&2
+			test_done "$testroot" 1
+		fi
+	done
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_merge_basic
 run_test test_merge_continue
@@ -1404,3 +1435,4 @@ run_test test_merge_interrupt
 run_test test_merge_no_op
 run_test test_merge_imported_branch
 run_test test_merge_interrupt
+run_test test_merge_umask
blob - f8f949b792887413b26e30cab123e9e1c57ae123
blob + af677fff9e318f5b61edce4c73ec63709b02fd53
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1850,6 +1850,37 @@ test_parseargs "$@"
 	test_done $testroot $ret
 }
 
+test_patch_umask() {
+	local testroot=`test_init patch_umask`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+
+	cat <<EOF >$testroot/wt/patch
+--- alpha
++++ alpha
+@@ -1 +1 @@
+-alpha
++modified alpha
+EOF
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && got patch <patch) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-------; then
+		echo "alpha is not 0600 after patch" >&2
+		ls -l "$testroot/wt/alpha" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_patch_basic
 run_test test_patch_dont_apply
@@ -1878,3 +1909,4 @@ run_test test_patch_newfile_xbit_git_diff
 run_test test_patch_merge_reverse
 run_test test_patch_newfile_xbit_got_diff
 run_test test_patch_newfile_xbit_git_diff
+run_test test_patch_umask
blob - 62ae3b151c0c8c8e867585314a8e3bc15764c831
blob + f809879812d293f5b44a4ad0e1a299f1384dc027
--- regress/cmdline/rebase.sh
+++ regress/cmdline/rebase.sh
@@ -1790,6 +1790,50 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_rebase_umask() {
+	local testroot=`test_init rebase_umask`
+	local commit0=`git_show_head "$testroot/repo"`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+	(cd "$testroot/wt" && got branch newbranch) >/dev/null
+
+	echo "modified alpha on branch" >$testroot/wt/alpha
+	(cd "$testroot/wt" && got commit -m 'modified alpha on newbranch') \
+		>/dev/null
+
+	(cd "$testroot/wt" && got update -b master) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got update failed!" >&2
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	echo "modified beta on master" >$testroot/wt/beta
+	(cd "$testroot/wt" && got commit -m 'modified beta on master') \
+		>/dev/null
+	(cd "$testroot/wt" && got update) >/dev/null
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && got rebase newbranch) >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got rebase failed" >&2
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	ls -l "$testroot/wt/alpha" | grep -q ^-rw-------
+	if [ $? -ne 0 ]; then
+		echo "alpha is not 0600 after rebase" >&2
+		ls -l "$testroot/wt/alpha" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_rebase_basic
 run_test test_rebase_ancestry_check
@@ -1809,3 +1853,4 @@ run_test test_rebase_nonbranch
 run_test test_rebase_resets_committer
 run_test test_rebase_no_author_info
 run_test test_rebase_nonbranch
+run_test test_rebase_umask
blob - a565acdd0271008c6ea82910d668b93ed096e382
blob + 3d38c4d175937ca52bef1b76659276b50d0cf91e
--- regress/cmdline/revert.sh
+++ regress/cmdline/revert.sh
@@ -1489,6 +1489,30 @@ test_parseargs "$@"
 	test_done "$testroot" "$ret"
 }
 
+test_revert_umask() {
+	local testroot=`test_init revert_umask`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+	echo "edit alpha" > $testroot/wt/alpha
+
+	# using a subshell to avoid clobbering global umask
+	(umask 077 && cd "$testroot/wt" && got revert alpha) \
+		>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-------; then
+		echo "alpha is not 0600 after revert" >&2
+		ls -l "$testroot/wt/alpha" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_revert_basic
 run_test test_revert_rm
@@ -1506,3 +1530,4 @@ run_test test_revert_patch_symlink
 run_test test_revert_deleted_subtree
 run_test test_revert_symlink
 run_test test_revert_patch_symlink
+run_test test_revert_umask
blob - fce60c7f277d948ec481a744ed23e1b55ec128ed
blob + afb98ebb0b2de07c90dfa887d9d20fe0cb5f0db4
--- regress/cmdline/update.sh
+++ regress/cmdline/update.sh
@@ -3010,6 +3010,70 @@ test_parseargs "$@"
 	test_done "$testroot" "0"
 }
 
+test_update_umask() {
+	local testroot=`test_init update_binary_file`
+
+	got checkout "$testroot/repo" "$testroot/wt" >/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	rm "$testroot/wt/alpha"
+
+	# using a subshell to avoid clobbering global umask
+	(umask 022 && cd "$testroot/wt" && got update alpha) \
+		>/dev/null 2>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-r--r--; then
+		echo "alpha is not 0644" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	rm "$testroot/wt/alpha"
+
+	# using a subshell to avoid clobbering global umask
+	(umask 044 && cd "$testroot/wt" && got update alpha) \
+		>/dev/null 2>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-rw-------; then
+		echo "alpha is not 0600" >&2
+		test_done "$testroot" 1
+		return 1
+	fi
+
+	rm "$testroot/wt/alpha"
+
+	# using a subshell to avoid clobbering global umask
+	(umask 222 && cd "$testroot/wt" && got update alpha) \
+		>/dev/null 2>/dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" $ret
+		return 1
+	fi
+
+	if ! ls -l "$testroot/wt/alpha" | grep -q ^-r--r--r--; then
+		echo "alpha is not 0444" >&2
+		test_done "$testroot" 1
+		return 1;
+	fi
+
+	test_done "$testroot" 0
+}
+
 test_parseargs "$@"
 run_test test_update_basic
 run_test test_update_adds_file
@@ -3054,3 +3118,4 @@ run_test test_update_binary_file
 run_test test_update_file_skipped_due_to_obstruction
 run_test test_update_quiet
 run_test test_update_binary_file
+run_test test_update_umask