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

On Sat, Oct 29, 2022 at 04:13:28PM +0200, Omar Polo wrote:
> 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 ;-)

This looks great, thanks!

I have one remining question:
How does the umask preservation interact with versioning of executable-bits?

An "easy" way to deal with the intersection of both features might be to
ignore the umask as far as x-bits are concerned. Meaning if the umask filters
out x-bits, but the tree entry's mode has an x-bit set, then set x-bits in
the filesystem regardless of what the umask says. This way, we require users
to clear x-bits explicitly if they want to get rid of them, rather than
letting the umask clear versioned x-bits implicitly.

Again, this would need test coverage.

In any case, you can commit this patch with my OK already if you would
like to, and we can sort out the x-bit situation later.