[PATCH 0/3] commit-graph: write non-split graphs as read-only

[PATCH 0/3] commit-graph: write non-split graphs as read-only

[Taylor Blau]

Hi,

A colleague of mine pointed out they had occasionally observed that some
layers of a commit-graph chain appear to be have both read and write
permission bits for the user, while others are marked as read-only.

After some investigation, I realized that this only happens to the first
layer in a commit-graph-chain, and occurs when we move a non-split graph
into a chain as the new base. Since write_commit_graph() just renames in
this case, we carry over the 0644 permissions that we create non-split
graphs with, even though split graphs are created to be read-only.

Initially, I resolved this by 'chmod(graph, 0444)'-ing the graph after
renaming it into place, but it ends up being much cleaner if we
introduce an additional parameter in 'hold_lock_file_for_update' for a
mode.

The first two patches set this up, and the third patch uses it in
commit-graph.c, and corrects some test fallout. Eventually, we may want
to take another look at all of this and always create lockfiles with
permission 0444, but that change is much larger than this one and could
instead be done over time.

Thanks,
Taylor

Taylor Blau (3):
  tempfile.c: introduce 'create_tempfile_mode'
  lockfile.c: introduce 'hold_lock_file_for_update_mode'
  commit-graph.c: write non-split graphs as read-only

 commit-graph.c          |  3 ++-
 lockfile.c              | 18 ++++++++++--------
 lockfile.h              | 19 +++++++++++++++++--
 t/t5318-commit-graph.sh | 11 ++++++++++-
 t/t6600-test-reach.sh   |  2 ++
 tempfile.c              |  6 +++---
 tempfile.h              |  7 ++++++-
 7 files changed, 50 insertions(+), 16 deletions(-)

--
2.26.1.108.gadb95c98e4

[PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode'

[Taylor Blau]

In the next patch, 'hold_lock_file_for_update' will gain an additional
'mode' parameter to specify permissions for the associated temporary
file.

Since the lockfile.c machinery uses 'create_tempfile' which always
creates a temporary file with global read-write permissions, introduce a
variant here that allows specifying the mode.

Arguably, all temporary files should have permission 0444, since they
are likely to be renamed into place and then not written to again. This
is a much larger change than we may want to take on in this otherwise
small patch, so for the time being, make 'create_tempfile' behave as it
has always done by inlining it to 'create_tempfile_mode' with mode set
to '0666'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 tempfile.c | 6 +++---
 tempfile.h | 7 ++++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index d43ad8c191..94aa18f3f7 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)
 }
 
 /* Make sure errno contains a meaningful value on error */
-struct tempfile *create_tempfile(const char *path)
+struct tempfile *create_tempfile_mode(const char *path, int mode)
 {
 	struct tempfile *tempfile = new_tempfile();
 
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->fd = open(tempfile->filename.buf,
-			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
+			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);
 	if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)
 		/* Try again w/o O_CLOEXEC: the kernel might not support it */
 		tempfile->fd = open(tempfile->filename.buf,
-				    O_RDWR | O_CREAT | O_EXCL, 0666);
+				    O_RDWR | O_CREAT | O_EXCL, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
 		return NULL;
diff --git a/tempfile.h b/tempfile.h
index cddda0a33c..b5760d4581 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -89,7 +89,12 @@ struct tempfile {
  * a tempfile (whose "fd" member can be used for writing to it), or
  * NULL on error. It is an error if a file already exists at that path.
  */
-struct tempfile *create_tempfile(const char *path);
+struct tempfile *create_tempfile_mode(const char *path, int mode);
+
+static inline struct tempfile *create_tempfile(const char *path)
+{
+	return create_tempfile_mode(path, 0666);
+}
 
 /*
  * Register an existing file as a tempfile, meaning that it will be
-- 
2.26.1.108.gadb95c98e4

[PATCH 2/3] lockfile.c: introduce 'hold_lock_file_for_update_mode'

[Taylor Blau]

We use 'hold_lock_file_for_update' (and the '_timeout') variant to
acquire a lock when updating references, the commit-graph file, and so
on.

In particular, the commit-graph machinery uses this to acquire a
temporary file that is used to write a non-split commit-graph. In a
subsequent commit, an issue in the commit-graph machinery produces
graph files that have a different permission based on whether or not
they are part of a multi-layer graph will be addressed.

To do so, the commit-graph machinery will need a version of
'hold_lock_file_for_update' that takes the permission bits from the
caller.

Introduce such a function in this patch for both the
'hold_lock_file_for_update' and 'hold_lock_file_for_update_timeout'
functions, and leave the existing functions alone by inlining their
definitions in terms of the new mode variants.

Note that even though the commit-graph machinery only calls
'hold_lock_file_for_update', that this is defined in terms of
'hold_lock_file_for_update_timeout', and so both need an additional mode
parameter here.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 lockfile.c | 18 ++++++++++--------
 lockfile.h | 19 +++++++++++++++++--
 2 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 8e8ab4f29f..cc9a4b8428 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -70,7 +70,8 @@ static void resolve_symlink(struct strbuf *path)
 }
 
 /* Make sure errno contains a meaningful value on error */
-static int lock_file(struct lock_file *lk, const char *path, int flags)
+static int lock_file(struct lock_file *lk, const char *path, int flags,
+		     int mode)
 {
 	struct strbuf filename = STRBUF_INIT;
 
@@ -79,7 +80,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		resolve_symlink(&filename);
 
 	strbuf_addstr(&filename, LOCK_SUFFIX);
-	lk->tempfile = create_tempfile(filename.buf);
+	lk->tempfile = create_tempfile_mode(filename.buf, mode);
 	strbuf_release(&filename);
 	return lk->tempfile ? lk->tempfile->fd : -1;
 }
@@ -99,7 +100,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
  * exactly once. If timeout_ms is -1, try indefinitely.
  */
 static int lock_file_timeout(struct lock_file *lk, const char *path,
-			     int flags, long timeout_ms)
+			     int flags, long timeout_ms, int mode)
 {
 	int n = 1;
 	int multiplier = 1;
@@ -107,7 +108,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 	static int random_initialized = 0;
 
 	if (timeout_ms == 0)
-		return lock_file(lk, path, flags);
+		return lock_file(lk, path, flags, mode);
 
 	if (!random_initialized) {
 		srand((unsigned int)getpid());
@@ -121,7 +122,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 		long backoff_ms, wait_ms;
 		int fd;
 
-		fd = lock_file(lk, path, flags);
+		fd = lock_file(lk, path, flags, mode);
 
 		if (fd >= 0)
 			return fd; /* success */
@@ -169,10 +170,11 @@ NORETURN void unable_to_lock_die(const char *path, int err)
 }
 
 /* This should return a meaningful errno on failure */
-int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
-				      int flags, long timeout_ms)
+int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,
+					   const char *path, int flags,
+					   long timeout_ms, int mode)
 {
-	int fd = lock_file_timeout(lk, path, flags, timeout_ms);
+	int fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);
 	if (fd < 0) {
 		if (flags & LOCK_DIE_ON_ERROR)
 			unable_to_lock_die(path, errno);
diff --git a/lockfile.h b/lockfile.h
index 9843053ce8..12f25279b5 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -159,9 +159,17 @@ struct lock_file {
  * timeout_ms is -1, retry indefinitely. The flags argument and error
  * handling are described above.
  */
-int hold_lock_file_for_update_timeout(
+int hold_lock_file_for_update_timeout_mode(
 		struct lock_file *lk, const char *path,
-		int flags, long timeout_ms);
+		int flags, long timeout_ms, int mode);
+
+static inline int hold_lock_file_for_update_timeout(
+		struct lock_file *lk, const char *path,
+		int flags, long timeout_ms)
+{
+	return hold_lock_file_for_update_timeout_mode(lk, path, flags,
+						      timeout_ms, 0666);
+}
 
 /*
  * Attempt to create a lockfile for the file at `path` and return a
@@ -175,6 +183,13 @@ static inline int hold_lock_file_for_update(
 	return hold_lock_file_for_update_timeout(lk, path, flags, 0);
 }
 
+static inline int hold_lock_file_for_update_mode(
+		struct lock_file *lk, const char *path,
+		int flags, int mode)
+{
+	return hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);
+}
+
 /*
  * Return a nonzero value iff `lk` is currently locked.
  */
-- 
2.26.1.108.gadb95c98e4

[PATCH 3/3] commit-graph.c: write non-split graphs as read-only

[Taylor Blau]

In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
allow the caller to specify the permission bits used when acquiring a
temporary file.

Use this in the commit-graph machinery for writing a non-split graph to
acquire an opened temporary file with permissions read-only permissions
to match the split behavior. (In the split case, Git uses
'git_mkstemp_mode' for each of the commit-graph layers with permission
bits '0444').

One can notice this discrepancy when moving a non-split graph to be part
of a new chain. This causes a commit-graph chain where all layers have
read-only permission bits, except for the base layer, which is writable
for the current user.

Resolve this discrepancy by using the new
'hold_lock_file_for_update_mode' and passing the desired permission
bits.

Doing so causes some test fallout in t5318 and t6600. In t5318, this
occurs in tests that corrupt a commit-graph file by writing into it. For
these, 'chmod u+w'-ing the file beforehand resolves the issue. The
additional spot in 'corrupt_graph_verify' is necessary because of the
extra 'git commit-graph write' beforehand (which *does* rewrite the
commit-graph file). In t6600, this is caused by copying a read-only
commit-graph file into place and then trying to replace it. For these,
make these files writable.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c          |  3 ++-
 t/t5318-commit-graph.sh | 11 ++++++++++-
 t/t6600-test-reach.sh   |  2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index f013a84e29..5b5047a7dd 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1388,7 +1388,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 		f = hashfd(fd, ctx->graph_name);
 	} else {
-		hold_lock_file_for_update(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR);
+		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
+					       LOCK_DIE_ON_ERROR, 0444);
 		fd = lk.tempfile->fd;
 		f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
 	}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 9bf920ae17..fb0aae61c3 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -96,6 +96,13 @@ test_expect_success 'write graph' '
 	graph_read_expect "3"
 '
 
