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

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.

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?
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?)