* [PATCH 0/3] commit-graph: write non-split graphs as read-only
@ 2020-04-20 22:50 Taylor Blau
2020-04-20 22:51 ` [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau
` (5 more replies)
0 siblings, 6 replies; 36+ messages in thread
From: Taylor Blau @ 2020-04-20 22:50 UTC (permalink / raw)
To: git; +Cc: dstolee, mhagger, peff
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
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau @ 2020-04-20 22:51 ` Taylor Blau 2020-04-20 23:31 ` Junio C Hamano 2020-04-20 22:51 ` [PATCH 2/3] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau ` (4 subsequent siblings) 5 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2020-04-20 22:51 UTC (permalink / raw) To: git; +Cc: dstolee, mhagger, peff 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' 2020-04-20 22:51 ` [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau @ 2020-04-20 23:31 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2020-04-20 23:31 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, mhagger, peff 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/3] lockfile.c: introduce 'hold_lock_file_for_update_mode' 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau 2020-04-20 22:51 ` [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau @ 2020-04-20 22:51 ` Taylor Blau 2020-04-20 22:51 ` [PATCH 3/3] commit-graph.c: write non-split graphs as read-only Taylor Blau ` (3 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-20 22:51 UTC (permalink / raw) To: git; +Cc: dstolee, mhagger, peff 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 3/3] commit-graph.c: write non-split graphs as read-only 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau 2020-04-20 22:51 ` [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau 2020-04-20 22:51 ` [PATCH 2/3] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau @ 2020-04-20 22:51 ` Taylor Blau 2020-04-20 23:23 ` [PATCH 0/3] commit-graph: " Junio C Hamano ` (2 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-20 22:51 UTC (permalink / raw) To: git; +Cc: dstolee, mhagger, peff 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau ` (2 preceding siblings ...) 2020-04-20 22:51 ` [PATCH 3/3] commit-graph.c: write non-split graphs as read-only Taylor Blau @ 2020-04-20 23:23 ` Junio C Hamano 2020-04-20 23:39 ` Taylor Blau 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau 5 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2020-04-20 23:23 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, mhagger, peff 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/? ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only 2020-04-20 23:23 ` [PATCH 0/3] commit-graph: " Junio C Hamano @ 2020-04-20 23:39 ` Taylor Blau 2020-04-21 1:17 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2020-04-20 23:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, dstolee, mhagger, peff 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only 2020-04-20 23:39 ` Taylor Blau @ 2020-04-21 1:17 ` Junio C Hamano 2020-04-21 7:01 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2020-04-21 1:17 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, mhagger, peff 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(). ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only 2020-04-21 1:17 ` Junio C Hamano @ 2020-04-21 7:01 ` Jeff King 2020-04-21 18:59 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2020-04-21 7:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, dstolee, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 0/3] commit-graph: write non-split graphs as read-only 2020-04-21 7:01 ` Jeff King @ 2020-04-21 18:59 ` Junio C Hamano 0 siblings, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2020-04-21 18:59 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git, dstolee, mhagger 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 0/4] commit-graph: write non-split graphs as read-only 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau ` (3 preceding siblings ...) 2020-04-20 23:23 ` [PATCH 0/3] commit-graph: " Junio C Hamano @ 2020-04-27 16:27 ` Taylor Blau 2020-04-27 16:27 ` [PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau ` (3 more replies) 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau 5 siblings, 4 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-27 16:27 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode' 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau @ 2020-04-27 16:27 ` Taylor Blau 2020-04-27 16:27 ` [PATCH v2 2/4] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau ` (2 subsequent siblings) 3 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-27 16:27 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/4] lockfile.c: introduce 'hold_lock_file_for_update_mode' 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau 2020-04-27 16:27 ` [PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau @ 2020-04-27 16:27 ` Taylor Blau 2020-04-27 16:28 ` [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only Taylor Blau 2020-04-27 16:28 ` [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau 3 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-27 16:27 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau 2020-04-27 16:27 ` [PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau 2020-04-27 16:27 ` [PATCH v2 2/4] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau @ 2020-04-27 16:28 ` Taylor Blau 2020-04-27 23:54 ` Junio C Hamano 2020-04-27 16:28 ` [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau 3 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2020-04-27 16:28 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-27 16:28 ` [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only Taylor Blau @ 2020-04-27 23:54 ` Junio C Hamano 2020-04-27 23:59 ` Taylor Blau 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2020-04-27 23:54 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff, dstolee, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-27 23:54 ` Junio C Hamano @ 2020-04-27 23:59 ` Taylor Blau 2020-04-28 0:25 ` Junio C Hamano 2020-04-28 3:34 ` Jeff King 0 siblings, 2 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-27 23:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, peff, dstolee, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-27 23:59 ` Taylor Blau @ 2020-04-28 0:25 ` Junio C Hamano 2020-04-28 3:34 ` Jeff King 1 sibling, 0 replies; 36+ messages in thread From: Junio C Hamano @ 2020-04-28 0:25 UTC (permalink / raw) To: Taylor Blau; +Cc: git, peff, dstolee, mhagger 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-27 23:59 ` Taylor Blau 2020-04-28 0:25 ` Junio C Hamano @ 2020-04-28 3:34 ` Jeff King 2020-04-28 16:50 ` Junio C Hamano 1 sibling, 1 reply; 36+ messages in thread From: Jeff King @ 2020-04-28 3:34 UTC (permalink / raw) To: Taylor Blau; +Cc: Junio C Hamano, git, dstolee, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 3:34 ` Jeff King @ 2020-04-28 16:50 ` Junio C Hamano 2020-04-28 20:59 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2020-04-28 16:50 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git, dstolee, mhagger 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 16:50 ` Junio C Hamano @ 2020-04-28 20:59 ` Jeff King 2020-04-28 21:05 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2020-04-28 20:59 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, dstolee, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 20:59 ` Jeff King @ 2020-04-28 21:05 ` Junio C Hamano 2020-04-28 21:08 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2020-04-28 21:05 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git, dstolee, mhagger 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 ;-) ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 21:05 ` Junio C Hamano @ 2020-04-28 21:08 ` Jeff King 2020-04-28 21:44 ` Taylor Blau 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2020-04-28 21:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Taylor Blau, git, dstolee, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 21:08 ` Jeff King @ 2020-04-28 21:44 ` Taylor Blau 2020-04-28 21:58 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2020-04-28 21:44 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Taylor Blau, git, dstolee, mhagger 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/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 21:44 ` Taylor Blau @ 2020-04-28 21:58 ` Jeff King 2020-04-28 23:22 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2020-04-28 21:58 UTC (permalink / raw) To: Taylor Blau; +Cc: Junio C Hamano, git, dstolee, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 21:58 ` Jeff King @ 2020-04-28 23:22 ` Junio C Hamano 2020-04-29 11:52 ` Derrick Stolee 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2020-04-28 23:22 UTC (permalink / raw) To: Jeff King; +Cc: Taylor Blau, git, dstolee, mhagger 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only 2020-04-28 23:22 ` Junio C Hamano @ 2020-04-29 11:52 ` Derrick Stolee 0 siblings, 0 replies; 36+ messages in thread From: Derrick Stolee @ 2020-04-29 11:52 UTC (permalink / raw) To: Junio C Hamano, Jeff King; +Cc: Taylor Blau, git, dstolee, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau ` (2 preceding siblings ...) 2020-04-27 16:28 ` [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only Taylor Blau @ 2020-04-27 16:28 ` Taylor Blau 2020-04-27 17:21 ` Taylor Blau 3 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2020-04-27 16:28 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository 2020-04-27 16:28 ` [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau @ 2020-04-27 17:21 ` Taylor Blau 2020-04-27 20:58 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Taylor Blau @ 2020-04-27 17:21 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository 2020-04-27 17:21 ` Taylor Blau @ 2020-04-27 20:58 ` Jeff King 0 siblings, 0 replies; 36+ messages in thread From: Jeff King @ 2020-04-27 20:58 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, gitster, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 0/5] commit-graph: write non-split graphs as read-only 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau ` (4 preceding siblings ...) 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau @ 2020-04-29 17:36 ` Taylor Blau 2020-04-29 17:36 ` [PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau ` (5 more replies) 5 siblings, 6 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-29 17:36 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode' 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau @ 2020-04-29 17:36 ` Taylor Blau 2020-04-29 17:36 ` [PATCH v3 2/5] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau ` (4 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-29 17:36 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 2/5] lockfile.c: introduce 'hold_lock_file_for_update_mode' 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau 2020-04-29 17:36 ` [PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau @ 2020-04-29 17:36 ` Taylor Blau 2020-04-29 17:36 ` [PATCH v3 3/5] commit-graph.c: write non-split graphs as read-only Taylor Blau ` (3 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-29 17:36 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 3/5] commit-graph.c: write non-split graphs as read-only 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau 2020-04-29 17:36 ` [PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau 2020-04-29 17:36 ` [PATCH v3 2/5] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau @ 2020-04-29 17:36 ` Taylor Blau 2020-04-29 17:36 ` [PATCH v3 4/5] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau ` (2 subsequent siblings) 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-29 17:36 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 4/5] commit-graph.c: ensure graph layers respect core.sharedRepository 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau ` (2 preceding siblings ...) 2020-04-29 17:36 ` [PATCH v3 3/5] commit-graph.c: write non-split graphs as read-only Taylor Blau @ 2020-04-29 17:36 ` Taylor Blau 2020-04-29 17:36 ` [PATCH v3 5/5] commit-graph.c: make 'commit-graph-chain's read-only Taylor Blau 2020-05-01 5:52 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Jeff King 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-29 17:36 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v3 5/5] commit-graph.c: make 'commit-graph-chain's read-only 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau ` (3 preceding siblings ...) 2020-04-29 17:36 ` [PATCH v3 4/5] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau @ 2020-04-29 17:36 ` Taylor Blau 2020-05-01 5:52 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Jeff King 5 siblings, 0 replies; 36+ messages in thread From: Taylor Blau @ 2020-04-29 17:36 UTC (permalink / raw) To: git; +Cc: peff, dstolee, gitster, mhagger 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v3 0/5] commit-graph: write non-split graphs as read-only 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau ` (4 preceding siblings ...) 2020-04-29 17:36 ` [PATCH v3 5/5] commit-graph.c: make 'commit-graph-chain's read-only Taylor Blau @ 2020-05-01 5:52 ` Jeff King 5 siblings, 0 replies; 36+ messages in thread From: Jeff King @ 2020-05-01 5:52 UTC (permalink / raw) To: Taylor Blau; +Cc: git, dstolee, gitster, mhagger 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2020-05-01 5:59 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-20 22:50 [PATCH 0/3] commit-graph: write non-split graphs as read-only Taylor Blau 2020-04-20 22:51 ` [PATCH 1/3] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau 2020-04-20 23:31 ` Junio C Hamano 2020-04-20 22:51 ` [PATCH 2/3] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau 2020-04-20 22:51 ` [PATCH 3/3] commit-graph.c: write non-split graphs as read-only Taylor Blau 2020-04-20 23:23 ` [PATCH 0/3] commit-graph: " Junio C Hamano 2020-04-20 23:39 ` Taylor Blau 2020-04-21 1:17 ` Junio C Hamano 2020-04-21 7:01 ` Jeff King 2020-04-21 18:59 ` Junio C Hamano 2020-04-27 16:27 ` [PATCH v2 0/4] " Taylor Blau 2020-04-27 16:27 ` [PATCH v2 1/4] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau 2020-04-27 16:27 ` [PATCH v2 2/4] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau 2020-04-27 16:28 ` [PATCH v2 3/4] commit-graph.c: write non-split graphs as read-only Taylor Blau 2020-04-27 23:54 ` Junio C Hamano 2020-04-27 23:59 ` Taylor Blau 2020-04-28 0:25 ` Junio C Hamano 2020-04-28 3:34 ` Jeff King 2020-04-28 16:50 ` Junio C Hamano 2020-04-28 20:59 ` Jeff King 2020-04-28 21:05 ` Junio C Hamano 2020-04-28 21:08 ` Jeff King 2020-04-28 21:44 ` Taylor Blau 2020-04-28 21:58 ` Jeff King 2020-04-28 23:22 ` Junio C Hamano 2020-04-29 11:52 ` Derrick Stolee 2020-04-27 16:28 ` [PATCH v2 4/4] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau 2020-04-27 17:21 ` Taylor Blau 2020-04-27 20:58 ` Jeff King 2020-04-29 17:36 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Taylor Blau 2020-04-29 17:36 ` [PATCH v3 1/5] tempfile.c: introduce 'create_tempfile_mode' Taylor Blau 2020-04-29 17:36 ` [PATCH v3 2/5] lockfile.c: introduce 'hold_lock_file_for_update_mode' Taylor Blau 2020-04-29 17:36 ` [PATCH v3 3/5] commit-graph.c: write non-split graphs as read-only Taylor Blau 2020-04-29 17:36 ` [PATCH v3 4/5] commit-graph.c: ensure graph layers respect core.sharedRepository Taylor Blau 2020-04-29 17:36 ` [PATCH v3 5/5] commit-graph.c: make 'commit-graph-chain's read-only Taylor Blau 2020-05-01 5:52 ` [PATCH v3 0/5] commit-graph: write non-split graphs as read-only Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).