+test_expect_success POSIXPERM 'write graph has correct permissions' '
+	test_path_is_file $objdir/info/commit-graph &&
+	echo "-r--r--r--" >expect &&
+	test_modebits $objdir/info/commit-graph >actual &&
+	test_cmp expect actual
+'
+
 graph_git_behavior 'graph exists' full commits/3 commits/1
 
 test_expect_success 'Add more commits' '
@@ -421,7 +428,8 @@ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
 corrupt_graph_setup() {
 	cd "$TRASH_DIRECTORY/full" &&
 	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-	cp $objdir/info/commit-graph commit-graph-backup
+	cp $objdir/info/commit-graph commit-graph-backup &&
+	chmod u+w $objdir/info/commit-graph
 }
 
 corrupt_graph_verify() {
@@ -435,6 +443,7 @@ corrupt_graph_verify() {
 	fi &&
 	git status --short &&
 	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+	chmod u+w $objdir/info/commit-graph &&
 	git commit-graph verify
 }
 
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index b24d850036..475564bee7 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -51,8 +51,10 @@ test_expect_success 'setup' '
 	done &&
 	git commit-graph write --reachable &&
 	mv .git/objects/info/commit-graph commit-graph-full &&
+	chmod u+w commit-graph-full &&
 	git show-ref -s commit-5-5 | git commit-graph write --stdin-commits &&
 	mv .git/objects/info/commit-graph commit-graph-half &&
+	chmod u+w commit-graph-half &&
 	git config core.commitGraph true
 '
 
-- 
2.26.1.108.gadb95c98e4

Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> The first two patches set this up, and the third patch uses it in
> commit-graph.c, and corrects some test fallout. Eventually, we may want
> to take another look at all of this and always create lockfiles with
> permission 0444, but that change is much larger than this one and could
> instead be done over time.

So,... is the problem that this did not follow the common pattern of
creating (while honoring umask) and then call adjust_shared_perm(),
which I thought was the common pattern for files in $GIT_DIR/?

Re: [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode'

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Arguably, all temporary files should have permission 0444, since they
> are likely to be renamed into place and then not written to again.

If the repository is not shared, it is more than likely that these
files ought to have permission narrower than that.  As you'd be
concluding any file that you created and written to in $GIT_DIR/
with adjust_shared_perm(), I do not think it is a good idea to allow
callers to pass an arbitrary mode and let them expect the mode would
stick.  Shoudln't we instead be passing "int read_only" and depending
on the boolean value, choosing between 0444 and 0666 in the function?

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  tempfile.c | 6 +++---
>  tempfile.h | 7 ++++++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tempfile.c b/tempfile.c
> index d43ad8c191..94aa18f3f7 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)
>  }
>  
>  /* Make sure errno contains a meaningful value on error */
> -struct tempfile *create_tempfile(const char *path)
> +struct tempfile *create_tempfile_mode(const char *path, int mode)
>  {
>  	struct tempfile *tempfile = new_tempfile();
>  
>  	strbuf_add_absolute_path(&tempfile->filename, path);
>  	tempfile->fd = open(tempfile->filename.buf,
> -			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
> +			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);
>  	if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)
>  		/* Try again w/o O_CLOEXEC: the kernel might not support it */
>  		tempfile->fd = open(tempfile->filename.buf,
> -				    O_RDWR | O_CREAT | O_EXCL, 0666);
> +				    O_RDWR | O_CREAT | O_EXCL, mode);
>  	if (tempfile->fd < 0) {
>  		deactivate_tempfile(tempfile);
>  		return NULL;
> diff --git a/tempfile.h b/tempfile.h
> index cddda0a33c..b5760d4581 100644
> --- a/tempfile.h
> +++ b/tempfile.h
> @@ -89,7 +89,12 @@ struct tempfile {
>   * a tempfile (whose "fd" member can be used for writing to it), or
>   * NULL on error. It is an error if a file already exists at that path.
>   */
> -struct tempfile *create_tempfile(const char *path);
> +struct tempfile *create_tempfile_mode(const char *path, int mode);
> +
> +static inline struct tempfile *create_tempfile(const char *path)
> +{
> +	return create_tempfile_mode(path, 0666);
> +}
>  
>  /*
>   * Register an existing file as a tempfile, meaning that it will be

Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only

[Taylor Blau]

On Mon, Apr 20, 2020 at 04:23:13PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > The first two patches set this up, and the third patch uses it in
> > commit-graph.c, and corrects some test fallout. Eventually, we may want
> > to take another look at all of this and always create lockfiles with
> > permission 0444, but that change is much larger than this one and could
> > instead be done over time.
>
> So,... is the problem that this did not follow the common pattern of
> creating (while honoring umask) and then call adjust_shared_perm(),
> which I thought was the common pattern for files in $GIT_DIR/?

We do call adjust_shared_perm() from (based on what's currently in
master) 'write_commit_graph_file' -> 'hold_lockfile_for_update' ->
'lock_file_timeout' -> 'lock_file' -> 'create_tempfile'.

But do we want commit-graphs to be respecting 'core.sharedRepository'
here? Either we:

  * do want to respect core.sharedRepository, in which case the
    current behavior of always setting split-graph permissions to '0444'
    is wrong, or

  * we do not want to respect core.sharedRepository, in which case these
    patches are doing what we want by setting all commit-graph files to
    have read-only permissions.

My hunch is that we do not want to abide by core.sharedRepository here
for the same reason that, say, loose objects are read-only. What do you
think?


Thanks,
Taylor

Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

>   * do want to respect core.sharedRepository, in which case the
>     current behavior of always setting split-graph permissions to '0444'
>     is wrong, or

Yes, I would say "always 0444" is wrong.

>   * we do not want to respect core.sharedRepository, in which case these
>     patches are doing what we want by setting all commit-graph files to
>     have read-only permissions.
>
> My hunch is that we do not want to abide by core.sharedRepository here
> for the same reason that, say, loose objects are read-only. What do you
> think?

I thought that adjusting perm for sharedRepository is orthogonal to
the read-only vs read-write.  If a file ought to be writable by the
owner, we would make it group writable in a group shared repository.
If a file is readable by the owner, we make sure it is readable by
group in such a repository (and we do not want to flip write bit
on).  That happens by calling path.c::calc_shared_perm().

Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only

[Jeff King]

On Mon, Apr 20, 2020 at 06:17:05PM -0700, Junio C Hamano wrote:

> Taylor Blau <me@ttaylorr.com> writes:
> 
> >   * do want to respect core.sharedRepository, in which case the
> >     current behavior of always setting split-graph permissions to '0444'
> >     is wrong, or
> 
> Yes, I would say "always 0444" is wrong.

I'm not sure. That's what we do for loose objects, packs, etc. The mode
we feed to git_mkstemp_mode(), etc, is not the true mode we expect to
end up with. We know that it will be modified by the umask, and then
perhaps by adjust_shared_perm().

If you are arguing that there are only two interesting modes: 0444 and
0666, and those could be represented by a read-only/read-write enum, I'd
agree with that. But the rest of the code already spells those as 0444
and 0666, and I don't see that as a big problem (in fact, there's a bit
of an advantage because the same constants work at both high and low
levels of the call stack, so you don't have to decide where to put the
translation).

> >   * we do not want to respect core.sharedRepository, in which case these
> >     patches are doing what we want by setting all commit-graph files to
> >     have read-only permissions.
> >
> > My hunch is that we do not want to abide by core.sharedRepository here
> > for the same reason that, say, loose objects are read-only. What do you
> > think?
> 
> I thought that adjusting perm for sharedRepository is orthogonal to
> the read-only vs read-write.  If a file ought to be writable by the
> owner, we would make it group writable in a group shared repository.
> If a file is readable by the owner, we make sure it is readable by
> group in such a repository (and we do not want to flip write bit
> on).  That happens by calling path.c::calc_shared_perm().

Right. I think adjust_shared_perm() should already be doing what we
want, and we should continue to call it. But it should not be
responsible for this "read-only versus read-write" decision. That
happens much earlier, and it adjusts as appropriate.

-Peff

Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Yes, I would say "always 0444" is wrong.
>
> I'm not sure. That's what we do for loose objects, packs, etc. The mode
> we feed to git_mkstemp_mode(), etc, is not the true mode we expect to
> end up with. We know that it will be modified by the umask, and then
> perhaps by adjust_shared_perm().
>
> If you are arguing that there are only two interesting modes: 0444 and
> 0666, and those could be represented by a read-only/read-write enum, I'd
> agree with that.

Yup, that is what I meant.  I am aware that these 0444/0666 are
limited with umask at open(O_CREAT) time, and then we later call
adjust_shared_perm().  I thought Taylor meant to always make it
readable by everybody, which was the only point I was objecting to.

> Right. I think adjust_shared_perm() should already be doing what we
> want, and we should continue to call it. But it should not be
> responsible for this "read-only versus read-write" decision. That
> happens much earlier, and it adjusts as appropriate.

Yes.

[PATCH v2 0/4] commit-graph: write non-split graphs as read-only

[Taylor Blau]

Hi,

Here's an updated version of my series to resolve a difference between
the permissions that split- and non-split commit-graphs are written
with.

Not much has changed since last time, since the main discussion revolved
around differences between what mode the user passes and how that
interacts with 'adjust_shared_perm' as well as why split-graphs are
always read-only for everybody.

Patches 1-3 remain mostly the same, with only some additional
documentation in the patches and headers about the interaction between
the 'mode' parameter and the umask and 'core.sharedRepository'. The
fourth patch is new, and makes sure that split-graphs are also
respecting 'core.sharedRepository' (and never have permissions above
0444).

The end-state of this is that both split- and non-split graphs have at
most permission 0444, and both respect core.sharedRepository.

Thanks in advance for another look.


Taylor Blau (4):
  tempfile.c: introduce 'create_tempfile_mode'
  lockfile.c: introduce 'hold_lock_file_for_update_mode'
  commit-graph.c: write non-split graphs as read-only
  commit-graph.c: ensure graph layers respect core.sharedRepository

 commit-graph.c                |  9 ++++++++-
 lockfile.c                    | 18 ++++++++++--------
 lockfile.h                    | 32 ++++++++++++++++++++++++++++----
 t/t5318-commit-graph.sh       | 11 ++++++++++-
 t/t5324-split-commit-graph.sh | 18 ++++++++++++++++++
 t/t6600-test-reach.sh         |  2 ++
 tempfile.c                    |  6 +++---
 tempfile.h                    | 10 +++++++++-
 8 files changed, 88 insertions(+), 18 deletions(-)

Range-diff against v1:
1:  aa86e8df40 ! 1:  03c975b0bd tempfile.c: introduce 'create_tempfile_mode'
    @@ Commit message
         creates a temporary file with global read-write permissions, introduce a
         variant here that allows specifying the mode.

    +    Note that the mode given to 'create_tempfile_mode' is not guaranteed to
    +    be written to disk, since it is subject to both the umask and
    +    'core.sharedRepository'.
    +
         Arguably, all temporary files should have permission 0444, since they
         are likely to be renamed into place and then not written to again. This
         is a much larger change than we may want to take on in this otherwise
    @@ tempfile.c: static void deactivate_tempfile(struct tempfile *tempfile)

      ## tempfile.h ##
     @@ tempfile.h: struct tempfile {
    +  * Attempt to create a temporary file at the specified `path`. Return
       * a tempfile (whose "fd" member can be used for writing to it), or
       * NULL on error. It is an error if a file already exists at that path.
    ++ * Note that `mode` will be further modified by the umask, and possibly
    ++ * `core.sharedRepository`, so it is not guaranteed to have the given
    ++ * mode.
       */
     -struct tempfile *create_tempfile(const char *path);
     +struct tempfile *create_tempfile_mode(const char *path, int mode);
2:  dad37d4233 ! 2:  c1c84552bc lockfile.c: introduce 'hold_lock_file_for_update_mode'
    @@ Commit message
         functions, and leave the existing functions alone by inlining their
         definitions in terms of the new mode variants.

    -    Note that even though the commit-graph machinery only calls
    +    Note that, like in the previous commit, 'hold_lock_file_for_update_mode'
    +    is not guarenteed to set the given mode, since it may be modified by
    +    both the umask and 'core.sharedRepository'.
    +
    +    Note also that even though the commit-graph machinery only calls
         'hold_lock_file_for_update', that this is defined in terms of
         'hold_lock_file_for_update_timeout', and so both need an additional mode
         parameter here.
    @@ lockfile.c: NORETURN void unable_to_lock_die(const char *path, int err)
      			unable_to_lock_die(path, errno);

      ## lockfile.h ##
    +@@
    +  * functions. In particular, the state diagram and the cleanup
    +  * machinery are all implemented in the tempfile module.
    +  *
    ++ * Permission bits
    ++ * ---------------
    ++ *
    ++ * If you call either `hold_lock_file_for_update_mode` or
    ++ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested
    ++ * mode for the underlying temporary file. Note that the file isn't
    ++ * guaranteed to have this exact mode, since it may be limited by either
    ++ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`
    ++ * for more.
    +  *
    +  * Error handling
    +  * --------------
     @@ lockfile.h: struct lock_file {
    -  * timeout_ms is -1, retry indefinitely. The flags argument and error
    -  * handling are described above.
    +  * file descriptor for writing to it, or -1 on error. If the file is
    +  * currently locked, retry with quadratic backoff for at least
    +  * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if
    +- * timeout_ms is -1, retry indefinitely. The flags argument and error
    +- * handling are described above.
    ++ * timeout_ms is -1, retry indefinitely. The flags argument, error
    ++ * handling, and mode are described above.
       */
     -int hold_lock_file_for_update_timeout(
     +int hold_lock_file_for_update_timeout_mode(
3:  622fd92cee ! 3:  86cf29ce9c commit-graph.c: write non-split graphs as read-only
    @@ Commit message
         commit-graph.c: write non-split graphs as read-only

         In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
    -    allow the caller to specify the permission bits used when acquiring a
    -    temporary file.
    +    allow the caller to specify the permission bits (prior to further
    +    adjustment by the umask and shared repository permissions) used when
    +    acquiring a temporary file.

         Use this in the commit-graph machinery for writing a non-split graph to
         acquire an opened temporary file with permissions read-only permissions
         to match the split behavior. (In the split case, Git uses
    -    'git_mkstemp_mode' for each of the commit-graph layers with permission
    +    git_mkstemp_mode' for each of the commit-graph layers with permission
         bits '0444').

         One can notice this discrepancy when moving a non-split graph to be part
-:  ---------- > 4:  f83437f130 commit-graph.c: ensure graph layers respect core.sharedRepository
--
2.26.0.113.ge9739cdccc

[PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode'

[Taylor Blau]

In the next patch, 'hold_lock_file_for_update' will gain an additional
'mode' parameter to specify permissions for the associated temporary
file.

Since the lockfile.c machinery uses 'create_tempfile' which always
creates a temporary file with global read-write permissions, introduce a
variant here that allows specifying the mode.

Note that the mode given to 'create_tempfile_mode' is not guaranteed to
be written to disk, since it is subject to both the umask and
'core.sharedRepository'.

Arguably, all temporary files should have permission 0444, since they
are likely to be renamed into place and then not written to again. This
is a much larger change than we may want to take on in this otherwise
small patch, so for the time being, make 'create_tempfile' behave as it
has always done by inlining it to 'create_tempfile_mode' with mode set
to '0666'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 tempfile.c |  6 +++---
 tempfile.h | 10 +++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index d43ad8c191..94aa18f3f7 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)
 }
 
 /* Make sure errno contains a meaningful value on error */
-struct tempfile *create_tempfile(const char *path)
+struct tempfile *create_tempfile_mode(const char *path, int mode)
 {
 	struct tempfile *tempfile = new_tempfile();
 
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->fd = open(tempfile->filename.buf,
-			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
+			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);
 	if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)
 		/* Try again w/o O_CLOEXEC: the kernel might not support it */
 		tempfile->fd = open(tempfile->filename.buf,
-				    O_RDWR | O_CREAT | O_EXCL, 0666);
+				    O_RDWR | O_CREAT | O_EXCL, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
 		return NULL;
diff --git a/tempfile.h b/tempfile.h
index cddda0a33c..4de3bc77d2 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -88,8 +88,16 @@ struct tempfile {
  * Attempt to create a temporary file at the specified `path`. Return
  * a tempfile (whose "fd" member can be used for writing to it), or
  * NULL on error. It is an error if a file already exists at that path.
+ * Note that `mode` will be further modified by the umask, and possibly
+ * `core.sharedRepository`, so it is not guaranteed to have the given
+ * mode.
  */
-struct tempfile *create_tempfile(const char *path);
+struct tempfile *create_tempfile_mode(const char *path, int mode);
+
+static inline struct tempfile *create_tempfile(const char *path)
+{
+	return create_tempfile_mode(path, 0666);
+}
 
 /*
  * Register an existing file as a tempfile, meaning that it will be
-- 
2.26.0.113.ge9739cdccc

[PATCH v2 2/4] lockfile.c: introduce 'hold_lock_file_for_update_mode'

[Taylor Blau]

We use 'hold_lock_file_for_update' (and the '_timeout') variant to
acquire a lock when updating references, the commit-graph file, and so
on.

In particular, the commit-graph machinery uses this to acquire a
temporary file that is used to write a non-split commit-graph. In a
subsequent commit, an issue in the commit-graph machinery produces
graph files that have a different permission based on whether or not
they are part of a multi-layer graph will be addressed.

To do so, the commit-graph machinery will need a version of
'hold_lock_file_for_update' that takes the permission bits from the
caller.

Introduce such a function in this patch for both the
'hold_lock_file_for_update' and 'hold_lock_file_for_update_timeout'
functions, and leave the existing functions alone by inlining their
definitions in terms of the new mode variants.

Note that, like in the previous commit, 'hold_lock_file_for_update_mode'
is not guarenteed to set the given mode, since it may be modified by
both the umask and 'core.sharedRepository'.

Note also that even though the commit-graph machinery only calls
'hold_lock_file_for_update', that this is defined in terms of
'hold_lock_file_for_update_timeout', and so both need an additional mode
parameter here.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 lockfile.c | 18 ++++++++++--------
 lockfile.h | 32 ++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 8e8ab4f29f..cc9a4b8428 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -70,7 +70,8 @@ static void resolve_symlink(struct strbuf *path)
 }
 
 /* Make sure errno contains a meaningful value on error */
-static int lock_file(struct lock_file *lk, const char *path, int flags)
+static int lock_file(struct lock_file *lk, const char *path, int flags,
+		     int mode)
 {
 	struct strbuf filename = STRBUF_INIT;
 
@@ -79,7 +80,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		resolve_symlink(&filename);
 
 	strbuf_addstr(&filename, LOCK_SUFFIX);
-	lk->tempfile = create_tempfile(filename.buf);
+	lk->tempfile = create_tempfile_mode(filename.buf, mode);
 	strbuf_release(&filename);
 	return lk->tempfile ? lk->tempfile->fd : -1;
 }
@@ -99,7 +100,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
  * exactly once. If timeout_ms is -1, try indefinitely.
  */
 static int lock_file_timeout(struct lock_file *lk, const char *path,
-			     int flags, long timeout_ms)
+			     int flags, long timeout_ms, int mode)
 {
 	int n = 1;
 	int multiplier = 1;
@@ -107,7 +108,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 	static int random_initialized = 0;
 
 	if (timeout_ms == 0)
-		return lock_file(lk, path, flags);
+		return lock_file(lk, path, flags, mode);
 
 	if (!random_initialized) {
 		srand((unsigned int)getpid());
@@ -121,7 +122,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 		long backoff_ms, wait_ms;
 		int fd;
 
-		fd = lock_file(lk, path, flags);
+		fd = lock_file(lk, path, flags, mode);
 
 		if (fd >= 0)
 			return fd; /* success */
@@ -169,10 +170,11 @@ NORETURN void unable_to_lock_die(const char *path, int err)
 }
 
 /* This should return a meaningful errno on failure */
-int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
-				      int flags, long timeout_ms)
+int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,
+					   const char *path, int flags,
+					   long timeout_ms, int mode)
 {
-	int fd = lock_file_timeout(lk, path, flags, timeout_ms);
+	int fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);
 	if (fd < 0) {
 		if (flags & LOCK_DIE_ON_ERROR)
 			unable_to_lock_die(path, errno);
diff --git a/lockfile.h b/lockfile.h
index 9843053ce8..db93e6ba73 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -90,6 +90,15 @@
  * functions. In particular, the state diagram and the cleanup
  * machinery are all implemented in the tempfile module.
  *
+ * Permission bits
+ * ---------------
+ *
+ * If you call either `hold_lock_file_for_update_mode` or
+ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested
+ * mode for the underlying temporary file. Note that the file isn't
+ * guaranteed to have this exact mode, since it may be limited by either
+ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`
+ * for more.
  *
  * Error handling
  * --------------
@@ -156,12 +165,20 @@ struct lock_file {
  * file descriptor for writing to it, or -1 on error. If the file is
  * currently locked, retry with quadratic backoff for at least
  * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if
- * timeout_ms is -1, retry indefinitely. The flags argument and error
- * handling are described above.
+ * timeout_ms is -1, retry indefinitely. The flags argument, error
+ * handling, and mode are described above.
  */
-int hold_lock_file_for_update_timeout(
+int hold_lock_file_for_update_timeout_mode(
 		struct lock_file *lk, const char *path,
-		int flags, long timeout_ms);
+		int flags, long timeout_ms, int mode);
+
+static inline int hold_lock_file_for_update_timeout(
+		struct lock_file *lk, const char *path,
+		int flags, long timeout_ms)
+{
+	return hold_lock_file_for_update_timeout_mode(lk, path, flags,
+						      timeout_ms, 0666);
+}
 
 /*
  * Attempt to create a lockfile for the file at `path` and return a
@@ -175,6 +192,13 @@ static inline int hold_lock_file_for_update(
 	return hold_lock_file_for_update_timeout(lk, path, flags, 0);
 }
 
+static inline int hold_lock_file_for_update_mode(
+		struct lock_file *lk, const char *path,
+		int flags, int mode)
+{
+	return hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);
+}
+
 /*
  * Return a nonzero value iff `lk` is currently locked.
  */
-- 
2.26.0.113.ge9739cdccc

[PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Taylor Blau]

In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
allow the caller to specify the permission bits (prior to further
adjustment by the umask and shared repository permissions) used when
acquiring a temporary file.

Use this in the commit-graph machinery for writing a non-split graph to
acquire an opened temporary file with permissions read-only permissions
to match the split behavior. (In the split case, Git uses
git_mkstemp_mode' for each of the commit-graph layers with permission
bits '0444').

One can notice this discrepancy when moving a non-split graph to be part
of a new chain. This causes a commit-graph chain where all layers have
read-only permission bits, except for the base layer, which is writable
for the current user.

Resolve this discrepancy by using the new
'hold_lock_file_for_update_mode' and passing the desired permission
bits.

Doing so causes some test fallout in t5318 and t6600. In t5318, this
occurs in tests that corrupt a commit-graph file by writing into it. For
these, 'chmod u+w'-ing the file beforehand resolves the issue. The
additional spot in 'corrupt_graph_verify' is necessary because of the
extra 'git commit-graph write' beforehand (which *does* rewrite the
commit-graph file). In t6600, this is caused by copying a read-only
commit-graph file into place and then trying to replace it. For these,
make these files writable.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c          |  3 ++-
 t/t5318-commit-graph.sh | 11 ++++++++++-
 t/t6600-test-reach.sh   |  2 ++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index f013a84e29..5b5047a7dd 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1388,7 +1388,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 		f = hashfd(fd, ctx->graph_name);
 	} else {
-		hold_lock_file_for_update(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR);
+		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
+					       LOCK_DIE_ON_ERROR, 0444);
 		fd = lk.tempfile->fd;
 		f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
 	}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 9bf920ae17..fb0aae61c3 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -96,6 +96,13 @@ test_expect_success 'write graph' '
 	graph_read_expect "3"
 '
 
+test_expect_success POSIXPERM 'write graph has correct permissions' '
+	test_path_is_file $objdir/info/commit-graph &&
+	echo "-r--r--r--" >expect &&
+	test_modebits $objdir/info/commit-graph >actual &&
+	test_cmp expect actual
+'
+
 graph_git_behavior 'graph exists' full commits/3 commits/1
 
 test_expect_success 'Add more commits' '
@@ -421,7 +428,8 @@ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
 corrupt_graph_setup() {
 	cd "$TRASH_DIRECTORY/full" &&
 	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-	cp $objdir/info/commit-graph commit-graph-backup
+	cp $objdir/info/commit-graph commit-graph-backup &&
+	chmod u+w $objdir/info/commit-graph
 }
 
 corrupt_graph_verify() {
@@ -435,6 +443,7 @@ corrupt_graph_verify() {
 	fi &&
 	git status --short &&
 	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+	chmod u+w $objdir/info/commit-graph &&
 	git commit-graph verify
 }
 
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index b24d850036..475564bee7 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -51,8 +51,10 @@ test_expect_success 'setup' '
 	done &&
 	git commit-graph write --reachable &&
 	mv .git/objects/info/commit-graph commit-graph-full &&
+	chmod u+w commit-graph-full &&
 	git show-ref -s commit-5-5 | git commit-graph write --stdin-commits &&
 	mv .git/objects/info/commit-graph commit-graph-half &&
+	chmod u+w commit-graph-half &&
 	git config core.commitGraph true
 '
 
-- 
2.26.0.113.ge9739cdccc

[PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository

[Taylor Blau]

Non-layered commit-graphs use 'adjust_shared_perm' to make the
commit-graph file readable (or not) to a combination of the user, group,
and others.

Call 'adjust_shared_perm' for split-graph layers to make sure that these
also respect 'core.sharedRepository'. The 'commit-graph-chain' file
already respects this configuration since it uses
'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually
in 'create_tempfile_mode').

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c                |  6 ++++++
 t/t5324-split-commit-graph.sh | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 5b5047a7dd..d05a55901d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1386,6 +1386,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 			return -1;
 		}
 
+		if (adjust_shared_perm(ctx->graph_name)) {
+			error(_("unable to adjust shared permissions for '%s'"),
+			      ctx->graph_name);
+			return -1;
+		}
+
 		f = hashfd(fd, ctx->graph_name);
 	} else {
 		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 53b2e6b455..61136c737f 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -351,4 +351,22 @@ test_expect_success 'split across alternate where alternate is not split' '
 	test_cmp commit-graph .git/objects/info/commit-graph
 '
 
+while read mode modebits
+do
+	test_expect_success POSIXPERM "split commit-graph respects core.sharedrepository $mode" '
+		rm -rf $graphdir $infodir/commit-graph &&
+		git reset --hard commits/1 &&
+		test_config core.sharedrepository "$mode" &&
+		git commit-graph write --split --reachable &&
+		ls $graphdir/graph-*.graph >graph-files &&
+		test_line_count = 1 graph-files &&
+		echo "$modebits" >expect &&
+		test_modebits $graphdir/graph-*.graph >actual &&
+		test_cmp expect actual
+	'
+done <<\EOF
+0666 -r--r--r--
+0600 -r--------
+EOF
+
 test_done
-- 
2.26.0.113.ge9739cdccc

Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository

[Taylor Blau]

On Mon, Apr 27, 2020 at 10:28:05AM -0600, Taylor Blau wrote:
> Non-layered commit-graphs use 'adjust_shared_perm' to make the
> commit-graph file readable (or not) to a combination of the user, group,
> and others.
>
> Call 'adjust_shared_perm' for split-graph layers to make sure that these
> also respect 'core.sharedRepository'. The 'commit-graph-chain' file
> already respects this configuration since it uses
> 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually
> in 'create_tempfile_mode').

It occurs to me that we might want to apply the same treatment to
'commit-graph-chain's, too.

Junio: I'm not sure if you want to apply the below in this series on
top, or if you'd prefer me send it as a separate series. Either way,
here's a patch to do just that:

-- >8 --

Subject: [PATCH] commit-graph.c: make 'commit-graph-chain's read-only

In a previous commit, we made incremental graph layers read-only by
using 'git_mkstemp_mode' with permissions '0444'.

There is no reason that 'commit-graph-chain's should be modifiable by
the user, since they are generated at a temporary location and then
atomically renamed into place.

To ensure that these files are read-only, too, use
'hold_lock_file_for_update_mode' with the same read-only permission
bits, and let the umask and 'adjust_shared_perm' take care of the rest.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c                | 3 ++-
 t/t5324-split-commit-graph.sh | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index d05a55901d..b2dfd7701f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1378,7 +1378,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	if (ctx->split) {
 		char *lock_name = get_chain_filename(ctx->odb);

-		hold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);
+		hold_lock_file_for_update_mode(&lk, lock_name,
+					       LOCK_DIE_ON_ERROR, 0444);

 		fd = git_mkstemp_mode(ctx->graph_name, 0444);
 		if (fd < 0) {
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 61136c737f..a8b12c8110 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -362,6 +362,8 @@ do
 		test_line_count = 1 graph-files &&
 		echo "$modebits" >expect &&
 		test_modebits $graphdir/graph-*.graph >actual &&
+		test_cmp expect actual &&
+		test_modebits $graphdir/commit-graph-chain >actual &&
 		test_cmp expect actual
 	'
 done <<\EOF
--
2.26.0.113.ge9739cdccc

Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository

[Jeff King]

On Mon, Apr 27, 2020 at 11:21:11AM -0600, Taylor Blau wrote:

> On Mon, Apr 27, 2020 at 10:28:05AM -0600, Taylor Blau wrote:
> > Non-layered commit-graphs use 'adjust_shared_perm' to make the
> > commit-graph file readable (or not) to a combination of the user, group,
> > and others.
> >
> > Call 'adjust_shared_perm' for split-graph layers to make sure that these
> > also respect 'core.sharedRepository'. The 'commit-graph-chain' file
> > already respects this configuration since it uses
> > 'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually
> > in 'create_tempfile_mode').
> 
> It occurs to me that we might want to apply the same treatment to
> 'commit-graph-chain's, too.

Yeah, perhaps. I think we've been fairly inconsistent about modes here.

Really, just about _everything_ in .git is meant to be immutable,
because we generally use rename() to update files atomically. And
there's no reason anybody should ever be opening them for writing.

I think we started with dropping the write-bit on loose and packed
object files because those are extra-precious (even if everything else
were corrupted, your history, etc, is all still there). And certainly
you can't update them without invalidating their checksum trailers
anyway.

So I think there's really three levels that could make sense:

  1. Many files in .git should lose their write bits, because Git will
     never update them except through rename. This makes things safer
     against accidental changes, but more annoying when you do want to
     edit by hand. Probably .git/config should likely be exempted, as
     we'd encourage people to hand-edit even if it's not atomic. But
     having to chmod before hand-editing a packed-refs file while
     debugging is not a huge burden.

  2. Any file written via hashfd() should be marked immutable. It cannot
     be edited without rewriting the whole contents and updating the
     trailer anyway. That would imply that commit-graph and chain files
     should be immutable, but the commit-graph-chain file should not.

  3. Everything except actual object files should retain their write
     bit. It's a minor annoyance when touching the repo by hand (e.g.,
     "rm" is sometimes pickier even about deletion), and it's not like
     there's a rash of people accidentally overwriting their refs/
     files (they're just as likely to screw themselves by deleting
     them).

I admit I don't overly care much between the three. And your patches
look fine to me, as they're just making the commit-graph code consistent
with itself (and that inconsistency is wrong under any of the mental
models above ;) ).

The exception is the final patch, which is an actual bug-fix for people
using core.sharedRepository. I suspect the feature is used infrequently
enough that nobody noticed.

-Peff

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
> allow the caller to specify the permission bits (prior to further
> adjustment by the umask and shared repository permissions) used when
> acquiring a temporary file.
>
> Use this in the commit-graph machinery for writing a non-split graph to
> acquire an opened temporary file with permissions read-only permissions
> to match the split behavior. (In the split case, Git uses
> git_mkstemp_mode' for each of the commit-graph layers with permission
> bits '0444').
>
> One can notice this discrepancy when moving a non-split graph to be part
> of a new chain. This causes a commit-graph chain where all layers have
> read-only permission bits, except for the base layer, which is writable
> for the current user.
>
> Resolve this discrepancy by using the new
> 'hold_lock_file_for_update_mode' and passing the desired permission
> bits.
>
> Doing so causes some test fallout in t5318 and t6600. In t5318, this
> occurs in tests that corrupt a commit-graph file by writing into it. For
> these, 'chmod u+w'-ing the file beforehand resolves the issue. The
> additional spot in 'corrupt_graph_verify' is necessary because of the
> extra 'git commit-graph write' beforehand (which *does* rewrite the
> commit-graph file). In t6600, this is caused by copying a read-only
> commit-graph file into place and then trying to replace it. For these,
> make these files writable.
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  commit-graph.c          |  3 ++-
>  t/t5318-commit-graph.sh | 11 ++++++++++-
>  t/t6600-test-reach.sh   |  2 ++
>  3 files changed, 14 insertions(+), 2 deletions(-)

This step, queued as 3a5c7d70 (commit-graph.c: write non-split
graphs as read-only, 2020-04-27), starts failing 5318#9 at least for
me.  Do we need to loosen umask while running this test to something
not more strict than 022 or something silly like that?

Here is what I'll use as a workaround for today's pushout.

commit f062d1c028bcc839f961e8904c38c476b9deeec3
Author: Junio C Hamano <gitster@pobox.com>
Date:   Mon Apr 27 16:50:30 2020 -0700

    SQUASH??? force known umask if you are going to check the resulting mode bits

diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index fb0aae61c3..901eb3ecfb 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '
 	test_oid_init
 '
 
+test_expect_success POSIXPERM 'tweak umask for modebit tests' '
+	umask 022
+'
+
 test_expect_success 'verify graph with no graph file' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Taylor Blau]

On Mon, Apr 27, 2020 at 04:54:09PM -0700, Junio C Hamano wrote:
> This step, queued as 3a5c7d70 (commit-graph.c: write non-split
> graphs as read-only, 2020-04-27), starts failing 5318#9 at least for
> me.  Do we need to loosen umask while running this test to something
> not more strict than 022 or something silly like that?
>
> Here is what I'll use as a workaround for today's pushout.
>
> commit f062d1c028bcc839f961e8904c38c476b9deeec3
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Mon Apr 27 16:50:30 2020 -0700
>
>     SQUASH??? force known umask if you are going to check the resulting mode bits
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index fb0aae61c3..901eb3ecfb 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '
>  	test_oid_init
>  '
>
> +test_expect_success POSIXPERM 'tweak umask for modebit tests' '
> +	umask 022
> +'
> +
>  test_expect_success 'verify graph with no graph file' '
>  	cd "$TRASH_DIRECTORY/full" &&
>  	git commit-graph verify

Looks good to me; this is definitely necessary. For what it's worth, it
passes for me, but my system may not have as restrictive a umask as
others.

I'd be happy to re-send this patch, but alternatively if you want to
just squash this in, I'd be just as happy.

Thanks,
Taylor

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Looks good to me; this is definitely necessary. For what it's worth, it
> passes for me, but my system may not have as restrictive a umask as
> others.

Note that umask is not a system thing, but personal.  When I was
with smaller company, I used to have 002 as my umask, but these days
I use 077.

> I'd be happy to re-send this patch, but alternatively if you want to
> just squash this in, I'd be just as happy.

I'll keep it queued, until we need to replace it with an undated
series.

Thanks.

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Jeff King]

On Mon, Apr 27, 2020 at 05:59:35PM -0600, Taylor Blau wrote:

> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > index fb0aae61c3..901eb3ecfb 100755
> > --- a/t/t5318-commit-graph.sh
> > +++ b/t/t5318-commit-graph.sh
> > @@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '
> >  	test_oid_init
> >  '
> >
> > +test_expect_success POSIXPERM 'tweak umask for modebit tests' '
> > +	umask 022
> > +'
> > +
> >  test_expect_success 'verify graph with no graph file' '
> >  	cd "$TRASH_DIRECTORY/full" &&
> >  	git commit-graph verify
> 
> Looks good to me; this is definitely necessary. For what it's worth, it
> passes for me, but my system may not have as restrictive a umask as
> others.

If we're just doing this for a single test, perhaps it would be better
to set the umask in that test (perhaps even in a subshell to avoid
touching other tests). I guess that's a little awkward here because the
write and the mode-check happen in separate snippets.

Or going in the opposite direction: if we think that covering the rest
of the test suite with a diversity of umasks isn't worthwhile, we could
just set "umask" in test-lib.sh. That would solve this problem and any
future ones.

I also wondered if it would be simpler to just limit the scope of the
test, like so:

diff --git b/t/t5318-commit-graph.sh a/t/t5318-commit-graph.sh
index fb0aae61c3..5f1c746ad1 100755
--- b/t/t5318-commit-graph.sh
+++ a/t/t5318-commit-graph.sh
@@ -98,9 +98,10 @@ test_expect_success 'write graph' '
 
 test_expect_success POSIXPERM 'write graph has correct permissions' '
 	test_path_is_file $objdir/info/commit-graph &&
-	echo "-r--r--r--" >expect &&
 	test_modebits $objdir/info/commit-graph >actual &&
-	test_cmp expect actual
+	# check only user mode bits, as we do not want to rely on
+	# test environment umask
+	grep ^-r-- actual
 '
 
 graph_git_behavior 'graph exists' full commits/3 commits/1

but maybe there's some value in checking the whole thing.

-Peff

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If we're just doing this for a single test, perhaps it would be better
> to set the umask in that test (perhaps even in a subshell to avoid
> touching other tests). I guess that's a little awkward here because the
> write and the mode-check happen in separate snippets.

Yes, and we cannot afford to place the writing side under POSIXPERM
prerequisite.

> Or going in the opposite direction: if we think that covering the rest
> of the test suite with a diversity of umasks isn't worthwhile, we could
> just set "umask" in test-lib.sh. That would solve this problem and any
> future ones.

Seen from the point of view of giving us a stable testing
environment, it certainly feels like an easy and simple thing to do.
I do not offhand see any downsides in that approach.

On the other hand, we use and rely on test-specified umask only in a
few tests (t0001, t1301 and t1304).  Perhaps we should discourage
tests outside these to rely too heavily on exact perm bits?

For example, I wonder if we should have used

	test -r commit-graph && ! test -w commit-graph 

to ensure the file is read-only to the user who is testing, instead
of relying on parsing "ls -l" output, which IIRC has bitten us with
xattr and forced us to revise test_modebits helper in 5610e3b0 (Fix
testcase failure when extended attributes are in use, 2008-10-19).
That would make the test require SANITY instead, though.

> I also wondered if it would be simpler to just limit the scope of the
> test, like so:
> ...
> +	# check only user mode bits, as we do not want to rely on
> +	# test environment umask
> +	grep ^-r-- actual
>  '
> ...
> but maybe there's some value in checking the whole thing.

Yeah, I guess we are wondering about the same thing.

Among various approaches on plate, my preference is to use "umask
022" around the place where we prepare the $TRASH_DIRECTORY and do
so only when POSIXPERM is there in the test-lib.sh.  I do not know
if we should do so before or after creating the $TRASH_DIRECTORY;
my gut feeling is that in the ideal world, we should be able to

 - create trash directory

 - use the directory to automatically figure out POSIXPERM

 - if POSIXPERM is set, use umask 022 and chmod og=rx the trash
   directory

Automatically figuring out POSIXPERM the above approach shoots for
is a much larger change, so I am not in a haste to implement it and
it may be OK to only do "umask 022" after we set POSIXPERM for
everybody but MINGW at least as the initial cut, but that would mean
we would run for quite a long time with the testing user's umask
during the setup process, which is unsatisfying.

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Jeff King]

On Tue, Apr 28, 2020 at 09:50:02AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > If we're just doing this for a single test, perhaps it would be better
> > to set the umask in that test (perhaps even in a subshell to avoid
> > touching other tests). I guess that's a little awkward here because the
> > write and the mode-check happen in separate snippets.
> 
> Yes, and we cannot afford to place the writing side under POSIXPERM
> prerequisite.

Do we need POSIXPERM just to call umask?

We call it unconditionally in t1304, for example. I could certainly
believe it doesn't do anything useful or predictable on other systems,
but it would not surprise me if it is a silent noop. It might return
non-zero, though (the call in t1304 is not inside a test snippet).

> Among various approaches on plate, my preference is to use "umask
> 022" around the place where we prepare the $TRASH_DIRECTORY and do
> so only when POSIXPERM is there in the test-lib.sh.  I do not know
> if we should do so before or after creating the $TRASH_DIRECTORY;
> my gut feeling is that in the ideal world, we should be able to
> 
>  - create trash directory
> 
>  - use the directory to automatically figure out POSIXPERM
> 
>  - if POSIXPERM is set, use umask 022 and chmod og=rx the trash
>    directory

I don't think we do any actual filesystem tests for POSIXPERM. It's
purely based on "uname -s", and we could check it much earlier. So
unless actually probing the filesystem is worth doing, we could just
punt on that part easily.

That said, I think this does get complicated when interacting with
t1304, for example, which explicitly creates an 077 umask for the trash
directory.

This is looking like a much deeper rabbit hole than it's worth going
down. I think the pragmatic thing is to just stick a "umask 022" near
the new test (or possibly "test_might_fail umask 022" inside the
commit-graph writing test).

-Peff

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 28, 2020 at 09:50:02AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > If we're just doing this for a single test, perhaps it would be better
>> > to set the umask in that test (perhaps even in a subshell to avoid
>> > touching other tests). I guess that's a little awkward here because the
>> > write and the mode-check happen in separate snippets.
>> 
>> Yes, and we cannot afford to place the writing side under POSIXPERM
>> prerequisite.
>
> Do we need POSIXPERM just to call umask?

I checked "git grep umask t/" hits, and I thought everybody was
using it inside POSIXPERM.

> We call it unconditionally in t1304, for example. I could certainly
> believe it doesn't do anything useful or predictable on other systems,
> but it would not surprise me if it is a silent noop. It might return
> non-zero, though (the call in t1304 is not inside a test snippet).

That is sloppy, but it might be deliberate that it does not check
the result?

> I don't think we do any actual filesystem tests for POSIXPERM. It's
> purely based on "uname -s", and we could check it much earlier. So
> unless actually probing the filesystem is worth doing, we could just
> punt on that part easily.

Yup.

> That said, I think this does get complicated when interacting with
> t1304, for example, which explicitly creates an 077 umask for the trash
> directory.
>
> This is looking like a much deeper rabbit hole than it's worth going
> down. I think the pragmatic thing is to just stick a "umask 022" near
> the new test (or possibly "test_might_fail umask 022" inside the
> commit-graph writing test).

I think the most pragmatic would be to just squash in what is
already there ;-)

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Jeff King]

On Tue, Apr 28, 2020 at 02:05:21PM -0700, Junio C Hamano wrote:

> > This is looking like a much deeper rabbit hole than it's worth going
> > down. I think the pragmatic thing is to just stick a "umask 022" near
> > the new test (or possibly "test_might_fail umask 022" inside the
> > commit-graph writing test).
> 
> I think the most pragmatic would be to just squash in what is
> already there ;-)

That is OK with me. :)

-Peff

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Taylor Blau]

On Tue, Apr 28, 2020 at 05:08:21PM -0400, Jeff King wrote:
> On Tue, Apr 28, 2020 at 02:05:21PM -0700, Junio C Hamano wrote:
>
> > > This is looking like a much deeper rabbit hole than it's worth going
> > > down. I think the pragmatic thing is to just stick a "umask 022" near
> > > the new test (or possibly "test_might_fail umask 022" inside the
> > > commit-graph writing test).
> >
> > I think the most pragmatic would be to just squash in what is
> > already there ;-)
>
> That is OK with me. :)

Thanks for an interesting discussion. I squashed Junio's fix into the
third patch, but the fourth patch suffers from the same problem (so I
stuck another POSIXPERM test to tweak the umask there, too).

What do you want to do about the final patch that I stuck on the end of
this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in
v3 and we can feel free to not apply it if it's controversial.

> -Peff

Thanks,
Taylor

[1]: https://lore.kernel.org/git/20200427172111.GA58509@syl.local/

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Jeff King]

On Tue, Apr 28, 2020 at 03:44:13PM -0600, Taylor Blau wrote:

> What do you want to do about the final patch that I stuck on the end of
> this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in
> v3 and we can feel free to not apply it if it's controversial.

I have to admit I don't care that much either way about it (see my
earlier response on three mental models). I'm happy for you or Junio to
decide. :)

-Peff

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Apr 28, 2020 at 03:44:13PM -0600, Taylor Blau wrote:
>
>> What do you want to do about the final patch that I stuck on the end of
>> this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in
>> v3 and we can feel free to not apply it if it's controversial.
>
> I have to admit I don't care that much either way about it (see my
> earlier response on three mental models). I'm happy for you or Junio to
> decide. :)

My gut feeling is that our longer term goal (if we had timeperiod
during which the codebase is quiescent enough and infinite energy to
dedicate on code clean-up) among one or your options should be to
consistently create files that are rewritten-and-renamed read-only,
to discourage casual tampering, so I am OK with that 5th patch.

Having said that, I suspect that Derrick and friends are larger
stakeholders in the "chain" file, so I'd prefer to see us basing
the choice on their input.

Thanks.

Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only

[Derrick Stolee]

On 4/28/2020 7:22 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Tue, Apr 28, 2020 at 03:44:13PM -0600, Taylor Blau wrote:
>>
>>> What do you want to do about the final patch that I stuck on the end of
>>> this series in [1]? If I don't hear from anybody, I'll send it as 5/5 in
>>> v3 and we can feel free to not apply it if it's controversial.
>>
>> I have to admit I don't care that much either way about it (see my
>> earlier response on three mental models). I'm happy for you or Junio to
>> decide. :)
> 
> My gut feeling is that our longer term goal (if we had timeperiod
> during which the codebase is quiescent enough and infinite energy to
> dedicate on code clean-up) among one or your options should be to
> consistently create files that are rewritten-and-renamed read-only,
> to discourage casual tampering, so I am OK with that 5th patch.
> 
> Having said that, I suspect that Derrick and friends are larger
> stakeholders in the "chain" file, so I'd prefer to see us basing
> the choice on their input.

I'm happy with how this discussion has gone. I'm sure the only reason
for the permissions I wrote was because I found them somewhere else in
the codebase and I copied them from there. Memory is fuzzy, but I can
guarantee the deviation from the norm was not in purpose.

Thanks,
-Stolee

[PATCH v3 0/5] commit-graph: write non-split graphs as read-only

[Taylor Blau]

Hi,

Here's a brief reroll to incorporate suggestions from Junio and Peff in
the sub-thread beginning at [1]. Not much at all has changed since v2,
with the exception of:

  * fixing the aforementioned issue by sprinkling in some 'umask 022's
    in throughout the new tests

  * adding in a new final patch to apply the same treatment to
    the 'commit-graph-chain' file

[1]: https://lore.kernel.org/git/xmqqr1w85vtq.fsf@gitster.c.googlers.com/

Taylor Blau (5):
  tempfile.c: introduce 'create_tempfile_mode'
  lockfile.c: introduce 'hold_lock_file_for_update_mode'
  commit-graph.c: write non-split graphs as read-only
  commit-graph.c: ensure graph layers respect core.sharedRepository
  commit-graph.c: make 'commit-graph-chain's read-only

 commit-graph.c                | 12 ++++++++++--
 lockfile.c                    | 18 ++++++++++--------
 lockfile.h                    | 32 ++++++++++++++++++++++++++++----
 t/t5318-commit-graph.sh       | 15 ++++++++++++++-
 t/t5324-split-commit-graph.sh | 24 ++++++++++++++++++++++++
 t/t6600-test-reach.sh         |  2 ++
 tempfile.c                    |  6 +++---
 tempfile.h                    | 10 +++++++++-
 8 files changed, 100 insertions(+), 19 deletions(-)

Range-diff against v2:
1:  03c975b0bd = 1:  03c975b0bd tempfile.c: introduce 'create_tempfile_mode'
2:  c1c84552bc = 2:  c1c84552bc lockfile.c: introduce 'hold_lock_file_for_update_mode'
3:  86cf29ce9c ! 3:  8d5503d2e6 commit-graph.c: write non-split graphs as read-only
    @@ Commit message
         commit-graph file into place and then trying to replace it. For these,
         make these files writable.

    +    Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Taylor Blau <me@ttaylorr.com>

      ## commit-graph.c ##
    @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con
      	}

      ## t/t5318-commit-graph.sh ##
    +@@ t/t5318-commit-graph.sh: test_expect_success 'setup full repo' '
    + 	test_oid_init
    + '
    +
    ++test_expect_success POSIXPERM 'tweak umask for modebit tests' '
    ++	umask 022
    ++'
    ++
    + test_expect_success 'verify graph with no graph file' '
    + 	cd "$TRASH_DIRECTORY/full" &&
    + 	git commit-graph verify
     @@ t/t5318-commit-graph.sh: test_expect_success 'write graph' '
      	graph_read_expect "3"
      '
4:  f83437f130 ! 4:  4b74e23af2 commit-graph.c: ensure graph layers respect core.sharedRepository
    @@ commit-graph.c: static int write_commit_graph_file(struct write_commit_graph_con
      		hold_lock_file_for_update_mode(&lk, ctx->graph_name,

      ## t/t5324-split-commit-graph.sh ##
    +@@ t/t5324-split-commit-graph.sh: graph_read_expect() {
    + 	test_cmp expect output
    + }
    +
    ++test_expect_success POSIXPERM 'tweak umask for modebit tests' '
    ++	umask 022
    ++'
    ++
    + test_expect_success 'create commits and write commit-graph' '
    + 	for i in $(test_seq 3)
    + 	do
     @@ t/t5324-split-commit-graph.sh: test_expect_success 'split across alternate where alternate is not split' '
      	test_cmp commit-graph .git/objects/info/commit-graph
      '
-:  ---------- > 5:  864c916067 commit-graph.c: make 'commit-graph-chain's read-only
--
2.26.0.113.ge9739cdccc

[PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode'

[Taylor Blau]

In the next patch, 'hold_lock_file_for_update' will gain an additional
'mode' parameter to specify permissions for the associated temporary
file.

Since the lockfile.c machinery uses 'create_tempfile' which always
creates a temporary file with global read-write permissions, introduce a
variant here that allows specifying the mode.

Note that the mode given to 'create_tempfile_mode' is not guaranteed to
be written to disk, since it is subject to both the umask and
'core.sharedRepository'.

Arguably, all temporary files should have permission 0444, since they
are likely to be renamed into place and then not written to again. This
is a much larger change than we may want to take on in this otherwise
small patch, so for the time being, make 'create_tempfile' behave as it
has always done by inlining it to 'create_tempfile_mode' with mode set
to '0666'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 tempfile.c |  6 +++---
 tempfile.h | 10 +++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index d43ad8c191..94aa18f3f7 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -130,17 +130,17 @@ static void deactivate_tempfile(struct tempfile *tempfile)
 }
 
 /* Make sure errno contains a meaningful value on error */
-struct tempfile *create_tempfile(const char *path)
+struct tempfile *create_tempfile_mode(const char *path, int mode)
 {
 	struct tempfile *tempfile = new_tempfile();
 
 	strbuf_add_absolute_path(&tempfile->filename, path);
 	tempfile->fd = open(tempfile->filename.buf,
-			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, 0666);
+			    O_RDWR | O_CREAT | O_EXCL | O_CLOEXEC, mode);
 	if (O_CLOEXEC && tempfile->fd < 0 && errno == EINVAL)
 		/* Try again w/o O_CLOEXEC: the kernel might not support it */
 		tempfile->fd = open(tempfile->filename.buf,
-				    O_RDWR | O_CREAT | O_EXCL, 0666);
+				    O_RDWR | O_CREAT | O_EXCL, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
 		return NULL;
diff --git a/tempfile.h b/tempfile.h
index cddda0a33c..4de3bc77d2 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -88,8 +88,16 @@ struct tempfile {
  * Attempt to create a temporary file at the specified `path`. Return
  * a tempfile (whose "fd" member can be used for writing to it), or
  * NULL on error. It is an error if a file already exists at that path.
+ * Note that `mode` will be further modified by the umask, and possibly
+ * `core.sharedRepository`, so it is not guaranteed to have the given
+ * mode.
  */
-struct tempfile *create_tempfile(const char *path);
+struct tempfile *create_tempfile_mode(const char *path, int mode);
+
+static inline struct tempfile *create_tempfile(const char *path)
+{
+	return create_tempfile_mode(path, 0666);
+}
 
 /*
  * Register an existing file as a tempfile, meaning that it will be
-- 
2.26.0.113.ge9739cdccc

[PATCH v3 2/5] lockfile.c: introduce 'hold_lock_file_for_update_mode'

[Taylor Blau]

We use 'hold_lock_file_for_update' (and the '_timeout') variant to
acquire a lock when updating references, the commit-graph file, and so
on.

In particular, the commit-graph machinery uses this to acquire a
temporary file that is used to write a non-split commit-graph. In a
subsequent commit, an issue in the commit-graph machinery produces
graph files that have a different permission based on whether or not
they are part of a multi-layer graph will be addressed.

To do so, the commit-graph machinery will need a version of
'hold_lock_file_for_update' that takes the permission bits from the
caller.

Introduce such a function in this patch for both the
'hold_lock_file_for_update' and 'hold_lock_file_for_update_timeout'
functions, and leave the existing functions alone by inlining their
definitions in terms of the new mode variants.

Note that, like in the previous commit, 'hold_lock_file_for_update_mode'
is not guarenteed to set the given mode, since it may be modified by
both the umask and 'core.sharedRepository'.

Note also that even though the commit-graph machinery only calls
'hold_lock_file_for_update', that this is defined in terms of
'hold_lock_file_for_update_timeout', and so both need an additional mode
parameter here.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 lockfile.c | 18 ++++++++++--------
 lockfile.h | 32 ++++++++++++++++++++++++++++----
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 8e8ab4f29f..cc9a4b8428 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -70,7 +70,8 @@ static void resolve_symlink(struct strbuf *path)
 }
 
 /* Make sure errno contains a meaningful value on error */
-static int lock_file(struct lock_file *lk, const char *path, int flags)
+static int lock_file(struct lock_file *lk, const char *path, int flags,
+		     int mode)
 {
 	struct strbuf filename = STRBUF_INIT;
 
@@ -79,7 +80,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
 		resolve_symlink(&filename);
 
 	strbuf_addstr(&filename, LOCK_SUFFIX);
-	lk->tempfile = create_tempfile(filename.buf);
+	lk->tempfile = create_tempfile_mode(filename.buf, mode);
 	strbuf_release(&filename);
 	return lk->tempfile ? lk->tempfile->fd : -1;
 }
@@ -99,7 +100,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
  * exactly once. If timeout_ms is -1, try indefinitely.
  */
 static int lock_file_timeout(struct lock_file *lk, const char *path,
-			     int flags, long timeout_ms)
+			     int flags, long timeout_ms, int mode)
 {
 	int n = 1;
 	int multiplier = 1;
@@ -107,7 +108,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 	static int random_initialized = 0;
 
 	if (timeout_ms == 0)
-		return lock_file(lk, path, flags);
+		return lock_file(lk, path, flags, mode);
 
 	if (!random_initialized) {
 		srand((unsigned int)getpid());
@@ -121,7 +122,7 @@ static int lock_file_timeout(struct lock_file *lk, const char *path,
 		long backoff_ms, wait_ms;
 		int fd;
 
-		fd = lock_file(lk, path, flags);
+		fd = lock_file(lk, path, flags, mode);
 
 		if (fd >= 0)
 			return fd; /* success */
@@ -169,10 +170,11 @@ NORETURN void unable_to_lock_die(const char *path, int err)
 }
 
 /* This should return a meaningful errno on failure */
-int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
-				      int flags, long timeout_ms)
+int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,
+					   const char *path, int flags,
+					   long timeout_ms, int mode)
 {
-	int fd = lock_file_timeout(lk, path, flags, timeout_ms);
+	int fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);
 	if (fd < 0) {
 		if (flags & LOCK_DIE_ON_ERROR)
 			unable_to_lock_die(path, errno);
diff --git a/lockfile.h b/lockfile.h
index 9843053ce8..db93e6ba73 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -90,6 +90,15 @@
  * functions. In particular, the state diagram and the cleanup
  * machinery are all implemented in the tempfile module.
  *
+ * Permission bits
+ * ---------------
+ *
+ * If you call either `hold_lock_file_for_update_mode` or
+ * `hold_lock_file_for_update_timeout_mode`, you can specify a suggested
+ * mode for the underlying temporary file. Note that the file isn't
+ * guaranteed to have this exact mode, since it may be limited by either
+ * the umask, 'core.sharedRepository', or both. See `adjust_shared_perm`
+ * for more.
  *
  * Error handling
  * --------------
@@ -156,12 +165,20 @@ struct lock_file {
  * file descriptor for writing to it, or -1 on error. If the file is
  * currently locked, retry with quadratic backoff for at least
  * timeout_ms milliseconds. If timeout_ms is 0, try exactly once; if
- * timeout_ms is -1, retry indefinitely. The flags argument and error
- * handling are described above.
+ * timeout_ms is -1, retry indefinitely. The flags argument, error
+ * handling, and mode are described above.
  */
-int hold_lock_file_for_update_timeout(
+int hold_lock_file_for_update_timeout_mode(
 		struct lock_file *lk, const char *path,
-		int flags, long timeout_ms);
+		int flags, long timeout_ms, int mode);
+
+static inline int hold_lock_file_for_update_timeout(
+		struct lock_file *lk, const char *path,
+		int flags, long timeout_ms)
+{
+	return hold_lock_file_for_update_timeout_mode(lk, path, flags,
+						      timeout_ms, 0666);
+}
 
 /*
  * Attempt to create a lockfile for the file at `path` and return a
@@ -175,6 +192,13 @@ static inline int hold_lock_file_for_update(
 	return hold_lock_file_for_update_timeout(lk, path, flags, 0);
 }
 
+static inline int hold_lock_file_for_update_mode(
+		struct lock_file *lk, const char *path,
+		int flags, int mode)
+{
+	return hold_lock_file_for_update_timeout_mode(lk, path, flags, 0, mode);
+}
+
 /*
  * Return a nonzero value iff `lk` is currently locked.
  */
-- 
2.26.0.113.ge9739cdccc

[PATCH v3 3/5] commit-graph.c: write non-split graphs as read-only

[Taylor Blau]

In the previous commit, Git learned 'hold_lock_file_for_update_mode' to
allow the caller to specify the permission bits (prior to further
adjustment by the umask and shared repository permissions) used when
acquiring a temporary file.

Use this in the commit-graph machinery for writing a non-split graph to
acquire an opened temporary file with permissions read-only permissions
to match the split behavior. (In the split case, Git uses
git_mkstemp_mode' for each of the commit-graph layers with permission
bits '0444').

One can notice this discrepancy when moving a non-split graph to be part
of a new chain. This causes a commit-graph chain where all layers have
read-only permission bits, except for the base layer, which is writable
for the current user.

Resolve this discrepancy by using the new
'hold_lock_file_for_update_mode' and passing the desired permission
bits.

Doing so causes some test fallout in t5318 and t6600. In t5318, this
occurs in tests that corrupt a commit-graph file by writing into it. For
these, 'chmod u+w'-ing the file beforehand resolves the issue. The
additional spot in 'corrupt_graph_verify' is necessary because of the
extra 'git commit-graph write' beforehand (which *does* rewrite the
commit-graph file). In t6600, this is caused by copying a read-only
commit-graph file into place and then trying to replace it. For these,
make these files writable.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c          |  3 ++-
 t/t5318-commit-graph.sh | 15 ++++++++++++++-
 t/t6600-test-reach.sh   |  2 ++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/commit-graph.c b/commit-graph.c
index f013a84e29..5b5047a7dd 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1388,7 +1388,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 
 		f = hashfd(fd, ctx->graph_name);
 	} else {
-		hold_lock_file_for_update(&lk, ctx->graph_name, LOCK_DIE_ON_ERROR);
+		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
+					       LOCK_DIE_ON_ERROR, 0444);
 		fd = lk.tempfile->fd;
 		f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);
 	}
diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
index 9bf920ae17..901eb3ecfb 100755
--- a/t/t5318-commit-graph.sh
+++ b/t/t5318-commit-graph.sh
@@ -12,6 +12,10 @@ test_expect_success 'setup full repo' '
 	test_oid_init
 '
 
+test_expect_success POSIXPERM 'tweak umask for modebit tests' '
+	umask 022
+'
+
 test_expect_success 'verify graph with no graph file' '
 	cd "$TRASH_DIRECTORY/full" &&
 	git commit-graph verify
@@ -96,6 +100,13 @@ test_expect_success 'write graph' '
 	graph_read_expect "3"
 '
 
+test_expect_success POSIXPERM 'write graph has correct permissions' '
+	test_path_is_file $objdir/info/commit-graph &&
+	echo "-r--r--r--" >expect &&
+	test_modebits $objdir/info/commit-graph >actual &&
+	test_cmp expect actual
+'
+
 graph_git_behavior 'graph exists' full commits/3 commits/1
 
 test_expect_success 'Add more commits' '
@@ -421,7 +432,8 @@ GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
 corrupt_graph_setup() {
 	cd "$TRASH_DIRECTORY/full" &&
 	test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
-	cp $objdir/info/commit-graph commit-graph-backup
+	cp $objdir/info/commit-graph commit-graph-backup &&
+	chmod u+w $objdir/info/commit-graph
 }
 
 corrupt_graph_verify() {
@@ -435,6 +447,7 @@ corrupt_graph_verify() {
 	fi &&
 	git status --short &&
 	GIT_TEST_COMMIT_GRAPH_DIE_ON_LOAD=true git commit-graph write &&
+	chmod u+w $objdir/info/commit-graph &&
 	git commit-graph verify
 }
 
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index b24d850036..475564bee7 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -51,8 +51,10 @@ test_expect_success 'setup' '
 	done &&
 	git commit-graph write --reachable &&
 	mv .git/objects/info/commit-graph commit-graph-full &&
+	chmod u+w commit-graph-full &&
 	git show-ref -s commit-5-5 | git commit-graph write --stdin-commits &&
 	mv .git/objects/info/commit-graph commit-graph-half &&
+	chmod u+w commit-graph-half &&
 	git config core.commitGraph true
 '
 
-- 
2.26.0.113.ge9739cdccc

[PATCH v3 4/5] commit-graph.c: ensure graph layers respect core.sharedRepository

[Taylor Blau]

Non-layered commit-graphs use 'adjust_shared_perm' to make the
commit-graph file readable (or not) to a combination of the user, group,
and others.

Call 'adjust_shared_perm' for split-graph layers to make sure that these
also respect 'core.sharedRepository'. The 'commit-graph-chain' file
already respects this configuration since it uses
'hold_lock_file_for_update' (which calls 'adjust_shared_perm' eventually
in 'create_tempfile_mode').

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c                |  6 ++++++
 t/t5324-split-commit-graph.sh | 22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/commit-graph.c b/commit-graph.c
index 5b5047a7dd..d05a55901d 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1386,6 +1386,12 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 			return -1;
 		}
 
+		if (adjust_shared_perm(ctx->graph_name)) {
+			error(_("unable to adjust shared permissions for '%s'"),
+			      ctx->graph_name);
+			return -1;
+		}
+
 		f = hashfd(fd, ctx->graph_name);
 	} else {
 		hold_lock_file_for_update_mode(&lk, ctx->graph_name,
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 53b2e6b455..699c23d077 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -36,6 +36,10 @@ graph_read_expect() {
 	test_cmp expect output
 }
 
+test_expect_success POSIXPERM 'tweak umask for modebit tests' '
+	umask 022
+'
+
 test_expect_success 'create commits and write commit-graph' '
 	for i in $(test_seq 3)
 	do
@@ -351,4 +355,22 @@ test_expect_success 'split across alternate where alternate is not split' '
 	test_cmp commit-graph .git/objects/info/commit-graph
 '
 
+while read mode modebits
+do
+	test_expect_success POSIXPERM "split commit-graph respects core.sharedrepository $mode" '
+		rm -rf $graphdir $infodir/commit-graph &&
+		git reset --hard commits/1 &&
+		test_config core.sharedrepository "$mode" &&
+		git commit-graph write --split --reachable &&
+		ls $graphdir/graph-*.graph >graph-files &&
+		test_line_count = 1 graph-files &&
+		echo "$modebits" >expect &&
+		test_modebits $graphdir/graph-*.graph >actual &&
+		test_cmp expect actual
+	'
+done <<\EOF
+0666 -r--r--r--
+0600 -r--------
+EOF
+
 test_done
-- 
2.26.0.113.ge9739cdccc

[PATCH v3 5/5] commit-graph.c: make 'commit-graph-chain's read-only

[Taylor Blau]

In a previous commit, we made incremental graph layers read-only by
using 'git_mkstemp_mode' with permissions '0444'.

There is no reason that 'commit-graph-chain's should be modifiable by
the user, since they are generated at a temporary location and then
atomically renamed into place.

To ensure that these files are read-only, too, use
'hold_lock_file_for_update_mode' with the same read-only permission
bits, and let the umask and 'adjust_shared_perm' take care of the rest.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 commit-graph.c                | 3 ++-
 t/t5324-split-commit-graph.sh | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/commit-graph.c b/commit-graph.c
index d05a55901d..b2dfd7701f 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1378,7 +1378,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)
 	if (ctx->split) {
 		char *lock_name = get_chain_filename(ctx->odb);
 
-		hold_lock_file_for_update(&lk, lock_name, LOCK_DIE_ON_ERROR);
+		hold_lock_file_for_update_mode(&lk, lock_name,
+					       LOCK_DIE_ON_ERROR, 0444);
 
 		fd = git_mkstemp_mode(ctx->graph_name, 0444);
 		if (fd < 0) {
diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
index 699c23d077..cff5a41f48 100755
--- a/t/t5324-split-commit-graph.sh
+++ b/t/t5324-split-commit-graph.sh
@@ -366,6 +366,8 @@ do
 		test_line_count = 1 graph-files &&
 		echo "$modebits" >expect &&
 		test_modebits $graphdir/graph-*.graph >actual &&
+		test_cmp expect actual &&
+		test_modebits $graphdir/commit-graph-chain >actual &&
 		test_cmp expect actual
 	'
 done <<\EOF
-- 
2.26.0.113.ge9739cdccc

Re: [PATCH v3 0/5] commit-graph: write non-split graphs as read-only

[Jeff King]

On Wed, Apr 29, 2020 at 11:36:26AM -0600, Taylor Blau wrote:

> Here's a brief reroll to incorporate suggestions from Junio and Peff in
> the sub-thread beginning at [1]. Not much at all has changed since v2,
> with the exception of:
> 
>   * fixing the aforementioned issue by sprinkling in some 'umask 022's
>     in throughout the new tests
> 
>   * adding in a new final patch to apply the same treatment to
>     the 'commit-graph-chain' file

Thanks, this version looks good to me.

-Peff